Fix session property removed in verifier

Removing a session property from verifier runs when the override strategy was `NO_ACTION` involved changing an `ImmutableMap` and hence an exception was thrown. Fix the code by making the map mutable.
cjjxfli · Aug 13, 2021 · ccfb095 · ccfb095
1 parent 247a020
commit ccfb095
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 8 deletions.
diff --git a/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/QueryConfiguration.java b/presto-verifier/src/main/java/com/facebook/presto/verifier/framework/QueryConfiguration.java
@@ -19,7 +19,6 @@
 
 import java.util.HashMap;
 import java.util.Map;
-import java.util.Map.Entry;
 import java.util.Objects;
 import java.util.Optional;
 
@@ -53,14 +52,14 @@ public QueryConfiguration(
 
     public QueryConfiguration applyOverrides(QueryConfigurationOverrides overrides)
     {
-        Map<String, String> sessionProperties = this.sessionProperties;
+        Map<String, String> sessionProperties;
         if (overrides.getSessionPropertiesOverrideStrategy() == OVERRIDE) {
-            sessionProperties = overrides.getSessionPropertiesOverride();
+            sessionProperties = new HashMap<>(overrides.getSessionPropertiesOverride());
         }
-        else if (overrides.getSessionPropertiesOverrideStrategy() == SUBSTITUTE) {
-            sessionProperties = new HashMap<>(sessionProperties);
-            for (Entry<String, String> entry : overrides.getSessionPropertiesOverride().entrySet()) {
-                sessionProperties.put(entry.getKey(), entry.getValue());
+        else {
+            sessionProperties = new HashMap<>(this.sessionProperties);
+            if (overrides.getSessionPropertiesOverrideStrategy() == SUBSTITUTE) {
+                sessionProperties.putAll(overrides.getSessionPropertiesOverride());
             }
         }
         overrides.getSessionPropertiesToRemove().forEach(sessionProperties::remove);

diff --git a/...verifier/src/test/java/com/facebook/presto/verifier/framework/TestQueryConfiguration.java b/...verifier/src/test/java/com/facebook/presto/verifier/framework/TestQueryConfiguration.java
@@ -22,6 +22,7 @@
 
 import static com.facebook.airlift.json.JsonCodec.mapJsonCodec;
 import static com.facebook.presto.testing.assertions.Assert.assertEquals;
+import static com.facebook.presto.verifier.framework.QueryConfigurationOverrides.SessionPropertiesOverrideStrategy.NO_ACTION;
 import static com.facebook.presto.verifier.framework.QueryConfigurationOverrides.SessionPropertiesOverrideStrategy.OVERRIDE;
 import static com.facebook.presto.verifier.framework.QueryConfigurationOverrides.SessionPropertiesOverrideStrategy.SUBSTITUTE;
 
@@ -115,7 +116,7 @@ public void testSessionPropertySubstitute()
     }
 
     @Test
-    public void testSessionPropertyRemoval()
+    public void testSessionPropertyRemovalWithOverrides()
     {
         overrides.setSessionPropertiesToRemove("property_1, property_2");
         overrides.setSessionPropertiesOverrideStrategy(OVERRIDE);
@@ -144,4 +145,19 @@ public void testSessionPropertySubstituteAndRemove()
 
         assertEquals(CONFIGURATION_1.applyOverrides(overrides), removed);
     }
+
+    @Test
+    public void testSessionPropertyRemoval()
+    {
+        overrides.setSessionPropertiesToRemove("property_2");
+        overrides.setSessionPropertiesOverrideStrategy(NO_ACTION);
+        QueryConfiguration removed = new QueryConfiguration(
+                CATALOG_OVERRIDE,
+                SCHEMA_OVERRIDE,
+                Optional.of(USERNAME_OVERRIDE),
+                Optional.of(PASSWORD_OVERRIDE),
+                Optional.of(ImmutableMap.of("property_1", "value_1")));
+
+        assertEquals(CONFIGURATION_1.applyOverrides(overrides), removed);
+    }
 }