diff options
author | Geir Storli <geirst@yahooinc.com> | 2023-07-04 15:18:13 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2023-07-04 15:18:13 +0200 |
commit | b3abb357a1d64c19f9be1efeb60b43da244c4391 (patch) | |
tree | f443c532beffc83e6c7c0ece1764156daadb9038 | |
parent | 6caebd61a22ae23ea6c8bc04a9c5b1c053014f14 (diff) | |
parent | c10aa1c8336a46a8106781fddcf1c99c6bf1d02e (diff) |
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). " + |