Merge pull request #31339 from vespa-engine/hmusum/throw-if-document-summary-inherits-non-existent-summary-2

Throw if inheriting non-existent document summary

4 files changed, 15 insertions, 23 deletions

diff --git a/config-model/src/main/java/com/yahoo/schema/Schema.java b/config-model/src/main/java/com/yahoo/schema/Schema.java
index 3402ba31be9..127d12594b4 100644
--- a/config-model/src/main/java/com/yahoo/schema/Schema.java
+++ b/config-model/src/main/java/com/yahoo/schema/Schema.java
@@ -721,7 +721,7 @@ public class Schema implements ImmutableSchema {
                                                    "', but this schema does not exist");
 
             // Require schema and document type inheritance to be consistent to keep things simple
-            // And require it to be explicit so we have the option to support other possibilities later
+            // and require it to be explicit, so we have the option to support other possibilities later
             var parentDocument = owner.schemas().get(inherited.get()).getDocument();
             if ( ! getDocument().inheritedTypes().containsKey(new DataTypeName(parentDocument.getName())))
                 throw new IllegalArgumentException(this + " inherits '" + inherited.get() +
diff --git a/config-model/src/main/java/com/yahoo/vespa/documentmodel/DocumentSummary.java b/config-model/src/main/java/com/yahoo/vespa/documentmodel/DocumentSummary.java
index 2f61c1c631d..e48aa0eef7c 100644
--- a/config-model/src/main/java/com/yahoo/vespa/documentmodel/DocumentSummary.java
+++ b/config-model/src/main/java/com/yahoo/vespa/documentmodel/DocumentSummary.java
@@ -9,7 +9,8 @@ import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.logging.Level;
+
+import static java.util.logging.Level.WARNING;
 
 /**
  * A document summary definition - a list of summary fields.
@@ -121,11 +122,11 @@ public class DocumentSummary extends FieldView {
     public void validate(DeployLogger logger) {
         for (var inheritedName : inherited) {
             var inheritedSummary = owner.getSummary(inheritedName);
-            if (inheritedSummary == null) {
-                // TODO Vespa 9: Throw IllegalArgumentException instead
-                logger.logApplicationPackage(Level.WARNING,
-                                             this + " inherits '" + inheritedName + "' but this is not present in " + owner);
-            }
+            // TODO: Throw when no one is doing this anymore
+            if (inheritedName.equals("default"))
+                logger.logApplicationPackage(WARNING, this + " inherits '" + inheritedName + "', which makes no sense. Remove this inheritance");
+            else if (inheritedSummary == null )
+                throw new IllegalArgumentException(this + " inherits '" + inheritedName + "', but this is not present in " + owner);
         }
     }
 
diff --git a/config-model/src/test/java/com/yahoo/schema/SummaryTestCase.java b/config-model/src/test/java/com/yahoo/schema/SummaryTestCase.java
index 8ffbab84fd7..539ec91ee5f 100644
--- a/config-model/src/test/java/com/yahoo/schema/SummaryTestCase.java
+++ b/config-model/src/test/java/com/yahoo/schema/SummaryTestCase.java
@@ -6,16 +6,13 @@ import com.yahoo.vespa.documentmodel.DocumentSummary;
 import com.yahoo.vespa.model.test.utils.DeployLoggerStub;
 import com.yahoo.vespa.objects.FieldBase;
 import com.yahoo.yolean.Exceptions;
-import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 
 import static com.yahoo.config.model.test.TestUtil.joinLines;
 
 import java.util.Collection;
 import java.util.List;
-import java.util.Optional;
 import java.util.logging.Level;
-import java.util.stream.Collectors;
 
 import static org.junit.jupiter.api.Assertions.*;
 
@@ -233,16 +230,12 @@ public class SummaryTestCase {
                             "  document-summary test_summary inherits nonesuch {" +
                             "  }" +
                             "}");
-            DeployLoggerStub logger = new DeployLoggerStub();
-            ApplicationBuilder.createFromStrings(logger, schema);
-            assertEquals("document-summary 'test_summary' inherits 'nonesuch' but this is not present in schema 'test'",
-                         logger.entries.get(0).message);
-            // fail("Expected failure");
+            ApplicationBuilder.createFromString(schema);
+            fail("Expected failure");
         }
         catch (IllegalArgumentException e) {
-            fail();
-            // assertEquals("document-summary 'test_summary' inherits nonesuch but this is not present in schema 'test'",
-            //             e.getMessage());
+            assertEquals("document-summary 'test_summary' inherits 'nonesuch', but this is not present in schema 'test'",
+            e.getMessage());
         }
     }
 
diff --git a/config-model/src/test/java/com/yahoo/schema/processing/SummaryConsistencyTestCase.java b/config-model/src/test/java/com/yahoo/schema/processing/SummaryConsistencyTestCase.java
index 1184e49b381..5ea097f13fb 100644
--- a/config-model/src/test/java/com/yahoo/schema/processing/SummaryConsistencyTestCase.java
+++ b/config-model/src/test/java/com/yahoo/schema/processing/SummaryConsistencyTestCase.java
@@ -5,11 +5,11 @@ import com.yahoo.schema.Schema;
 import com.yahoo.schema.ApplicationBuilder;
 import com.yahoo.schema.parser.ParseException;
 import com.yahoo.vespa.documentmodel.SummaryTransform;
-import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 
 import static com.yahoo.config.model.test.TestUtil.joinLines;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 public class SummaryConsistencyTestCase {
 
@@ -45,8 +45,7 @@ public class SummaryConsistencyTestCase {
     }
 
     @Test
-    @Disabled
-    void testDocumentTypesWithInheritanceOfNonExistingField() throws ParseException {
+    void testDocumentSummaryWithInheritanceOfNonExistingSummary() {
         String schemaString = """
                 schema foo {
                   document foo {
@@ -61,8 +60,7 @@ public class SummaryConsistencyTestCase {
                   }
                 }
                 """;
-        var schema = ApplicationBuilder.createFromString(schemaString).getSchema();
-        schema.getSummaryField("foo_summary");
+        assertThrows(IllegalArgumentException.class, () -> ApplicationBuilder.createFromString(schemaString).getSchema());
     }
 
 }