Add multiplatform SystemConfigReader interface with JVM-specific implementation#1228
Closed
Add multiplatform SystemConfigReader interface with JVM-specific implementation#1228
SystemConfigReader interface with JVM-specific implementation#1228Conversation
EugeneTheDev
requested changes
Dec 5, 2025
| */ | ||
| override fun get(name: String): String? { | ||
| return cache.computeIfAbsent(name) { | ||
| System.getenv(name) ?: System.getProperty(name.lowercase().replace('_', '.')) |
Collaborator
There was a problem hiding this comment.
Custom logic with lowercase and replace doesn't look straightforward and can lead to confusion. Let's use name as it is everywhere
Contributor
Author
There was a problem hiding this comment.
I’m not sure about that. It’s a pretty standard way/convention to handle things by using CONFIG_PROP as an environment variable and config.prop as a system property. Debugger is using the same pattern as well
Collaborator
There was a problem hiding this comment.
It's fine that it's an established patter, the main concern here is that your input gets modified unexpectedly. So e.g. if I pass org.my_propery, it would be transformed to org.my.property and the function will return null. And it can be confusing if I know that the property is there
Contributor
Author
There was a problem hiding this comment.
@EugeneTheDev, how should we handle this? Should we only normalize the name if it’s in UPPER_CASE_WITH_UNDERSCORES?
Is the current setup working well enough?
I suggest keeping this implementation, and users can implement their own config reader
Collaborator
There was a problem hiding this comment.
Do you think it should be reader's concern? IMHO we should keep reader as simple as possible - only read what is asked. And if the application allows several configuration options, like in your Spring example with env MY_PROPERTY and system my.property, it should be application's concern, and I would expect the usage site to implement something as follows:
val myProperty = reader.getConfigVariable("MY_PROPERTY")
?: reader.getConfigVariable("my.property")
?: throw NoSuchElementException("Can't find myProperty")
Contributor
Author
There was a problem hiding this comment.
This wouldn't work as convention over configuration then.
This behavior is only applicable to JVM. For other platforms there should not be such convention.
@Ololoshechkin, what do you think?
Qodana for JVM1175 new problems were found
@@ Code coverage @@
+ 72% total lines covered
16818 lines analyzed, 12198 lines covered
# Calculated according to the filters of your coverage tool☁️ View the detailed Qodana report Contact Qodana teamContact us at qodana-support@jetbrains.com
|
kpavlov
force-pushed
the
kpavlov/system-config-reader
branch
2 times, most recently
from
f739f23 to
90d46a7
Compare
Introduce `SystemConfigReader` and `SystemSecretsReader` interfaces to standardize access to configuration values and secrets across different platforms. This change utilizes the `expect`/`actual` mechanism to define common contracts with specific platform implementations. * Implement fully functional readers for the JVM target: the configuration reader resolves values from environment variables and system properties (supports name normalization), while the secrets reader strictly uses environment variables for security. * Add stub implementations for Android, Apple, JS, and WasmJS platforms that currently throw a `NotImplementedError` when accessed. * Define the common interfaces and factory functions to allow retrieval of configuration strings and secrets.
kpavlov
force-pushed
the
kpavlov/system-config-reader
branch
from
90d46a7 to
72149ef
Compare
15 tasks
kpavlov
added a commit
that referenced
this pull request
Dec 5, 2025
# Add multiplatform system configuration and secrets readers with JVM-specific implementation Introduce `SystemConfigReader` and `SystemSecretsReader` interfaces to standardize access to configuration values and secrets across different platforms. This change utilizes the `expect`/`actual` mechanism to define common contracts with specific platform implementations. * Implement fully functional readers for the JVM target: the configuration reader resolves values from environment variables and system properties (supports name normalization), while the secrets reader strictly uses environment variables for security. * Add stub implementations for Android, Apple, JS, and WasmJS platforms that currently throw a `NotImplementedError` when accessed. * Define the common interfaces and factory functions to allow retrieval of configuration strings and secrets. * Add `UserDefaultsSystemConfigReader` backed by Apple's NSUserDefaults for retrieving configuration properties. Update `systemConfigReader` to use this implementation and introduce comprehensive unit tests to ensure correctness. ## Motivation and Context Should be used in Debugger Config and LLM Clients See also #1228 ## Breaking Changes No --- #### Type of the changes - [x] New feature (non-breaking change which adds functionality) - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update - [ ] Tests improvement - [ ] Refactoring #### Checklist - [x] The pull request has a description of the proposed change - [x] I read the [Contributing Guidelines](https://github.com/JetBrains/koog/blob/main/CONTRIBUTING.md) before opening the pull request - [x] The pull request uses **`develop`** as the base branch - [x] Tests for the changes have been added - [x] All new and existing tests passed ##### Additional steps for pull requests adding a new feature - [ ] An issue describing the proposed change exists - [ ] The pull request includes a link to the issue - [ ] The change was discussed and approved in the issue - [ ] Docs have been added / updated
Contributor
Author
|
Merged as a part of #1231 |
vova-jb
pushed a commit
that referenced
this pull request
Jan 27, 2026
# Add multiplatform system configuration and secrets readers with JVM-specific implementation Introduce `SystemConfigReader` and `SystemSecretsReader` interfaces to standardize access to configuration values and secrets across different platforms. This change utilizes the `expect`/`actual` mechanism to define common contracts with specific platform implementations. * Implement fully functional readers for the JVM target: the configuration reader resolves values from environment variables and system properties (supports name normalization), while the secrets reader strictly uses environment variables for security. * Add stub implementations for Android, Apple, JS, and WasmJS platforms that currently throw a `NotImplementedError` when accessed. * Define the common interfaces and factory functions to allow retrieval of configuration strings and secrets. * Add `UserDefaultsSystemConfigReader` backed by Apple's NSUserDefaults for retrieving configuration properties. Update `systemConfigReader` to use this implementation and introduce comprehensive unit tests to ensure correctness. ## Motivation and Context Should be used in Debugger Config and LLM Clients See also #1228 ## Breaking Changes No --- #### Type of the changes - [x] New feature (non-breaking change which adds functionality) - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update - [ ] Tests improvement - [ ] Refactoring #### Checklist - [x] The pull request has a description of the proposed change - [x] I read the [Contributing Guidelines](https://github.com/JetBrains/koog/blob/main/CONTRIBUTING.md) before opening the pull request - [x] The pull request uses **`develop`** as the base branch - [x] Tests for the changes have been added - [x] All new and existing tests passed ##### Additional steps for pull requests adding a new feature - [ ] An issue describing the proposed change exists - [ ] The pull request includes a link to the issue - [ ] The change was discussed and approved in the issue - [ ] Docs have been added / updated
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
2 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.
Add multiplatform system configuration and secrets readers with JVM-specific implementation
Introduce
SystemConfigReaderandSystemSecretsReaderinterfaces to standardize access to configuration values and secrets across different platforms. This change utilizes theexpect/actualmechanism to define common contracts with specific platform implementations.NotImplementedErrorwhen accessed.Motivation and Context
Should be used in Debugger Config and LLM Clients
See also #1231
Breaking Changes
No
Type of the changes
Checklist
developas the base branchAdditional steps for pull requests adding a new feature