skoppu22 commented on code in PR #369: URL: https://github.com/apache/cassandra-sidecar/pull/369#discussion_r3570749024
########## server/src/main/java/org/apache/cassandra/sidecar/configmanagement/ConfigurationPatchApplier.java: ########## @@ -0,0 +1,491 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.cassandra.sidecar.configmanagement; + +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; + +import io.vertx.core.json.JsonObject; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +import static org.apache.cassandra.sidecar.configmanagement.ConfigurationPatchValidator.CASSANDRA_YAML_SECTION; +import static org.apache.cassandra.sidecar.configmanagement.ConfigurationPatchValidator.EXTRA_JVM_OPTS_SECTION; + +/** + * Applies validated patch operations to the configuration overlay. + * + * <p>Paths reference the effective configuration (base + overlay merged), + * but mutations target the overlay only. For nested paths within a top-level {@code cassandraYaml} key, + * the entire top-level key value is copied from the effective config into the overlay before applying + * the leaf change (copy-siblings strategy). + * + * <p>All precondition checks are performed before any mutations, ensuring atomicity: + * either all operations succeed or none are applied. + */ +public final class ConfigurationPatchApplier +{ + private ConfigurationPatchApplier() + { + throw new UnsupportedOperationException(); + } + + /** + * Applies a list of validated patch operations and returns the updated overlay. + * + * @param parsedOps the validated and parsed operations + * @param effectiveConfig the current effective configuration (base + overlay merged) + * @param currentOverlay the current overlay (may be empty) + * @return the updated overlay with all operations applied + * @throws ConfigurationPatchException if any precondition check fails + */ + @NotNull + public static CassandraConfigurationOverlay apply( + @NotNull List<ConfigurationPatchValidator.ParsedPatchOperation> parsedOps, + @NotNull CassandraConfigurationOverlay effectiveConfig, + @NotNull CassandraConfigurationOverlay currentOverlay) + { + // Phase 1: Precondition checks (no mutations) + for (ConfigurationPatchValidator.ParsedPatchOperation parsed : parsedOps) + { + checkPreconditions(parsed, effectiveConfig, currentOverlay); + } + + // Phase 1b: Reject patches that would leave conflicting boolean JVM options in the + // effective configuration (e.g. adding -XX:-UseG1GC while the base already sets -XX:+UseG1GC). + // Checking against the effective opts, rather than the overlay alone, ensures the stored + // overlay and the returned effective configuration stay consistent. + checkResultingJvmOptConflicts(parsedOps, effectiveConfig.extraJvmOpts()); + + // Phase 2: Apply mutations + JsonObject updatedYaml = currentOverlay.cassandraYaml().copy(); + Map<String, String> updatedOpts = new LinkedHashMap<>(currentOverlay.extraJvmOpts()); + + for (ConfigurationPatchValidator.ParsedPatchOperation parsed : parsedOps) + { + applyMutation(parsed, effectiveConfig, updatedYaml, updatedOpts); + } + + return new CassandraConfigurationOverlay(updatedYaml, updatedOpts); + } + + private static void checkPreconditions(ConfigurationPatchValidator.ParsedPatchOperation parsed, + CassandraConfigurationOverlay effectiveConfig, + CassandraConfigurationOverlay currentOverlay) + { + if (parsed.section().equals(CASSANDRA_YAML_SECTION)) + { + checkYamlPreconditions(parsed, effectiveConfig.cassandraYaml(), currentOverlay.cassandraYaml()); + } + else if (parsed.section().equals(EXTRA_JVM_OPTS_SECTION)) + { + checkJvmOptsPreconditions(parsed, effectiveConfig.extraJvmOpts(), currentOverlay.extraJvmOpts()); + } + } + + private static void checkYamlPreconditions(ConfigurationPatchValidator.ParsedPatchOperation parsed, + JsonObject effectiveYaml, + JsonObject overlayYaml) + { + ConfigurationPatchOperation op = parsed.operation(); + + switch (op.op()) + { + case TEST: + checkNestedPathTraversable(effectiveYaml, parsed); + Object actualValue = resolveValue(effectiveYaml, parsed.topLevelKey(), parsed.nestedSegments()); + if (actualValue == null) + { + throw new ConfigurationPatchException( + "Test failed: path does not exist in effective config: '" + op.path() + "'", op); + } + // Both sides are Vert.x JSON types: resolveValue returns JsonObject/JsonArray/scalars, + // and op.value() is read from the request via JsonObject.getValue (see + // ConfigurationPatchHandler), which wraps objects/arrays the same way. + if (!Objects.equals(actualValue, op.value())) + { + throw new ConfigurationPatchException( + "Test failed: expected " + op.value() + " but found " + actualValue + + " at path '" + op.path() + "'", op); + } + break; + + case REPLACE: + checkNestedPathTraversable(effectiveYaml, parsed); + Object existingValue = resolveValue(effectiveYaml, parsed.topLevelKey(), parsed.nestedSegments()); + if (existingValue == null) + { + throw new ConfigurationPatchException( + "Replace failed: path does not exist in effective config: '" + op.path() + "'", op); + } + break; + + case REMOVE: + checkNestedPathTraversable(overlayYaml, parsed); + Object overlayValue = resolveValue(overlayYaml, parsed.topLevelKey(), parsed.nestedSegments()); + if (overlayValue == null) + { + throw new ConfigurationPatchException( + "Remove failed: path does not exist in overlay (may only exist in base template): '" + + op.path() + "'", op); + } + break; + + case ADD: + if (parsed.isNested()) + { + // Parent must exist in effective config (RFC 6902) + List<String> parentSegments = parsed.nestedSegments() + .subList(0, parsed.nestedSegments().size() - 1); + checkNestedPathTraversable(effectiveYaml, parsed); + Object parent = resolveValue(effectiveYaml, parsed.topLevelKey(), parentSegments); + if (parent == null && !parentSegments.isEmpty()) + { + throw new ConfigurationPatchException( + "Add failed: parent path does not exist in effective config: '" + op.path() + "'", op); + } + if (parent != null && !(parent instanceof JsonObject) && !(parent instanceof Map)) + { + throw new ConfigurationPatchException( + "Add failed: parent is not an object at path: '" + op.path() + "'", op); + } + } + break; + } + } + + /** + * Validates that the nested path can be traversed without hitting a non-object (e.g., an array) + * at an intermediate segment. Throws a clear error if a segment resolves to a value that + * cannot be traversed further. + */ + private static void checkNestedPathTraversable(JsonObject yaml, + ConfigurationPatchValidator.ParsedPatchOperation parsed) + { + if (!parsed.isNested()) + { + return; + } + ConfigurationPatchOperation op = parsed.operation(); + Object current = yaml.getValue(parsed.topLevelKey()); + if (current == null) + { + return; // will be caught by subsequent resolveValue null check + } + + for (int i = 0; i < parsed.nestedSegments().size() - 1; i++) + { + JsonObject currentObj = asJsonObject(current); + if (currentObj == null) + { + throw new ConfigurationPatchException( + "Path traversal failed: intermediate value is not an object at path: '" + + op.path() + "'", op); + } + current = currentObj.getValue(parsed.nestedSegments().get(i)); + if (current == null) + { + return; // will be caught by subsequent resolveValue null check + } + } + } + + private static void checkJvmOptsPreconditions(ConfigurationPatchValidator.ParsedPatchOperation parsed, + Map<String, String> effectiveOpts, + Map<String, String> overlayOpts) + { + ConfigurationPatchOperation op = parsed.operation(); + String key = parsed.topLevelKey(); + + switch (op.op()) + { + case TEST: + if (!effectiveOpts.containsKey(key)) + { + throw new ConfigurationPatchException( + "Test failed: key does not exist in effective extraJvmOpts: '" + op.path() + "'", op); + } + String actual = effectiveOpts.get(key); + if (!Objects.equals(actual, op.value())) + { + throw new ConfigurationPatchException( + "Test failed: expected '" + op.value() + "' but found '" + actual + + "' at path '" + op.path() + "'", op); + } + break; + + case REPLACE: + if (!effectiveOpts.containsKey(key)) + { + throw new ConfigurationPatchException( + "Replace failed: key does not exist in effective extraJvmOpts: '" + op.path() + "'", op); + } + break; + + case REMOVE: + if (!overlayOpts.containsKey(key)) + { + throw new ConfigurationPatchException( + "Remove failed: key does not exist in overlay extraJvmOpts " + + "(may only exist in base template): '" + op.path() + "'", op); + } + break; + + case ADD: + // No precondition — extraJvmOpts is flat, parent always exists + break; + } + } + + private static void applyMutation(ConfigurationPatchValidator.ParsedPatchOperation parsed, + CassandraConfigurationOverlay effectiveConfig, + JsonObject updatedYaml, + Map<String, String> updatedOpts) + { + ConfigurationPatchOperation op = parsed.operation(); + + if (op.op() == ConfigurationPatchOperation.Op.TEST) + { + return; // test is read-only + } + + if (parsed.section().equals(CASSANDRA_YAML_SECTION)) + { + applyYamlMutation(parsed, effectiveConfig.cassandraYaml(), updatedYaml); + } + else if (parsed.section().equals(EXTRA_JVM_OPTS_SECTION)) + { + applyJvmOptsMutation(parsed, updatedOpts); + } + } + + private static void applyYamlMutation(ConfigurationPatchValidator.ParsedPatchOperation parsed, + JsonObject effectiveYaml, + JsonObject updatedYaml) + { + ConfigurationPatchOperation op = parsed.operation(); + + if (!parsed.isNested()) + { + // Top-level key: direct put/remove on overlay yaml + if (op.op() == ConfigurationPatchOperation.Op.REMOVE) + { + updatedYaml.remove(parsed.topLevelKey()); + } + else + { + updatedYaml.put(parsed.topLevelKey(), op.value()); + } + } + else if (op.op() == ConfigurationPatchOperation.Op.REMOVE) + { + // Nested remove: modify the existing overlay subtree in-place. + // The precondition check already verified the leaf exists in the overlay. + // updatedYaml is already a deep copy so in-place mutation is safe. + JsonObject topLevel = asJsonObject(updatedYaml.getValue(parsed.topLevelKey())); + if (topLevel != null) + { + removeAtPath(topLevel, parsed.nestedSegments()); + } + } + else + { + // Nested add/replace: copy-siblings strategy. + // If a prior op in this batch already copied the top-level key into the overlay, + // operate on that copy. Otherwise, copy from effective config to ensure the overlay + // is self-contained at the top-level key granularity. + JsonObject topLevel; + if (updatedYaml.containsKey(parsed.topLevelKey())) + { + topLevel = asJsonObject(updatedYaml.getValue(parsed.topLevelKey())); + } + else + { + topLevel = deepCopyValue(effectiveYaml.getValue(parsed.topLevelKey())); + } + if (topLevel == null) + { + topLevel = new JsonObject(); + } + setAtPath(topLevel, parsed.nestedSegments(), op.value()); + updatedYaml.put(parsed.topLevelKey(), topLevel); + } + } + + private static void applyJvmOptsMutation(ConfigurationPatchValidator.ParsedPatchOperation parsed, + Map<String, String> updatedOpts) + { + ConfigurationPatchOperation op = parsed.operation(); + String key = parsed.topLevelKey(); + + if (op.op() == ConfigurationPatchOperation.Op.REMOVE) + { + updatedOpts.remove(key); + } + else + { + // Boolean-opt conflicts are validated up-front against the effective configuration + // in checkResultingJvmOptConflicts, so the mutation here is unconditional. + updatedOpts.put(key, String.valueOf(op.value())); + } + } + + /** + * Rejects patches that would leave conflicting boolean JVM options (e.g. both {@code -XX:+UseG1GC} + * and {@code -XX:-UseG1GC}) in the effective configuration once applied. + * + * <p>The check is performed against a projection of the effective options after the patch's + * add/remove/replace operations are applied in order. This catches conflicts against options that + * originate in the base template (not just the overlay), keeping the stored overlay and the + * returned effective configuration consistent. + */ + private static void checkResultingJvmOptConflicts( + List<ConfigurationPatchValidator.ParsedPatchOperation> parsedOps, + Map<String, String> effectiveOpts) + { + Map<String, String> projected = new LinkedHashMap<>(effectiveOpts); + for (ConfigurationPatchValidator.ParsedPatchOperation parsed : parsedOps) + { + if (!parsed.section().equals(EXTRA_JVM_OPTS_SECTION)) + { + continue; + } + ConfigurationPatchOperation op = parsed.operation(); + String key = parsed.topLevelKey(); + switch (op.op()) + { + case REMOVE: + projected.remove(key); + break; + case ADD: + case REPLACE: + if (CassandraConfigurationOverlay.hasConflictingBooleanOpt(projected, key)) + { + String conflicting = CassandraConfigurationOverlay.conflictingBooleanOpt(key); + throw new ConfigurationPatchException( + "Conflicting boolean JVM option: '" + key + "' conflicts with existing '" + + conflicting + "' in the effective configuration", op); + } + projected.put(key, String.valueOf(op.value())); + break; + case TEST: + break; + } + } + } + + /** + * Resolves a value at the given path within a JsonObject. + * + * @param root the root object to traverse + * @param topLevelKey the first key to look up + * @param nestedSegments remaining path segments after the top-level key + * @return the value at the path, or {@code null} if any segment is missing + */ + @Nullable + @SuppressWarnings("unchecked") + static Object resolveValue(JsonObject root, String topLevelKey, List<String> nestedSegments) + { + Object current = root.getValue(topLevelKey); + if (current == null) + { + return null; + } + + for (String segment : nestedSegments) + { + JsonObject obj = asJsonObject(current); + if (obj == null) + { + return null; + } + current = obj.getValue(segment); + if (current == null) + { + return null; + } + } + return current; + } + + private static void setAtPath(JsonObject root, List<String> segments, Object value) + { + JsonObject parent = root; + for (int i = 0; i < segments.size() - 1; i++) + { + Object child = parent.getValue(segments.get(i)); + JsonObject childObj = asJsonObject(child); + if (childObj == null) + { + childObj = new JsonObject(); + parent.put(segments.get(i), childObj); + } + parent = childObj; + } + parent.put(segments.get(segments.size() - 1), value); + } + + private static void removeAtPath(JsonObject root, List<String> segments) + { + JsonObject parent = root; + for (int i = 0; i < segments.size() - 1; i++) + { + Object child = parent.getValue(segments.get(i)); + JsonObject childObj = asJsonObject(child); + if (childObj == null) + { + return; + } + parent = childObj; + } + parent.remove(segments.get(segments.size() - 1)); Review Comment: [nit] Don't we need to remove parent if it is empty after only child removal? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
