Skip to content

Commit dbfcd52

Browse files
committed
Fix schema diffs when comments or security labels are modified
Patch by Sam Tunnicliffe; reviewed by Marcus Eriksson for CASSANDRA-21045
1 parent 209288c commit dbfcd52

File tree

7 files changed

+309
-22
lines changed

7 files changed

+309
-22
lines changed

src/java/org/apache/cassandra/cql3/ColumnSpecification.java

Lines changed: 1 addition & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -28,28 +28,17 @@
2828

2929
public class ColumnSpecification
3030
{
31-
protected static final String EMPTY_COMMENT = "";
32-
protected static final String EMPTY_SECURITY_LABEL = "";
3331
public final String ksName;
3432
public final String cfName;
3533
public final ColumnIdentifier name;
3634
public final AbstractType<?> type;
37-
public final String comment;
38-
public final String securityLabel;
3935

4036
public ColumnSpecification(String ksName, String cfName, ColumnIdentifier name, AbstractType<?> type)
41-
{
42-
this(ksName, cfName, name, type, EMPTY_COMMENT, EMPTY_SECURITY_LABEL);
43-
}
44-
45-
public ColumnSpecification(String ksName, String cfName, ColumnIdentifier name, AbstractType<?> type, String comment, String securityLabel)
4637
{
4738
this.ksName = ksName;
4839
this.cfName = cfName;
4940
this.name = name;
5041
this.type = type;
51-
this.comment = comment == null ? EMPTY_COMMENT : comment;
52-
this.securityLabel = securityLabel == null ? EMPTY_SECURITY_LABEL : securityLabel;
5342
}
5443

5544
/**
@@ -60,7 +49,7 @@ public ColumnSpecification(String ksName, String cfName, ColumnIdentifier name,
6049
*/
6150
public ColumnSpecification withAlias(ColumnIdentifier alias)
6251
{
63-
return new ColumnSpecification(ksName, cfName, alias, type, comment, securityLabel);
52+
return new ColumnSpecification(ksName, cfName, alias, type);
6453
}
6554

6655
public boolean isReversedType()
@@ -111,8 +100,6 @@ public String toString()
111100
return MoreObjects.toStringHelper(this)
112101
.add("name", name)
113102
.add("type", type)
114-
.add("comment", comment)
115-
.add("securityLabel", securityLabel)
116103
.toString();
117104
}
118105
}

src/java/org/apache/cassandra/db/marshal/UserType.java

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -366,7 +366,7 @@ public AbstractType<?> freezeNestedMulticellTypes()
366366
@Override
367367
public int hashCode()
368368
{
369-
return Objects.hashCode(keyspace, name, fieldNames, types, isMultiCell);
369+
return Objects.hashCode(keyspace, name, fieldNames, types, isMultiCell, comment, securityLabel, fieldComments, fieldSecurityLabels);
370370
}
371371

372372
@Override
@@ -413,11 +413,18 @@ private boolean equalsWithoutTypes(UserType other)
413413
return name.equals(other.name)
414414
&& fieldNames.equals(other.fieldNames)
415415
&& keyspace.equals(other.keyspace)
416-
&& isMultiCell == other.isMultiCell;
416+
&& isMultiCell == other.isMultiCell
417+
&& comment.equals(other.comment)
418+
&& securityLabel.equals(other.securityLabel)
419+
&& fieldComments.equals(other.fieldComments)
420+
&& fieldSecurityLabels.equals(other.fieldSecurityLabels);
417421
}
418422

