From e7fc1cbca38bcf2aa4c6722a3d5832c9363bcda9 Mon Sep 17 00:00:00 2001 From: jwalker Date: Tue, 11 Aug 2026 10:17:18 -0500 Subject: [PATCH] Read spring.config.location instead of a property that does not exist 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 --- .../configuration/ConfigurationManager.java | 2 +- .../ConfigurationManagerTest.java | 150 ++++++++++++++++++ 2 files changed, 151 insertions(+), 1 deletion(-) create mode 100644 base/src/test/java/ai/javaclaw/configuration/ConfigurationManagerTest.java diff --git a/base/src/main/java/ai/javaclaw/configuration/ConfigurationManager.java b/base/src/main/java/ai/javaclaw/configuration/ConfigurationManager.java index 884f4197..13166977 100644 --- a/base/src/main/java/ai/javaclaw/configuration/ConfigurationManager.java +++ b/base/src/main/java/ai/javaclaw/configuration/ConfigurationManager.java @@ -23,7 +23,7 @@ public class ConfigurationManager { public ConfigurationManager(Environment environment, ApplicationEventPublisher eventPublisher) { this.eventPublisher = eventPublisher; - this.configPath = resolveConfigPath(environment.getProperty("spring.allConfig.location")); + this.configPath = resolveConfigPath(environment.getProperty("spring.config.location")); } public void updateProperty(String key, Object value) throws IOException { diff --git a/base/src/test/java/ai/javaclaw/configuration/ConfigurationManagerTest.java b/base/src/test/java/ai/javaclaw/configuration/ConfigurationManagerTest.java new file mode 100644 index 00000000..b5b0e984 --- /dev/null +++ b/base/src/test/java/ai/javaclaw/configuration/ConfigurationManagerTest.java @@ -0,0 +1,150 @@ +package ai.javaclaw.configuration; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.springframework.context.ApplicationEventPublisher; +import org.springframework.mock.env.MockEnvironment; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + +import static org.assertj.core.api.Assertions.assertThat; + +class ConfigurationManagerTest { + + @TempDir + Path configDir; + + private final List publishedEvents = new ArrayList<>(); + private final ApplicationEventPublisher eventPublisher = publishedEvents::add; + + private ConfigurationManager managerAt(String configLocation) { + MockEnvironment environment = new MockEnvironment(); + if (configLocation != null) { + environment.setProperty("spring.config.location", configLocation); + } + return new ConfigurationManager(environment, eventPublisher); + } + + // ----------------------------------------------------------------------- + // spring.config.location is honoured when resolving the config file + // + // Every assertion below keys off MARKER, which only ever exists in the + // per-test temporary directory. Asserting on a property the real + // application.private.yaml also carries would let these tests pass by + // reading the fallback location instead of the configured one. + // ----------------------------------------------------------------------- + + private static final String MARKER = "javaclaw-configuration-manager-test"; + + private Path seedConfigFile(String fileName) throws IOException { + Path configFile = configDir.resolve(fileName); + Files.writeString(configFile, "marker: " + MARKER + "\n"); + return configFile; + } + + @Test + void readsFromTheLocationPointingAtAFile() throws IOException { + Path configFile = seedConfigFile("application.private.yaml"); + + Map config = managerAt(configFile.toString()).readApplicationYaml(); + + assertThat(config).containsEntry("marker", MARKER); + } + + @Test + void readsFromTheLocationPointingAtADirectory() throws IOException { + seedConfigFile("application.private.yaml"); + + Map config = managerAt(configDir.toString()).readApplicationYaml(); + + assertThat(config).containsEntry("marker", MARKER); + } + + @Test + void stripsTheFilePrefixFromTheLocation() throws IOException { + Path configFile = seedConfigFile("application.private.yaml"); + + Map config = managerAt("file:" + configFile).readApplicationYaml(); + + assertThat(config).containsEntry("marker", MARKER); + } + + @Test + void usesTheFirstEntryOfACommaSeparatedLocationList() throws IOException { + Path configFile = seedConfigFile("application.private.yaml"); + + Map config = managerAt(configFile + ",classpath:/other.yaml").readApplicationYaml(); + + assertThat(config).containsEntry("marker", MARKER); + } + + // ----------------------------------------------------------------------- + // Writing goes to the same place reading does + // ----------------------------------------------------------------------- + + @Test + void writesToTheConfiguredLocation() throws IOException { + ConfigurationManager manager = managerAt(configDir.toString()); + + manager.updateProperty("agent.onboarding.completed", true); + + Path written = configDir.resolve("application.private.yaml"); + assertThat(written).exists(); + assertThat(Files.readString(written)).contains("completed: true"); + } + + @Test + void writesNestedKeysWithoutDiscardingSiblings() throws IOException { + ConfigurationManager manager = managerAt(configDir.toString()); + + manager.updateProperty("spring.ai.model.chat", "ollama"); + manager.updateProperty("spring.ai.ollama.chat.options.model", "gemma4:12b"); + + Map reloaded = manager.readApplicationYaml(); + assertThat(reloaded).containsKey("spring"); + String written = Files.readString(configDir.resolve("application.private.yaml")); + assertThat(written).contains("chat: ollama").contains("model: gemma4:12b"); + } + + @Test + void updatesSeveralPropertiesInASingleWrite() throws IOException { + ConfigurationManager manager = managerAt(configDir.toString()); + + manager.updateProperties(Map.of("agent.onboarding.completed", true, "spring.ai.model.chat", "ollama")); + + String written = Files.readString(configDir.resolve("application.private.yaml")); + assertThat(written).contains("completed: true").contains("chat: ollama"); + assertThat(publishedEvents).hasSize(1); + } + + @Test + void publishesAConfigurationChangedEventOnEveryWrite() throws IOException { + ConfigurationManager manager = managerAt(configDir.toString()); + + manager.updateProperty("agent.onboarding.completed", true); + + assertThat(publishedEvents).hasSize(1).allSatisfy(event -> + assertThat(event).isInstanceOf(ConfigurationChangedEvent.class)); + } + + // ----------------------------------------------------------------------- + // First run, before any config file has been written + // + // This covers a configured location whose file does not exist yet. The + // separate branch where no location is configured at all resolves to a + // hardcoded path inside the source tree, so it cannot be asserted without + // writing there; it is left uncovered on purpose. + // ----------------------------------------------------------------------- + + @Test + void returnsAnEmptyConfigWhenTheConfiguredFileDoesNotExistYet() throws IOException { + Map config = managerAt(configDir.resolve("does-not-exist.yaml").toString()).readApplicationYaml(); + + assertThat(config).isEmpty(); + } +}