Merge pull request #27618 from vespa-engine/geirst/improve-complex-fields-validator

Improve validation of complex fields using struct field attributes.

7 files changed, 96 insertions, 37 deletions

diff --git a/config-model/src/main/java/com/yahoo/schema/derived/AttributeFields.java b/config-model/src/main/java/com/yahoo/schema/derived/AttributeFields.java
index c3531d03d3f..12ca67bf2c9 100644
--- a/config-model/src/main/java/com/yahoo/schema/derived/AttributeFields.java
+++ b/config-model/src/main/java/com/yahoo/schema/derived/AttributeFields.java
@@ -51,9 +51,9 @@ public class AttributeFields extends Derived implements AttributesConfig.Produce
         if (unsupportedFieldType(field)) {
             return; // Ignore complex struct and map fields for indexed search (only supported for streaming search)
         }
-        if (isArrayOfSimpleStruct(field)) {
+        if (isArrayOfSimpleStruct(field, false)) {
             deriveArrayOfSimpleStruct(field);
-        } else if (isMapOfSimpleStruct(field)) {
+        } else if (isMapOfSimpleStruct(field, false)) {
             deriveMapOfSimpleStruct(field);
         } else if (isMapOfPrimitiveType(field)) {
             deriveMapOfPrimitiveType(field);
diff --git a/config-model/src/main/java/com/yahoo/schema/derived/ImportedFields.java b/config-model/src/main/java/com/yahoo/schema/derived/ImportedFields.java
index fa3f49f06d5..122048d02b9 100644
--- a/config-model/src/main/java/com/yahoo/schema/derived/ImportedFields.java
+++ b/config-model/src/main/java/com/yahoo/schema/derived/ImportedFields.java
@@ -61,9 +61,9 @@ public class ImportedFields extends Derived implements ImportedFieldsConfig.Prod
         ImmutableSDField targetField = field.targetField();
         if (GeoPos.isAnyPos(targetField)) {
             // no action needed
-        } else if (isArrayOfSimpleStruct(targetField)) {
+        } else if (isArrayOfSimpleStruct(targetField, false)) {
             considerNestedFields(builder, field);
-        } else if (isMapOfSimpleStruct(targetField)) {
+        } else if (isMapOfSimpleStruct(targetField, false)) {
             considerSimpleField(builder, field.getNestedField("key"));
             considerNestedFields(builder, field.getNestedField("value"));
         } else if (isMapOfPrimitiveType(targetField)) {
diff --git a/config-model/src/main/java/com/yahoo/schema/document/ComplexAttributeFieldUtils.java b/config-model/src/main/java/com/yahoo/schema/document/ComplexAttributeFieldUtils.java
index 993c9180f78..5e4ee6d4b27 100644
--- a/config-model/src/main/java/com/yahoo/schema/document/ComplexAttributeFieldUtils.java
+++ b/config-model/src/main/java/com/yahoo/schema/document/ComplexAttributeFieldUtils.java
@@ -22,26 +22,31 @@ import com.yahoo.document.StructDataType;
 public class ComplexAttributeFieldUtils {
 
     public static boolean isSupportedComplexField(ImmutableSDField field) {
-        return (isArrayOfSimpleStruct(field) ||
-                isMapOfSimpleStruct(field) ||
+        return isSupportedComplexField(field, false);
+    }
+
+    // TODO: Remove the stricterValidation flag when this is changed to being always on.
+    public static boolean isSupportedComplexField(ImmutableSDField field, boolean stricterValidation) {
+        return (isArrayOfSimpleStruct(field, stricterValidation) ||
+                isMapOfSimpleStruct(field, stricterValidation) ||
                 isMapOfPrimitiveType(field));
     }
 
-    public static boolean isArrayOfSimpleStruct(ImmutableSDField field) {
+    public static boolean isArrayOfSimpleStruct(ImmutableSDField field, boolean stricterValidation) {
         if (field.getDataType() instanceof ArrayDataType) {
             ArrayDataType arrayType = (ArrayDataType)field.getDataType();
-            return isStructWithPrimitiveStructFieldAttributes(arrayType.getNestedType(), field);
+            return isStructWithPrimitiveStructFieldAttributes(arrayType.getNestedType(), field, stricterValidation);
         } else {
             return false;
         }
     }
 
-    public static boolean isMapOfSimpleStruct(ImmutableSDField field) {
+    public static boolean isMapOfSimpleStruct(ImmutableSDField field, boolean stricterValidation) {
         if (field.getDataType() instanceof MapDataType) {
             MapDataType mapType = (MapDataType)field.getDataType();
             return isPrimitiveType(mapType.getKeyType()) &&
                     isStructWithPrimitiveStructFieldAttributes(mapType.getValueType(),
-                            field.getStructField("value"));
+                            field.getStructField("value"), stricterValidation);
         } else {
             return false;
         }
@@ -57,7 +62,7 @@ public class ComplexAttributeFieldUtils {
         }
     }
 
-    private static boolean isStructWithPrimitiveStructFieldAttributes(DataType type, ImmutableSDField field) {
+    private static boolean isStructWithPrimitiveStructFieldAttributes(DataType type, ImmutableSDField field, boolean stricterValidation) {
         if (type instanceof StructDataType && ! GeoPos.isPos(type)) {
             for (ImmutableSDField structField : field.getStructFields()) {
                 Attribute attribute = structField.getAttributes().get(structField.getName());
@@ -70,6 +75,9 @@ public class ComplexAttributeFieldUtils {
                         return false;
                     }
                 }
+                if (stricterValidation && !structField.isImportedField() && hasStructFieldAttributes(structField)) {
+                    return false;
+                }
             }
             return true;
         } else {
@@ -77,6 +85,19 @@ public class ComplexAttributeFieldUtils {
         }
     }
 
+    private static boolean hasStructFieldAttributes(ImmutableSDField field) {
+        for (var structField : field.getStructFields()) {
+            var attribute = structField.getAttributes().get(structField.getName());
+            if (attribute != null) {
+                return true;
+            }
+            if (hasStructFieldAttributes(structField)) {
+                return true;
+            }
+        }
+        return false;
+    }
+
     public static boolean isPrimitiveType(Attribute attribute) {
         return attribute.getCollectionType().equals(Attribute.CollectionType.SINGLE) &&
                 isPrimitiveType(attribute.getDataType());
@@ -92,9 +113,9 @@ public class ComplexAttributeFieldUtils {
     }
 
     public static boolean isComplexFieldWithOnlyStructFieldAttributes(ImmutableSDField field) {
-        if (isArrayOfSimpleStruct(field)) {
+        if (isArrayOfSimpleStruct(field, false)) {
             return hasOnlyStructFieldAttributes(field);
-        } else if (isMapOfSimpleStruct(field)) {
+        } else if (isMapOfSimpleStruct(field, false)) {
             return (field.getStructField("key").hasSingleAttribute()) &&
                     hasOnlyStructFieldAttributes(field.getStructField("value"));
         } else if (isMapOfPrimitiveType(field)) {
diff --git a/config-model/src/main/java/com/yahoo/schema/processing/ImportedFieldsResolver.java b/config-model/src/main/java/com/yahoo/schema/processing/ImportedFieldsResolver.java
index ee465be44f2..8e44bd026a3 100644
--- a/config-model/src/main/java/com/yahoo/schema/processing/ImportedFieldsResolver.java
+++ b/config-model/src/main/java/com/yahoo/schema/processing/ImportedFieldsResolver.java
@@ -52,9 +52,9 @@ public class ImportedFieldsResolver extends Processor {
         ImmutableSDField targetField = getTargetField(importedField, reference);
         if (GeoPos.isAnyPos(targetField)) {
             resolveImportedPositionField(importedField, reference, targetField, validate);
-        } else if (isArrayOfSimpleStruct(targetField)) {
+        } else if (isArrayOfSimpleStruct(targetField, false)) {
             resolveImportedArrayOfStructField(importedField, reference, targetField, validate);
-        } else if (isMapOfSimpleStruct(targetField)) {
+        } else if (isMapOfSimpleStruct(targetField, false)) {
             resolveImportedMapOfStructField(importedField, reference, targetField, validate);
         } else if (isMapOfPrimitiveType(targetField)) {
             resolveImportedMapOfPrimitiveField(importedField, reference, targetField, validate);
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/ComplexFieldsWithStructFieldAttributesValidator.java b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/ComplexFieldsWithStructFieldAttributesValidator.java
index 8515c34a377..d2999a24775 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/ComplexFieldsWithStructFieldAttributesValidator.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/ComplexFieldsWithStructFieldAttributesValidator.java
@@ -1,6 +1,7 @@
 // Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.vespa.model.application.validation;
 
+import com.yahoo.config.application.api.DeployLogger;
 import com.yahoo.config.model.deploy.DeployState;
 import com.yahoo.schema.Schema;
 import com.yahoo.schema.derived.SchemaInfo;
@@ -13,6 +14,7 @@ import com.yahoo.vespa.model.search.SearchCluster;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
+import java.util.logging.Level;
 import java.util.stream.Collectors;
 
 /**
@@ -31,34 +33,44 @@ public class ComplexFieldsWithStructFieldAttributesValidator extends Validator {
             if (cluster.isStreaming()) continue;
 
             for (SchemaInfo spec : cluster.schemas().values()) {
-                validateComplexFields(cluster.getClusterName(), spec.fullSchema());
+                validateComplexFields(cluster.getClusterName(), spec.fullSchema(), deployState.getDeployLogger());
             }
         }
     }
 
-    private static void validateComplexFields(String clusterName, Schema schema) {
-        String unsupportedFields = schema.allFields()
-                                         .filter(field -> isUnsupportedComplexField(field))
-                                         .map(ComplexFieldsWithStructFieldAttributesValidator::toString)
-                                         .collect(Collectors.joining(", "));
-
+    private static void validateComplexFields(String clusterName, Schema schema, DeployLogger logger) {
+        String unsupportedFields = validateComplexFields(clusterName, schema, false);
+        if (!unsupportedFields.isEmpty()) {
+            throw new IllegalArgumentException(getErrorMessage(clusterName, schema, unsupportedFields));
+        }
+        unsupportedFields = validateComplexFields(clusterName, schema, true);
         if (!unsupportedFields.isEmpty()) {
-            throw new IllegalArgumentException(
-                    String.format("For cluster '%s', search '%s': The following complex fields do not support using struct field attributes: %s. " +
-                                  "Only supported for the following complex field types: array or map of struct with primitive types, map of primitive types. " +
-                                  "The supported primitive types are: byte, int, long, float, double and string",
-                                  clusterName, schema.getName(), unsupportedFields));
+            logger.logApplicationPackage(Level.WARNING, getErrorMessage(clusterName, schema, unsupportedFields));
         }
     }
 
-    private static boolean isUnsupportedComplexField(ImmutableSDField field) {
+    private static String validateComplexFields(String clusterName, Schema schema, boolean stricterValidation) {
+        return schema.allFields()
+                .filter(field -> isUnsupportedComplexField(field, stricterValidation))
+                .map(ComplexFieldsWithStructFieldAttributesValidator::toString)
+                .collect(Collectors.joining(", "));
+    }
+
+    private static String getErrorMessage(String clusterName, Schema schema, String unsupportedFields) {
+        return String.format("For cluster '%s', search '%s': The following complex fields do not support using struct field attributes: %s. " +
+                             "Only supported for the following complex field types: array or map of struct with primitive types, map of primitive types. " +
+                             "The supported primitive types are: byte, int, long, float, double and string",
+                             clusterName, schema.getName(), unsupportedFields);
+    }
+
+    private static boolean isUnsupportedComplexField(ImmutableSDField field, boolean stricterValidation) {
         return (field.usesStructOrMap() &&
-                !isSupportedComplexField(field) &&
+                !isSupportedComplexField(field, stricterValidation) &&
                 hasStructFieldAttributes(field.getStructFields()));
     }
 
-    private static boolean isSupportedComplexField(ImmutableSDField field) {
-        return (ComplexAttributeFieldUtils.isSupportedComplexField(field) ||
+    private static boolean isSupportedComplexField(ImmutableSDField field, boolean stricterValidation) {
+        return (ComplexAttributeFieldUtils.isSupportedComplexField(field, stricterValidation) ||
                 GeoPos.isAnyPos(field));
     }
 
@@ -82,7 +94,8 @@ public class ComplexFieldsWithStructFieldAttributesValidator extends Validator {
             if (structField.usesStructOrMap() && structField.wasConfiguredToDoAttributing()) {
                 result.add(structField.getName());
             }
-            result.addAll(getStructFieldAttributes(structField.getStructFields(), returnAllTypes));
+            // If we encounter struct field attributes underneath this level, those are not supported and should be returned.
+            result.addAll(getStructFieldAttributes(structField.getStructFields(), true));
         }
         return result;
     }
diff --git a/config-model/src/test/java/com/yahoo/schema/document/ComplexAttributeFieldUtilsTestCase.java b/config-model/src/test/java/com/yahoo/schema/document/ComplexAttributeFieldUtilsTestCase.java
index 7a89f52268f..310ede6bae2 100644
--- a/config-model/src/test/java/com/yahoo/schema/document/ComplexAttributeFieldUtilsTestCase.java
+++ b/config-model/src/test/java/com/yahoo/schema/document/ComplexAttributeFieldUtilsTestCase.java
@@ -30,11 +30,11 @@ public class ComplexAttributeFieldUtilsTestCase {
         }
 
         boolean isArrayOfSimpleStruct() {
-            return ComplexAttributeFieldUtils.isArrayOfSimpleStruct(field());
+            return ComplexAttributeFieldUtils.isArrayOfSimpleStruct(field(), false);
         }
 
         boolean isMapOfSimpleStruct() {
-            return ComplexAttributeFieldUtils.isMapOfSimpleStruct(field());
+            return ComplexAttributeFieldUtils.isMapOfSimpleStruct(field(), false);
         }
 
         boolean isMapOfPrimitiveType() {
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/ComplexFieldsValidatorTestCase.java b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/ComplexFieldsValidatorTestCase.java
index c673d5899e8..04abd4e4836 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/ComplexFieldsValidatorTestCase.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/ComplexFieldsValidatorTestCase.java
@@ -18,7 +18,6 @@ import java.util.List;
 import java.util.logging.Level;
 
 import static com.yahoo.config.model.test.TestUtil.joinLines;
-import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
@@ -68,7 +67,33 @@ public class ComplexFieldsValidatorTestCase {
                     "}",
                     "}"));
         });
-        assertTrue(exception.getMessage().contains(getExpectedMessage("docTopics (docTopics.topics)")));
+        assertTrue(exception.getMessage().contains(getExpectedMessage("docTopics (docTopics.topics, docTopics.topics.id, docTopics.topics.label)")));
+    }
+
+    @Test
+    void logs_warning_when_struct_field_inside_nested_struct_array_is_specified_as_attribute() throws IOException, SAXException {
+        var logger = new MyLogger();
+        createModelAndValidate(joinLines(
+                    "schema test {",
+                    "document test {",
+                    "struct item {",
+                        "field name type string {}",
+                        "field color type string {}",
+                        "field type type string {}",
+                    "}",
+                    "struct itembox {",
+                        "field items type array<item> {}",
+                    "}",
+                    "field cabinet type map<string, itembox> {",
+                        "struct-field key { indexing: attribute }",
+                        "struct-field value.items {",
+                            "struct-field name  { indexing: attribute }",
+                            "struct-field color { indexing: attribute }",
+                        "}",
+                    "}",
+                    "}",
+                    "}"), logger);
+        assertTrue(logger.message.toString().contains(getExpectedMessage("cabinet (cabinet.value.items.name, cabinet.value.items.color)")));
     }
 
     private String getExpectedMessage(String unsupportedFields) {
@@ -105,7 +130,7 @@ public class ComplexFieldsValidatorTestCase {
                 "}",
                 "}",
                 "}"), logger);
-        assertThat(logger.message.toString().contains(
+        assertTrue(logger.message.toString().contains(
                 "For cluster 'mycluster', schema 'test': " +
                         "The following complex fields have struct fields with 'indexing: index' which is not supported and has no effect: " +
                         "topics (topics.id, topics.label). " +