Refactor ConfigLoader to accept InputStream and add unit tests - #72
Merged
Conversation
📝 WalkthroughWalkthroughConfigLoader now supports dependency-injected loading via a package-private constructor taking an InputStream. Loading was refactored to a single InputStream-based method, adds null-safety, applies defaults (port 8080, rootDirectory "static", logLevel "INFO"), introduces safe map casting, and improves exception chaining. Tests added for parsing, defaults, and null input. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/juv25d/util/ConfigLoader.java (1)
57-58:⚠️ Potential issue | 🟡 MinorOriginal exception cause is discarded.
The
catchblock wraps every failure in aRuntimeExceptionwith a generic message but drops the original exceptione. This makes debugging harder — callers and logs won't see the root cause (e.g., theIllegalArgumentExceptionfrom line 35, or a YAML parse error).Proposed fix: chain the cause
- throw new RuntimeException("Failed to load application config"); + throw new RuntimeException("Failed to load application config", e);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/util/ConfigLoader.java` around lines 57 - 58, In ConfigLoader (the catch block that currently does "throw new RuntimeException(\"Failed to load application config\")"), preserve the original exception by chaining it as the cause: replace the throw to include the caught Exception e (e.g., new RuntimeException("Failed to load application config", e)) or rethrow the original where appropriate so callers/logs can see the root cause from methods in ConfigLoader.
🧹 Nitpick comments (3)
src/test/java/org/juv25d/util/ConfigLoaderTest.java (2)
66-68: Formatting is inconsistent with the rest of the file.This test method is compressed onto fewer lines compared to the others. Consider matching the style used for the other test methods for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/util/ConfigLoaderTest.java` around lines 66 - 68, The test method throwsWhenYamlMissing is formatted on a single compressed line and should match the file's existing style; reformat the method to the same multi-line layout as the other tests so it's clearer and consistent — expand the method to its own lines with the `@Test` annotation on one line, the method signature on the next, and the assertThrows invocation (assertThrows(RuntimeException.class, () -> new ConfigLoader(null))) on its own indented line; keep references to throwsWhenYamlMissing, ConfigLoader, and assertThrows to locate and update the code.
43-58: Consider adding a test where theserverkey is entirely absent.This test covers
server: {}(key present, sub-keys missing), but doesn't cover the case where theserverkey is completely absent from the YAML. As noted in theConfigLoaderreview, that scenario currently results inport=0androotDirectory=nullinstead of the expected defaults.`@Test` void usesDefaultsWhenServerKeyAbsent() { String yaml = """ logging: level: "INFO" """; ConfigLoader loader = new ConfigLoader( new ByteArrayInputStream(yaml.getBytes()) ); assertEquals(8080, loader.getPort()); assertEquals("static", loader.getRootDirectory()); assertEquals("INFO", loader.getLogLevel()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/juv25d/util/ConfigLoaderTest.java` around lines 43 - 58, Add a new unit test in ConfigLoaderTest named usesDefaultsWhenServerKeyAbsent that constructs a ConfigLoader with YAML that omits the server key entirely (e.g., only a logging.level entry) using new ByteArrayInputStream(yaml.getBytes()), then assert that loader.getPort() == 8080, loader.getRootDirectory() == "static", and loader.getLogLevel() == "INFO" to validate defaults when the server node is missing.src/main/java/org/juv25d/util/ConfigLoader.java (1)
33-36: Null check oninputis unreachable wheninputis trulynullviatry-with-resourcesin some edge cases.In Java 9+,
try (input)with anullresource doesn't throw on entry — it only skipsclose(). So this currently works. However, the intent reads oddly: you open a try-with-resources on a potentially null resource, then check for null inside. Consider moving the null check before the try block for clarity and to avoid reliance on this subtle behavior.Proposed restructuring
private void loadConfiguration(InputStream input) { + if (input == null) { + throw new IllegalArgumentException("Did not find application-properties.yml"); + } Yaml yaml = new Yaml(); - - try (input) { - if (input == null) { - throw new IllegalArgumentException("Did not find application-properties.yml"); - } + try (input) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/util/ConfigLoader.java` around lines 33 - 36, The null check for the InputStream named `input` should be moved before the try-with-resources to avoid relying on subtle Java behavior; in the `ConfigLoader` method that obtains `input` (the variable `input` in your diff), first check `if (input == null) throw new IllegalArgumentException("Did not find application-properties.yml");` and only then enter the try-with-resources to read/close it (e.g., `try (InputStream input = ... ) { ... }` or `try (input) { ... }` after the null check), preserving the same exception message and resource closing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/juv25d/util/ConfigLoader.java`:
- Around line 30-48: The loadConfiguration method fails to apply default server
values when the top-level "server" key is missing; ensure defaults are set
unconditionally before or after parsing so port and rootDirectory never stay
unset: initialize this.port to 8080 and this.rootDirectory to "static" (or
assign those defaults when serverConfig is null) and then, if serverConfig (from
asStringObjectMap(config.get("server"))) is non-null, override these defaults
with values found (use serverConfig.getOrDefault("port", 8080) and
serverConfig.getOrDefault("root-dir", "static") to update port and rootDirectory
accordingly).
---
Outside diff comments:
In `@src/main/java/org/juv25d/util/ConfigLoader.java`:
- Around line 57-58: In ConfigLoader (the catch block that currently does "throw
new RuntimeException(\"Failed to load application config\")"), preserve the
original exception by chaining it as the cause: replace the throw to include the
caught Exception e (e.g., new RuntimeException("Failed to load application
config", e)) or rethrow the original where appropriate so callers/logs can see
the root cause from methods in ConfigLoader.
---
Nitpick comments:
In `@src/main/java/org/juv25d/util/ConfigLoader.java`:
- Around line 33-36: The null check for the InputStream named `input` should be
moved before the try-with-resources to avoid relying on subtle Java behavior; in
the `ConfigLoader` method that obtains `input` (the variable `input` in your
diff), first check `if (input == null) throw new IllegalArgumentException("Did
not find application-properties.yml");` and only then enter the
try-with-resources to read/close it (e.g., `try (InputStream input = ... ) { ...
}` or `try (input) { ... }` after the null check), preserving the same exception
message and resource closing behavior.
In `@src/test/java/org/juv25d/util/ConfigLoaderTest.java`:
- Around line 66-68: The test method throwsWhenYamlMissing is formatted on a
single compressed line and should match the file's existing style; reformat the
method to the same multi-line layout as the other tests so it's clearer and
consistent — expand the method to its own lines with the `@Test` annotation on one
line, the method signature on the next, and the assertThrows invocation
(assertThrows(RuntimeException.class, () -> new ConfigLoader(null))) on its own
indented line; keep references to throwsWhenYamlMissing, ConfigLoader, and
assertThrows to locate and update the code.
- Around line 43-58: Add a new unit test in ConfigLoaderTest named
usesDefaultsWhenServerKeyAbsent that constructs a ConfigLoader with YAML that
omits the server key entirely (e.g., only a logging.level entry) using new
ByteArrayInputStream(yaml.getBytes()), then assert that loader.getPort() ==
8080, loader.getRootDirectory() == "static", and loader.getLogLevel() == "INFO"
to validate defaults when the server node is missing.
lindaeskilsson
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/juv25d/util/ConfigLoader.java`:
- Around line 38-45: The code assumes yaml.load(...) returns a non-null Map
causing a NullPointerException when the YAML is empty; update ConfigLoader so
after Map<String,Object> config = yaml.load(input) you check for null and
replace it with an empty map (or guard before calling asStringObjectMap) so the
defaults (this.port, this.rootDirectory) remain in effect; specifically ensure
any subsequent call like asStringObjectMap(config.get("server")) is protected by
using an empty map when config == null or by null-checking config.get("server")
before passing it to asStringObjectMap.
---
Duplicate comments:
In `@src/main/java/org/juv25d/util/ConfigLoader.java`:
- Around line 40-42: Defaults for server settings are now assigned
unconditionally but ensure the ConfigLoader constructor (or initializer) sets
this.port = 8080 and this.rootDirectory = "static" before attempting to read
from the YAML so these fields are always populated; keep the unconditional
assignments in the ConfigLoader class (e.g., constructor where port and
rootDirectory are defined) and then overwrite them only if the YAML provides
valid values when parsing.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/org/juv25d/util/ConfigLoader.java (1)
14-20: Unconventional closing-brace placement on lines 16 and 20.The
}is appended on the same line as the last statement, which hurts readability. Consider placing it on its own line.♻️ Suggested formatting
private ConfigLoader() { - loadConfiguration(getClass().getClassLoader() - .getResourceAsStream("application-properties.yml")); } + loadConfiguration(getClass().getClassLoader() + .getResourceAsStream("application-properties.yml")); + } - // new constructor for testing - ConfigLoader(InputStream input) { - loadConfiguration(input); } + // new constructor for testing + ConfigLoader(InputStream input) { + loadConfiguration(input); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/util/ConfigLoader.java` around lines 14 - 20, The private constructor ConfigLoader() and the package-private testing constructor ConfigLoader(InputStream) have their closing braces placed on the same line as the last statement; move each trailing '}' onto its own line to follow conventional Java formatting and improve readability around the calls to loadConfiguration(...). Locate the constructors (ConfigLoader() and ConfigLoader(InputStream)) and place the closing brace on a new line after the loadConfiguration(...) call for each.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/juv25d/util/ConfigLoader.java`:
- Around line 55-60: The ConfigLoader currently leaves the field logLevel null
when the logging section is missing; update the ConfigLoader initialization so
logLevel defaults to a sensible value (e.g., "INFO") like port/rootDirectory do:
in the block that reads loggingConfig (using asStringObjectMap and the logLevel
field), ensure that if loggingConfig is null or loggingConfig.get("level") is
null you assign this.logLevel = "INFO" (or another chosen default) so callers of
getLogLevel() never receive null.
---
Nitpick comments:
In `@src/main/java/org/juv25d/util/ConfigLoader.java`:
- Around line 14-20: The private constructor ConfigLoader() and the
package-private testing constructor ConfigLoader(InputStream) have their closing
braces placed on the same line as the last statement; move each trailing '}'
onto its own line to follow conventional Java formatting and improve readability
around the calls to loadConfiguration(...). Locate the constructors
(ConfigLoader() and ConfigLoader(InputStream)) and place the closing brace on a
new line after the loadConfiguration(...) call for each.
SandraNelj
self-requested a review
February 18, 2026 09:15
SandraNelj
previously approved these changes
Feb 18, 2026
SandraNelj
left a comment
SandraNelj
left a comment
There was a problem hiding this comment.
Great refactor. Injecting an InputStream significantly improves testability and removes the need for classloader-dependent tests.
Defensive parsing and safe casting make the loader more robust against malformed YAML structures.
The unit tests clearly document expected behaviour for both defaults and error handling.
Nice improvement!
|
Unfortunately it seems like a "}" disappeared between line 99 and 100 in ConfigLoader.java when trying to resolve a conflict here! I'm sorry @lindaeskilsson! Could you please edit this on your end? |
@lindaeskilsson I went ahead and just added the missing curly bracket. Conflicts are now resolved. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/juv25d/util/ConfigLoader.java`:
- Around line 36-38: The null-check for the InputStream named "input" is
currently inside the try-with-resources in class ConfigLoader (the method that
opens "input" and wraps it with try (input) { ... }), which causes the
IllegalArgumentException thrown for a missing config to be caught and re-wrapped
as a RuntimeException; move the guard that throws new IllegalArgumentException
when input == null to just before the try-with-resources (i.e., validate "input"
before entering try (input) { ... }) so the IllegalArgumentException propagates
directly to callers instead of being swallowed by the catch block that re-throws
RuntimeException("Failed to load application config").
In `@src/test/java/org/juv25d/util/ConfigLoaderTest.java`:
- Around line 44-58: In the test usesDefaultsWhenServerKeysMissing in
ConfigLoaderTest, the YAML currently includes logging.level which prevents
exercising the default-fallback; remove the logging.level line from the YAML
string so the loader must use its default and keep the assertion
assertEquals("INFO", loader.getLogLevel()) to verify default behavior of
ConfigLoader.getLogLevel(). Ensure the YAML still contains server: {} and other
parts unchanged so getPort() and getRootDirectory() defaults are still
validated.
- Line 29: Replace platform-default getBytes() usage in ConfigLoaderTest by
specifying UTF-8 explicitly: update the ByteArrayInputStream calls that
currently use yaml.getBytes() (and the same call in
usesDefaultsWhenServerKeysMissing) to call getBytes with StandardCharsets.UTF_8,
and add the necessary import for java.nio.charset.StandardCharsets; this ensures
deterministic encoding for the ByteArrayInputStream construction in the
ConfigLoaderTest class.
kristinaxm
previously approved these changes
Feb 18, 2026
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/juv25d/util/ConfigLoader.java (1)
67-75:⚠️ Potential issue | 🟡 MinorTwo consistency gaps in the rate-limiting block.
A — Line 67: Direct unchecked cast instead of
asStringObjectMap.
Lines 50 and 60 both useasStringObjectMap(config.get(...))to safely extract nested maps; line 67 skips that helper and uses a raw unchecked cast. This is inconsistent, generates a compiler warning without a local@SuppressWarnings, and will throwClassCastException(wrapped intoRuntimeException) if the YAML value is not aMap.B — Lines 69–71: Unsafe
(Boolean)/(Number)direct casts.
The server section usesinstanceof Number n(line 53) and a null guard (line 56) before assigning fields. The rate-limiting section skips that protection: a string value likeenabled: "true"orrequests-per-minute: "60"in the YAML will throw aClassCastExceptionat runtime rather than falling back to defaults.♻️ Proposed fix for both issues
- Map<String, Object> rateLimitingConfig = (Map<String, Object>) config.get("rate-limiting"); + Map<String, Object> rateLimitingConfig = asStringObjectMap(config.get("rate-limiting")); if (rateLimitingConfig != null) { - this.rateLimitingEnabled = (Boolean) rateLimitingConfig.getOrDefault("enabled", true); - this.requestsPerMinute = ((Number) rateLimitingConfig.getOrDefault("requests-per-minute", 60L)).longValue(); - this.burstCapacity = ((Number) rateLimitingConfig.getOrDefault("burst-capacity", 100L)).longValue(); + Object enabledValue = rateLimitingConfig.getOrDefault("enabled", Boolean.TRUE); + this.rateLimitingEnabled = enabledValue instanceof Boolean b ? b : true; + Object rpmValue = rateLimitingConfig.getOrDefault("requests-per-minute", 60L); + this.requestsPerMinute = rpmValue instanceof Number n ? n.longValue() : 60L; + Object burstValue = rateLimitingConfig.getOrDefault("burst-capacity", 100L); + this.burstCapacity = burstValue instanceof Number n ? n.longValue() : 100L; } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/juv25d/util/ConfigLoader.java` around lines 67 - 75, In ConfigLoader, replace the raw cast of rateLimitingConfig with a call to asStringObjectMap(config.get("rate-limiting")) and then guard each extracted value with instanceof checks like you do in the server block: for rateLimitingEnabled check whether the value is a Boolean (or parse a String boolean fallback) and for requestsPerMinute and burstCapacity check instanceof Number (use .longValue()) or parse numeric strings, falling back to the existing defaults; update assignments to rateLimitingEnabled, requestsPerMinute, and burstCapacity accordingly to avoid ClassCastException and compiler warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main/java/org/juv25d/util/ConfigLoader.java`:
- Around line 67-75: In ConfigLoader, replace the raw cast of rateLimitingConfig
with a call to asStringObjectMap(config.get("rate-limiting")) and then guard
each extracted value with instanceof checks like you do in the server block: for
rateLimitingEnabled check whether the value is a Boolean (or parse a String
boolean fallback) and for requestsPerMinute and burstCapacity check instanceof
Number (use .longValue()) or parse numeric strings, falling back to the existing
defaults; update assignments to rateLimitingEnabled, requestsPerMinute, and
burstCapacity accordingly to avoid ClassCastException and compiler warnings.
Author
|
All comments addressed and tests are green! |
kristinaxm
approved these changes
Feb 19, 2026
jesperlarsson1910
approved these changes
Feb 19, 2026
jesperlarsson1910
left a comment
jesperlarsson1910
left a comment
There was a problem hiding this comment.
Looking through the PR history and the commits it looks like you have done a good job with a solid implimentation.
The tests you've created seem to cover the added logic, nicely done.
lindaeskilsson
added a commit
that referenced
this pull request
Feb 20, 2026
* test(config-loader): add test skeleton for ConfigLoader * test(config-loader): add initial test for loading config * refactor(config-loader): extract configuration loading to InputStream constructor * test(config-loader): verify values are loaded from yaml input * test(config-loader): add test for default values when server keys missing * test(config-loader): add null-input error handling test * refactor(config-loader): add safe map casting and robust value parsing * fix: handle missing server config and keep original exception cause * fix(config-loader): handle empty yaml config safely * fix (config-loader): add default log level for consistent config values * Add missing curly bracket. * fix(config-loader): address review rabbit comments and improve tests --------- Co-authored-by: Simon Forsberg <simon.co.forsberg@gmail.com> Co-authored-by: mattknatt <mattiashagstrommusic@gmail.com>
lindaeskilsson
added a commit
that referenced
this pull request
Feb 20, 2026
…sponse defaults (#87) * fix(httpresponse): add safe defaults, null-safe headers and defensive body handling * test(httpresponse): verify default values and null-safe behaviour * refactor(httpresponse): enforce non-null statusText in constructor * implement securityheadersfilter to harden http responses (#91) * Add SecurityHeadersFilter for hardened HTTP responses * Add SecurityHeadersFilter for hardened HTTP responses * Changed X-XSS-Protection value to recommended 0, * address code review feedback from CodeRabbit * Add @global annotation to SecurityHeadersFilter for automatic registration * Removed line of code in App.java * Added IpFilterTest class with unit test verifying IpFilter allows whi… (#76) * Added IpFilterTest class with unit test verifying IpFilter allows whitelisted IPs. * Fix IpFilterTest to verify response interaction instead of mock state * Added unit test for blocking IP that is not in the whitelist, results in 403 Forbidden response. Fixed HttpResponse construtors to always initialize headers and body to prevent NPE when filters call setHeader or setBody. * Update IpFilter whitelist allow test to use real HttpResponse * Assert expected status code in IpFilter whitelist allow test * Refactor ConfigLoader to accept InputStream and add unit tests (#72) * test(config-loader): add test skeleton for ConfigLoader * test(config-loader): add initial test for loading config * refactor(config-loader): extract configuration loading to InputStream constructor * test(config-loader): verify values are loaded from yaml input * test(config-loader): add test for default values when server keys missing * test(config-loader): add null-input error handling test * refactor(config-loader): add safe map casting and robust value parsing * fix: handle missing server config and keep original exception cause * fix(config-loader): handle empty yaml config safely * fix (config-loader): add default log level for consistent config values * Add missing curly bracket. * fix(config-loader): address review rabbit comments and improve tests --------- Co-authored-by: Simon Forsberg <simon.co.forsberg@gmail.com> Co-authored-by: mattknatt <mattiashagstrommusic@gmail.com> --------- Co-authored-by: KM <kristina0x7@gmail.com> Co-authored-by: johanbriger <johanbriger@gmail.com> Co-authored-by: SandraNelj <229708855+SandraNelj@users.noreply.github.com> Co-authored-by: Simon Forsberg <simon.co.forsberg@gmail.com> Co-authored-by: mattknatt <mattiashagstrommusic@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR improves the testability and robustness of ConfigLoader by decoupling configuration parsing from file loading and adding unit tests that verify its behaviour.
The loader can now be constructed with an InputStream, making it possible to test configuration parsing without relying on classpath resources or complex classloader setups.
Changes
Refactor
Tests
Result
Summary by CodeRabbit
Bug Fixes
Tests