Merge pull request #6182 from vespa-engine/geirst/add-struct-field-attribute-change-validator-in-config-model

Add validator for changes in the set of struct field attributes that …

7 files changed, 318 insertions, 6 deletions

diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/derived/AttributeFields.java b/config-model/src/main/java/com/yahoo/searchdefinition/derived/AttributeFields.java
index 0f1918f99d6..6070aeb0ee1 100644
--- a/config-model/src/main/java/com/yahoo/searchdefinition/derived/AttributeFields.java
+++ b/config-model/src/main/java/com/yahoo/searchdefinition/derived/AttributeFields.java
@@ -18,6 +18,7 @@ import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.stream.Collectors;
 
 import static com.yahoo.searchdefinition.document.ComplexAttributeFieldUtils.isArrayOfSimpleStruct;
 import static com.yahoo.searchdefinition.document.ComplexAttributeFieldUtils.isMapOfPrimitiveType;
@@ -153,6 +154,13 @@ public class AttributeFields extends Derived implements AttributesConfig.Produce
         return Collections.unmodifiableCollection(attributes.values());
     }
 
+    public Collection<Attribute> structFieldAttributes(String baseFieldName) {
+        String structPrefix = baseFieldName + ".";
+        return attributes().stream()
+                .filter(attribute -> attribute.getName().startsWith(structPrefix))
+                .collect(Collectors.toList());
+    }
+
     public String toString() {
         return "attributes " + getName();
     }
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 89d72756512..a56364a6975 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
@@ -1,3 +1,4 @@
+// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 package com.yahoo.searchdefinition.document;
 
 import com.yahoo.document.ArrayDataType;
