Disallow importing a field that is also an imported field.

2 files changed, 76 insertions, 25 deletions

diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/processing/ImportedFieldsResolver.java b/config-model/src/main/java/com/yahoo/searchdefinition/processing/ImportedFieldsResolver.java
index c33c69b878f..4de582c60ee 100644
--- a/config-model/src/main/java/com/yahoo/searchdefinition/processing/ImportedFieldsResolver.java
+++ b/config-model/src/main/java/com/yahoo/searchdefinition/processing/ImportedFieldsResolver.java
@@ -55,7 +55,11 @@ public class ImportedFieldsResolver extends Processor {
 
     private SDField validateTargetField(TemporaryImportedField importedField, DocumentReference reference) {
         String targetFieldName = importedField.targetFieldName();
-        SDField targetField = reference.targetSearch().getField(targetFieldName);
+        Search targetSearch = reference.targetSearch();
+        if (isImportedField(targetSearch, targetFieldName)) {
+            fail(importedField, targetFieldAsString(targetFieldName, reference) + ": Is an imported field. Importing such field not supported");
+        }
+        SDField targetField = targetSearch.getField(targetFieldName);
         if (targetField == null) {
             fail(importedField, targetFieldAsString(targetFieldName, reference) + ": Not found");
         } else if (!targetField.doesAttributing()) {
@@ -68,6 +72,10 @@ public class ImportedFieldsResolver extends Processor {
         return targetField;
     }
 
+    private static boolean isImportedField(Search targetSearch, String targetFieldName) {
+        return targetSearch.importedFields().isPresent() && targetSearch.importedFields().get().fields().containsKey(targetFieldName);
+    }
+
     private static String targetFieldAsString(String targetFieldName, DocumentReference reference) {
         return "Field '" + targetFieldName + "' via reference field '" + reference.referenceField().getName() + "'";
     }
diff --git a/config-model/src/test/java/com/yahoo/searchdefinition/processing/ImportedFieldsResolverTestCase.java b/config-model/src/test/java/com/yahoo/searchdefinition/processing/ImportedFieldsResolverTestCase.java
index 5f8b4b2b38f..1cf90fcfcd1 100644
--- a/config-model/src/test/java/com/yahoo/searchdefinition/processing/ImportedFieldsResolverTestCase.java
+++ b/config-model/src/test/java/com/yahoo/searchdefinition/processing/ImportedFieldsResolverTestCase.java
@@ -39,7 +39,7 @@ public class ImportedFieldsResolverTestCase {
     @Test
     public void valid_imported_fields_are_resolved() {
         SearchModel model = new SearchModel();
-        model.add(new TemporaryImportedField("my_attribute_field", "ref", "attribute_field")).resolve();
+        model.addImportedField("my_attribute_field", "ref", "attribute_field").resolve();
 
         assertEquals(1, model.importedFields.fields().size());
         ImportedField myField = model.importedFields.fields().get("my_attribute_field");
@@ -55,7 +55,7 @@ public class ImportedFieldsResolverTestCase {
         exceptionRule.expect(IllegalArgumentException.class);
         exceptionRule.expectMessage("For search 'child', import field 'my_attribute_field': "
                 + "Reference field 'not_ref' not found");
-        new SearchModel().add(new TemporaryImportedField("my_attribute_field", "not_ref", "budget")).resolve();
+        new SearchModel().addImportedField("my_attribute_field", "not_ref", "budget").resolve();
     }
 
     @Test
@@ -63,7 +63,7 @@ public class ImportedFieldsResolverTestCase {
         exceptionRule.expect(IllegalArgumentException.class);
         exceptionRule.expectMessage("For search 'child', import field 'my_attribute_field': "
                 + "Field 'not_existing' via reference field 'ref': Not found");
-        new SearchModel().add(new TemporaryImportedField("my_attribute_field", "ref", "not_existing")).resolve();
+        new SearchModel().addImportedField("my_attribute_field", "ref", "not_existing").resolve();
     }
 
     @Test
@@ -71,7 +71,7 @@ public class ImportedFieldsResolverTestCase {
         exceptionRule.expect(IllegalArgumentException.class);
         exceptionRule.expectMessage("For search 'child', import field 'my_not_attribute': "
                 + "Field 'not_attribute' via reference field 'ref': Is not an attribute");
-        new SearchModel().add(new TemporaryImportedField("my_not_attribute", "ref", "not_attribute")).resolve();
+        new SearchModel().addImportedField("my_not_attribute", "ref", "not_attribute").resolve();
     }
 
     @Test
@@ -81,7 +81,7 @@ public class ImportedFieldsResolverTestCase {
                 "For search 'child', import field 'my_attribute_and_index': " +
                         "Field 'attribute_and_index' via reference field 'ref': Index field not supported");
         new SearchModel()
-                .add(new TemporaryImportedField("my_attribute_and_index", "ref", "attribute_and_index"))
+                .addImportedField("my_attribute_and_index", "ref", "attribute_and_index")
                 .resolve();
     }
 
@@ -92,47 +92,90 @@ public class ImportedFieldsResolverTestCase {
                 "For search 'child', import field 'my_tensor_field': " +
                         "Field 'tensor_field' via reference field 'ref': Type 'tensor' not supported");
         new SearchModel()
-                .add(new TemporaryImportedField("my_tensor_field", "ref", "tensor_field"))
+                .addImportedField("my_tensor_field", "ref", "tensor_field")
+                .resolve();
+    }
+
+    @Test
+    public void resolver_fails_if_imported_field_is_also_an_imported_field() {
+        exceptionRule.expect(IllegalArgumentException.class);
+        exceptionRule.expectMessage(
+                "For search 'child', import field 'my_ancient_field': " +
+                        "Field 'ancient_field' via reference field 'ref': Is an imported field. Importing such field not supported");
+        new SearchModel()
+                .addImportedField("my_ancient_field", "ref", "ancient_field")
                 .resolve();
     }
 
     private static class SearchModel {
+
+        private final ApplicationPackage app = MockApplicationPackage.createEmpty();
+        public final Search grandParentSearch;
         public final Search parentSearch;
         public final Search childSearch;
         public ImportedFields importedFields;
+
         public SearchModel() {
-            ApplicationPackage app = MockApplicationPackage.createEmpty();
+            grandParentSearch = createSearch("grandparent");
+            grandParentSearch.getDocument().addField(createField("ancient_field", DataType.INT, "{ attribute }"));
 
-            parentSearch = new Search("parent", app);
-            parentSearch.addDocument(new SDDocumentType("parent"));
+            parentSearch = createSearch("parent");
             parentSearch.getDocument().addField(createField("attribute_field", DataType.INT, "{ attribute }"));
             parentSearch.getDocument().addField(createField("attribute_and_index", DataType.INT, "{ attribute | index }"));
             parentSearch.getDocument().addField(new TemporarySDField("not_attribute", DataType.INT));
             parentSearch.getDocument().addField(createField("tensor_field", new TensorDataType(TensorType.fromSpec("tensor(x[])")), "{ attribute }"));
+            addRefField(parentSearch, grandParentSearch, "ref");
+            addImportedField(parentSearch, "ancient_field", "ref", "ancient_field");
 
-            childSearch = new Search("child", app);
-            childSearch.addDocument(new SDDocumentType("child"));
-            SDField parentRefField = new TemporarySDField("ref", ReferenceDataType.createWithInferredId(TemporaryStructuredDataType.create("parent")));
-            childSearch.getDocument().addField(parentRefField);
-            childSearch.getDocument().setDocumentReferences(new DocumentReferences(ImmutableMap.of(parentRefField.getName(),
-                    new DocumentReference(parentRefField, parentSearch))));
+            childSearch = createSearch("child");
+            addRefField(childSearch, parentSearch, "ref");
+        }
+
+        private Search createSearch(String name) {
+            Search result = new Search(name, app);
+            result.addDocument(new SDDocumentType(name));
+            return result;
         }
-        private TemporarySDField createField(String name, DataType dataType, String indexingScript) {
+
+        private static TemporarySDField createField(String name, DataType dataType, String indexingScript) {
             TemporarySDField result = new TemporarySDField(name, dataType);
             result.parseIndexingScript(indexingScript);
             return result;
         }
-        public SearchModel add(TemporaryImportedField importedField) {
-            childSearch.temporaryImportedFields().get().add(importedField);
+
+        private static SDField createRefField(String parentType, String fieldName) {
+            return new TemporarySDField(fieldName, ReferenceDataType.createWithInferredId(TemporaryStructuredDataType.create(parentType)));
+        }
+
+        private static void addRefField(Search child, Search parent, String fieldName) {
+            SDField refField = createRefField(parent.getName(), fieldName);
+            child.getDocument().addField(refField);
+            child.getDocument().setDocumentReferences(new DocumentReferences(ImmutableMap.of(refField.getName(),
+                    new DocumentReference(refField, parent))));
+        }
+
+        public SearchModel addImportedField(String fieldName, String referenceFieldName, String targetFieldName) {
+            return addImportedField(childSearch, fieldName, referenceFieldName, targetFieldName);
+        }
+
+        private SearchModel addImportedField(Search search, String fieldName, String referenceFieldName, String targetFieldName) {
+            search.temporaryImportedFields().get().add(new TemporaryImportedField(fieldName, referenceFieldName, targetFieldName));
             return this;
         }
+
         public void resolve() {
-            assertNotNull(childSearch.temporaryImportedFields().get());
-            assertFalse(childSearch.importedFields().isPresent());
-            new ImportedFieldsResolver(childSearch, null, null, null).process();
-            assertFalse(childSearch.temporaryImportedFields().isPresent());
-            assertNotNull(childSearch.importedFields().get());
-            importedFields = childSearch.importedFields().get();
+            resolve(grandParentSearch);
+            resolve(parentSearch);
+            importedFields = resolve(childSearch);
+        }
+
+        private static ImportedFields resolve(Search search) {
+            assertNotNull(search.temporaryImportedFields().get());
+            assertFalse(search.importedFields().isPresent());
+            new ImportedFieldsResolver(search, null, null, null).process();
+            assertFalse(search.temporaryImportedFields().isPresent());
+            assertNotNull(search.importedFields().get());
+            return search.importedFields().get();
         }
     }