Merge pull request #12176 from vespa-engine/bratseth/clearAll-in-query-profiles

Support clearing values in query profiles

8 files changed, 128 insertions, 23 deletions

diff --git a/container-search/abi-spec.json b/container-search/abi-spec.json
index 5b86c3587cd..82d3223c8fe 100644
--- a/container-search/abi-spec.json
+++ b/container-search/abi-spec.json
@@ -5790,6 +5790,7 @@
       "public com.yahoo.search.query.profile.compiled.CompiledQueryProfile getQueryProfile()",
       "public java.lang.Object get(com.yahoo.processing.request.CompoundName, java.util.Map, com.yahoo.processing.request.Properties)",
       "public void set(com.yahoo.processing.request.CompoundName, java.lang.Object, java.util.Map)",
+      "public void clearAll(com.yahoo.processing.request.CompoundName, java.util.Map)",
       "public java.util.Map listProperties(com.yahoo.processing.request.CompoundName, java.util.Map, com.yahoo.processing.request.Properties)",
       "public boolean isComplete(java.lang.StringBuilder, java.util.Map)",
       "public com.yahoo.search.query.profile.QueryProfileProperties clone()",
diff --git a/container-search/src/main/java/com/yahoo/search/query/profile/ModelObjectMap.java b/container-search/src/main/java/com/yahoo/search/query/profile/ModelObjectMap.java
index 4dc9ade62e5..e32d2dc226d 100644
--- a/container-search/src/main/java/com/yahoo/search/query/profile/ModelObjectMap.java
+++ b/container-search/src/main/java/com/yahoo/search/query/profile/ModelObjectMap.java
@@ -22,8 +22,7 @@ public class ModelObjectMap extends PropertyMap {
      */
     @Override
     protected boolean shouldSet(CompoundName name, Object value) {
-        if (value == null) return true;
-        return ! FieldType.isLegalFieldValue(value);
+        return value != null && ! FieldType.isLegalFieldValue(value);
     }
 
 }
