Merge pull request #7411 from vespa-engine/lesters/revert-revert-disallow-complex-keys

Revert "Revert "Disallow complex types as keys for nested types in arrays""

9 files changed, 106 insertions, 33 deletions

diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/processing/DisallowComplexMapAndWsetKeyTypes.java b/config-model/src/main/java/com/yahoo/searchdefinition/processing/DisallowComplexMapAndWsetKeyTypes.java
index 076161a8584..1994b1096ce 100644
--- a/config-model/src/main/java/com/yahoo/searchdefinition/processing/DisallowComplexMapAndWsetKeyTypes.java
+++ b/config-model/src/main/java/com/yahoo/searchdefinition/processing/DisallowComplexMapAndWsetKeyTypes.java
@@ -2,6 +2,7 @@
 package com.yahoo.searchdefinition.processing;
 
 import com.yahoo.config.application.api.DeployLogger;
+import com.yahoo.document.ArrayDataType;
 import com.yahoo.searchdefinition.RankProfileRegistry;
 import com.yahoo.document.DataType;
 import com.yahoo.document.MapDataType;
@@ -26,20 +27,30 @@ public class DisallowComplexMapAndWsetKeyTypes extends Processor {
     public void process(boolean validate, boolean documentsOnly) {
         if ( ! validate) return;
 
-    	// TODO also traverse struct types to search for bad map or wset types there. Do this after document manager is fixed, do
-    	// not start using the static stuff on SDDocumentTypes any more.
+    	// TODO also traverse struct types to search for bad map or wset types.
+        // Do this after document manager is fixed, do not start using the static stuff on SDDocumentTypes any more.
         for (SDField field : search.allConcreteFields()) {
-            if (field.getDataType() instanceof WeightedSetDataType) {
-                DataType nestedType = ((WeightedSetDataType)field.getDataType()).getNestedType();
-                if ( ! (nestedType instanceof PrimitiveDataType)) {
-                    fail(search, field, "Weighted set must have a primitive key type.");
-                }
-            } else if (field.getDataType() instanceof MapDataType) {
-                if ( ! (((MapDataType)field.getDataType()).getKeyType() instanceof PrimitiveDataType)) {
-                    fail(search, field, "Map key type must be a primitive type");
-                }
+            checkFieldType(field, field.getDataType());
+        }
+    }
+
+    private void checkFieldType(SDField field, DataType dataType) {
+        if (dataType instanceof ArrayDataType) {
+            DataType nestedType = ((ArrayDataType) dataType).getNestedType();
+            checkFieldType(field, nestedType);
+        } else if (dataType instanceof WeightedSetDataType) {
+            DataType nestedType = ((WeightedSetDataType) dataType).getNestedType();
+            if ( ! (nestedType instanceof PrimitiveDataType)) {
+                fail(search, field, "Weighted set must have a primitive key type.");
             }
+        } else if (dataType instanceof MapDataType) {
+            DataType keyType = ((MapDataType) dataType).getKeyType();
+            if ( ! (keyType instanceof PrimitiveDataType)) {
+                fail(search, field, "Map key type must be a primitive type.");
+            }
+            checkFieldType(field, ((MapDataType) dataType).getValueType());
         }
+
     }
 
 }
diff --git a/config-model/src/test/configmodel/types/documentmanager.cfg b/config-model/src/test/configmodel/types/documentmanager.cfg
index 6b01934307a..aaaebccecfd 100644
--- a/config-model/src/test/configmodel/types/documentmanager.cfg
+++ b/config-model/src/test/configmodel/types/documentmanager.cfg
@@ -212,11 +212,11 @@ datatype[24].structtype[0].field[27].detailedtype ""
 datatype[24].structtype[0].field[28].name "summaryfeatures"
 datatype[24].structtype[0].field[28].datatype 2
 datatype[24].structtype[0].field[28].detailedtype ""
-datatype[25].id 171503364
-datatype[25].maptype[0].keytype 1707615575
-datatype[25].maptype[0].valtype 0
-datatype[26].id 1100964733
-datatype[26].arraytype[0].datatype 171503364
+datatype[25].id -372512406
+datatype[25].maptype[0].keytype 0
+datatype[25].maptype[0].valtype 1707615575
+datatype[26].id 1416345047
+datatype[26].arraytype[0].datatype -372512406
 datatype[27].id 348447225
 datatype[27].structtype[0].name "types.body"
 datatype[27].structtype[0].version 0
@@ -225,7 +225,7 @@ datatype[27].structtype[0].compresslevel 0
 datatype[27].structtype[0].compressthreshold 95
 datatype[27].structtype[0].compressminsize 800
 datatype[27].structtype[0].field[0].name "complexarray"
-datatype[27].structtype[0].field[0].datatype 1100964733
+datatype[27].structtype[0].field[0].datatype 1416345047
 datatype[27].structtype[0].field[0].detailedtype ""
 datatype[28].id -853072901
 datatype[28].documenttype[0].name "types"
diff --git a/config-model/src/test/configmodel/types/documenttypes.cfg b/config-model/src/test/configmodel/types/documenttypes.cfg
index dc7962adec7..97785618e8e 100644
--- a/config-model/src/test/configmodel/types/documenttypes.cfg
+++ b/config-model/src/test/configmodel/types/documenttypes.cfg
@@ -560,11 +560,11 @@ documenttype[0].datatype[23].sstruct.field[28].id 1840337115
 documenttype[0].datatype[23].sstruct.field[28].id_v6 1981648971
 documenttype[0].datatype[23].sstruct.field[28].datatype 2
 documenttype[0].datatype[23].sstruct.field[28].detailedtype ""
-documenttype[0].datatype[24].id 171503364
+documenttype[0].datatype[24].id -372512406
 documenttype[0].datatype[24].type MAP
 documenttype[0].datatype[24].array.element.id 0
-documenttype[0].datatype[24].map.key.id 1707615575
-documenttype[0].datatype[24].map.value.id 0
+documenttype[0].datatype[24].map.key.id 0
+documenttype[0].datatype[24].map.value.id 1707615575
 documenttype[0].datatype[24].wset.key.id 0
 documenttype[0].datatype[24].wset.createifnonexistent false
 documenttype[0].datatype[24].wset.removeifzero false
@@ -575,9 +575,9 @@ documenttype[0].datatype[24].sstruct.compression.type NONE
 documenttype[0].datatype[24].sstruct.compression.level 0
 documenttype[0].datatype[24].sstruct.compression.threshold 95
 documenttype[0].datatype[24].sstruct.compression.minsize 200
-documenttype[0].datatype[25].id 1100964733
+documenttype[0].datatype[25].id 1416345047
 documenttype[0].datatype[25].type ARRAY
-documenttype[0].datatype[25].array.element.id 171503364
+documenttype[0].datatype[25].array.element.id -372512406
 documenttype[0].datatype[25].map.key.id 0
 documenttype[0].datatype[25].map.value.id 0
 documenttype[0].datatype[25].wset.key.id 0
@@ -606,9 +606,9 @@ documenttype[0].datatype[26].sstruct.compression.level 0
 documenttype[0].datatype[26].sstruct.compression.threshold 95
 documenttype[0].datatype[26].sstruct.compression.minsize 200
 documenttype[0].datatype[26].sstruct.field[0].name "complexarray"
-documenttype[0].datatype[26].sstruct.field[0].id 1028383787
+documenttype[0].datatype[26].sstruct.field[0].id 795629533
 documenttype[0].datatype[26].sstruct.field[0].id_v6 658530305
-documenttype[0].datatype[26].sstruct.field[0].datatype 1100964733
+documenttype[0].datatype[26].sstruct.field[0].datatype 1416345047
 documenttype[0].datatype[26].sstruct.field[0].detailedtype ""
 documenttype[0].fieldsets{[document]}.fields[0] "Folders"
 documenttype[0].fieldsets{[document]}.fields[1] "abyte"
diff --git a/config-model/src/test/configmodel/types/types.sd b/config-model/src/test/configmodel/types/types.sd
index c55c5611bc5..f34a6776b11 100644
--- a/config-model/src/test/configmodel/types/types.sd
+++ b/config-model/src/test/configmodel/types/types.sd
@@ -92,7 +92,7 @@ search types {
 
     field arrarr type array<array<array<string>>> {header}
     field maparr type array<map<string, string>> {header}
-    field complexarray type array< map<array<array<string>>, int> > {body}
+    field complexarray type array< map<int, array<array<string>>> > {body}
 
     struct mystruct {
       field bytearr type array<byte>{}
diff --git a/config-model/src/test/derived/types/documentmanager.cfg b/config-model/src/test/derived/types/documentmanager.cfg
index c490db59736..647c26e1316 100644
--- a/config-model/src/test/derived/types/documentmanager.cfg
+++ b/config-model/src/test/derived/types/documentmanager.cfg
@@ -209,11 +209,11 @@ datatype[].structtype[].field[].detailedtype ""
 datatype[].structtype[].field[].name "summaryfeatures"
 datatype[].structtype[].field[].datatype 2
 datatype[].structtype[].field[].detailedtype ""
-datatype[].id 171503364
-datatype[].maptype[].keytype 1707615575
-datatype[].maptype[].valtype 0
-datatype[].id 1100964733
-datatype[].arraytype[].datatype 171503364
+datatype[].id -372512406
+datatype[].maptype[].keytype 0
+datatype[].maptype[].valtype 1707615575
+datatype[].id 1416345047
+datatype[].arraytype[].datatype -372512406
 datatype[].id 348447225
 datatype[].structtype[].name "types.body"
 datatype[].structtype[].version 0
@@ -222,7 +222,7 @@ datatype[].structtype[].compresslevel 0
 datatype[].structtype[].compressthreshold 95
 datatype[].structtype[].compressminsize 800
 datatype[].structtype[].field[].name "complexarray"
-datatype[].structtype[].field[].datatype 1100964733
+datatype[].structtype[].field[].datatype 1416345047
 datatype[].structtype[].field[].detailedtype ""
 datatype[].id -853072901
 datatype[].documenttype[].name "types"
diff --git a/config-model/src/test/derived/types/types.sd b/config-model/src/test/derived/types/types.sd
index 0590625e1c6..c908b648340 100644
--- a/config-model/src/test/derived/types/types.sd
+++ b/config-model/src/test/derived/types/types.sd
@@ -87,7 +87,7 @@ search types {
 
     field arrarr type array<array<array<string>>> {header}
     field maparr type array<map<string, string>> {header}
-    field complexarray type array< map<array<array<string>>, int> > {body}
+    field complexarray type array< map<int, array<array<string>>> > {body}
 
     struct mystruct {
       field bytearr type array<byte>{}
diff --git a/config-model/src/test/java/com/yahoo/searchdefinition/RankingExpressionShadowingTestCase.java b/config-model/src/test/java/com/yahoo/searchdefinition/RankingExpressionShadowingTestCase.java
index 4f99922a422..6a1e5b207c6 100644
--- a/config-model/src/test/java/com/yahoo/searchdefinition/RankingExpressionShadowingTestCase.java
+++ b/config-model/src/test/java/com/yahoo/searchdefinition/RankingExpressionShadowingTestCase.java
@@ -16,6 +16,9 @@ import java.util.List;
 
 import static org.junit.Assert.assertEquals;
 
+/**
+ * @author lesters
+ */
 public class RankingExpressionShadowingTestCase extends SearchDefinitionTestCase {
 
     @Test
diff --git a/config-model/src/test/java/com/yahoo/searchdefinition/processing/DisallowComplexMapAndWsetKeyTypesTestCase.java b/config-model/src/test/java/com/yahoo/searchdefinition/processing/DisallowComplexMapAndWsetKeyTypesTestCase.java
new file mode 100644
index 00000000000..d6e31ac8934
--- /dev/null
+++ b/config-model/src/test/java/com/yahoo/searchdefinition/processing/DisallowComplexMapAndWsetKeyTypesTestCase.java
@@ -0,0 +1,52 @@
+// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.searchdefinition.processing;
+
+import com.yahoo.searchdefinition.RankProfileRegistry;
+import com.yahoo.searchdefinition.SearchBuilder;
+import com.yahoo.searchdefinition.parser.ParseException;
+import org.junit.Test;
+
+/**
+ * @author lesters
+ */
+public class DisallowComplexMapAndWsetKeyTypesTestCase {
+
+    @Test(expected = IllegalArgumentException.class)
+    public void requireThatComplexTypesForMapKeysFail() throws ParseException {
+        testFieldType("map<mystruct,string>");
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void requireThatComplexTypesForWsetFail() throws ParseException {
+        testFieldType("weightedset<mystruct>");
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void requireThatNestedComplexTypesForMapFail() throws ParseException {
+        testFieldType("array<map<mystruct,string>>");
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void requireThatNestedComplexTypesForWsetFail() throws ParseException {
+        testFieldType("array<weightedset<mystruct>>");
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void requireThatDeepNestedComplexTypesForMapFail() throws ParseException {
+        testFieldType("map<string,map<mystruct,string>>");
+    }
+
+    private void testFieldType(String fieldType) throws ParseException {
+        RankProfileRegistry rankProfileRegistry = new RankProfileRegistry();
+        SearchBuilder builder = new SearchBuilder(rankProfileRegistry);
+        builder.importString(
+                "search test {\n" +
+                        "    document test { \n" +
+                        "        struct mystruct {}\n" +
+                        "        field a type " + fieldType + " {}\n" +
+                        "    }\n" +
+                        "}\n");
+        builder.build();
+    }
+
+}
diff --git a/document/src/main/java/com/yahoo/document/json/JsonSerializationHelper.java b/document/src/main/java/com/yahoo/document/json/JsonSerializationHelper.java
index 8e934001381..afe14cf1e6a 100644
--- a/document/src/main/java/com/yahoo/document/json/JsonSerializationHelper.java
+++ b/document/src/main/java/com/yahoo/document/json/JsonSerializationHelper.java
@@ -2,9 +2,11 @@
 package com.yahoo.document.json;
 
 import com.fasterxml.jackson.core.JsonGenerator;
+import com.yahoo.document.DataType;
 import com.yahoo.document.DocumentId;
 import com.yahoo.document.Field;
 import com.yahoo.document.PositionDataType;
+import com.yahoo.document.PrimitiveDataType;
 import com.yahoo.document.datatypes.Array;
 import com.yahoo.document.datatypes.ByteFieldValue;
 import com.yahoo.document.datatypes.CollectionFieldValue;
@@ -186,7 +188,12 @@ public class JsonSerializationHelper {
             generator.writeStartObject();
 
             for (Map.Entry<K, V> entry : map.entrySet()) {
-                generator.writeFieldName(entry.getKey().toString());
+                K key = entry.getKey();
+                DataType keyType = key.getDataType();
+                if ( ! (keyType instanceof PrimitiveDataType)) {
+                    throw new IllegalArgumentException("Can't use complex types as keys for map fields. Type: " + keyType);
+                }
+                generator.writeFieldName(key.toString());
                 entry.getValue().serialize(null, fieldWriter);
             }