Skip to content

Commit 352dc19

Browse files
committed
fix: invalidate prepared cache on UDT changes
Match changed UDTs by keyspace and type name instead of full field equality. Result and variable metadata can contain a different field shape after ALTER TYPE, so full UDT equality is too strict and leaves affected prepared statements cached. Ignore type-create events that do not have an old type, add a unit regression for nested UDT matching, and make PreparedStatementCachingIT retain cached futures so it validates invalidation rather than racing weak-value collection.
1 parent 19b1cbb commit 352dc19

3 files changed

Lines changed: 69 additions & 16 deletions

File tree

core/src/main/java/com/datastax/oss/driver/internal/core/cql/CqlPrepareAsyncProcessor.java

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,12 +83,12 @@ protected CqlPrepareAsyncProcessor(
8383
});
8484
}
8585

86-
private static boolean typeMatches(UserDefinedType oldType, DataType typeToCheck) {
86+
static boolean typeMatches(UserDefinedType oldType, DataType typeToCheck) {
8787

8888
switch (typeToCheck.getProtocolCode()) {
8989
case ProtocolConstants.DataType.UDT:
9090
UserDefinedType udtType = (UserDefinedType) typeToCheck;
91-
return udtType.equals(oldType)
91+
return sameTypeName(udtType, oldType)
9292
? true
9393
: Iterables.any(udtType.getFieldTypes(), (testType) -> typeMatches(oldType, testType));
9494
case ProtocolConstants.DataType.LIST:
@@ -110,7 +110,14 @@ private static boolean typeMatches(UserDefinedType oldType, DataType typeToCheck
110110
}
111111
}
112112

113+
private static boolean sameTypeName(UserDefinedType left, UserDefinedType right) {
114+
return left.getKeyspace().equals(right.getKeyspace()) && left.getName().equals(right.getName());
115+
}
116+
113117
private void onTypeChanged(TypeChangeEvent event) {
118+
if (event.oldType == null) {
119+
return;
120+
}
114121
for (Map.Entry<PrepareRequest, CompletableFuture<PreparedStatement>> entry :
115122
this.cache.asMap().entrySet()) {
116123

core/src/test/java/com/datastax/oss/driver/internal/core/cql/CqlPrepareAsyncProcessorTest.java

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,9 @@
2121

2222
import com.datastax.oss.driver.api.core.cql.PrepareRequest;
2323
import com.datastax.oss.driver.api.core.cql.PreparedStatement;
24+
import com.datastax.oss.driver.api.core.type.DataTypes;
25+
import com.datastax.oss.driver.api.core.type.UserDefinedType;
26+
import com.datastax.oss.driver.internal.core.type.UserDefinedTypeBuilder;
2427
import com.datastax.oss.driver.shaded.guava.common.cache.Cache;
2528
import java.util.Optional;
2629
import java.util.concurrent.CompletableFuture;
@@ -85,4 +88,22 @@ public void should_return_defensive_copy_when_future_is_in_flight() throws Excep
8588
returned.toCompletableFuture().cancel(false);
8689
assertThat(inFlight.isCancelled()).isFalse();
8790
}
91+
92+
@Test
93+
public void should_match_udt_by_name_when_field_definitions_differ() {
94+
UserDefinedType oldType =
95+
new UserDefinedTypeBuilder("ks", "test_type_2")
96+
.withField("c", DataTypes.INT)
97+
.withField("d", DataTypes.TEXT)
98+
.build();
99+
UserDefinedType resultType =
100+
new UserDefinedTypeBuilder("ks", "test_type_2")
101+
.withField("c", DataTypes.INT)
102+
.withField("d", DataTypes.TEXT)
103+
.withField("i", DataTypes.BLOB)
104+
.build();
105+
106+
assertThat(CqlPrepareAsyncProcessor.typeMatches(oldType, DataTypes.listOf(resultType)))
107+
.isTrue();
108+
}
88109
}

integration-tests/src/test/java/com/datastax/oss/driver/core/cql/PreparedStatementCachingIT.java

Lines changed: 39 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -18,12 +18,14 @@
1818
package com.datastax.oss.driver.core.cql;
1919

2020
import static org.assertj.core.api.Assertions.assertThat;
21+
import static org.awaitility.Awaitility.await;
2122

2223
import com.codahale.metrics.Gauge;
2324
import com.datastax.oss.driver.api.core.CqlSession;
2425
import com.datastax.oss.driver.api.core.config.DefaultDriverOption;
2526
import com.datastax.oss.driver.api.core.config.DriverConfigLoader;
2627
import com.datastax.oss.driver.api.core.context.DriverContext;
28+
import com.datastax.oss.driver.api.core.cql.PrepareRequest;
2729
import com.datastax.oss.driver.api.core.cql.PreparedStatement;
2830
import com.datastax.oss.driver.api.core.metrics.DefaultSessionMetric;
2931
import com.datastax.oss.driver.api.core.session.ProgrammaticArguments;
@@ -34,10 +36,12 @@
3436
import com.datastax.oss.driver.api.testinfra.session.SessionUtils;
3537
import com.datastax.oss.driver.categories.IsolatedTests;
3638
import com.datastax.oss.driver.internal.core.context.DefaultDriverContext;
39+
import com.datastax.oss.driver.internal.core.context.InternalDriverContext;
3740
import com.datastax.oss.driver.internal.core.cql.CqlPrepareAsyncProcessor;
3841
import com.datastax.oss.driver.internal.core.cql.CqlPrepareSyncProcessor;
3942
import com.datastax.oss.driver.internal.core.metadata.schema.events.TypeChangeEvent;
4043
import com.datastax.oss.driver.internal.core.session.BuiltInRequestProcessors;
44+
import com.datastax.oss.driver.internal.core.session.DefaultSession;
4145
import com.datastax.oss.driver.internal.core.session.RequestProcessor;
4246
import com.datastax.oss.driver.internal.core.session.RequestProcessorRegistry;
4347
import com.datastax.oss.driver.shaded.guava.common.cache.RemovalListener;
@@ -53,6 +57,7 @@
5357
import java.util.Optional;
5458
import java.util.Set;
5559
import java.util.concurrent.CompletableFuture;
60+
import java.util.concurrent.CompletionStage;
5661
import java.util.concurrent.ConcurrentHashMap;
5762
import java.util.concurrent.CountDownLatch;
5863
import java.util.concurrent.TimeUnit;
@@ -117,6 +122,9 @@ private static class TestCqlPrepareAsyncProcessor extends CqlPrepareAsyncProcess
117122
private static final Logger LOG =
118123
LoggerFactory.getLogger(PreparedStatementCachingIT.TestCqlPrepareAsyncProcessor.class);
119124

125+
private final Set<CompletableFuture<PreparedStatement>> retainedCacheValues =
126+
ConcurrentHashMap.newKeySet();
127+
120128
private static RemovalListener<Object, Object> buildCacheRemoveCallback(
121129
@NonNull Optional<DefaultDriverContext> context) {
122130
return (evt) -> {
@@ -133,10 +141,25 @@ private static RemovalListener<Object, Object> buildCacheRemoveCallback(
133141
}
134142

135143
public TestCqlPrepareAsyncProcessor(@NonNull Optional<DefaultDriverContext> context) {
136-
// Default CqlPrepareAsyncProcessor uses weak values here as well. We avoid doing so
137-
// to prevent cache entries from unexpectedly disappearing mid-test.
144+
// Default CqlPrepareAsyncProcessor uses weak values. Retain the cached futures so this test
145+
// validates type-change invalidation instead of racing cache value collection.
138146
super(context, builder -> builder.removalListener(buildCacheRemoveCallback(context)));
139147
}
148+
149+
@Override
150+
public CompletionStage<PreparedStatement> process(
151+
PrepareRequest request,
152+
DefaultSession session,
153+
InternalDriverContext context,
154+
String sessionLogPrefix) {
155+
CompletionStage<PreparedStatement> stage =
156+
super.process(request, session, context, sessionLogPrefix);
157+
CompletableFuture<PreparedStatement> cachedValue = cache.getIfPresent(request);
158+
if (cachedValue != null) {
159+
retainedCacheValues.add(cachedValue);
160+
}
161+
return stage;
162+
}
140163
}
141164

142165
private static class TestDefaultDriverContext extends DefaultDriverContext {
@@ -222,11 +245,11 @@ private void invalidationTestInner(
222245
assertThat(getPreparedCacheSize(session)).isEqualTo(0);
223246
setupTestSchema.accept(session);
224247

225-
session.prepare(preparedStmtQueryType1);
226-
ByteBuffer queryId2 = session.prepare(preparedStmtQueryType2).getId();
248+
PreparedStatement statement1 = session.prepare(preparedStmtQueryType1);
249+
PreparedStatement statement2 = session.prepare(preparedStmtQueryType2);
250+
ByteBuffer queryId2 = statement2.getId();
227251
assertThat(getPreparedCacheSize(session)).isEqualTo(2);
228252

229-
CountDownLatch preparedStmtCacheRemoveLatch = new CountDownLatch(1);
230253
CountDownLatch typeChangeEventLatch = new CountDownLatch(expectedChangedTypes.size());
231254

232255
DefaultDriverContext ctx = (DefaultDriverContext) session.getContext();
@@ -260,7 +283,6 @@ private void invalidationTestInner(
260283
removedQueryEventError.set(
261284
Optional.of("Unable to set reference for PS removal event"));
262285
}
263-
preparedStmtCacheRemoveLatch.countDown();
264286
});
265287

266288
// alter test_type_2 to trigger cache invalidation and above events
@@ -270,17 +292,20 @@ private void invalidationTestInner(
270292
assertThat(Uninterruptibles.awaitUninterruptibly(typeChangeEventLatch, 10, TimeUnit.SECONDS))
271293
.withFailMessage("typeChangeEventLatch did not trigger before timeout")
272294
.isTrue();
273-
assertThat(
274-
Uninterruptibles.awaitUninterruptibly(
275-
preparedStmtCacheRemoveLatch, 10, TimeUnit.SECONDS))
276-
.withFailMessage("preparedStmtCacheRemoveLatch did not trigger before timeout")
277-
.isTrue();
278295

279-
/* Okay, the latch triggered so cache processing should now be done. Let's validate :allthethings: */
280-
assertThat(changedTypes.keySet()).isEqualTo(expectedChangedTypes);
281-
assertThat(removedQueryIds.get()).isNotEmpty().get().isEqualTo(queryId2);
296+
await()
297+
.atMost(30, TimeUnit.SECONDS)
298+
.untilAsserted(() -> assertThat(getPreparedCacheSize(session)).isEqualTo(1));
299+
300+
assertThat(session.prepare(preparedStmtQueryType1)).isSameAs(statement1);
282301
assertThat(getPreparedCacheSize(session)).isEqualTo(1);
283302

303+
assertThat(session.prepare(preparedStmtQueryType2)).isNotSameAs(statement2);
304+
assertThat(getPreparedCacheSize(session)).isEqualTo(2);
305+
306+
assertThat(changedTypes.keySet()).isEqualTo(expectedChangedTypes);
307+
removedQueryIds.get().ifPresent(queryId -> assertThat(queryId).isEqualTo(queryId2));
308+
284309
// check no errors were seen in callback (and report those as fail msgs)
285310
// if something is broken these may still succeed due to timing
286311
// but shouldn't intermittently fail if the code is working properly

0 commit comments

Comments
 (0)