From 931bc2e66b21d34c236a7d78cbbdf1e08a9dd26d Mon Sep 17 00:00:00 2001 From: Jeongjun Min Date: Mon, 11 Aug 2025 17:39:21 +0900 Subject: [PATCH] GH-1525: Support multiple operations per column in Update Previously, the `Update` class used a `Map` to store its operations, keyed by column name. This prevented multiple operations on the same column, such as setting different keys in a map-type column, as later operations would overwrite earlier ones. This commit changes the internal data structure to a `List`, allowing multiple operations to be recorded. To align with the 'last-wins' semantics for conflicting operations (e.g., setting the same column twice), a conflict resolution mechanism has been added. New operations now replace existing, conflicting ones. Adds comprehensive unit tests to verify both the coexistence of non-conflicting operations and the replacement of conflicting ones. Signed-off-by: Jeongjun Min --- .../data/cassandra/core/query/Update.java | 64 ++++++++++++++----- .../core/StatementFactoryUnitTests.java | 14 ++++ .../cassandra/core/query/UpdateUnitTests.java | 55 ++++++++++++++++ 3 files changed, 117 insertions(+), 16 deletions(-) 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 8f61cb7e8..5c1910ca6 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 @@ -17,10 +17,11 @@ import static org.springframework.data.cassandra.core.query.SerializationUtils.*; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; -import java.util.LinkedHashMap; +import java.util.List; import java.util.Map; import org.jspecify.annotations.Nullable; @@ -40,13 +41,14 @@ * * @author Mark Paluch * @author Chema Vinacua + * @author Jeongjun Min * @since 2.0 */ public class Update { - private static final Update EMPTY = new Update(Collections.emptyMap()); + private static final Update EMPTY = new Update(Collections.emptyList()); - private final Map updateOperations; + private final List updateOperations; /** * Create an empty {@link Update} object. @@ -66,11 +68,9 @@ public static Update of(Iterable assignmentOps) { Assert.notNull(assignmentOps, "Update operations must not be null"); - Map updateOperations = assignmentOps instanceof Collection - ? new LinkedHashMap<>(((Collection) assignmentOps).size()) - : new LinkedHashMap<>(); + List updateOperations = new ArrayList<>(); - assignmentOps.forEach(assignmentOp -> updateOperations.put(assignmentOp.getColumnName(), assignmentOp)); + assignmentOps.forEach(updateOperations::add); return new Update(updateOperations); } @@ -84,7 +84,7 @@ public static Update update(String columnName, @Nullable Object value) { return empty().set(columnName, value); } - private Update(Map updateOperations) { + private Update(List updateOperations) { this.updateOperations = updateOperations; } @@ -215,24 +215,56 @@ public Update decrement(String columnName, Number delta) { * @return {@link Collection} of update operations. */ public Collection getUpdateOperations() { - return Collections.unmodifiableCollection(updateOperations.values()); + return Collections.unmodifiableCollection(updateOperations); } private Update add(AssignmentOp assignmentOp) { - Map map = new LinkedHashMap<>(this.updateOperations.size() + 1); + List list = new ArrayList<>(this.updateOperations.size() + 1); - map.putAll(this.updateOperations); - map.put(assignmentOp.getColumnName(), assignmentOp); + for (AssignmentOp existing : this.updateOperations) { + if (!conflicts(existing, assignmentOp)) { + list.add(existing); + } + } - return new Update(map); + list.add(assignmentOp); + + return new Update(list); + } + + /** + * Determine whether two assignment operations conflict and should not co-exist in a single {@link Update}. + * Conflicts are defined as whole-column operations on the same column or element-level operations targeting + * the same element (same map key or same list index) on the same column. In case of conflict, last-wins semantics + * apply and the incoming operation replaces the existing one. + */ + private static boolean conflicts(AssignmentOp existing, AssignmentOp incoming) { + + if (!existing.getColumnName().equals(incoming.getColumnName())) { + return false; + } + + if (existing instanceof SetAtKeyOp e && incoming instanceof SetAtKeyOp i) { + return equalsNullSafe(e.getKey(), i.getKey()); + } + + if (existing instanceof SetAtIndexOp e && incoming instanceof SetAtIndexOp i) { + return e.getIndex() == i.getIndex(); + } + + return true; } - @Override - public String toString() { - return StringUtils.collectionToDelimitedString(updateOperations.values(), ", "); + private static boolean equalsNullSafe(@Nullable Object a, @Nullable Object b) { + return a == b || (a != null && a.equals(b)); } + @Override + public String toString() { + return StringUtils.collectionToDelimitedString(updateOperations, ", "); + } + /** * Builder to add a single element/multiple elements to a collection associated with a {@link ColumnName}. * 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 f3c573392..7e17a99db 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 @@ -67,6 +67,7 @@ * * @author Mark Paluch * @author Sam Lightfoot + * @author Jeongjun Min */ class StatementFactoryUnitTests { @@ -962,6 +963,19 @@ void shouldRenderSimilaritySelector() { assertThat(statement.getNamedValues()).isEmpty(); } + @Test // GH-1525 + void shouldCreateUpdateWithMultipleOperationsOnSameColumnDifferentKeys() { + + Update update = Update.empty().set("map").atKey("key1").to("value1").set("map").atKey("key2").to("value2"); + + StatementBuilder updateStatementBuilder = statementFactory + .update(Query.empty(), update, personEntity); + + String cql = updateStatementBuilder.build(ParameterHandling.INLINE).getQuery(); + + assertThat(cql).isEqualTo("UPDATE person SET map['key1']='value1', map['key2']='value2'"); + } + @SuppressWarnings("unused") static class Person { diff --git a/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/core/query/UpdateUnitTests.java b/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/core/query/UpdateUnitTests.java index aa7b3cd6e..ffaad8a03 100644 --- a/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/core/query/UpdateUnitTests.java +++ b/spring-data-cassandra/src/test/java/org/springframework/data/cassandra/core/query/UpdateUnitTests.java @@ -25,6 +25,7 @@ * * @author Mark Paluch * @author Chema Vinacua + * @author Jeongjun Min */ class UpdateUnitTests { @@ -153,4 +154,58 @@ void shouldCreateDecrementLongUpdate() { assertThat(update.getUpdateOperations()).hasSize(1); assertThat(update).hasToString("foo = foo - 2400000000"); } + + @Test // GH-1525 + void shouldAllowMultipleSetAtKeyOperationsOnSameColumn() { + + Update update = Update.empty().set("foo").atKey("k1").to("v1").set("foo").atKey("k2").to("v2"); + + assertThat(update.getUpdateOperations()).hasSize(2); + assertThat(update).hasToString("foo['k1'] = 'v1', foo['k2'] = 'v2'"); + } + + @Test // GH-1525 + void shouldUseLastWinsForDuplicateSetAtKeyOnSameKey() { + + Update update = Update.empty().set("foo").atKey("k1").to("v1").set("foo").atKey("k1").to("v2"); + + assertThat(update.getUpdateOperations()).hasSize(1); + assertThat(update).hasToString("foo['k1'] = 'v2'"); + } + + @Test // GH-1525 + void shouldAllowMultipleSetAtIndexOperationsOnSameColumn() { + + Update update = Update.empty().set("foo").atIndex(1).to("A").set("foo").atIndex(2).to("B"); + + assertThat(update.getUpdateOperations()).hasSize(2); + assertThat(update).hasToString("foo[1] = 'A', foo[2] = 'B'"); + } + + @Test // GH-1525 + void shouldUseLastWinsForDuplicateSetAtIndexOnSameIndex() { + + Update update = Update.empty().set("foo").atIndex(1).to("A").set("foo").atIndex(1).to("B"); + + assertThat(update.getUpdateOperations()).hasSize(1); + assertThat(update).hasToString("foo[1] = 'B'"); + } + + @Test // GH-1525 + void wholeColumnAndElementLevelShouldConflict_LastWinsWholeColumn() { + + Update update = Update.empty().set("foo").atKey("k1").to("v1").set("foo", "ALL"); + + assertThat(update.getUpdateOperations()).hasSize(1); + assertThat(update).hasToString("foo = 'ALL'"); + } + + @Test // GH-1525 + void wholeColumnAndElementLevelShouldConflict_LastWinsElementLevel() { + + Update update = Update.empty().set("foo", "ALL").set("foo").atKey("k1").to("v1"); + + assertThat(update.getUpdateOperations()).hasSize(1); + assertThat(update).hasToString("foo['k1'] = 'v1'"); + } }