diff --git a/container-search/src/main/java/com/yahoo/search/query/profile/QueryProfileProperties.java b/container-search/src/main/java/com/yahoo/search/query/profile/QueryProfileProperties.java
index 05e3c4fe9a0..701ea7690f4 100644
--- a/container-search/src/main/java/com/yahoo/search/query/profile/QueryProfileProperties.java
+++ b/container-search/src/main/java/com/yahoo/search/query/profile/QueryProfileProperties.java
@@ -32,7 +32,12 @@ public class QueryProfileProperties extends Properties {
 
     /** Values which has been overridden at runtime, or null if none */
     private Map<CompoundName, Object> values = null;
-    /** Query profile references which has been overridden at runtime, or null if none. Earlier values has precedence */
+
+    /**
+     * Query profile references which has been overridden at runtime, possibly to the null value to clear values,
+     * or null if none (i.e this is lazy).
+     * Earlier values has precedence
+     */
     private List<Pair<CompoundName, CompiledQueryProfile>> references = null;
 
     /** Creates an instance from a profile, throws an exception if the given profile is null */
@@ -49,20 +54,21 @@ public class QueryProfileProperties extends Properties {
     public Object get(CompoundName name, Map<String, String> context,
                       com.yahoo.processing.request.Properties substitution) {
         name = unalias(name, context);
-        Object value = null;
-        if (values != null)
-            value = values.get(name);
-        if (value == null) {
-            Pair<CompoundName, CompiledQueryProfile> reference = findReference(name);
-            if (reference != null)
-                return reference.getSecond().get(name.rest(reference.getFirst().size()), context, substitution); // yes; even if null
+        if (values != null && values.containsKey(name))
+            return values.get(name); // Returns this value, even if null
+
+        Pair<CompoundName, CompiledQueryProfile> reference = findReference(name);
+        if (reference != null) {
+            if (reference.getSecond() == null)
+                return null; // cleared
+            else
+                return reference.getSecond().get(name.rest(reference.getFirst().size()), context, substitution); // even if null
         }
 
-        if (value == null)
-            value = profile.get(name, context, substitution);
-        if (value == null)
-            value = super.get(name, context, substitution);
-        return value;
+        Object value = profile.get(name, context, substitution);
+        if (value != null)
+            return value;
+        return super.get(name, context, substitution);
     }
 
     /**
@@ -143,12 +149,26 @@ public class QueryProfileProperties extends Properties {
     }
 
     @Override
-    public Map<String, Object> listProperties(CompoundName path, Map<String,String> context,
+    public void clearAll(CompoundName name, Map<String, String> context) {
+        if (references == null)
+            references = new ArrayList<>();
+        references.add(new Pair<>(name, null));
+
+        if (values != null)
+            values.keySet().removeIf(key -> key.hasPrefix(name));
+    }
+
+    @Override
+    public Map<String, Object> listProperties(CompoundName path, Map<String, String> context,
                                               com.yahoo.processing.request.Properties substitution) {
         path = unalias(path, context);
         if (context == null) context = Collections.emptyMap();
 
-        Map<String, Object> properties = profile.listValues(path, context, substitution);
+        Map<String, Object> properties = new HashMap<>();
+        for (var entry : profile.listValues(path, context, substitution).entrySet()) {
+            if (references != null && containsNullParentOf(path, references)) continue;
+            properties.put(entry.getKey(), entry.getValue());
+        }
         properties.putAll(super.listProperties(path, context, substitution));
 
         if (references != null) {
@@ -165,8 +185,14 @@ public class QueryProfileProperties extends Properties {
                     pathInReference = path.rest(refEntry.getFirst().size());
                     prefixToReferenceKeys = CompoundName.empty;
                 }
-                for (Map.Entry<String, Object> valueEntry : refEntry.getSecond().listValues(pathInReference, context, substitution).entrySet()) {
-                    properties.put(prefixToReferenceKeys.append(new CompoundName(valueEntry.getKey())).toString(), valueEntry.getValue());
+                if (refEntry.getSecond() == null) {
+                    if (refEntry.getFirst().hasPrefix(path))
+                        properties.put(prefixToReferenceKeys.toString(), null);
+                }
+                else {
+                    for (Map.Entry<String, Object> valueEntry : refEntry.getSecond().listValues(pathInReference, context, substitution).entrySet()) {
+                        properties.put(prefixToReferenceKeys.append(new CompoundName(valueEntry.getKey())).toString(), valueEntry.getValue());
+                    }
                 }
             }
 
@@ -241,6 +267,12 @@ public class QueryProfileProperties extends Properties {
         return null;
     }
 
+    private boolean containsNullParentOf(CompoundName path, List<Pair<CompoundName, CompiledQueryProfile>> properties) {
+        if (properties.contains(new Pair<>(path, (CompiledQueryProfile)null))) return true;
+        if (path.size() > 0 && containsNullParentOf(path.first(path.size() - 1), properties)) return true;
+        return false;
+    }
+
     CompoundName unalias(CompoundName name, Map<String,String> context) {
         if (profile.getTypes().isEmpty()) return name;
 
diff --git a/container-search/src/main/java/com/yahoo/search/query/profile/compiled/CompiledQueryProfile.java b/container-search/src/main/java/com/yahoo/search/query/profile/compiled/CompiledQueryProfile.java
index 644d366e7d0..94f4e4747a0 100644
--- a/container-search/src/main/java/com/yahoo/search/query/profile/compiled/CompiledQueryProfile.java
+++ b/container-search/src/main/java/com/yahoo/search/query/profile/compiled/CompiledQueryProfile.java
@@ -134,7 +134,6 @@ public class CompiledQueryProfile extends AbstractComponent implements Cloneable
     public Map<String, Object> listValues(CompoundName prefix, Map<String, String> context, Properties substitution) {
         Map<String, Object> values = new HashMap<>();
         for (Map.Entry<CompoundName, DimensionalValue<ValueWithSource>> entry : entries.entrySet()) {
-            if ( entry.getKey().size() <= prefix.size()) continue;
             if ( ! entry.getKey().hasPrefix(prefix)) continue;
 
             ValueWithSource valueWithSource = entry.getValue().get(context);
diff --git a/container-search/src/main/java/com/yahoo/search/query/properties/PropertyMap.java b/container-search/src/main/java/com/yahoo/search/query/properties/PropertyMap.java
index 4f30331e738..643e215daef 100644
--- a/container-search/src/main/java/com/yahoo/search/query/properties/PropertyMap.java
+++ b/container-search/src/main/java/com/yahoo/search/query/properties/PropertyMap.java
@@ -26,7 +26,10 @@ public class PropertyMap extends Properties {
     /** The properties of this */
     private Map<CompoundName, Object> properties = new LinkedHashMap<>();
 
-    public void set(CompoundName name, Object value, Map<String,String> context) {
+    public void set(CompoundName name, Object value, Map<String, String> context) {
+        if (value == null) // Both clear and forward
+            properties.remove(name);
+
         if (shouldSet(name, value))
             properties.put(name, value);
         else
diff --git a/container-search/src/main/java/com/yahoo/search/query/properties/QueryProperties.java b/container-search/src/main/java/com/yahoo/search/query/properties/QueryProperties.java
index dfe6c2af44b..96f73e925af 100644
--- a/container-search/src/main/java/com/yahoo/search/query/properties/QueryProperties.java
+++ b/container-search/src/main/java/com/yahoo/search/query/properties/QueryProperties.java
@@ -294,7 +294,7 @@ public class QueryProperties extends Properties {
                 super.set(key,value,context);
         }
         catch (Exception e) { // Make sure error messages are informative. This should be moved out of this properties implementation
-            if (e.getMessage().startsWith("Could not set"))
+            if (e.getMessage() != null && e.getMessage().startsWith("Could not set"))
                 throw e;
             else
                 throw new IllegalArgumentException("Could not set '" + key + "' to '" + value + "'", e);
diff --git a/container-search/src/test/java/com/yahoo/search/query/profile/test/QueryProfileTestCase.java b/container-search/src/test/java/com/yahoo/search/query/profile/test/QueryProfileTestCase.java
index bda191ee910..eb1584efe84 100644
--- a/container-search/src/test/java/com/yahoo/search/query/profile/test/QueryProfileTestCase.java
+++ b/container-search/src/test/java/com/yahoo/search/query/profile/test/QueryProfileTestCase.java
@@ -504,7 +504,8 @@ public class QueryProfileTestCase {
         p.set("a","a-value", null);
         p.set("a.b","a.b-value", null);
         Map<String, Object> values = p.compile(null).listValues("a");
-        assertEquals(1, values.size());
+        assertEquals(2, values.size());
+        p.set("a","a-value", null);
         assertEquals("a.b-value", values.get("b"));
     }
 
diff --git a/container-search/src/test/java/com/yahoo/search/test/QueryTestCase.java b/container-search/src/test/java/com/yahoo/search/test/QueryTestCase.java
index e71ed308aaf..34c3da395b7 100644
--- a/container-search/src/test/java/com/yahoo/search/test/QueryTestCase.java
+++ b/container-search/src/test/java/com/yahoo/search/test/QueryTestCase.java
@@ -45,6 +45,8 @@ import org.junit.Test;
 import java.io.UnsupportedEncodingException;
 import java.net.URLEncoder;
 import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
@@ -345,6 +347,61 @@ public class QueryTestCase {
     }
 
     @Test
+    public void testQueryProfileClearAndSet() {
+        QueryProfile profile = new QueryProfile("myProfile");
+        profile.set("b", "b-value", null);
+        Query q = new Query(QueryTestCase.httpEncode("/search?queryProfile=myProfile"), profile.compile(null));
+        assertEquals("b-value", q.properties().get("b"));
+        assertContains(q.properties().listProperties("b"), "b-value");
+
+        q.properties().set("b", null, null);
+        assertContains(q.properties().listProperties("b"), (Object)null);
+
+        q.properties().set("b", "b-value", null);
+        assertEquals("b-value", q.properties().get("b"));
+        assertContains(q.properties().listProperties("b"), "b-value");
+    }
+
+    @Test
+    public void testQueryProfileClearValue() {
+        QueryProfile profile = new QueryProfile("myProfile");
+        profile.set("a", "a-value", null);
+        profile.set("b", "b-value", null);
+        profile.set("b.c", "b.c-value", null);
+        profile.set("b.d", "b.d-value", null);
+        Query q = new Query(QueryTestCase.httpEncode("/search?queryProfile=myProfile"), profile.compile(null));
+        assertEquals("a-value", q.properties().get("a"));
+        assertEquals("b-value", q.properties().get("b"));
+        assertEquals("b.c-value", q.properties().get("b.c"));
+        assertEquals("b.d-value", q.properties().get("b.d"));
+        assertContains(q.properties().listProperties("b"), "b-value", "b.c-value", "b.d-value");
+
+        q.properties().set("a", null, null);
+        assertEquals(null, q.properties().get("a"));
+
+        q.properties().set("b", null, null);
+        assertEquals(null, q.properties().get("b"));
+        assertEquals("b.c-value", q.properties().get("b.c"));
+        assertEquals("b.d-value", q.properties().get("b.d"));
+        assertContains(q.properties().listProperties("b"), null, "b.c-value", "b.d-value");
+
+        q.properties().set("b", "b-value", null);
+        q.properties().set("b.e", "b.e-value", null);
+        q.properties().set("b.f", "b.f-value", null);
+        assertEquals("b-value", q.properties().get("b"));
+        assertEquals("b.e-value", q.properties().get("b.e"));
+        assertContains(q.properties().listProperties("b"), "b-value", "b.c-value", "b.d-value", "b.e-value", "b.f-value");
+
+        q.properties().clearAll("b");
+        assertEquals(null, q.properties().get("b"));
+        assertEquals(null, q.properties().get("b.c"));
+        assertEquals(null, q.properties().get("b.d"));
+        assertEquals(null, q.properties().get("b.e"));
+        assertEquals(null, q.properties().get("b.f"));
+        assertContains(q.properties().listProperties("b"), (Object)null);
+    }
+
+    @Test
     public void testNotEqual() {
         Query q = new Query("/?query=something+test&nocache");
         Query p = new Query("/?query=something+test");
@@ -985,6 +1042,19 @@ public class QueryTestCase {
         assertEquals(expectedDetectionText, mockLinguistics.detector.lastDetectionText);
     }
 
+    private void assertContains(Map<String, Object> properties, Object ... expectedValues) {
+        if (expectedValues == null) {
+            assertEquals(1, properties.size());
+            assertTrue("Contains value null", properties.containsValue(null));
+        }
+        else {
+            assertEquals(properties + " contains values " + Arrays.toString(expectedValues),
+                         expectedValues.length, properties.size());
+            for (Object expectedValue : expectedValues)
+                assertTrue("Contains value " + expectedValue, properties.containsValue(expectedValue));
+        }
+    }
+
     /** A linguistics instance which records the last language detection text passed to it */
     private static class MockLinguistics extends SimpleLinguistics {