Cloud Spanner: Check duplicate columns in Mutation

skrulcik · skrulcik · commit c9ddfcc1e352 · 2017-07-20T17:41:42.000-04:00
close #2257 * Adds `checkDuplicateColumns`, executed before a mutation is built, to ensure each column name is only used once. Note that column names are case-insensitive according to the [DDL docs](https://cloud.google.com/spanner/docs/data-definition-language#create_table). * Tests duplicate columns with the same case and different cases. * Removes duplicate column check in `asMap` because all `Mutation`s are now guaranteed not to have duplicate columns. Removes corresponding test code too.
diff --git a/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Mutation.java b/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Mutation.java
@@ -24,10 +24,12 @@
 
 import java.io.Serializable;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Set;
 import javax.annotation.Nullable;
 
 /**
@@ -188,9 +190,17 @@ public ValueBinder<WriteBuilder> set(String columnName) {
       return binder;
     }
 
+    /**
+     * Returns a newly created {@code Mutation} based on the contents of the {@code Builder}.
+     *
+     * @throws IllegalStateException if any duplicate columns are present. Duplicate detection is
+     * case-insensitive.
+     */
     public Mutation build() {
       checkBindingInProgress(false);
-      return new Mutation(table, operation, columns.build(), values.build(), null);
+      ImmutableList<String> columnNames = columns.build();
+      checkDuplicateColumns(columnNames);
+      return new Mutation(table, operation, columnNames, values.build(), null);
     }
 
     private void checkBindingInProgress(boolean expectInProgress) {
@@ -200,6 +210,17 @@ private void checkBindingInProgress(boolean expectInProgress) {
         throw new IllegalStateException("Incomplete binding for column " + currentColumn);
       }
     }
+
+    private void checkDuplicateColumns(ImmutableList<String> columnNames) {
+      Set<String> columnNameSet = new HashSet<>();
+      for (String columnName : columns.build()) {
+        columnName = columnName.toLowerCase();
+        if (columnNameSet.contains(columnName)) {
+          throw new IllegalStateException("Duplicate column: " + columnName);
+        }
+        columnNameSet.add(columnName);
+      }
+    }
   }
 
   /** Returns the name of the table that this mutation will affect. */
@@ -247,9 +268,6 @@ public Map<String, Value> asMap() {
     LinkedHashMap<String, Value> map = new LinkedHashMap<>();
     for (int i = 0; i < columns.size(); ++i) {
       Value existing = map.put(columns.get(i), values.get(i));
-      if (existing != null) {
-        throw new IllegalStateException("Duplicate column: " + columns.get(i));
-      }
     }
     return Collections.unmodifiableMap(map);
   }
diff --git a/google-cloud-spanner/src/test/java/com/google/cloud/spanner/MutationTest.java b/google-cloud-spanner/src/test/java/com/google/cloud/spanner/MutationTest.java
@@ -124,13 +124,16 @@ public void replace() {
 
   @Test
   public void duplicateColumn() {
-    // The current implementation does not repeat validation performed by the server; duplicates
-    // are permitted but will later cause an operation failure.
+    expectedException.expect(IllegalStateException.class);
+    expectedException.expectMessage("Duplicate column");
     Mutation m = Mutation.newInsertBuilder("T1").set("C1").to(true).set("C1").to(false).build();
-    assertThat(m.getOperation()).isEqualTo(Mutation.Op.INSERT);
-    assertThat(m.getColumns()).containsExactly("C1", "C1").inOrder();
-    assertThat(m.getValues()).containsExactly(Value.bool(true), Value.bool(false)).inOrder();
-    assertThat(m.toString()).isEqualTo("insert(T1{C1=true,C1=false})");
+  }
+
+  @Test
+  public void duplicateColumnCaseInsensitive() {
+    expectedException.expect(IllegalStateException.class);
+    expectedException.expectMessage("Duplicate column");
+    Mutation m = Mutation.newInsertBuilder("T1").set("C1").to(true).set("c1").to(false).build();
   }
 
   @Test
@@ -141,11 +144,6 @@ public void asMap() {
     m = Mutation.newInsertBuilder("T").set("C1").to(true).set("C2").to(1234).build();
     assertThat(m.asMap())
         .isEqualTo(ImmutableMap.of("C1", Value.bool(true), "C2", Value.int64(1234)));
-
-    m = Mutation.newInsertBuilder("T").set("C1").to(true).set("C1").to(false).build();
-    expectedException.expect(IllegalStateException.class);
-    expectedException.expectMessage("Duplicate column");
-    m.asMap();
   }
 
   @Test
