From 4a621fc64accdfdc5d2edd775d8614540a991e38 Mon Sep 17 00:00:00 2001 From: Francisco Guerrero Date: Wed, 12 Aug 2026 15:40:48 -0700 Subject: [PATCH] Render SubnetGroups as JSON in system_views.settings SubnetGroups.Group has no getters, so a Set (e.g. the *_error_reporting_exclusions settings) fails Jackson bean introspection and SettingsTable silently falls back to the legacy toString() format while every other collection renders JSON. This fork ships those settings populated by default, so the table always showed a mix of JSON and non-JSON values. Add @JsonValue to Group so it serializes as its CIDR string. patch by Francisco Guerrero; reviewed by Stefan Miklosovic for CASSANDRA-21579 --- CHANGES.txt | 1 + .../apache/cassandra/config/DataRateSpec.java | 2 + .../cassandra/config/DataStorageSpec.java | 2 + .../apache/cassandra/config/DurationSpec.java | 2 + .../apache/cassandra/config/SubnetGroups.java | 3 + .../db/virtual/SettingsTableTest.java | 186 ++++++++++++++++++ 6 files changed, 196 insertions(+) diff --git a/CHANGES.txt b/CHANGES.txt index b0fa97d25615..2e022df8380c 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -23,6 +23,7 @@ Merged from 6.0: * Make cqlsh prompt to reset to no keyspace set by USE after dropping that keyspace (CASSANDRA-21548) * Implement CMS rediscovery and recovery protocol (CASSANDRA-20476) Merged from 5.0: + * Render SubnetGroups as JSON in system_views.settings (CASSANDRA-21579) * Avoid rebuilding per-SSTable SAI components unless missing or corrupted (CASSANDRA-21515) * Propagate trickle_fsync settings to compressed SSTable writers (CASSANDRA-21487) * Allow setCompressedReadAheadBufferSizeInKb(0) to disable read-ahead buffer (CASSANDRA-21522) diff --git a/src/java/org/apache/cassandra/config/DataRateSpec.java b/src/java/org/apache/cassandra/config/DataRateSpec.java index a89cd8413ec8..e9786fc4a770 100644 --- a/src/java/org/apache/cassandra/config/DataRateSpec.java +++ b/src/java/org/apache/cassandra/config/DataRateSpec.java @@ -23,6 +23,7 @@ import java.util.regex.Pattern; import java.util.stream.Collectors; +import com.fasterxml.jackson.annotation.JsonValue; import com.google.common.math.DoubleMath; import com.google.common.primitives.Ints; @@ -210,6 +211,7 @@ public boolean equals(Object obj) return unit.convert(other.quantity, other.unit) == quantity && other.unit.convert(quantity, unit) == other.quantity; } + @JsonValue @Override public String toString() { diff --git a/src/java/org/apache/cassandra/config/DataStorageSpec.java b/src/java/org/apache/cassandra/config/DataStorageSpec.java index 2b962b72b69c..03b7802bcad7 100644 --- a/src/java/org/apache/cassandra/config/DataStorageSpec.java +++ b/src/java/org/apache/cassandra/config/DataStorageSpec.java @@ -23,6 +23,7 @@ import java.util.regex.Pattern; import java.util.stream.Collectors; +import com.fasterxml.jackson.annotation.JsonValue; import com.google.common.primitives.Ints; import static org.apache.cassandra.config.DataStorageSpec.DataStorageUnit.BYTES; @@ -157,6 +158,7 @@ public boolean equals(Object obj) return unit.convert(other.quantity, other.unit) == quantity && other.unit.convert(quantity, unit) == other.quantity; } + @JsonValue @Override public String toString() { diff --git a/src/java/org/apache/cassandra/config/DurationSpec.java b/src/java/org/apache/cassandra/config/DurationSpec.java index b70dca1657a2..13dd9a89b76c 100644 --- a/src/java/org/apache/cassandra/config/DurationSpec.java +++ b/src/java/org/apache/cassandra/config/DurationSpec.java @@ -25,6 +25,7 @@ import java.util.regex.Pattern; import java.util.stream.Collectors; +import com.fasterxml.jackson.annotation.JsonValue; import com.google.common.primitives.Ints; import static java.util.concurrent.TimeUnit.DAYS; @@ -199,6 +200,7 @@ public boolean equals(Object obj) return unit.convert(other.quantity, other.unit) == quantity && other.unit.convert(quantity, unit) == other.quantity; } + @JsonValue @Override public String toString() { diff --git a/src/java/org/apache/cassandra/config/SubnetGroups.java b/src/java/org/apache/cassandra/config/SubnetGroups.java index 4f460fab3855..9d579fca07ca 100644 --- a/src/java/org/apache/cassandra/config/SubnetGroups.java +++ b/src/java/org/apache/cassandra/config/SubnetGroups.java @@ -29,6 +29,8 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableSet; +import com.fasterxml.jackson.annotation.JsonValue; + import inet.ipaddr.IPAddressNetwork; import inet.ipaddr.IPAddressString; @@ -130,6 +132,7 @@ public int hashCode() return Objects.hash(subnet); } + @JsonValue @Override public String toString() { diff --git a/test/unit/org/apache/cassandra/db/virtual/SettingsTableTest.java b/test/unit/org/apache/cassandra/db/virtual/SettingsTableTest.java index 305a3c8b1c84..05b4d1524466 100644 --- a/test/unit/org/apache/cassandra/db/virtual/SettingsTableTest.java +++ b/test/unit/org/apache/cassandra/db/virtual/SettingsTableTest.java @@ -18,16 +18,26 @@ package org.apache.cassandra.db.virtual; +import java.lang.reflect.Field; +import java.lang.reflect.Method; +import java.lang.reflect.Modifier; +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; import java.util.HashMap; +import java.util.IdentityHashMap; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.Set; import java.util.TreeMap; +import java.util.TreeSet; import java.util.stream.Collectors; import com.datastax.driver.core.ResultSet; import com.datastax.driver.core.Row; +import com.fasterxml.jackson.annotation.JsonValue; +import com.fasterxml.jackson.databind.JsonNode; import com.google.common.collect.ImmutableList; import org.junit.Assert; @@ -45,10 +55,13 @@ import org.apache.cassandra.config.EncryptionOptions.ServerEncryptionOptions.InternodeEncryption; import org.apache.cassandra.config.JMXServerOptions; import org.apache.cassandra.config.ParameterizedClass; +import org.apache.cassandra.config.Properties; import org.apache.cassandra.config.Redacted; +import org.apache.cassandra.config.SubnetGroups; import org.apache.cassandra.config.TransparentDataEncryptionOptions; import org.apache.cassandra.cql3.CQLTester; import org.apache.cassandra.distributed.shared.WithProperties; +import org.apache.cassandra.repair.autorepair.AutoRepairConfig; import org.apache.cassandra.security.SSLFactory; import org.apache.cassandra.utils.JsonUtils; @@ -87,6 +100,31 @@ public void config() "alias", new ParameterizedClass("SomeClass", params)); + + // populate settings that default to null, so their rendering is exercised by the tests + // (in particular testCollectionSettingsRenderAsValidJson) instead of being skipped + config.crypto_provider = new ParameterizedClass("org.apache.cassandra.security.JREProvider", + Map.of("fail_on_missing_provider", "false")); + config.internode_authenticator = new ParameterizedClass("org.apache.cassandra.auth.AllowAllInternodeAuthenticator", + Map.of()); + config.commitlog_compression = new ParameterizedClass("LZ4Compressor", + Map.of("lz4_compressor_type", "fast")); + config.hints_compression = new ParameterizedClass("SnappyCompressor", + Map.of("chunk_length_in_kb", "64")); + config.default_compaction = new ParameterizedClass("SizeTieredCompactionStrategy", + Map.of("min_threshold", "4")); + // populated (rather than default-empty) so the CASSANDRA-21579 fix for SubnetGroups.Group + // is exercised through the production render path + config.client_error_reporting_exclusions = new SubnetGroups(List.of("127.0.0.1", "10.110.60.0/26")); + config.internode_error_reporting_exclusions = new SubnetGroups(List.of("192.168.0.0/16")); + // likewise for the DurationSpec fix: repair_type_overrides is empty by default, but any node + // that touches auto-repair populates it via AutoRepairConfig.getOptions(), and a single + // unserializable nested type makes Jackson fail for the whole setting + AutoRepairConfig.Options autoRepairOverrides = new AutoRepairConfig.Options(); + autoRepairOverrides.min_repair_interval = new DurationSpec.IntSecondsBound("24h"); + autoRepairOverrides.table_max_repair_time = new DurationSpec.IntSecondsBound("6h"); + config.auto_repair.repair_type_overrides.put("full", autoRepairOverrides); + table = new SettingsTable(KS_NAME, config); VirtualKeyspaceRegistry.instance.register(new VirtualKeyspace(KS_NAME, ImmutableList.of(table))); disablePreparedReuseForTest(); @@ -416,4 +454,152 @@ public void testOldBehaviourForComplexSettingsFormatProperty() check("json_false.settings", "seed_provider.parameters", "{seeds=127.0.0.1:7000}"); } } + + /** + * Every array/collection/map-valued setting (the only shapes {@link SettingsTable} renders as + * JSON; scalars are rendered via toString() by design) must produce valid JSON: + * 1) whatever the current config renders must parse as a JSON array or object, and + * 2) every element type must be visible to Jackson, including nested properties, so that a + * populated value cannot silently fall back to toString() the way SubnetGroups.Group and + * the DurationSpec family did (CASSANDRA-21579). + */ + @Test + public void testCollectionSettingsRenderAsValidJson() + { + // note: normally-null and normally-empty settings are populated in config() so this check + // exercises their real rendering rather than skipping nulls / serializing empty collections + Set failures = new TreeSet<>(); + + for (Map.Entry e : Properties.defaultLoader().flatten(Config.class).entrySet()) + { + Property prop = e.getValue(); + Class type = prop.getType(); + + List> elements = new ArrayList<>(); + if (type.isArray()) + { + elements.add(type.getComponentType()); + } + else if (Collection.class.isAssignableFrom(type) || Map.class.isAssignableFrom(type)) + { + Class[] args = prop.getActualTypeArguments(); + if (args != null) + { + if (Map.class.isAssignableFrom(type) && args.length == 2) + elements.add(args[1]); // keys are stringified by SettingsTable; values serialize as-is + else if (args.length >= 1) + elements.add(args[0]); + } + } + else + { + continue; // scalar settings are rendered via toString() by design + } + + // 1) the rendered value, through the production path, must be valid JSON + String rendered = table.getValue(prop); + if (rendered != null) + { + try + { + JsonNode node = JsonUtils.JSON_OBJECT_MAPPER.readTree(rendered); + if (!node.isArray() && !node.isObject()) + failures.add(e.getKey() + ": rendered as neither JSON array nor object: " + rendered); + } + catch (Exception ex) + { + failures.add(e.getKey() + ": rendered value is not valid JSON: " + rendered); + } + } + + // 2) element types must be Jackson-visible, or a populated value will silently degrade + for (Class element : elements) + { + if (element == null) + continue; + Class nestedType = unrenderableType(element); + if (nestedType != null) + failures.add(e.getKey() + ": element type " + element.getName() + " is not JSON-renderable" + + (nestedType == element ? "" : ", because it reaches " + nestedType.getName()) + + ": no Jackson-visible properties. Annotate that type (e.g. @JsonValue, the way" + + " SubnetGroups.Group and DurationSpec were fixed in CASSANDRA-21579) or this" + + " setting will silently fall back to toString() when populated"); + } + } + + Assert.assertTrue(String.join("\n", failures), failures.isEmpty()); + } + + /** + * @return null if Jackson can serialize {@code type}, else the type in its property graph that + * Jackson would choke on -- {@code type} itself, or something it reaches + */ + private static Class unrenderableType(Class type) + { + return unrenderableType(type, Collections.newSetFromMap(new IdentityHashMap<>())); + } + + /** + * Approximates Jackson's default serialization visibility. A type is renderable if Jackson can + * discover at least one property on it (or a @JsonValue) and, transitively, every property it + * discovers is itself renderable. The recursion is the point: {@link AutoRepairConfig.Options} has + * plenty of visible properties but nests {@link DurationSpec}, and one unserializable nested type + * makes Jackson throw for the whole setting. + * + * This is a bounded approximation. Property types are inspected erased, so a type hidden inside a + * property's own generic parameters (a {@code Set} field, say) is not caught here; check 1 of + * {@link #testCollectionSettingsRenderAsValidJson} covers that once the setting is populated. + * + * @param visiting types on the current path, so a cyclic reference does not recurse forever + */ + private static Class unrenderableType(Class type, Set> visiting) + { + if (type.isArray()) + return unrenderableType(type.getComponentType(), visiting); + if (type.isPrimitive() || type.isEnum() || type.isInterface()) + return null; // interfaces: the runtime type decides, cannot be judged statically + if (type.getName().startsWith("java.") || type.getName().startsWith("javax.")) + return null; + if (CharSequence.class.isAssignableFrom(type) || Number.class.isAssignableFrom(type) + || Boolean.class == type || Character.class == type) + return null; + if (!visiting.add(type)) + return null; // already being checked further up the path; a cycle is not itself a failure + + try + { + List> properties = new ArrayList<>(); + for (Method method : type.getMethods()) + { + if (method.isAnnotationPresent(JsonValue.class)) + return null; + if (Modifier.isStatic(method.getModifiers()) || method.getParameterCount() != 0 + || method.getReturnType() == void.class) + continue; + String name = method.getName(); + if ((name.startsWith("get") && name.length() > 3 && !name.equals("getClass")) + || (name.startsWith("is") && name.length() > 2 && (method.getReturnType() == boolean.class || method.getReturnType() == Boolean.class))) + properties.add(method.getReturnType()); + } + for (Field field : type.getFields()) + if (!Modifier.isStatic(field.getModifiers())) + properties.add(field.getType()); + + if (properties.isEmpty()) + return type; // Jackson discovers nothing to serialize and fails the enclosing value + + for (Class property : properties) + { + Class nestedTypes = unrenderableType(property, visiting); + if (nestedTypes != null) + return nestedTypes; + } + + return null; + } + finally + { + visiting.remove(type); + } + } }