@@ -19,7 +20,10 @@ import com.yahoo.document.StructDataType;
 public class ComplexAttributeFieldUtils {
 
     public static boolean isArrayOfSimpleStruct(ImmutableSDField field) {
-        DataType fieldType = field.getDataType();
+        return isArrayOfSimpleStruct(field.getDataType());
+    }
+
+    public static boolean isArrayOfSimpleStruct(DataType fieldType) {
         if (fieldType instanceof ArrayDataType) {
             ArrayDataType arrayType = (ArrayDataType)fieldType;
             return isSimpleStruct(arrayType.getNestedType());
@@ -29,7 +33,10 @@ public class ComplexAttributeFieldUtils {
     }
 
     public static boolean isMapOfSimpleStruct(ImmutableSDField field) {
-        DataType fieldType = field.getDataType();
+        return isMapOfSimpleStruct(field.getDataType());
+    }
+
+    public static boolean isMapOfSimpleStruct(DataType fieldType) {
         if (fieldType instanceof MapDataType) {
             MapDataType mapType = (MapDataType)fieldType;
             return isPrimitiveType(mapType.getKeyType()) &&
@@ -40,7 +47,10 @@ public class ComplexAttributeFieldUtils {
     }
 
     public static boolean isMapOfPrimitiveType(ImmutableSDField field) {
-        DataType fieldType = field.getDataType();
+        return isMapOfPrimitiveType(field.getDataType());
+    }
+
+    public static boolean isMapOfPrimitiveType(DataType fieldType) {
         if (fieldType instanceof MapDataType) {
             MapDataType mapType = (MapDataType)fieldType;
             return isPrimitiveType(mapType.getKeyType()) &&
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/DocumentDatabaseChangeValidator.java b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/DocumentDatabaseChangeValidator.java
index bd287f83a1a..deae0a89c56 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/DocumentDatabaseChangeValidator.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/DocumentDatabaseChangeValidator.java
@@ -36,6 +36,7 @@ public class DocumentDatabaseChangeValidator {
     public List<VespaConfigChangeAction> validate(ValidationOverrides overrides, Instant now) {
         List<VespaConfigChangeAction> result = new ArrayList<>();
         result.addAll(validateAttributeChanges(overrides, now));
+        result.addAll(validateStructFieldAttributeChanges(overrides, now));
         result.addAll(validateIndexingScriptChanges(overrides, now));
         result.addAll(validateDocumentTypeChanges(overrides, now));
         return result;
@@ -49,6 +50,11 @@ public class DocumentDatabaseChangeValidator {
                 nextDatabase.getDerivedConfiguration().getIndexSchema(), nextDocType).validate(overrides, now);
     }
 
+    private List<VespaConfigChangeAction> validateStructFieldAttributeChanges(ValidationOverrides overrides, Instant now) {
+        return new StructFieldAttributeChangeValidator(currentDocType, currentDatabase.getDerivedConfiguration().getAttributeFields(),
+                nextDocType, nextDatabase.getDerivedConfiguration().getAttributeFields()).validate(overrides, now);
+    }
+
     private List<VespaConfigChangeAction> validateIndexingScriptChanges(ValidationOverrides overrides, Instant now) {
         return new IndexingScriptChangeValidator(currentDatabase.getDerivedConfiguration().getSearch(),
                 nextDatabase.getDerivedConfiguration().getSearch()).validate(overrides, now);
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/StructFieldAttributeChangeValidator.java b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/StructFieldAttributeChangeValidator.java
new file mode 100644
index 00000000000..520494f1697
--- /dev/null
+++ b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/StructFieldAttributeChangeValidator.java
@@ -0,0 +1,132 @@
+// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.model.application.validation.change.search;
+
+import com.yahoo.config.application.api.ValidationOverrides;
+import com.yahoo.document.ArrayDataType;
+import com.yahoo.document.DataType;
+import com.yahoo.document.Field;
+import com.yahoo.document.MapDataType;
+import com.yahoo.document.StructDataType;
+import com.yahoo.documentmodel.NewDocumentType;
+import com.yahoo.searchdefinition.derived.AttributeFields;
+import com.yahoo.searchdefinition.document.Attribute;
+import com.yahoo.vespa.model.application.validation.change.VespaConfigChangeAction;
+import com.yahoo.vespa.model.application.validation.change.VespaRefeedAction;
+
+import java.time.Instant;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.StringTokenizer;
+import java.util.stream.Collectors;
+
+import static com.yahoo.searchdefinition.document.ComplexAttributeFieldUtils.isArrayOfSimpleStruct;
+import static com.yahoo.searchdefinition.document.ComplexAttributeFieldUtils.isMapOfPrimitiveType;
+import static com.yahoo.searchdefinition.document.ComplexAttributeFieldUtils.isMapOfSimpleStruct;
+
+/**
+ * Validates the changes between the current and next set of struct field attributes in a document database.
+
+ * Complex fields of the following types are considered (as they might have struct field attributes):
+ *   - array of simple struct
+ *   - map of simple struct
+ *   - map of primitive types
+ *
+ * @author geirst
+ */
+public class StructFieldAttributeChangeValidator {
+
+    private final NewDocumentType currentDocType;
+    private final AttributeFields currentAttributes;
+    private final NewDocumentType nextDocType;
+    private final AttributeFields nextAttributes;
+
+    public StructFieldAttributeChangeValidator(NewDocumentType currentDocType,
+                                               AttributeFields currentAttributes,
+                                               NewDocumentType nextDocType,
+                                               AttributeFields nextAttributes) {
+        this.currentDocType = currentDocType;
+        this.currentAttributes = currentAttributes;
+        this.nextDocType = nextDocType;
+        this.nextAttributes = nextAttributes;
+    }
+
+    public List<VespaConfigChangeAction> validate(ValidationOverrides overrides, Instant now) {
+        List<VespaConfigChangeAction> result = new ArrayList();
+        for (Field currentField : currentDocType.getAllFields()) {
+            Field nextField = nextDocType.getField(currentField.getName());
+            if (nextField != null) {
+                result.addAll(validateAddAttributeAspect(new Context(currentField, currentAttributes),
+                        new Context(nextField, nextAttributes),
+                        overrides, now));
+            }
+        }
+        return result;
+    }
+
+    private List<VespaConfigChangeAction> validateAddAttributeAspect(Context current, Context next, ValidationOverrides overrides, Instant now) {
+        return next.structFieldAttributes.stream()
+                .filter(nextAttr -> current.hasFieldFor(nextAttr) &&
+                        !current.hasStructFieldAttribute(nextAttr))
+                .map(nextAttr -> VespaRefeedAction.of("field-type-change",
+                        overrides,
+                        new ChangeMessageBuilder(nextAttr.getName())
+                                .addChange("add attribute aspect").build(),
+                        now))
+                .collect(Collectors.toList());
+    }
+
+    private static class Context {
+        public Field field;
+        public Collection<Attribute> structFieldAttributes;
+
+        public Context(Field field, AttributeFields attributes) {
+            this.field = field;
+            this.structFieldAttributes = attributes.structFieldAttributes(field.getName());
+        }
+
+        public DataType dataType() {
+            return field.getDataType();
+        }
+
+        public boolean hasStructFieldAttribute(Attribute structFieldAttribute) {
+            return structFieldAttributes.stream()
+                    .anyMatch(attr -> attr.getName().equals(structFieldAttribute.getName()));
+        }
+
+        public boolean hasFieldFor(Attribute structFieldAttribute) {
+            StringTokenizer fieldNames = new StringTokenizer(structFieldAttribute.getName(), ".");
+            if (!fieldNames.nextToken().equals(field.getName())) {
+                return false;
+            }
+            if (isArrayOfSimpleStruct(dataType())) {
+                StructDataType nestedType = (StructDataType)((ArrayDataType)dataType()).getNestedType();
+                if (hasLastFieldInStructType(fieldNames, nestedType)) {
+                    return true;
+                }
+            } else if (isMapOfSimpleStruct(dataType())) {
+                MapDataType mapType = (MapDataType)dataType();
+                StructDataType valueType = (StructDataType)mapType.getValueType();
+                String subFieldName = fieldNames.nextToken();
+                if (subFieldName.equals("key") && !fieldNames.hasMoreTokens()) {
+                    return true;
+                } else if (subFieldName.equals("value") && hasLastFieldInStructType(fieldNames, valueType)) {
+                    return true;
+                }
+            } else if (isMapOfPrimitiveType(dataType())) {
+                String subFieldName = fieldNames.nextToken();
+                if ((subFieldName.equals("key") || subFieldName.equals("value")) &&
+                        !fieldNames.hasMoreTokens()) {
+                    return true;
+                }
+            }
+            return false;
+        }
+
+        private static boolean hasLastFieldInStructType(StringTokenizer fieldNames, StructDataType structType) {
+            return structType.getField(fieldNames.nextToken()) != null && !fieldNames.hasMoreTokens();
+        }
+
+    }
+
+}
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/ConfigChangeTestUtils.java b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/ConfigChangeTestUtils.java
index 9572cfebb2a..65ac6c7625e 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/ConfigChangeTestUtils.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/ConfigChangeTestUtils.java
@@ -22,6 +22,10 @@ public class ConfigChangeTestUtils {
         return new VespaRestartAction(message, services);
     }
 
+    public static VespaConfigChangeAction newRefeedAction(String name, String message) {
+        return VespaRefeedAction.of(name, ValidationOverrides.empty, message, Instant.now());
+    }
+
     public static VespaConfigChangeAction newRefeedAction(String name, ValidationOverrides overrides, String message, Instant now) {
         return VespaRefeedAction.of(name, overrides, message, now);
     }
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/search/DocumentDatabaseChangeValidatorTest.java b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/search/DocumentDatabaseChangeValidatorTest.java
index ba736af2159..339f8514f9f 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/search/DocumentDatabaseChangeValidatorTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/search/DocumentDatabaseChangeValidatorTest.java
@@ -31,14 +31,20 @@ public class DocumentDatabaseChangeValidatorTest {
 
     @Test
     public void requireThatAttributeIndexAndDocumentTypeChangesAreDiscovered() throws Exception {
-        Fixture f = new Fixture("field f1 type string { indexing: summary } " +
+        Fixture f = new Fixture("struct s { field s1 type string {} } " +
+                "field f1 type string { indexing: summary } " +
                 "field f2 type string { indexing: summary } " +
-                "field f3 type int { indexing: summary }",
+                "field f3 type int { indexing: summary } " +
+                "field f4 type array<s> { } ",
+                "struct s { field s1 type string {} } " +
                 "field f1 type string { indexing: attribute | summary } " +
                 "field f2 type string { indexing: index | summary } " +
-                "field f3 type string { indexing: summary }");
+                "field f3 type string { indexing: summary } " +
+                "field f4 type array<s> { struct-field s1 { indexing: attribute } }");
         f.assertValidation(Arrays.asList(
                 newRestartAction("Field 'f1' changed: add attribute aspect"),
+                newRefeedAction("field-type-change",
+                        "Field 'f4.s1' changed: add attribute aspect"),
                 newRefeedAction("indexing-change",
                                 ValidationOverrides.empty,
                                 "Field 'f2' changed: add index aspect, indexing script: '{ input f2 | summary f2; }' -> " +
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/search/StructFieldAttributeChangeValidatorTestCase.java b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/search/StructFieldAttributeChangeValidatorTestCase.java
new file mode 100644
index 00000000000..c224e801fa3
--- /dev/null
+++ b/config-model/src/test/java/com/yahoo/vespa/model/application/validation/change/search/StructFieldAttributeChangeValidatorTestCase.java
@@ -0,0 +1,146 @@
+// Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+package com.yahoo.vespa.model.application.validation.change.search;
+
+import com.yahoo.config.application.api.ValidationOverrides;
+import com.yahoo.vespa.model.application.validation.change.VespaConfigChangeAction;
+import org.junit.Test;
+
+import java.time.Instant;
+import java.util.ArrayList;
+import java.util.List;
+
+import static com.yahoo.vespa.model.application.validation.change.ConfigChangeTestUtils.newRefeedAction;
+
+/**
+ * @author geirst
+ */
+public class StructFieldAttributeChangeValidatorTestCase {
+
+    private static class Fixture extends ContentClusterFixture {
+        private StructFieldAttributeChangeValidator structFieldAttributeValidator;
+        private DocumentTypeChangeValidator docTypeValidator;
+
+        public Fixture(String currentSd, String nextSd) throws Exception {
+            super(currentSd, nextSd);
+            structFieldAttributeValidator = new StructFieldAttributeChangeValidator(currentDocType(),
+                    currentDb().getDerivedConfiguration().getAttributeFields(),
+                    nextDocType(),
+                    nextDb().getDerivedConfiguration().getAttributeFields());
+            docTypeValidator = new DocumentTypeChangeValidator(currentDocType(), nextDocType());
+        }
+
+        @Override
+        public List<VespaConfigChangeAction> validate() {
+            List<VespaConfigChangeAction> result = new ArrayList<>();
+            result.addAll(structFieldAttributeValidator.validate(ValidationOverrides.empty, Instant.now()));
+            result.addAll(docTypeValidator.validate(ValidationOverrides.empty, Instant.now()));
+            return result;
+        }
+    }
+
+    private static void validate(String currentSd, String nextSd) throws Exception {
+        new Fixture(currentSd, nextSd).assertValidation();
+    }
+
+    private static void validate(String currentSd, String nextSd, VespaConfigChangeAction expAction) throws Exception {
+        new Fixture(currentSd, nextSd).assertValidation(expAction);
+    }
+
+    @Test
+    public void adding_attribute_aspect_to_struct_field_requires_refeed() throws Exception {
+        validate(arrayOfStruct(oneFieldStruct(), ""),
+                arrayOfStruct(oneFieldStruct(), structAttribute("s1")),
+                newRefeedAction("field-type-change", "Field 'f1.s1' changed: add attribute aspect"));
+
+        validate(mapOfStruct(oneFieldStruct(), ""),
+                mapOfStruct(oneFieldStruct(), structAttribute("key")),
+                newRefeedAction("field-type-change", "Field 'f1.key' changed: add attribute aspect"));
+
+        validate(mapOfStruct(oneFieldStruct(), ""),
+                mapOfStruct(oneFieldStruct(), structAttribute("value.s1")),
+                newRefeedAction("field-type-change", "Field 'f1.value.s1' changed: add attribute aspect"));
+
+        validate(mapOfPrimitive(""), mapOfPrimitive(structAttribute("key")),
+                newRefeedAction("field-type-change", "Field 'f1.key' changed: add attribute aspect"));
+
+        validate(mapOfPrimitive(""), mapOfPrimitive(structAttribute("value")),
+                newRefeedAction("field-type-change", "Field 'f1.value' changed: add attribute aspect"));
+    }
+
+    @Test
+    public void removing_attribute_aspect_from_struct_field_is_ok() throws Exception {
+        validate(arrayOfStruct(oneFieldStruct(), structAttribute("s1")),
+                arrayOfStruct(oneFieldStruct(), ""));
+
+        validate(mapOfStruct(oneFieldStruct(), structAttribute("key")),
+                mapOfStruct(oneFieldStruct(), ""));
+
+        validate(mapOfStruct(oneFieldStruct(), structAttribute("value.s1")),
+                mapOfStruct(oneFieldStruct(), ""));
+
+        validate(mapOfPrimitive(structAttribute("key")), mapOfPrimitive(""));
+
+        validate(mapOfPrimitive(structAttribute("value")), mapOfPrimitive(""));
+    }
+
+    @Test
+    public void adding_struct_field_with_attribute_aspect_is_ok() throws Exception {
+        validate(arrayOfStruct(oneFieldStruct(), ""),
+                arrayOfStruct(twoFieldStruct(), structAttribute("s2")));
+
+        validate(mapOfStruct(oneFieldStruct(), ""),
+                mapOfStruct(twoFieldStruct(), structAttribute("value.s2")));
+    }
+
+    @Test
+    public void removing_struct_field_with_attribute_aspect_is_ok() throws Exception {
+        validate(arrayOfStruct(twoFieldStruct(), structAttribute("s2")),
+                arrayOfStruct(oneFieldStruct(), ""));
+
+        validate(mapOfStruct(twoFieldStruct(), structAttribute("value.s2")),
+                mapOfStruct(oneFieldStruct(), ""));
+    }
+
+    @Test
+    public void adding_struct_field_without_attribute_aspect_is_ok() throws Exception {
+        validate(arrayOfStruct(oneFieldStruct(), ""),
+                arrayOfStruct(twoFieldStruct(), ""));
+
+        validate(mapOfStruct(oneFieldStruct(), ""),
+                mapOfStruct(twoFieldStruct(), ""));
+    }
+
+    @Test
+    public void removing_struct_field_without_attribute_aspect_is_ok() throws Exception {
+        validate(arrayOfStruct(twoFieldStruct(), ""),
+                arrayOfStruct(oneFieldStruct(), ""));
+
+        validate(mapOfStruct(twoFieldStruct(), ""),
+                mapOfStruct(oneFieldStruct(), ""));
+    }
+
+    private static String oneFieldStruct() {
+        return "struct s { field s1 type string {} }";
+    }
+
+    private static String twoFieldStruct() {
+        return "struct s { field s1 type string {} field s2 type int {} }";
+    }
+
+    private static String structAttribute(String fieldName) {
+        return "struct-field " + fieldName + " { indexing: attribute }";
+    }
+
+    private static String arrayOfStruct(String struct, String structField) {
+        return struct + " field f1 type array<s> { " + structField + "}";
+    }
+
+    private static String mapOfStruct(String struct, String structField) {
+        return struct + " field f1 type map<string,s> { " + structField + " }";
+    }
+
+    private static String mapOfPrimitive(String structField) {
+        return "field f1 type map<string,int> { " + structField + " }";
+    }
+
+}