mirror of
https://github.com/Stirling-Tools/Stirling-PDF.git
synced 2026-09-02 21:03:34 +03:00
fix(settings): save a section transactionally and stop null values corrupting settings.yml
This commit is contained in:
Anthony Stirling
@@ -176,6 +176,13 @@ public class YamlHelper {
|
||||
new ScalarNode(tag, String.valueOf(obj), ScalarStyle.PLAIN));
|
||||
}
|
||||
newValueNode = new SequenceNode(Tag.SEQ, sequenceNodes, FlowStyle.FLOW);
|
||||
} else if (newValue == null) {
|
||||
// A null must not inherit the old tag: !!int 'null' and !!map 'null'
|
||||
// make settings.yml unloadable and the app will not boot.
|
||||
newValueNode = new ScalarNode(Tag.NULL, "null", ScalarStyle.PLAIN);
|
||||
} else if (tag == Tag.INT || tag == Tag.FLOAT) {
|
||||
// Numeric values were handled above, so this one is not numeric.
|
||||
newValueNode = convertValueToNode(newValue);
|
||||
} else if (tag == Tag.NULL) {
|
||||
if ("true".equals(newValue)
|
||||
|| "false".equals(newValue)
|
||||
@@ -209,6 +216,12 @@ public class YamlHelper {
|
||||
* Applies each entry of {@code values} onto {@code target} in place, keeping any key of {@code
|
||||
* target} the map does not mention (along with its comments). Keys absent from {@code target}
|
||||
* are appended.
|
||||
*
|
||||
* <p>The merge is additive-only: an entry can add or overwrite a key but never remove one, and
|
||||
* a null entry writes a null value rather than deleting the key.
|
||||
*
|
||||
* <p>An appended key the settings template does not contain is dropped on the next restart,
|
||||
* because ConfigInitializer merges the user file into the template.
|
||||
*/
|
||||
private void mergeIntoMappingNode(MappingNode target, Map<?, ?> values) {
|
||||
for (Map.Entry<?, ?> entry : values.entrySet()) {
|
||||
|
||||
@@ -102,6 +102,122 @@ class YamlHelperMoreTest {
|
||||
assertThat(h.getValueByExactKeyPath("sharing", "emailEnabled")).isEqualTo("true");
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("a deep partial merge keeps every unrelated nested key, on disk too")
|
||||
void deepMergeKeepsUnrelatedNestedKeys() {
|
||||
YamlHelper h =
|
||||
helper(
|
||||
"oauth2:\n"
|
||||
+ " enabled: false\n"
|
||||
+ " client:\n"
|
||||
+ " google:\n"
|
||||
+ " clientId: OLD\n"
|
||||
+ " clientSecret: SEC\n"
|
||||
+ " github:\n"
|
||||
+ " clientId: GH\n");
|
||||
assertThat(
|
||||
h.updateValue(
|
||||
List.of("oauth2"),
|
||||
Map.of("client", Map.of("google", Map.of("clientId", "NEW")))))
|
||||
.isTrue();
|
||||
|
||||
// Re-parse the emitted YAML: the siblings must survive the round-trip to disk,
|
||||
// not just the in-memory node tree.
|
||||
YamlHelper reloaded = helper(h.convertNodeToYaml(h.getUpdatedRootNode()));
|
||||
assertThat(reloaded.getValueByExactKeyPath("oauth2", "client", "google", "clientId"))
|
||||
.isEqualTo("NEW");
|
||||
assertThat(
|
||||
reloaded.getValueByExactKeyPath(
|
||||
"oauth2", "client", "google", "clientSecret"))
|
||||
.isEqualTo("SEC");
|
||||
assertThat(reloaded.getValueByExactKeyPath("oauth2", "client", "github", "clientId"))
|
||||
.isEqualTo("GH");
|
||||
assertThat(reloaded.getValueByExactKeyPath("oauth2", "enabled")).isEqualTo("false");
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("a list inside a merge replaces the whole sequence instead of appending")
|
||||
void listInsideMergeReplacesTheWholeSequence() {
|
||||
YamlHelper h =
|
||||
helper(
|
||||
"mcp:\n"
|
||||
+ " enabled: true\n"
|
||||
+ " allowedOperations:\n"
|
||||
+ " - a\n"
|
||||
+ " - b\n");
|
||||
assertThat(h.updateValue(List.of("mcp"), Map.of("allowedOperations", List.of("z"))))
|
||||
.isTrue();
|
||||
|
||||
// A security allowlist must still be shortenable, so the sequence is replaced.
|
||||
List<?> operations = (List<?>) h.getValueByExactKeyPath("mcp", "allowedOperations");
|
||||
assertThat(operations).hasSize(1);
|
||||
assertThat(operations.getFirst()).isEqualTo("z");
|
||||
assertThat(h.getValueByExactKeyPath("mcp", "enabled")).isEqualTo("true");
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("a null entry in a merge writes null and does not delete the key")
|
||||
void nullEntryDoesNotDeleteTheKey() {
|
||||
YamlHelper h =
|
||||
helper(
|
||||
"sharing:\n"
|
||||
+ " enabled: true\n"
|
||||
+ " linkEnabled: true\n"
|
||||
+ " emailEnabled: false\n"
|
||||
+ " linkExpirationDays: 3\n");
|
||||
Map<String, Object> values = new LinkedHashMap<>();
|
||||
values.put("emailEnabled", null);
|
||||
assertThat(h.updateValue(List.of("sharing"), values)).isTrue();
|
||||
|
||||
assertThat(h.getValueByExactKeyPath("sharing", "emailEnabled")).isEqualTo("null");
|
||||
assertThat(h.getValueByExactKeyPath("sharing", "enabled")).isEqualTo("true");
|
||||
assertThat(h.getValueByExactKeyPath("sharing", "linkEnabled")).isEqualTo("true");
|
||||
assertThat(h.getValueByExactKeyPath("sharing", "linkExpirationDays")).isEqualTo("3");
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("merging an empty map leaves every key in the block unchanged")
|
||||
void emptyMapLeavesTheBlockUnchanged() {
|
||||
YamlHelper h =
|
||||
helper(
|
||||
"sharing:\n"
|
||||
+ " enabled: true\n"
|
||||
+ " linkEnabled: true\n"
|
||||
+ " emailEnabled: false\n"
|
||||
+ " linkExpirationDays: 3\n");
|
||||
h.updateValue(List.of("sharing"), Map.of());
|
||||
|
||||
assertThat(h.getValueByExactKeyPath("sharing", "enabled")).isEqualTo("true");
|
||||
assertThat(h.getValueByExactKeyPath("sharing", "linkEnabled")).isEqualTo("true");
|
||||
assertThat(h.getValueByExactKeyPath("sharing", "emailEnabled")).isEqualTo("false");
|
||||
assertThat(h.getValueByExactKeyPath("sharing", "linkExpirationDays")).isEqualTo("3");
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("a non-numeric value does not inherit the existing INT tag")
|
||||
void nonNumericValueDoesNotInheritIntTag() {
|
||||
YamlHelper h = helper("sharing:\n linkExpirationDays: 3\n");
|
||||
assertThat(h.updateValue(List.of("sharing", "linkExpirationDays"), "notanumber"))
|
||||
.isTrue();
|
||||
|
||||
// !!int 'notanumber' would make settings.yml unloadable and brick the boot.
|
||||
String dumped = h.convertNodeToYaml(h.getUpdatedRootNode());
|
||||
assertThat(dumped).doesNotContain("!!int");
|
||||
assertThat(helper(dumped).getValueByExactKeyPath("sharing", "linkExpirationDays"))
|
||||
.isEqualTo("notanumber");
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("a null value does not inherit the existing INT tag")
|
||||
void nullValueDoesNotInheritIntTag() {
|
||||
YamlHelper h = helper("sharing:\n linkExpirationDays: 3\n");
|
||||
assertThat(h.updateValue(List.of("sharing", "linkExpirationDays"), null)).isTrue();
|
||||
|
||||
String dumped = h.convertNodeToYaml(h.getUpdatedRootNode());
|
||||
assertThat(dumped).doesNotContain("!!int");
|
||||
assertThat(h.getValueByExactKeyPath("sharing", "linkExpirationDays")).isEqualTo("null");
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("replaces a scalar with a List value (SequenceNode)")
|
||||
void listValue() {
|
||||
|
||||
+13
-11
@@ -367,11 +367,8 @@ public class AdminSettingsController {
|
||||
Map<String, Object> flattened = new LinkedHashMap<>();
|
||||
flattenSectionData(sectionName, sectionData, flattened);
|
||||
|
||||
int updatedCount = 0;
|
||||
for (Map.Entry<String, Object> entry : flattened.entrySet()) {
|
||||
String fullKey = entry.getKey();
|
||||
Object value = entry.getValue();
|
||||
|
||||
// Validate every key before writing, so an invalid one cannot half-update the file.
|
||||
for (String fullKey : flattened.keySet()) {
|
||||
if (!isValidSettingKey(fullKey)) {
|
||||
return ResponseEntity.badRequest()
|
||||
.body(
|
||||
@@ -380,19 +377,24 @@ public class AdminSettingsController {
|
||||
"Invalid setting key format: "
|
||||
+ HtmlUtils.htmlEscape(fullKey)));
|
||||
}
|
||||
}
|
||||
|
||||
// Load once, apply all, save once instead of one full rewrite per leaf key.
|
||||
GeneralUtils.updateSettingsTransactional(flattened);
|
||||
|
||||
for (Map.Entry<String, Object> entry : flattened.entrySet()) {
|
||||
String fullKey = entry.getKey();
|
||||
Object value = entry.getValue();
|
||||
log.info(
|
||||
"Admin updating section setting: {} = {}",
|
||||
fullKey,
|
||||
logSafeValue(fullKey, value));
|
||||
GeneralUtils.saveKeyToSettings(fullKey, value);
|
||||
|
||||
// Track this as a pending change
|
||||
pendingChanges.put(fullKey, value);
|
||||
|
||||
updatedCount++;
|
||||
// pendingChanges is a ConcurrentHashMap and rejects null values.
|
||||
pendingChanges.put(fullKey, value != null ? value : "");
|
||||
}
|
||||
|
||||
int updatedCount = flattened.size();
|
||||
|
||||
String escapedSectionName = HtmlUtils.htmlEscape(sectionName);
|
||||
return ResponseEntity.ok(
|
||||
Map.of(
|
||||
|
||||
+45
-6
@@ -3,7 +3,6 @@ package stirling.software.proprietary.security.controller.api;
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
import static org.mockito.ArgumentMatchers.any;
|
||||
import static org.mockito.ArgumentMatchers.argThat;
|
||||
import static org.mockito.ArgumentMatchers.eq;
|
||||
import static org.mockito.Mockito.mockStatic;
|
||||
import static org.mockito.Mockito.never;
|
||||
import static org.mockito.Mockito.verify;
|
||||
@@ -432,7 +431,10 @@ class AdminSettingsControllerTest {
|
||||
|
||||
assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK);
|
||||
assertThat(response.getBody().get("message").toString()).contains("Successfully");
|
||||
mocked.verify(() -> GeneralUtils.saveKeyToSettings("ui.appName", "New"));
|
||||
mocked.verify(
|
||||
() ->
|
||||
GeneralUtils.updateSettingsTransactional(
|
||||
Map.of("ui.appName", "New")));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -448,9 +450,16 @@ class AdminSettingsControllerTest {
|
||||
|
||||
assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK);
|
||||
mocked.verify(
|
||||
() -> GeneralUtils.saveKeyToSettings("storage.sharing.emailEnabled", true));
|
||||
() ->
|
||||
GeneralUtils.updateSettingsTransactional(
|
||||
Map.of("storage.sharing.emailEnabled", true)));
|
||||
// The whole "storage.sharing" block is never written, so its siblings survive.
|
||||
mocked.verify(
|
||||
() -> GeneralUtils.saveKeyToSettings(eq("storage.sharing"), any()),
|
||||
() ->
|
||||
GeneralUtils.updateSettingsTransactional(
|
||||
argThat(
|
||||
(Map<String, Object> m) ->
|
||||
m.containsKey("storage.sharing"))),
|
||||
never());
|
||||
}
|
||||
}
|
||||
@@ -467,7 +476,37 @@ class AdminSettingsControllerTest {
|
||||
|
||||
assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK);
|
||||
// enabled flag auto-added and persisted
|
||||
mocked.verify(() -> GeneralUtils.saveKeyToSettings("premium.enabled", true));
|
||||
mocked.verify(
|
||||
() ->
|
||||
GeneralUtils.updateSettingsTransactional(
|
||||
argThat(
|
||||
(Map<String, Object> m) ->
|
||||
Boolean.TRUE.equals(
|
||||
m.get("premium.enabled")))));
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("a null leaf value is persisted and does not blow up pendingChanges")
|
||||
void nullLeafValueStillReturns200() {
|
||||
String leafKey = "storage.sharing.emailEnabled";
|
||||
java.util.Map<String, Object> sharing = new java.util.HashMap<>();
|
||||
sharing.put("emailEnabled", null);
|
||||
java.util.Map<String, Object> section = new java.util.HashMap<>();
|
||||
section.put("sharing", sharing);
|
||||
|
||||
try (MockedStatic<GeneralUtils> mocked = mockStatic(GeneralUtils.class)) {
|
||||
ResponseEntity<Map<String, Object>> response =
|
||||
controller.updateSettingsSection("storage", section);
|
||||
|
||||
assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK);
|
||||
mocked.verify(
|
||||
() ->
|
||||
GeneralUtils.updateSettingsTransactional(
|
||||
argThat(
|
||||
(Map<String, Object> m) ->
|
||||
m.containsKey(leafKey)
|
||||
&& m.get(leafKey) == null)));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -475,7 +514,7 @@ class AdminSettingsControllerTest {
|
||||
@DisplayName("returns 500 when persistence throws IOException")
|
||||
void persistenceIOException() {
|
||||
try (MockedStatic<GeneralUtils> mocked = mockStatic(GeneralUtils.class)) {
|
||||
mocked.when(() -> GeneralUtils.saveKeyToSettings("ui.appName", "New"))
|
||||
mocked.when(() -> GeneralUtils.updateSettingsTransactional(any()))
|
||||
.thenThrow(new IOException("io"));
|
||||
|
||||
ResponseEntity<Map<String, Object>> response =
|
||||
|
||||
Reference in New Issue
Block a user