Merge pull request #15936 from vespa-engine/geirst/improve-validation-of-struct-field-attributes

Make validation of struct field attributes reflect reality.

6 files changed, 75 insertions, 13 deletions

diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/document/ComplexAttributeFieldUtils.java b/config-model/src/main/java/com/yahoo/searchdefinition/document/ComplexAttributeFieldUtils.java
index c3ac8c77173..c3c55a77f85 100644
--- a/config-model/src/main/java/com/yahoo/searchdefinition/document/ComplexAttributeFieldUtils.java
+++ b/config-model/src/main/java/com/yahoo/searchdefinition/document/ComplexAttributeFieldUtils.java
@@ -67,6 +67,10 @@ public class ComplexAttributeFieldUtils {
                     if (!isPrimitiveType(attribute)) {
                         return false;
                     }
+                } else if (structField.wasConfiguredToDoAttributing()) {
+                    if (!isPrimitiveType(structField.getDataType())) {
+                        return false;
+                    }
                 }
             }
             return true;
@@ -75,7 +79,7 @@ public class ComplexAttributeFieldUtils {
         }
     }
 
-    private static boolean isPrimitiveType(Attribute attribute) {
+    public static boolean isPrimitiveType(Attribute attribute) {
         return attribute.getCollectionType().equals(Attribute.CollectionType.SINGLE) &&
                 isPrimitiveType(attribute.getDataType());
     }
diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/document/ImmutableImportedSDField.java b/config-model/src/main/java/com/yahoo/searchdefinition/document/ImmutableImportedSDField.java
index 9c183d99435..32671ceee5b 100644
--- a/config-model/src/main/java/com/yahoo/searchdefinition/document/ImmutableImportedSDField.java
+++ b/config-model/src/main/java/com/yahoo/searchdefinition/document/ImmutableImportedSDField.java
@@ -78,6 +78,11 @@ public class ImmutableImportedSDField implements ImmutableSDField {
     }
 
     @Override
+    public boolean wasConfiguredToDoAttributing() {
+        return importedField.targetField().wasConfiguredToDoAttributing();
+    }
+
+    @Override
     public DataType getDataType() {
         return importedField.targetField().getDataType();
     }
diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/document/ImmutableSDField.java b/config-model/src/main/java/com/yahoo/searchdefinition/document/ImmutableSDField.java
index 7f92e676118..45828db92a5 100644
--- a/config-model/src/main/java/com/yahoo/searchdefinition/document/ImmutableSDField.java
+++ b/config-model/src/main/java/com/yahoo/searchdefinition/document/ImmutableSDField.java
@@ -37,6 +37,14 @@ public interface ImmutableSDField {
 
     boolean usesStructOrMap();
 
+    /**
+     * Whether this field at some time was configured to do attributing.
+     *
+     * This function can typically return a different value than doesAttributing(),
+     * which uses the final state of the underlying indexing script instead.
+     */
+    boolean wasConfiguredToDoAttributing();
+
     DataType getDataType();
 
     Index getIndex(String name);
diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/document/SDField.java b/config-model/src/main/java/com/yahoo/searchdefinition/document/SDField.java
index 3fea5cc16e9..d81394382c2 100644
--- a/config-model/src/main/java/com/yahoo/searchdefinition/document/SDField.java
+++ b/config-model/src/main/java/com/yahoo/searchdefinition/document/SDField.java
@@ -117,6 +117,8 @@ public class SDField extends Field implements TypedKey, FieldOperationContainer,
 
     private boolean isExtraField = false;
 
+    private boolean wasConfiguredToDoAttributing = false;
+
     /**
      * Creates a new field. This method is only used to create reserved fields.
      *
@@ -414,6 +416,11 @@ public class SDField extends Field implements TypedKey, FieldOperationContainer,
         return (dt != null);
     }
 
+    @Override
+    public boolean wasConfiguredToDoAttributing() {
+        return wasConfiguredToDoAttributing;
+    }
+
     /** Parse an indexing expression which will use the simple linguistics implementatino suitable for testing */
     public void parseIndexingScript(String script) {
         parseIndexingScript(script, new SimpleLinguistics());
@@ -438,6 +445,9 @@ public class SDField extends Field implements TypedKey, FieldOperationContainer,
         if (indexingScript.isEmpty()) {
             return; // TODO: This causes empty expressions not to be propagate to struct fields!! BAD BAD BAD!!
         }
+        if (!wasConfiguredToDoAttributing()) {
+            wasConfiguredToDoAttributing = doesAttributing();
+        }
         if (!usesStructOrMap()) {
             new ExpressionVisitor() {
 
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/ComplexAttributeFieldsValidator.java b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/ComplexAttributeFieldsValidator.java
index 43c1a88b0a1..4cc8439566e 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/ComplexAttributeFieldsValidator.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/ComplexAttributeFieldsValidator.java
@@ -66,18 +66,26 @@ public class ComplexAttributeFieldsValidator extends Validator {
     }
 
     private static String toString(ImmutableSDField field) {
-        return field.getName() + " (" + String.join(", ", getStructFieldAttributes(field.getStructFields())) + ")";
+        return field.getName() + " (" + String.join(", ", getStructFieldAttributes(field.getStructFields(), false)) + ")";
     }
 
     private static boolean hasStructFieldAttributes(Collection<? extends ImmutableSDField> structFields) {
-        return !getStructFieldAttributes(structFields).isEmpty();
+        return !getStructFieldAttributes(structFields, true).isEmpty();
     }
 
-    private static List<String> getStructFieldAttributes(Collection<? extends ImmutableSDField> structFields) {
-        List<String> result = new ArrayList<>();
-        for (ImmutableSDField structField : structFields) {
-            structField.getAttributes().values().forEach(attr -> result.add(attr.getName()));
-            result.addAll(getStructFieldAttributes(structField.getStructFields()));
+    private static List<String> getStructFieldAttributes(Collection<? extends ImmutableSDField> structFields,
+                                                         boolean returnAllTypes) {
+        var result = new ArrayList<String>();
+        for (var structField : structFields) {
+            for (var attr : structField.getAttributes().values()) {
+                if (returnAllTypes || !ComplexAttributeFieldUtils.isPrimitiveType(attr)) {
+                    result.add(attr.getName());
+                }
+            }
+            if (structField.usesStructOrMap() && structField.wasConfiguredToDoAttributing()) {
+                result.add(structField.getName());
+            }
+            result.addAll(getStructFieldAttributes(structField.getStructFields(), returnAllTypes));
         }
         return result;
     }
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/ComplexAttributeFieldsValidatorTestCase.java b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/ComplexAttributeFieldsValidatorTestCase.java
index 3ba3745f46e..76f34cf4a81 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/ComplexAttributeFieldsValidatorTestCase.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/ComplexAttributeFieldsValidatorTestCase.java
@@ -29,11 +29,7 @@ public class ComplexAttributeFieldsValidatorTestCase {
     @Test
     public void throws_exception_when_unsupported_complex_fields_have_struct_field_attributes() throws IOException, SAXException {
         exceptionRule.expect(IllegalArgumentException.class);
-        exceptionRule.expectMessage("For cluster 'mycluster', search 'test': " +
-                "The following complex fields do not support using struct field attributes: " +
-                "struct_array (struct_array.f1), struct_map (struct_map.key, struct_map.value.f1). " +
-                "Only supported for the following complex field types: array or map of struct with primitive types, map of primitive types");
-
+        exceptionRule.expectMessage(getExpectedMessage("struct_array (struct_array.f1), struct_map (struct_map.value.f1)"));
         createModelAndValidate(joinLines("search test {",
                 "  document test {",
                 "    struct s { field f1 type array<int> {} }",
@@ -49,6 +45,37 @@ public class ComplexAttributeFieldsValidatorTestCase {
     }
 
     @Test
+    public void throws_exception_when_nested_struct_array_is_specified_as_struct_field_attribute() throws IOException, SAXException {
+        exceptionRule.expect(IllegalArgumentException.class);
+        exceptionRule.expectMessage(getExpectedMessage("docTopics (docTopics.topics)"));
+        createModelAndValidate(joinLines(
+                "schema test {",
+                "document test {",
+                "struct topic {",
+                "  field id type string {}",
+                "  field label type string {}",
+                "}",
+                "struct docTopic {",
+                "  field id type string {}",
+                "  field topics type array<topic> {}",
+                "}",
+                "field docTopics type array<docTopic> {",
+                "  indexing: summary",
+                "  struct-field id { indexing: attribute }",
+                "  struct-field topics { indexing: attribute }",
+                "}",
+                "}",
+                "}"));
+    }
+
+    private String getExpectedMessage(String unsupportedFields) {
+        return "For cluster 'mycluster', search 'test': " +
+                "The following complex fields do not support using struct field attributes: " +
+                unsupportedFields + ". " +
+                "Only supported for the following complex field types: array or map of struct with primitive types, map of primitive types";
+    }
+
+    @Test
     public void validation_passes_when_only_supported_struct_field_attributes_are_used() throws IOException, SAXException {
         createModelAndValidate(joinLines("search test {",
                 "  document test {",