Read spring.config.location instead of a property that does not exist - #83
Merged
arefbehboudi merged 1 commit intoAug 13, 2026
Merged
Conversation
ConfigurationManager resolved the file it reads and writes from the Spring property `spring.allConfig.location`. That property is not a Spring Boot property and nothing in the project sets it, so the lookup always returned null, the configured location was always ignored, and resolveConfigPath always took its hardcoded fallback branch. Its directory handling, `file:` prefix strip and comma-separated list handling were unreachable in practice. Read `spring.config.location`, which is what the surrounding resolution logic is written for. The class had no test coverage; add ConfigurationManagerTest covering location resolution and the write path. The assertions key off a marker value that only exists in the test's @tempdir, so they cannot pass by falling back to the repository's own application.private.yaml. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
arefbehboudi
self-requested a review
August 13, 2026 11:32
arefbehboudi
approved these changes
Aug 13, 2026
arefbehboudi
left a comment
Collaborator
There was a problem hiding this comment.
Thank you @JoseWalker. Great job!
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
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.
ConfigurationManagerresolves the config file it writes to from the Spring propertyspring.allConfig.location:That property does not exist. Nothing in the repository sets it, and it is not a Spring Boot
property, so
getPropertyalways returnsnullandresolveConfigPathalways takes itshardcoded fallback branch. The effect is that the entire
resolveConfigPathlogic below thenull check — directory handling, the
file:prefix strip, the comma-separated list handling —is unreachable in practice.
This changes the lookup to
spring.config.location, which is what the surrounding code isclearly written for: stripping a
file:prefix and taking the first entry of a comma-separatedlist only makes sense for Boot's own config-location property.
Tests
ConfigurationManagerhad no test coverage. This addsConfigurationManagerTest(9 tests)covering the resolution rules and the write path:
file:prefix strippedupdatePropertieswrites once and publishes oneConfigurationChangedEventThe assertions key off a marker value that only exists in the test's
@TempDir. That detailmatters: an earlier draft of these tests asserted on an
agent:key and passed before thefix, because with the typo in place the manager resolves the fallback path and reads the
repository's own
application.private.yaml. Pinning to a unique marker makes the tests failwithout the fix, which they now do (8 of 9 red before, all green after).
Scope, and two things left out on purpose
Deliberately limited to the property name.
While writing the tests I ran into a related but separate problem: the location this manager
writes to and the location Boot reads private config from are not the same, so onboarding
does not persist in a packaged jar or in the Docker image. That is a design question rather
than a typo, so I am filing it as its own issue rather than growing this PR.
Making the property live for the first time also exposes two pre-existing gaps in
resolveConfigPath, which this PR does not touch. Flagging them so they are a known choicerather than a surprise:
classpath:location is not handled — onlyfile:is stripped, soPath.of("classpath:/…")becomes a literal path that will not exist, and
readApplicationYamlthen quietly returns anempty map;
candidate.replace("file:", "")replaces every occurrence rather than a leading prefix.Happy to fix either here or in a follow-up, whichever you prefer.
Verified with
./gradlew test(128 tests, 0 failures) and the Modulith / ArchUnit gates.