419423
public boolean equalsWithOutKs(UserType other)
420424
{
425+
// Doesn't consider comments or security labels at either the
426+
// type or field level as this method is used to check compatibility of
427+
// UserTypes in different keyspaces for validation in CopyTableStatement
421428
return name.equals(other.name)
422429
&& fieldNames.equals(other.fieldNames)
423430
&& types.equals(other.types)

src/java/org/apache/cassandra/schema/ColumnMetadata.java

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,8 @@ public final class ColumnMetadata extends ColumnSpecification implements Selecta
7676

7777
public static final int NO_POSITION = -1;
7878
public static final int NO_UNIQUE_ID = Integer.MIN_VALUE;
79+
public static final String EMPTY_COMMENT = "";
80+
public static final String EMPTY_SECURITY_LABEL = "";
7981

8082
public enum ClusteringOrder
8183
{
@@ -118,6 +120,9 @@ public boolean isPrimaryKeyKind()
118120
private final int position;
119121
public final int uniqueId;
120122

123+
public final String comment;
124+
public final String securityLabel;
125+
121126
private final Comparator<CellPath> cellPathComparator;
122127
private final Comparator<Object> asymmetricCellPathComparator;
123128
private final Comparator<? super Cell<?>> cellComparator;
@@ -266,7 +271,7 @@ public ColumnMetadata(String ksName,
266271
String comment,
267272
String securityLabel)
268273
{
269-
super(ksName, cfName, name, type, comment, securityLabel);
274+
super(ksName, cfName, name, type);
270275
this.uniqueId = uniqueId;
271276
assert name != null && type != null && kind != null;
272277
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 ColumnMetadata(String ksName,
286291
this.mask = mask;
287292
this.columnConstraints = columnConstraints;
288293
this.columnConstraints.setColumnName(name);
294+
this.comment = comment;
295+
this.securityLabel = securityLabel;
289296
}
290297

291298
private static Comparator<CellPath> makeCellPathComparator(Kind kind, AbstractType<?> type)
@@ -458,7 +465,9 @@ private boolean equalsWithoutType(ColumnMetadata other)
458465
&& ksName.equals(other.ksName)
459466
&& cfName.equals(other.cfName)
460467
&& Objects.equals(mask, other.mask)
461-
&& Objects.equals(columnConstraints, other.columnConstraints);
468+
&& Objects.equals(columnConstraints, other.columnConstraints)
469+
&& comment.equals(other.comment)
470+
&& securityLabel.equals(other.securityLabel);
462471
}
463472

464473
Optional<Difference> compare(ColumnMetadata other)
@@ -490,6 +499,8 @@ public int hashCode()
490499
result = 31 * result + position;
491500
result = 31 * result + (mask == null ? 0 : mask.hashCode());
492501
result = 31 * result + (columnConstraints == null ? 0 : columnConstraints.hashCode());
502+
result = 31 * result + (comment == null ? 0 : comment.hashCode());
503+
result = 31 * result + (securityLabel == null ? 0 : securityLabel.hashCode());
493504
hash = result;
494505
}
495506
return result;
@@ -508,6 +519,10 @@ public String debugString()
508519
.add("type", type)
509520
.add("kind", kind)
510521
.add("position", position)
522+
.add("mask", mask)
523+
.add("columnConstraints", columnConstraints)
524+
.add("comment", comment)
525+
.add("securityLabel", securityLabel)
511526
.toString();
512527
}
513528

src/java/org/apache/cassandra/schema/KeyspaceParams.java

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -161,13 +161,17 @@ public boolean equals(Object o)
161161

162162
KeyspaceParams p = (KeyspaceParams) o;
163163

164-
return durableWrites == p.durableWrites && replication.equals(p.replication) && fastPath.equals(p.fastPath);
164+
return durableWrites == p.durableWrites
165+
&& replication.equals(p.replication)
166+
&& fastPath.equals(p.fastPath)
167+
&& comment.equals(p.comment)
168+
&& securityLabel.equals(p.securityLabel);
165169
}
166170

167171
@Override
168172
public int hashCode()
169173
{
170-
return Objects.hashCode(durableWrites, replication, fastPath);
174+
return Objects.hashCode(durableWrites, replication, fastPath, comment, securityLabel);
171175
}
172176

