From c4786458dc3ad020b9fcef15e0e3121f3422816d Mon Sep 17 00:00:00 2001 From: John Blum Date: Wed, 5 Feb 2020 14:59:30 -0800 Subject: [PATCH] DATACASS-726 - Upgrade to DataStax Java Driver 4.4. Fix broken tests. Fix compiler warnings. Replace incorrect use of FindBugs @NonNull and @Nullable annotations with equivalent Spring annoations. Edit Javadoc. Format source code. Optimize imports. --- pom.xml | 24 +++++-- spring-data-cassandra/pom.xml | 5 ++ .../data/cassandra/core/StatementFactory.java | 15 ++-- .../cassandra/core/convert/QueryMapper.java | 7 +- .../cassandra/core/cql/QueryOptionsUtil.java | 11 +-- .../core/cql/util/StatementBuilder.java | 70 ++++++++++++------- .../data/cassandra/core/query/Update.java | 12 ++-- .../core/StatementFactoryUnitTests.java | 54 ++++++++------ .../core/convert/UpdateMapperUnitTests.java | 45 +++++++----- 9 files changed, 154 insertions(+), 89 deletions(-) diff --git a/pom.xml b/pom.xml index ca60693f3..b30421e40 100644 --- a/pom.xml +++ b/pom.xml @@ -90,9 +90,11 @@ 17001 17000 3.11.5 - 4.3.1 + 4.4.0 spring-data-cassandra 1.0 + + 0.5.4 1.01 multi 2.3.0.BUILD-SNAPSHOT @@ -169,6 +171,13 @@ + + org.assertj + assertj-core + ${assertj} + test + + org.testcontainers testcontainers-bom @@ -184,6 +193,13 @@ test + + com.carrotsearch + hppc + ${hppc.version} + test + + edu.umd.cs.mtc multithreadedtc @@ -191,12 +207,6 @@ test - - org.assertj - assertj-core - ${assertj} - test - diff --git a/spring-data-cassandra/pom.xml b/spring-data-cassandra/pom.xml index 877d1a9ec..399b2ffdf 100644 --- a/spring-data-cassandra/pom.xml +++ b/spring-data-cassandra/pom.xml @@ -189,6 +189,11 @@ multithreadedtc + + com.carrotsearch + hppc + + org.jetbrains.kotlin diff --git a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/StatementFactory.java b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/StatementFactory.java index 5d4cba18e..fda86eb84 100644 --- a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/StatementFactory.java +++ b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/StatementFactory.java @@ -699,14 +699,16 @@ public class StatementFactory { .bind((statement, factory) -> { List assignments = mappedUpdate.getUpdateOperations().stream() - .map(assignmentOp -> getAssignment(assignmentOp, factory)).collect(Collectors.toList()); + .map(assignmentOp -> getAssignment(assignmentOp, factory)) + .collect(Collectors.toList()); return (com.datastax.oss.driver.api.querybuilder.update.Update) ((OngoingAssignment) statement) .set(assignments); }).bind((statement, factory) -> { - List relations = filter.stream().map(criteriaDefinition -> toClause(criteriaDefinition, factory)) + List relations = filter.stream() + .map(criteriaDefinition -> toClause(criteriaDefinition, factory)) .collect(Collectors.toList()); return statement.where(relations); @@ -735,10 +737,13 @@ public class StatementFactory { private static void applyUpdateIfCondition( StatementBuilder update, Filter criteriaDefinitions) { + update.bind((statement, factory) -> { - List conditions = criteriaDefinitions.stream().map(it -> toCondition(it, factory)) + List conditions = criteriaDefinitions.stream() + .map(it -> toCondition(it, factory)) .collect(Collectors.toList()); + return statement.if_(conditions); }); } @@ -898,8 +903,8 @@ public class StatementFactory { Assert.notNull(update, "Update must not be null"); - com.datastax.oss.driver.api.querybuilder.update.Update updateToUse = QueryOptionsUtil.addWriteOptions(update, - writeOptions); + com.datastax.oss.driver.api.querybuilder.update.Update updateToUse = + QueryOptionsUtil.addWriteOptions(update, writeOptions); if (writeOptions instanceof UpdateOptions) { diff --git a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/convert/QueryMapper.java b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/convert/QueryMapper.java index 36504601c..661274b6f 100644 --- a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/convert/QueryMapper.java +++ b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/convert/QueryMapper.java @@ -133,7 +133,8 @@ public class QueryMapper { Object value = predicate.getValue(); - Object mappedValue = value != null ? getConverter().convertToColumnType(value, getTypeInformation(field, value)) + Object mappedValue = value != null + ? getConverter().convertToColumnType(value, getTypeInformation(field, value)) : null; Predicate mappedPredicate = new Predicate(predicate.getOperator(), mappedValue); @@ -330,11 +331,11 @@ public class QueryMapper { Field createPropertyField(@Nullable CassandraPersistentEntity entity, ColumnName key) { - return Optional.ofNullable(entity). map(e -> new MetadataBackedField(key, e, getMappingContext())) + return Optional.ofNullable(entity) + . map(e -> new MetadataBackedField(key, e, getMappingContext())) .orElseGet(() -> new Field(key)); } - @SuppressWarnings("unchecked") TypeInformation getTypeInformation(Field field, @Nullable Object value) { if (field.getProperty().isPresent()) { diff --git a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/cql/QueryOptionsUtil.java b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/cql/QueryOptionsUtil.java index bdbfdb997..b45c8bd1f 100644 --- a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/cql/QueryOptionsUtil.java +++ b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/cql/QueryOptionsUtil.java @@ -42,9 +42,11 @@ public abstract class QueryOptionsUtil { * @param queryOptions query options (e.g. consistency level) to add to the CQL statement. * @return the given {@link Statement}. */ + @SuppressWarnings("unchecked") public static > T addQueryOptions(T statement, QueryOptions queryOptions) { Assert.notNull(statement, "Statement must not be null"); + Statement statementToUse = statement; if (queryOptions.getConsistencyLevel() != null) { @@ -66,11 +68,10 @@ public abstract class QueryOptionsUtil { } if (queryOptions.getTracing() != null) { - if (queryOptions.getTracing()) { - statementToUse = statementToUse.setTracing(true); - } else { - statementToUse = statementToUse.setTracing(false); - } + // While the following statement is null-safe, avoid setting Statement tracing if the tracing query option + // is null since Statements are immutable and the call creates a new object. Therefore keep the following + // statement wrapped in the conditional null check to avoid additional garbage and added GC pressure. + statementToUse = statementToUse.setTracing(Boolean.TRUE.equals(queryOptions.getTracing())); } return (T) statementToUse; diff --git a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/cql/util/StatementBuilder.java b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/cql/util/StatementBuilder.java index 9a88d616b..276a747cc 100644 --- a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/cql/util/StatementBuilder.java +++ b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/cql/util/StatementBuilder.java @@ -15,9 +15,6 @@ */ package org.springframework.data.cassandra.core.cql.util; -import edu.umd.cs.findbugs.annotations.NonNull; -import edu.umd.cs.findbugs.annotations.Nullable; - import java.util.ArrayList; import java.util.Collection; import java.util.LinkedHashMap; @@ -28,6 +25,8 @@ import java.util.function.Consumer; import java.util.function.Function; import java.util.function.UnaryOperator; +import org.springframework.lang.NonNull; +import org.springframework.lang.Nullable; import org.springframework.util.Assert; import com.datastax.oss.driver.api.core.cql.SimpleStatement; @@ -76,17 +75,15 @@ public class StatementBuilder { private List> onBuild = new ArrayList<>(); private List> onBuilt = new ArrayList<>(); - private StatementBuilder(S statement) { - this.statement = statement; - } - /** - * Create a new {@link StatementBuilder} with the given {@link BuildableQuery query stub}. The stub is used as base - * for the built query so each query inherits properties of this stub. + * Factory method used to create a new {@link StatementBuilder} with the given {@link BuildableQuery query stub}. + * The stub is used as base for the built query so each query inherits properties of this stub. * - * @param stub the query stub to use. * @param query type. - * @return the {@link StatementBuilder} for the {@link BuildableQuery query stub}. + * @param stub the {@link BuildableQuery query stub} to use. + * @return a {@link StatementBuilder} for the given {@link BuildableQuery query stub}. + * @throws IllegalArgumentException if the {@link BuildableQuery query stub} is {@literal null}. + * @see com.datastax.oss.driver.api.querybuilder.BuildableQuery */ public static StatementBuilder of(S stub) { @@ -95,6 +92,17 @@ public class StatementBuilder { return new StatementBuilder<>(stub); } + /** + * Constructs a new instance of this {@link StatementBuilder} with the given {@link BuildableQuery query stub}. + * + * @param statement the {@link BuildableQuery query stub} from which to build + * the {@link com.datastax.oss.driver.api.core.cql.Statement}. + * @see com.datastax.oss.driver.api.querybuilder.BuildableQuery + */ + private StatementBuilder(S statement) { + this.statement = statement; + } + /** * Apply a {@link BindFunction} to the statement. Bind functions are applied on {@link #build()}. * @@ -106,6 +114,7 @@ public class StatementBuilder { Assert.notNull(action, "BindFunction must not be null"); queryActions.add(action::bind); + return this; } @@ -115,11 +124,13 @@ public class StatementBuilder { * @param action the builder function to be applied to the statement. * @return {@code this} {@link StatementBuilder}. */ + @SuppressWarnings("unchecked") public StatementBuilder apply(Function action) { Assert.notNull(action, "BindFunction must not be null"); queryActions.add((source, termFactory) -> (S) action.apply(source)); + return this; } @@ -136,6 +147,7 @@ public class StatementBuilder { Assert.notNull(action, "Consumer must not be null"); onBuild.add(action); + return this; } @@ -152,6 +164,7 @@ public class StatementBuilder { Assert.notNull(mappingFunction, "Mapping function must not be null"); onBuilt.add(mappingFunction); + return this; } @@ -169,7 +182,7 @@ public class StatementBuilder { * Build a {@link SimpleStatement statement} by applying builder and bind functions using the given * {@link ParameterHandling}. * - * @param parameterHandling + * @param parameterHandling {@link ParameterHandling} used to determine how to render parameters. * @return the built {@link SimpleStatement}. */ public SimpleStatement build(ParameterHandling parameterHandling) { @@ -180,8 +193,8 @@ public class StatementBuilder { * Build a {@link SimpleStatement statement} by applying builder and bind functions using the given * {@link CodecRegistry} and {@link ParameterHandling}. * - * @param parameterHandling - * @param codecRegistry + * @param parameterHandling {@link ParameterHandling} used to determine how to render parameters. + * @param codecRegistry registry of Apache Cassandra codecs for converting to/from Java types and CQL types. * @return the built {@link SimpleStatement}. */ public SimpleStatement build(ParameterHandling parameterHandling, CodecRegistry codecRegistry) { @@ -199,12 +212,13 @@ public class StatementBuilder { statement = runnable.run(statement, termFactory); } - return StatementBuilder.this.build(statement.builder()); + return build(statement.builder()); } if (parameterHandling == ParameterHandling.BY_INDEX) { List values = new ArrayList<>(); + TermFactory termFactory = value -> { values.add(value); return QueryBuilder.bindMarker(); @@ -220,6 +234,7 @@ public class StatementBuilder { if (parameterHandling == ParameterHandling.BY_NAME) { Map values = new LinkedHashMap<>(); + TermFactory termFactory = value -> { String name = "p" + values.size(); values.put(name, value); @@ -231,6 +246,7 @@ public class StatementBuilder { } SimpleStatementBuilder builder = statement.builder(); + values.forEach(builder::addNamedValue); return build(builder); @@ -250,6 +266,14 @@ public class StatementBuilder { return statmentToUse; } + private SimpleStatementBuilder onBuild(SimpleStatementBuilder statementBuilder) { + + onBuild.forEach(it -> it.accept(statementBuilder)); + + return statementBuilder; + } + + @SuppressWarnings("unchecked") private static Term toLiteralTerms(@Nullable Object value, CodecRegistry codecRegistry) { if (value instanceof List) { @@ -278,9 +302,8 @@ public class StatementBuilder { Map terms = new LinkedHashMap<>(); - ((Map) value).forEach((k, v) -> { - terms.put(toLiteralTerms(k, codecRegistry), toLiteralTerms(v, codecRegistry)); - }); + ((Map) value).forEach((k, v) -> + terms.put(toLiteralTerms(k, codecRegistry), toLiteralTerms(v, codecRegistry))); return new MapTerm(terms); } @@ -288,13 +311,6 @@ public class StatementBuilder { return QueryBuilder.literal(value, codecRegistry); } - private SimpleStatementBuilder onBuild(SimpleStatementBuilder statementBuilder) { - - onBuild.forEach(it -> it.accept(statementBuilder)); - - return statementBuilder; - } - /** * Binding function. This function gets called with the current statement and {@link TermFactory}. * @@ -336,7 +352,7 @@ public class StatementBuilder { /** * Named bind markers. */ - BY_NAME; + BY_NAME } static class ListTerm implements Term { @@ -408,6 +424,7 @@ public class StatementBuilder { } @Override + @SuppressWarnings("all") public void appendTo(@NonNull StringBuilder builder) { if (components.isEmpty()) { @@ -429,6 +446,7 @@ public class StatementBuilder { builder.append(":"); entry.getValue().appendTo(builder); } + if (!first) { builder.append("}"); } diff --git a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/query/Update.java b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/query/Update.java index af0dc555c..ef4aabfda 100644 --- a/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/query/Update.java +++ b/spring-data-cassandra/src/main/java/org/springframework/data/cassandra/core/query/Update.java @@ -47,10 +47,6 @@ public class Update { private final Map updateOperations; - private Update(Map updateOperations) { - this.updateOperations = updateOperations; - } - /** * Create an empty {@link Update} object. * @@ -69,7 +65,7 @@ public class Update { Assert.notNull(assignmentOps, "Update operations must not be null"); - Map updateOperations = assignmentOps instanceof Collection + Map updateOperations = assignmentOps instanceof Collection ? new LinkedHashMap<>(((Collection) assignmentOps).size()) : new LinkedHashMap<>(); @@ -87,6 +83,10 @@ public class Update { return empty().set(columnName, value); } + private Update(Map updateOperations) { + this.updateOperations = updateOperations; + } + /** * Set the {@code columnName} to {@code value}. * @@ -387,7 +387,7 @@ public class Update { * @see org.springframework.data.cassandra.core.query.Update.AddToBuilder#addAll(java.util.Map) */ @Override - public Update addAll(Map map) { + public Update addAll(Map map) { Assert.notNull(map, "Map must not be null"); diff --git a/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/core/StatementFactoryUnitTests.java b/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/core/StatementFactoryUnitTests.java index f6c532e25..91a5cc46e 100644 --- a/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/core/StatementFactoryUnitTests.java +++ b/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/core/StatementFactoryUnitTests.java @@ -15,7 +15,7 @@ */ package org.springframework.data.cassandra.core; -import static org.assertj.core.api.Assertions.*; +import static org.assertj.core.api.Assertions.assertThat; import java.time.Duration; import java.util.Collections; @@ -331,35 +331,42 @@ public class StatementFactoryUnitTests { StatementBuilder update = statementFactory .update(Query.empty(), Update.empty().addTo("map").entry("foo", "Euro"), personEntity); - assertThat(update.build(ParameterHandling.INLINE).getQuery()).isEqualTo("UPDATE person SET map+={'foo':'Euro'}"); + assertThat(update.build(ParameterHandling.INLINE).getQuery()).isEqualTo("UPDATE person SET map=map+{'foo':'Euro'}"); } @Test // DATACASS-343 public void shouldPrependAllToList() { - StatementBuilder update = statementFactory - .update(Query.empty(), Update.empty().addTo("list").prependAll("foo", "Euro"), personEntity); + Update update = Update.empty().addTo("list").prependAll("foo", "Euro"); - assertThat(update.build(ParameterHandling.INLINE).getQuery()) + StatementBuilder updateStatementBuilder = + statementFactory.update(Query.empty(), update, personEntity); + + assertThat(updateStatementBuilder.build(ParameterHandling.INLINE).getQuery()) .isEqualTo("UPDATE person SET list=['foo','Euro']+list"); } @Test // DATACASS-343 public void shouldAppendAllToList() { - StatementBuilder update = statementFactory - .update(Query.empty(), Update.empty().addTo("list").appendAll("foo", "Euro"), personEntity); + Update update = Update.empty().addTo("list").appendAll("foo", "Euro"); - assertThat(update.build(ParameterHandling.INLINE).getQuery()).isEqualTo("UPDATE person SET list+=['foo','Euro']"); + StatementBuilder updateStatementBuilder = + statementFactory.update(Query.empty(), update, personEntity); + + assertThat(updateStatementBuilder.build(ParameterHandling.INLINE).getQuery()) + .isEqualTo("UPDATE person SET list=list+['foo','Euro']"); } @Test // DATACASS-343 public void shouldRemoveFromList() { - StatementBuilder update = statementFactory - .update(Query.empty(), Update.empty().remove("list", "Euro"), personEntity); + Update update = Update.empty().remove("list", "Euro"); - assertThat(update.build(ParameterHandling.INLINE).getQuery()).isEqualTo("UPDATE person SET list-=['Euro']"); + StatementBuilder updateStatementBuilder = + statementFactory.update(Query.empty(), update, personEntity); + + assertThat(updateStatementBuilder.build(ParameterHandling.INLINE).getQuery()).isEqualTo("UPDATE person SET list=list-['Euro']"); } @Test // DATACASS-343 @@ -374,20 +381,24 @@ public class StatementFactoryUnitTests { @Test // DATACASS-343 public void shouldAddAllToSet() { - StatementBuilder update = statementFactory - .update(Query.empty(), Update.empty().addTo("set").appendAll("foo", "Euro"), personEntity); + Update update = Update.empty().addTo("set").appendAll("foo", "Euro"); - assertThat(update.build(ParameterHandling.INLINE).getQuery()) - .isEqualTo("UPDATE person SET set_col+={'foo','Euro'}"); + StatementBuilder updateStatementBuilder = + statementFactory.update(Query.empty(), update, personEntity); + + assertThat(updateStatementBuilder.build(ParameterHandling.INLINE).getQuery()) + .isEqualTo("UPDATE person SET set_col=set_col+{'foo','Euro'}"); } @Test // DATACASS-343 public void shouldRemoveFromSet() { - StatementBuilder update = statementFactory - .update(Query.empty(), Update.empty().remove("set", "Euro"), personEntity); + Update update = Update.empty().remove("set", "Euro"); - assertThat(update.build(ParameterHandling.INLINE).getQuery()).isEqualTo("UPDATE person SET set_col-={'Euro'}"); + StatementBuilder updateStatementBuilder = + statementFactory.update(Query.empty(), update, personEntity); + + assertThat(updateStatementBuilder.build(ParameterHandling.INLINE).getQuery()).isEqualTo("UPDATE person SET set_col=set_col-{'Euro'}"); } @Test // DATACASS-343 @@ -577,15 +588,18 @@ public class StatementFactoryUnitTests { .isEqualTo("SELECT count(1) FROM group WHERE foo='bar'"); } + @SuppressWarnings("unused") static class Person { @Id String id; + Integer number; + List list; - @Column("set_col") Set set; + Map map; - Integer number; + @Column("set_col") Set set; @Column("first_name") String firstName; } diff --git a/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/core/convert/UpdateMapperUnitTests.java b/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/core/convert/UpdateMapperUnitTests.java index b33aedc6b..b3b0854eb 100644 --- a/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/core/convert/UpdateMapperUnitTests.java +++ b/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/core/convert/UpdateMapperUnitTests.java @@ -15,11 +15,9 @@ */ package org.springframework.data.cassandra.core.convert; -import static org.assertj.core.api.Assertions.*; -import static org.mockito.Mockito.*; - -import lombok.AllArgsConstructor; -import lombok.Data; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; +import static org.mockito.Mockito.when; import java.time.LocalTime; import java.util.Collections; @@ -48,6 +46,9 @@ import org.springframework.data.cassandra.support.UserDefinedTypeBuilder; import com.datastax.oss.driver.api.core.CqlIdentifier; import com.datastax.oss.driver.api.core.type.DataTypes; +import lombok.AllArgsConstructor; +import lombok.Data; + /** * Unit tests for {@link UpdateMapper}. * @@ -107,10 +108,12 @@ public class UpdateMapperUnitTests { Map map = Collections.singletonMap(manufacturer, currency); - Update update = updateMapper.getMappedObject(Update.empty().set("manufacturers", map), persistentEntity); + Update update = Update.empty().set("manufacturers", map); - assertThat(update.getUpdateOperations()).hasSize(1); - assertThat(update.toString()).isEqualTo("manufacturers = { {name:'foobar'} : 'Euro' }"); + Update mappedUpdate = updateMapper.getMappedObject(update, persistentEntity); + + assertThat(mappedUpdate.getUpdateOperations()).hasSize(1); + assertThat(mappedUpdate.toString()).isEqualTo("manufacturers = {{name:'foobar'}:'Euro'}"); } @Test // DATACASS-343 @@ -157,11 +160,12 @@ public class UpdateMapperUnitTests { Manufacturer manufacturer = new Manufacturer("foobar"); - Update update = updateMapper.getMappedObject(Update.empty().addTo("manufacturers").entry(manufacturer, currency), - persistentEntity); + Update update = Update.empty().addTo("manufacturers").entry(manufacturer, currency); - assertThat(update.getUpdateOperations()).hasSize(1); - assertThat(update.toString()).isEqualTo("manufacturers = manufacturers + { {name:'foobar'} : 'Euro' }"); + Update mappedUpdate = updateMapper.getMappedObject(update, persistentEntity); + + assertThat(mappedUpdate.getUpdateOperations()).hasSize(1); + assertThat(mappedUpdate.toString()).isEqualTo("manufacturers = manufacturers + {{name:'foobar'}:'Euro'}"); } @Test // DATACASS-343 @@ -255,21 +259,28 @@ public class UpdateMapperUnitTests { () -> this.updateMapper.getMappedObject(Update.empty().set("tuple.zip", "bar"), this.persistentEntity)); } + @SuppressWarnings("unused") static class Person { @Id String id; - List list; - @Column("set_col") Set set; - Map map; - Map manufacturers; Currency currency; - LocalTime localTime; Integer number; + + List list; + + LocalTime localTime; + + Map map; + Map manufacturers; + MappedTuple tuple; + @Column("set_col") Set set; + @Column("first_name") String firstName; + } @Tuple