From dbfcd52fd52dd2053844244b53e563d9d7a67065 Mon Sep 17 00:00:00 2001 From: Sam Tunnicliffe Date: Wed, 26 Nov 2025 17:30:24 +0000 Subject: [PATCH] Fix schema diffs when comments or security labels are modified Patch by Sam Tunnicliffe; reviewed by Marcus Eriksson for CASSANDRA-21045 --- .../cassandra/cql3/ColumnSpecification.java | 15 +- .../apache/cassandra/db/marshal/UserType.java | 11 +- .../cassandra/schema/ColumnMetadata.java | 19 ++- .../cassandra/schema/KeyspaceParams.java | 8 +- .../CommentAndSecurityLabelClientTest.java | 132 ++++++++++++++++ .../cassandra/schema/SchemaChangesTest.java | 142 ++++++++++++++++++ ...Test.java => ClientNotificationsTest.java} | 4 +- 7 files changed, 309 insertions(+), 22 deletions(-) create mode 100644 test/unit/org/apache/cassandra/cql3/CommentAndSecurityLabelClientTest.java rename test/unit/org/apache/cassandra/transport/{ClientNotificiationsTest.java => ClientNotificationsTest.java} (97%) diff --git a/src/java/org/apache/cassandra/cql3/ColumnSpecification.java b/src/java/org/apache/cassandra/cql3/ColumnSpecification.java index 863f9eb5b8..8cf869bd38 100644 --- a/src/java/org/apache/cassandra/cql3/ColumnSpecification.java +++ b/src/java/org/apache/cassandra/cql3/ColumnSpecification.java @@ -28,28 +28,17 @@ import java.util.Iterator; public class ColumnSpecification { - protected static final String EMPTY_COMMENT = ""; - protected static final String EMPTY_SECURITY_LABEL = ""; public final String ksName; public final String cfName; public final ColumnIdentifier name; public final AbstractType type; - public final String comment; - public final String securityLabel; public ColumnSpecification(String ksName, String cfName, ColumnIdentifier name, AbstractType type) - { - this(ksName, cfName, name, type, EMPTY_COMMENT, EMPTY_SECURITY_LABEL); - } - - public ColumnSpecification(String ksName, String cfName, ColumnIdentifier name, AbstractType type, String comment, String securityLabel) { this.ksName = ksName; this.cfName = cfName; this.name = name; this.type = type; - this.comment = comment == null ? EMPTY_COMMENT : comment; - this.securityLabel = securityLabel == null ? EMPTY_SECURITY_LABEL : securityLabel; } /** @@ -60,7 +49,7 @@ public class ColumnSpecification */ public ColumnSpecification withAlias(ColumnIdentifier alias) { - return new ColumnSpecification(ksName, cfName, alias, type, comment, securityLabel); + return new ColumnSpecification(ksName, cfName, alias, type); } public boolean isReversedType() @@ -111,8 +100,6 @@ public class ColumnSpecification return MoreObjects.toStringHelper(this) .add("name", name) .add("type", type) - .add("comment", comment) - .add("securityLabel", securityLabel) .toString(); } } diff --git a/src/java/org/apache/cassandra/db/marshal/UserType.java b/src/java/org/apache/cassandra/db/marshal/UserType.java index 01d1d6a64e..23b172c816 100644 --- a/src/java/org/apache/cassandra/db/marshal/UserType.java +++ b/src/java/org/apache/cassandra/db/marshal/UserType.java @@ -366,7 +366,7 @@ public class UserType extends TupleType implements SchemaElement @Override public int hashCode() { - return Objects.hashCode(keyspace, name, fieldNames, types, isMultiCell); + return Objects.hashCode(keyspace, name, fieldNames, types, isMultiCell, comment, securityLabel, fieldComments, fieldSecurityLabels); } @Override @@ -413,11 +413,18 @@ public class UserType extends TupleType implements SchemaElement return name.equals(other.name) && fieldNames.equals(other.fieldNames) && keyspace.equals(other.keyspace) - && isMultiCell == other.isMultiCell; + && isMultiCell == other.isMultiCell + && comment.equals(other.comment) + && securityLabel.equals(other.securityLabel) + && fieldComments.equals(other.fieldComments) + && fieldSecurityLabels.equals(other.fieldSecurityLabels); } public boolean equalsWithOutKs(UserType other) { + // Doesn't consider comments or security labels at either the + // type or field level as this method is used to check compatibility of + // UserTypes in different keyspaces for validation in CopyTableStatement return name.equals(other.name) && fieldNames.equals(other.fieldNames) && types.equals(other.types) diff --git a/src/java/org/apache/cassandra/schema/ColumnMetadata.java b/src/java/org/apache/cassandra/schema/ColumnMetadata.java index 69cb56bc59..d6ffa1c1ca 100644 --- a/src/java/org/apache/cassandra/schema/ColumnMetadata.java +++ b/src/java/org/apache/cassandra/schema/ColumnMetadata.java @@ -76,6 +76,8 @@ public final class ColumnMetadata extends ColumnSpecification implements Selecta public static final int NO_POSITION = -1; public static final int NO_UNIQUE_ID = Integer.MIN_VALUE; + public static final String EMPTY_COMMENT = ""; + public static final String EMPTY_SECURITY_LABEL = ""; public enum ClusteringOrder { @@ -118,6 +120,9 @@ public final class ColumnMetadata extends ColumnSpecification implements Selecta private final int position; public final int uniqueId; + public final String comment; + public final String securityLabel; + private final Comparator cellPathComparator; private final Comparator asymmetricCellPathComparator; private final Comparator> cellComparator; @@ -266,7 +271,7 @@ public final class ColumnMetadata extends ColumnSpecification implements Selecta String comment, String securityLabel) { - super(ksName, cfName, name, type, comment, securityLabel); + super(ksName, cfName, name, type); this.uniqueId = uniqueId; assert name != null && type != null && kind != null; assert (position == NO_POSITION) == !kind.isPrimaryKeyKind(); // The position really only make sense for partition and clustering columns (and those must have one), @@ -286,6 +291,8 @@ public final class ColumnMetadata extends ColumnSpecification implements Selecta this.mask = mask; this.columnConstraints = columnConstraints; this.columnConstraints.setColumnName(name); + this.comment = comment; + this.securityLabel = securityLabel; } private static Comparator makeCellPathComparator(Kind kind, AbstractType type) @@ -458,7 +465,9 @@ public final class ColumnMetadata extends ColumnSpecification implements Selecta && ksName.equals(other.ksName) && cfName.equals(other.cfName) && Objects.equals(mask, other.mask) - && Objects.equals(columnConstraints, other.columnConstraints); + && Objects.equals(columnConstraints, other.columnConstraints) + && comment.equals(other.comment) + && securityLabel.equals(other.securityLabel); } Optional compare(ColumnMetadata other) @@ -490,6 +499,8 @@ public final class ColumnMetadata extends ColumnSpecification implements Selecta result = 31 * result + position; result = 31 * result + (mask == null ? 0 : mask.hashCode()); result = 31 * result + (columnConstraints == null ? 0 : columnConstraints.hashCode()); + result = 31 * result + (comment == null ? 0 : comment.hashCode()); + result = 31 * result + (securityLabel == null ? 0 : securityLabel.hashCode()); hash = result; } return result; @@ -508,6 +519,10 @@ public final class ColumnMetadata extends ColumnSpecification implements Selecta .add("type", type) .add("kind", kind) .add("position", position) + .add("mask", mask) + .add("columnConstraints", columnConstraints) + .add("comment", comment) + .add("securityLabel", securityLabel) .toString(); } diff --git a/src/java/org/apache/cassandra/schema/KeyspaceParams.java b/src/java/org/apache/cassandra/schema/KeyspaceParams.java index ffc3167358..35b68bbf3f 100644 --- a/src/java/org/apache/cassandra/schema/KeyspaceParams.java +++ b/src/java/org/apache/cassandra/schema/KeyspaceParams.java @@ -161,13 +161,17 @@ public final class KeyspaceParams KeyspaceParams p = (KeyspaceParams) o; - return durableWrites == p.durableWrites && replication.equals(p.replication) && fastPath.equals(p.fastPath); + return durableWrites == p.durableWrites + && replication.equals(p.replication) + && fastPath.equals(p.fastPath) + && comment.equals(p.comment) + && securityLabel.equals(p.securityLabel); } @Override public int hashCode() { - return Objects.hashCode(durableWrites, replication, fastPath); + return Objects.hashCode(durableWrites, replication, fastPath, comment, securityLabel); } @Override diff --git a/test/unit/org/apache/cassandra/cql3/CommentAndSecurityLabelClientTest.java b/test/unit/org/apache/cassandra/cql3/CommentAndSecurityLabelClientTest.java new file mode 100644 index 0000000000..07735b7cc6 --- /dev/null +++ b/test/unit/org/apache/cassandra/cql3/CommentAndSecurityLabelClientTest.java @@ -0,0 +1,132 @@ +/* + * 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.cql3; + +import java.io.IOException; +import java.util.List; + +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +import org.apache.cassandra.ServerTestUtils; +import org.apache.cassandra.db.ConsistencyLevel; +import org.apache.cassandra.transport.ClientNotificationsTest; +import org.apache.cassandra.transport.Event; +import org.apache.cassandra.transport.ProtocolVersion; +import org.apache.cassandra.transport.SimpleClient; +import org.apache.cassandra.transport.messages.RegisterMessage; +import org.apache.cassandra.transport.messages.ResultMessage; + +import static java.lang.String.format; +import static org.apache.cassandra.transport.Event.SchemaChange.Change.UPDATED; +import static org.apache.cassandra.transport.Event.SchemaChange.Target.TABLE; +import static org.apache.cassandra.transport.Event.SchemaChange.Target.TYPE; +import static org.junit.Assert.assertEquals; + +public class CommentAndSecurityLabelClientTest extends CQLTester +{ + private static final String KEYSPACE_NAME = "ks_comment"; + private static final String TABLE_NAME = "tbl_comment"; + private static final String COLUMN_NAME = "name"; + private static final String TYPE_NAME = "type_comment"; + private static final String FIELD_NAME = "f1"; + + @BeforeClass + public static void setUpClass() + { + ServerTestUtils.daemonInitialization(); + CQLTester.setUpClass(); + } + + @Before + public void setup() + { + requireNetwork(); + } + + @Test + public void testClientResponses() throws IOException + { + createSchema(); + try (SimpleClient client = newSimpleClient(ProtocolVersion.CURRENT)) + { + // Register to receive schema change notifications, the client uses a simple event handler to + // record and inspect these. + client.execute(new RegisterMessage(List.of(Event.Type.SCHEMA_CHANGE))); + ClientNotificationsTest.EventHandler handler = new ClientNotificationsTest.EventHandler(); + client.setEventHandler(handler); + + // Comments + makeRequestAndVerify(client, handler, + format("COMMENT ON KEYSPACE %s IS 'test comment'", KEYSPACE_NAME), + new Event.SchemaChange(UPDATED, KEYSPACE_NAME)); + makeRequestAndVerify(client, handler, + format("COMMENT ON TABLE %s.%s IS 'test comment'", KEYSPACE_NAME, TABLE_NAME), + new Event.SchemaChange(UPDATED, TABLE, KEYSPACE_NAME, TABLE_NAME)); + makeRequestAndVerify(client, handler, + format("COMMENT ON COLUMN %s.%s.%s IS 'test comment'", KEYSPACE_NAME, TABLE_NAME, COLUMN_NAME), + new Event.SchemaChange(UPDATED, TABLE, KEYSPACE_NAME, TABLE_NAME)); + makeRequestAndVerify(client, handler, + format("COMMENT ON TYPE %s.%s IS 'test comment'", KEYSPACE_NAME, TYPE_NAME), + new Event.SchemaChange(UPDATED, TYPE, KEYSPACE_NAME, TYPE_NAME)); + makeRequestAndVerify(client, handler, + format("COMMENT ON FIELD %s.%s.%s IS 'test comment'", KEYSPACE_NAME, TYPE_NAME, FIELD_NAME), + new Event.SchemaChange(UPDATED, TYPE, KEYSPACE_NAME, TYPE_NAME)); + + // Security labels + makeRequestAndVerify(client, handler, + format("SECURITY LABEL ON KEYSPACE %s IS 'test label'", KEYSPACE_NAME), + new Event.SchemaChange(UPDATED, KEYSPACE_NAME)); + makeRequestAndVerify(client, handler, + format("SECURITY LABEL ON TABLE %s.%s IS 'test label'", KEYSPACE_NAME, TABLE_NAME), + new Event.SchemaChange(UPDATED, TABLE, KEYSPACE_NAME, TABLE_NAME)); + makeRequestAndVerify(client, handler, + format("SECURITY LABEL ON COLUMN %s.%s.%s IS 'test label'", KEYSPACE_NAME, TABLE_NAME, COLUMN_NAME), + new Event.SchemaChange(UPDATED, TABLE, KEYSPACE_NAME, TABLE_NAME)); + makeRequestAndVerify(client, handler, + format("SECURITY LABEL ON TYPE %s.%s IS 'test label'", KEYSPACE_NAME, TYPE_NAME), + new Event.SchemaChange(UPDATED, TYPE, KEYSPACE_NAME, TYPE_NAME)); + makeRequestAndVerify(client, handler, + format("SECURITY LABEL ON FIELD %s.%s.%s IS 'test label'", KEYSPACE_NAME, TYPE_NAME, FIELD_NAME), + new Event.SchemaChange(UPDATED, TYPE, KEYSPACE_NAME, TYPE_NAME)); + } + } + + private void makeRequestAndVerify(SimpleClient client, + ClientNotificationsTest.EventHandler handler, + String cql, + Event.SchemaChange expected) + { + // Assert that the correct response type and content is received + ResultMessage result = client.execute(cql, ConsistencyLevel.ONE); + assertEquals(result.kind, ResultMessage.Kind.SCHEMA_CHANGE); + ResultMessage.SchemaChange message = (ResultMessage.SchemaChange) result; + assertEquals(message.change, expected); + // Verify that the expected notification was received and passed to the event handler + handler.assertNextEvent(expected); + } + + private void createSchema() + { + createKeyspace(format("CREATE KEYSPACE %s WITH replication = {'class': 'SimpleStrategy', 'replication_factor': '1'}", KEYSPACE_NAME)); + createTable(format("CREATE TABLE %s.%s (id int PRIMARY KEY, name text)", KEYSPACE_NAME, TABLE_NAME)); + execute(format("CREATE TYPE %s.%s (%s int)", KEYSPACE_NAME, TYPE_NAME, FIELD_NAME)); + } +} diff --git a/test/unit/org/apache/cassandra/schema/SchemaChangesTest.java b/test/unit/org/apache/cassandra/schema/SchemaChangesTest.java index c2d3796a35..9d3223aa73 100644 --- a/test/unit/org/apache/cassandra/schema/SchemaChangesTest.java +++ b/test/unit/org/apache/cassandra/schema/SchemaChangesTest.java @@ -20,11 +20,16 @@ package org.apache.cassandra.schema; import java.nio.ByteBuffer; import java.util.HashMap; +import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.function.Supplier; import com.google.common.collect.ImmutableMap; +import org.apache.cassandra.cql3.FieldIdentifier; +import org.apache.cassandra.db.marshal.Int32Type; +import org.apache.cassandra.db.marshal.UserType; import org.apache.cassandra.exceptions.InvalidRequestException; import org.junit.BeforeClass; import org.junit.Rule; @@ -49,6 +54,7 @@ import org.apache.cassandra.io.sstable.Descriptor; import org.apache.cassandra.io.sstable.format.SSTableFormat.Components; import org.apache.cassandra.io.util.File; import org.apache.cassandra.locator.NetworkTopologyStrategy; +import org.apache.cassandra.utils.ByteBufferUtil; import org.apache.cassandra.utils.FBUtilities; import static org.apache.cassandra.Util.throwAssert; @@ -582,6 +588,142 @@ public class SchemaChangesTest assertEquals(keyspace1, diff.altered.get(0).after); } + @Test + public void testColumnComment() + { + TableMetadata t1 = addTestTable("ks1", "t", ""); + ColumnIdentifier col = new ColumnIdentifier("col", false); + assertNotNull(t1.getColumn(col)); + TableMetadata t2 = t1.unbuild().alterColumnComment(col, "added comment").build(); + verifyTableDifferencesDetected(t1, t2); + } + + @Test + public void testColumnSecurityLabel() + { + TableMetadata t1 = addTestTable("ks1", "t", ""); + ColumnIdentifier col = new ColumnIdentifier("col", false); + assertNotNull(t1.getColumn(col)); + TableMetadata t2 = t1.unbuild().alterColumnSecurityLabel(col, "added security label").build(); + verifyTableDifferencesDetected(t1, t2); + } + + @Test + public void testTableComment() + { + TableMetadata t1 = addTestTable("ks1", "t", ""); + assertEquals("", t1.params.comment); + TableMetadata t2 = t1.unbuild().comment("added comment").build(); + verifyTableDifferencesDetected(t1, t2); + } + + @Test + public void testTableSecurityLabel() + { + TableMetadata t1 = addTestTable("ks1", "t", ""); + assertEquals("", t1.params.comment); + TableMetadata t2 = t1.unbuild().securityLabel("added security label").build(); + verifyTableDifferencesDetected(t1, t2); + } + + @Test + public void testKeyspaceComment() + { + KeyspaceMetadata ks1 = KeyspaceMetadata.create("ks1", KeyspaceParams.simple(1)); + KeyspaceMetadata ks2 = ks1.withSwapped(ks1.params.withComment("added comment")); + verifyKeyspaceDifferencesDetected(ks1, ks2); + } + + @Test + public void testKeyspaceSecurityLabel() + { + KeyspaceMetadata ks1 = KeyspaceMetadata.create("ks1", KeyspaceParams.simple(1)); + KeyspaceMetadata ks2 = ks1.withSwapped(ks1.params.withSecurityLabel("added security label")); + verifyKeyspaceDifferencesDetected(ks1, ks2); + } + + @Test + public void testUserTypeComment() + { + UserType t1 = new UserType("ks1", + ByteBufferUtil.bytes("type_1"), + List.of(FieldIdentifier.forUnquoted("f1"), FieldIdentifier.forUnquoted("f2")), + List.of(Int32Type.instance, Int32Type.instance), + true); + UserType t2 = t1.withComment("added comment"); + verifyUserTypeDifferencesDetected(t1, t2); + } + + @Test + public void testUserTypeSecurityLabel() + { + UserType t1 = new UserType("ks1", + ByteBufferUtil.bytes("type_1"), + List.of(FieldIdentifier.forUnquoted("f1"), FieldIdentifier.forUnquoted("f2")), + List.of(Int32Type.instance, Int32Type.instance), + true); + UserType t2 = t1.withSecurityLabel("added comment"); + verifyUserTypeDifferencesDetected(t1, t2); + } + + @Test + public void testUserTypeFieldComment() + { + UserType t1 = new UserType("ks1", + ByteBufferUtil.bytes("type_1"), + List.of(FieldIdentifier.forUnquoted("f1"), FieldIdentifier.forUnquoted("f2")), + List.of(Int32Type.instance, Int32Type.instance), + true); + UserType t2 = t1.withFieldComment(FieldIdentifier.forUnquoted("f1"), "added comment"); + verifyUserTypeDifferencesDetected(t1, t2); + } + + @Test + public void testUserTypeFieldSecurityLabel() + { + UserType t1 = new UserType("ks1", + ByteBufferUtil.bytes("type_1"), + List.of(FieldIdentifier.forUnquoted("f1"), FieldIdentifier.forUnquoted("f2")), + List.of(Int32Type.instance, Int32Type.instance), + true); + UserType t2 = t1.withFieldSecurityLabel(FieldIdentifier.forUnquoted("f2"), "added comment"); + verifyUserTypeDifferencesDetected(t1, t2); + } + + private void verifyUserTypeDifferencesDetected(UserType t1, UserType t2) + { + assertFalse(t1.equals(t2)); + Types types1 = Types.of(t1); + Types types2 = Types.of(t2); + Types.TypesDiff diff = Types.diff(types1, types2); + assertEquals(1, diff.altered.size()); + Diff.Altered altered = diff.altered.iterator().next(); + assertEquals(t1, altered.before); + assertEquals(t2, altered.after); + } + + private void verifyKeyspaceDifferencesDetected(KeyspaceMetadata ks1, KeyspaceMetadata ks2) + { + assertFalse(ks1.equals(ks2)); + Optional opt = KeyspaceMetadata.diff(ks1, ks2); + assertTrue(opt.isPresent()); + KeyspaceMetadata.KeyspaceDiff diff = opt.get(); + assertEquals(ks1, diff.before); + assertEquals(ks2, diff.after); + } + + private static void verifyTableDifferencesDetected(TableMetadata t1, TableMetadata t2) + { + assertFalse(t1.equals(t2)); + Tables tables1 = Tables.of(t1); + Tables tables2 = Tables.of(t2); + Tables.TablesDiff diff = Tables.diff(tables1, tables2); + assertEquals(1, diff.altered.size()); + Diff.Altered altered = diff.altered.iterator().next(); + assertEquals(altered.before, t1); + assertEquals(altered.after, t2); + } + private TableMetadata addTestTable(String ks, String cf, String comment) { return diff --git a/test/unit/org/apache/cassandra/transport/ClientNotificiationsTest.java b/test/unit/org/apache/cassandra/transport/ClientNotificationsTest.java similarity index 97% rename from test/unit/org/apache/cassandra/transport/ClientNotificiationsTest.java rename to test/unit/org/apache/cassandra/transport/ClientNotificationsTest.java index 9f95784e43..4c77444d48 100644 --- a/test/unit/org/apache/cassandra/transport/ClientNotificiationsTest.java +++ b/test/unit/org/apache/cassandra/transport/ClientNotificationsTest.java @@ -37,7 +37,7 @@ import org.apache.cassandra.utils.FBUtilities; import static org.junit.Assert.assertEquals; @RunWith(Parameterized.class) -public class ClientNotificiationsTest extends CQLTester +public class ClientNotificationsTest extends CQLTester { private static Server.EventNotifier notifier = new Server.EventNotifier(); @@ -101,7 +101,7 @@ public class ClientNotificiationsTest extends CQLTester } } - static class EventHandler extends SimpleClient.SimpleEventHandler + public static class EventHandler extends SimpleClient.SimpleEventHandler { public void assertNextEvent(Event expected) {