173177
@Override
Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing, software
13+
* distributed under the License is distributed on an "AS IS" BASIS,
14+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
* See the License for the specific language governing permissions and
16+
* limitations under the License.
17+
*/
18+
19+
package org.apache.cassandra.cql3;
20+
21+
import java.io.IOException;
22+
import java.util.List;
23+
24+
import org.junit.Before;
25+
import org.junit.BeforeClass;
26+
import org.junit.Test;
27+
28+
import org.apache.cassandra.ServerTestUtils;
29+
import org.apache.cassandra.db.ConsistencyLevel;
30+
import org.apache.cassandra.transport.ClientNotificationsTest;
31+
import org.apache.cassandra.transport.Event;
32+
import org.apache.cassandra.transport.ProtocolVersion;
33+
import org.apache.cassandra.transport.SimpleClient;
34+
import org.apache.cassandra.transport.messages.RegisterMessage;
35+
import org.apache.cassandra.transport.messages.ResultMessage;
36+
37+
import static java.lang.String.format;
38+
import static org.apache.cassandra.transport.Event.SchemaChange.Change.UPDATED;
39+
import static org.apache.cassandra.transport.Event.SchemaChange.Target.TABLE;
40+
import static org.apache.cassandra.transport.Event.SchemaChange.Target.TYPE;
41+
import static org.junit.Assert.assertEquals;
42+
43+
public class CommentAndSecurityLabelClientTest extends CQLTester
44+
{
45+
private static final String KEYSPACE_NAME = "ks_comment";
46+
private static final String TABLE_NAME = "tbl_comment";
47+
private static final String COLUMN_NAME = "name";
48+
private static final String TYPE_NAME = "type_comment";
49+
private static final String FIELD_NAME = "f1";
50+
51+
@BeforeClass
52+
public static void setUpClass()
53+
{
54+
ServerTestUtils.daemonInitialization();
55+
CQLTester.setUpClass();
56+
}
57+
58+
@Before
59+
public void setup()
60+
{
61+
requireNetwork();
62+
}
63+
64+
@Test
65+
public void testClientResponses() throws IOException
66+
{
67+
createSchema();
68+
try (SimpleClient client = newSimpleClient(ProtocolVersion.CURRENT))
69+
{
70+
// Register to receive schema change notifications, the client uses a simple event handler to
71+
// record and inspect these.
72+
client.execute(new RegisterMessage(List.of(Event.Type.SCHEMA_CHANGE)));
73+
ClientNotificationsTest.EventHandler handler = new ClientNotificationsTest.EventHandler();
74+
client.setEventHandler(handler);
75+
76+
// Comments
77+
makeRequestAndVerify(client, handler,
78+
format("COMMENT ON KEYSPACE %s IS 'test comment'", KEYSPACE_NAME),
79+
new Event.SchemaChange(UPDATED, KEYSPACE_NAME));
80+
makeRequestAndVerify(client, handler,
81+
format("COMMENT ON TABLE %s.%s IS 'test comment'", KEYSPACE_NAME, TABLE_NAME),
82+
new Event.SchemaChange(UPDATED, TABLE, KEYSPACE_NAME, TABLE_NAME));
83+
makeRequestAndVerify(client, handler,
84+
format("COMMENT ON COLUMN %s.%s.%s IS 'test comment'", KEYSPACE_NAME, TABLE_NAME, COLUMN_NAME),
85+
new Event.SchemaChange(UPDATED, TABLE, KEYSPACE_NAME, TABLE_NAME));
86+
makeRequestAndVerify(client, handler,
87+
format("COMMENT ON TYPE %s.%s IS 'test comment'", KEYSPACE_NAME, TYPE_NAME),
88+
new Event.SchemaChange(UPDATED, TYPE, KEYSPACE_NAME, TYPE_NAME));
89+
makeRequestAndVerify(client, handler,
90+
format("COMMENT ON FIELD %s.%s.%s IS 'test comment'", KEYSPACE_NAME, TYPE_NAME, FIELD_NAME),
91+
new Event.SchemaChange(UPDATED, TYPE, KEYSPACE_NAME, TYPE_NAME));
92+
93+
// Security labels
94+
makeRequestAndVerify(client, handler,
95+
format("SECURITY LABEL ON KEYSPACE %s IS 'test label'", KEYSPACE_NAME),
96+
new Event.SchemaChange(UPDATED, KEYSPACE_NAME));
97+
makeRequestAndVerify(client, handler,
98+
format("SECURITY LABEL ON TABLE %s.%s IS 'test label'", KEYSPACE_NAME, TABLE_NAME),
99+
new Event.SchemaChange(UPDATED, TABLE, KEYSPACE_NAME, TABLE_NAME));
100+
makeRequestAndVerify(client, handler,
101+
format("SECURITY LABEL ON COLUMN %s.%s.%s IS 'test label'", KEYSPACE_NAME, TABLE_NAME, COLUMN_NAME),
102+
new Event.SchemaChange(UPDATED, TABLE, KEYSPACE_NAME, TABLE_NAME));
103+
makeRequestAndVerify(client, handler,
104+
format("SECURITY LABEL ON TYPE %s.%s IS 'test label'", KEYSPACE_NAME, TYPE_NAME),
105+
new Event.SchemaChange(UPDATED, TYPE, KEYSPACE_NAME, TYPE_NAME));
106+
makeRequestAndVerify(client, handler,
107+
format("SECURITY LABEL ON FIELD %s.%s.%s IS 'test label'", KEYSPACE_NAME, TYPE_NAME, FIELD_NAME),
108+
new Event.SchemaChange(UPDATED, TYPE, KEYSPACE_NAME, TYPE_NAME));
109+
}
110+
}
111+
112+
private void makeRequestAndVerify(SimpleClient client,
113+
ClientNotificationsTest.EventHandler handler,
114+
String cql,
115+
Event.SchemaChange expected)
116+
{
117+
// Assert that the correct response type and content is received
118+
ResultMessage result = client.execute(cql, ConsistencyLevel.ONE);
119+
assertEquals(result.kind, ResultMessage.Kind.SCHEMA_CHANGE);
120+
ResultMessage.SchemaChange message = (ResultMessage.SchemaChange) result;
121+
assertEquals(message.change, expected);
122+
// Verify that the expected notification was received and passed to the event handler
123+
handler.assertNextEvent(expected);
124+
}
125+
126+
private void createSchema()
127+
{
128+
createKeyspace(format("CREATE KEYSPACE %s WITH replication = {'class': 'SimpleStrategy', 'replication_factor': '1'}", KEYSPACE_NAME));
129+
createTable(format("CREATE TABLE %s.%s (id int PRIMARY KEY, name text)", KEYSPACE_NAME, TABLE_NAME));
130+
execute(format("CREATE TYPE %s.%s (%s int)", KEYSPACE_NAME, TYPE_NAME, FIELD_NAME));
131+
}
132+
}

0 commit comments

Comments
 (0)