Feature/13 implement config file - #22
Conversation
…s before creating an ObjectMapper object.
📝 WalkthroughWalkthroughAdds immutable AppConfig records, a thread-safe cached ConfigLoader that loads YAML/JSON with defaults, a default application.yml resource, and tests exercising loading, defaults, caching, error handling, and validation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ConfigLoader
participant FileSystem
participant ObjectMapper
Client->>ConfigLoader: loadOnce(configPath)
alt cached
ConfigLoader-->>Client: return cached AppConfig
else not cached
ConfigLoader->>FileSystem: exists(configPath)?
FileSystem-->>ConfigLoader: yes/no
alt not exists
ConfigLoader->>ObjectMapper: choose mapper (fallback YAML)
ObjectMapper-->>ConfigLoader: produce AppConfig.defaults()
else exists
ConfigLoader->>ConfigLoader: createMapperFor(configPath)
ConfigLoader->>FileSystem: read file bytes
FileSystem-->>ObjectMapper: file contents
ObjectMapper-->>ConfigLoader: parse -> AppConfig (or null/error)
ConfigLoader->>ConfigLoader: withDefaultsApplied()
end
ConfigLoader->>ConfigLoader: cache volatile AppConfig (double-checked)
ConfigLoader-->>Client: return AppConfig
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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: 2
🤖 Fix all issues with AI agents
In `@src/main/java/org/example/config/ConfigLoader.java`:
- Around line 81-104: The cached Config returned by loadOnce is mutable because
Config’s fields (server, logging) are public and shared; change loadOnce to
avoid exposing the singleton instance directly by returning a defensive deep
copy (e.g., clone/copy constructor or a toImmutable method) or alternately make
Config immutable: make fields private final with only getters and update Jackson
annotations or use a factory/Builder used by deserialization; ensure
Config.defaults() and withDefaultsApplied() produce/return immutable instances
(or the copy) so callers cannot mutate the shared cached config.
- Around line 58-60: In ConfigLoader's exception handling (the catch block that
throws new IllegalStateException), the error message concatenates "failed to
read config file" directly with configPath; change the message to include a
separator (e.g., ": ") before appending configPath.toAbsolutePath() so the
thrown IllegalStateException contains a readable message like "failed to read
config file: /abs/path" while preserving the original exception as the cause.
🧹 Nitpick comments (1)
src/main/java/org/example/config/ConfigLoader.java (1)
22-39: Static cache with no reset hampers testability.The
cachedfield is never cleared, so onceloadOnceis called in one test, every subsequent test in the same JVM sees the stale value. Consider adding a package-privatereset()(or@VisibleForTesting) method to clearcachedbetween tests.♻️ Suggested addition
private ConfigLoader() {} + + /** For testing only – resets the cached config. */ + static void resetForTesting() { + cached = null; + }
|
Glöm inte att skriva tester till din kod. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/main/java/org/example/config/AppConfig.java`:
- Around line 3-4: AppConfig.java imports the 2.x Jackson package
(com.fasterxml.jackson.annotation.JsonIgnoreProperties and
com.fasterxml.jackson.annotation.JsonProperty) which mismatches
ConfigLoader.java's 3.x usage; update the imports in AppConfig.java to the
tools.jackson equivalents (tools.jackson.annotation.JsonIgnoreProperties and
tools.jackson.annotation.JsonProperty) so both files use the same Jackson
namespace/version.
In `@src/main/java/org/example/config/ConfigLoader.java`:
- Around line 3-6: ConfigLoader is importing Jackson 3 (tools.jackson.*) while
AppConfig uses Jackson 2 (com.fasterxml.jackson.*); align them by switching
ConfigLoader's imports to the Jackson 2.x namespace
(com.fasterxml.jackson.databind.ObjectMapper,
com.fasterxml.jackson.databind.json.JsonMapper,
com.fasterxml.jackson.dataformat.yaml.YAMLFactory,
com.fasterxml.jackson.dataformat.yaml.YAMLMapper) so both ConfigLoader and
AppConfig use the same Jackson API, and verify the build dependency (pom.xml)
references the matching Jackson 2.x artifacts; alternatively, if you choose
Jackson 3.x, update AppConfig annotations to tools.jackson.annotation.* and
ensure the pom uses Jackson 3.x.
🧹 Nitpick comments (3)
src/main/java/org/example/config/ConfigLoader.java (2)
8-8: Unused import.
IOExceptionis never referenced directly (thecatchblock catchesException).-import java.io.IOException;
56-67: Remove redundantYAMLFactoryargument and consolidate duplicate YAML mapper creation.Lines 60 and 65 both use
YAMLMapper.builder(new YAMLFactory()).build(), butYAMLMapper.builder()with no arguments already defaults toYAMLFactoryinternally. The else branch duplicates the YAML mapper creation from the if branch—simplify by removing the explicit factory and consolidating both YAML cases into a single return statement.src/main/java/org/example/config/AppConfig.java (1)
6-49: Clean immutable design — nice use of records with defaults.The
withDefaultsApplied()pattern is a solid approach. One minor observation: the default values (8080,"./www","INFO") are duplicated in bothdefaults()andwithDefaultsApplied(). Consider extracting them into constants to keep them DRY.Example for ServerConfig
public record ServerConfig( `@JsonProperty`("port") Integer port, `@JsonProperty`("rootDir") String rootDir ) { + private static final int DEFAULT_PORT = 8080; + private static final String DEFAULT_ROOT_DIR = "./www"; + public static ServerConfig defaults() { - return new ServerConfig(8080, "./www"); + return new ServerConfig(DEFAULT_PORT, DEFAULT_ROOT_DIR); } public ServerConfig withDefaultsApplied() { - int p = (port == null ? 8080 : port); - String r = (rootDir == null || rootDir.isBlank()) ? "./www" : rootDir; + int p = (port == null ? DEFAULT_PORT : port); + String r = (rootDir == null || rootDir.isBlank()) ? DEFAULT_ROOT_DIR : rootDir; return new ServerConfig(p, r); } }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/org/example/config/AppConfig.java`:
- Around line 30-33: ServerConfig.withDefaultsApplied() currently accepts any
numeric port, so out-of-range values (<=0 or >65535) will cause confusing
runtime bind errors; add a port validation step in withDefaultsApplied() that
checks the resolved port (port variable after applying null) is within 1..65535
and handle invalid values explicitly (either replace with the default 8080 or
throw an IllegalArgumentException with a clear message); update the method so it
validates the resolved port before constructing the new ServerConfig(p, rd) and
include the symbol names (port, p, withDefaultsApplied, ServerConfig) to locate
the change.
🧹 Nitpick comments (1)
src/main/java/org/example/config/AppConfig.java (1)
26-34: Default values are duplicated betweendefaults()andwithDefaultsApplied().
8080and"./www"are hardcoded in both methods. If a default ever changes, it's easy to update one and miss the other. Extract them into constants or havewithDefaultsApplied()delegate todefaults().♻️ Proposed refactor using constants
public record ServerConfig( `@JsonProperty`("port") Integer port, `@JsonProperty`("rootDir") String rootDir ) { + private static final int DEFAULT_PORT = 8080; + private static final String DEFAULT_ROOT_DIR = "./www"; + public static ServerConfig defaults() { - return new ServerConfig(8080, "./www"); + return new ServerConfig(DEFAULT_PORT, DEFAULT_ROOT_DIR); } public ServerConfig withDefaultsApplied() { - int p = (port == null ? 8080 : port); - String rd = (rootDir == null || rootDir.isBlank()) ? "./www" : rootDir; + int p = (port == null ? DEFAULT_PORT : port); + String rd = (rootDir == null || rootDir.isBlank()) ? DEFAULT_ROOT_DIR : rootDir; return new ServerConfig(p, rd); } }The same applies to
LoggingConfigwith"INFO"on lines 42 and 46.
gitnes94
left a comment
gitnes94
left a comment
There was a problem hiding this comment.
Verkade vara problem med coderabbit och inte dependencies så allting ser ju bra ut nu!
c2130cd
* Added basic YAML config-file. * Added class ConfigLoader with static classes for encapsulation * Added static metod loadOnce and step one of static method load * Added static method createMapperFor that checks for YAML or JSON-files before creating an ObjectMapper object. * implement ConfigLoader refs #13 * Added AppConfig.java record for config after coderabbit feedback * Updated ConfigLoader to use AppConfig record and jackson 3 * Added tests for ConfigLoader and reset cached method in ConfigLoader to ensure test isolation with static cache * Removed unused dependency. Minor readability tweaks in AppConfig. * Added check for illegal port numbers to withDefaultsApplied-method. * Added test for illegal port numbers.
5 participants
implements config file loading for port, rootDir, and logging.level
reads config from path on startup; if missing, uses defaults
supports both .yml/.yaml and .json via jackson (YAMLFactory)
caches loaded config with loadOnce to avoid repeated file reads
applies defaults when sections/fields are missing; ignores unknown fields for forward compatibility
add application.yml with:
server:
port: 8080
rootDir: ./www
logging:
level: INFO
run app:
with file present → settings are used
without file → defaults are used (8080, ./www, INFO)
issue link
refs #13
Summary by CodeRabbit
New Features
Behavior
Bug Fixes / Reliability
Tests