Merge pull request #18237 from vespa-engine/bratseth/check-for-default-as-indexing-chain

Disallow 'default' as indexing chain

3 files changed, 94 insertions, 85 deletions

diff --git a/config-model/src/main/java/com/yahoo/vespa/model/content/Content.java b/config-model/src/main/java/com/yahoo/vespa/model/content/Content.java
index ea52f9689ff..4a8002ba3dc 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/content/Content.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/content/Content.java
@@ -111,13 +111,15 @@ public class Content extends ConfigModel {
         return null;
     }
 
-    private static void checkThatExplicitIndexingChainInheritsCorrectly(ComponentRegistry<DocprocChain> allChains, ChainSpecification chainSpec) {
+    private static void checkThatExplicitIndexingChainInheritsCorrectly(ComponentRegistry<DocprocChain> allChains,
+                                                                        ChainSpecification chainSpec) {
         ChainSpecification.Inheritance inheritance = chainSpec.inheritance;
         for (ComponentSpecification componentSpec : inheritance.chainSpecifications) {
             ChainSpecification parentSpec = getChainSpec(allChains, componentSpec);
             if (containsIndexingChain(allChains, parentSpec)) return;
         }
-        throw new IllegalArgumentException("Docproc chain '" + chainSpec.componentId + "' does not inherit from 'indexing' chain.");
+        throw new IllegalArgumentException("Docproc chain '" + chainSpec.componentId +
+                                           "' must inherit from the 'indexing' chain");
     }
 
     public static List<Content> getContent(ConfigModelRepo pc) {
@@ -261,9 +263,17 @@ public class Content extends ConfigModel {
             if (cluster.hasExplicitIndexingChain()) {
                 indexingChain = allChains.getComponent(cluster.getIndexingChainName());
                 if (indexingChain == null) {
-                    throw new RuntimeException("Indexing cluster " + cluster.getClusterName() + " refers to docproc " +
-                                               "chain " + cluster.getIndexingChainName() + " for indexing, which does not exist.");
-                } else {
+                    throw new IllegalArgumentException(cluster + " refers to docproc " +
+                                                       "chain '" + cluster.getIndexingChainName() +
+                                                       "' for indexing, but this chain does not exist");
+                }
+                else if (indexingChain.getId().getName().equals("default")) {
+                    throw new IllegalArgumentException(cluster + " specifies the chain " +
+                                                       "'default' as indexing chain. As the 'default' chain is run by default, " +
+                                                       "using it as the indexing chain will run it twice. " +
+                                                       "Use a different name for the indexing chain.");
+                }
+                else {
                     checkThatExplicitIndexingChainInheritsCorrectly(allChains, indexingChain.getChainSpecification());
                 }
             } else {
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/search/IndexedSearchCluster.java b/config-model/src/main/java/com/yahoo/vespa/model/search/IndexedSearchCluster.java
index 99f1b3ad34e..c77f44d649c 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/search/IndexedSearchCluster.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/search/IndexedSearchCluster.java
@@ -336,6 +336,11 @@ public class IndexedSearchCluster extends SearchCluster
     @Override
     public int getRowBits() { return 8; }
 
+    @Override
+    public String toString() {
+        return "Indexing cluster '" + getClusterName() + "'";
+    }
+
     /**
      * Class used to retrieve combined configuration from multiple document databases.
      * It is not a {@link com.yahoo.config.ConfigInstance.Producer} of those configs,
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/content/IndexingAndDocprocRoutingTest.java b/config-model/src/test/java/com/yahoo/vespa/model/content/IndexingAndDocprocRoutingTest.java
index 177b86c953e..e16862230fc 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/content/IndexingAndDocprocRoutingTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/content/IndexingAndDocprocRoutingTest.java
@@ -6,7 +6,6 @@ import com.yahoo.messagebus.routing.HopBlueprint;
 import com.yahoo.messagebus.routing.PolicyDirective;
 import com.yahoo.messagebus.routing.Route;
 import com.yahoo.messagebus.routing.RoutingTable;
-import com.yahoo.searchdefinition.parser.ParseException;
 import com.yahoo.vespa.model.VespaModel;
 import com.yahoo.vespa.model.container.ContainerCluster;
 import com.yahoo.vespa.model.container.docproc.ContainerDocproc;
@@ -17,50 +16,48 @@ import com.yahoo.vespa.model.routing.Routing;
 import com.yahoo.vespa.model.test.utils.ApplicationPackageUtils;
 import com.yahoo.vespa.model.test.utils.VespaModelCreatorWithMockPkg;
 import org.junit.Test;
-import org.xml.sax.SAXException;
 
-import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
 import static org.hamcrest.Matchers.*;
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 /**
  * @author Einar M R Rosenvinge
  */
 public class IndexingAndDocprocRoutingTest extends ContentBaseTest {
+
     @Test
-    public void oneContentOneDoctypeImplicitIndexingClusterImplicitIndexingChain()
-            throws IOException, SAXException, ParseException {
+    public void oneContentOneDoctypeImplicitIndexingClusterImplicitIndexingChain() {
         final String CLUSTERNAME = "musiccluster";
         SearchClusterSpec searchCluster = new SearchClusterSpec(CLUSTERNAME, null, null);
         searchCluster.searchDefs.add(new SearchDefSpec("music", "artist", "album"));
-        VespaModel model = getIndexedContentVespaModel(Collections.<DocprocClusterSpec>emptyList(), Arrays.asList(searchCluster));
+        VespaModel model = getIndexedContentVespaModel(List.of(), List.of(searchCluster));
         assertIndexing(model, new DocprocClusterSpec("container", new DocprocChainSpec("container/chain.indexing")));
         assertFeedingRoute(model, CLUSTERNAME, "container/chain.indexing");
     }
 
     @Test
-    public void oneContentTwoDoctypesImplicitIndexingClusterImplicitIndexingChain()
-            throws IOException, SAXException, ParseException {
+    public void oneContentTwoDoctypesImplicitIndexingClusterImplicitIndexingChain() {
         final String CLUSTERNAME = "musicandbookscluster";
         SearchClusterSpec searchCluster = new SearchClusterSpec(CLUSTERNAME, null, null);
         searchCluster.searchDefs.add(new SearchDefSpec("music", "artist", "album"));
         searchCluster.searchDefs.add(new SearchDefSpec("book", "author", "title"));
-        VespaModel model = getIndexedContentVespaModel(Collections.<DocprocClusterSpec>emptyList(), Arrays.asList(searchCluster));
+        VespaModel model = getIndexedContentVespaModel(List.of(), List.of(searchCluster));
         assertIndexing(model, new DocprocClusterSpec("container", new DocprocChainSpec("container/chain.indexing")));
         assertFeedingRoute(model, CLUSTERNAME, "container/chain.indexing");
     }
 
     @Test
-    public void twoContentTwoDoctypesImplicitIndexingClusterImplicitIndexingChain()
-            throws IOException, SAXException, ParseException {
+    public void twoContentTwoDoctypesImplicitIndexingClusterImplicitIndexingChain() {
         final String MUSIC = "musiccluster";
         SearchClusterSpec musicCluster = new SearchClusterSpec(MUSIC, null, null);
         musicCluster.searchDefs.add(new SearchDefSpec("music", "artist", "album"));
@@ -69,10 +66,10 @@ public class IndexingAndDocprocRoutingTest extends ContentBaseTest {
         SearchClusterSpec booksCluster = new SearchClusterSpec(BOOKS, null, null);
         booksCluster.searchDefs.add(new SearchDefSpec("book", "author", "title"));
 
-        VespaModel model = getIndexedContentVespaModel(Collections.<DocprocClusterSpec>emptyList(), Arrays.asList(musicCluster, booksCluster));
+        VespaModel model = getIndexedContentVespaModel(List.of(), List.of(musicCluster, booksCluster));
 
         assertIndexing(model,
-                new DocprocClusterSpec("container", new DocprocChainSpec("container/chain.indexing")));
+                       new DocprocClusterSpec("container", new DocprocChainSpec("container/chain.indexing")));
 
         assertFeedingRoute(model, MUSIC, "container/chain.indexing");
         assertFeedingRoute(model, BOOKS, "container/chain.indexing");
@@ -80,19 +77,17 @@ public class IndexingAndDocprocRoutingTest extends ContentBaseTest {
 
 
     @Test
-    public void oneContentOneDoctypeExplicitIndexingClusterImplicitIndexingChain()
-            throws IOException, SAXException, ParseException {
+    public void oneContentOneDoctypeExplicitIndexingClusterImplicitIndexingChain() {
         final String CLUSTERNAME = "musiccluster";
         SearchClusterSpec searchCluster = new SearchClusterSpec(CLUSTERNAME, "dpcluster", null);
         searchCluster.searchDefs.add(new SearchDefSpec("music", "artist", "album"));
-        VespaModel model = getIndexedContentVespaModel(Arrays.asList(new DocprocClusterSpec("dpcluster")), Arrays.asList(searchCluster));
+        VespaModel model = getIndexedContentVespaModel(List.of(new DocprocClusterSpec("dpcluster")), List.of(searchCluster));
         assertIndexing(model, new DocprocClusterSpec("dpcluster", new DocprocChainSpec("dpcluster/chain.indexing")));
         assertFeedingRoute(model, CLUSTERNAME, "dpcluster/chain.indexing");
     }
 
     @Test
-    public void oneSearchOneDoctypeExplicitIndexingClusterExplicitIndexingChain()
-            throws IOException, SAXException, ParseException {
+    public void oneSearchOneDoctypeExplicitIndexingClusterExplicitIndexingChain() {
         String xml =
                 "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
                 "<services version=\"1.0\">\n" +
@@ -130,8 +125,7 @@ public class IndexingAndDocprocRoutingTest extends ContentBaseTest {
     }
 
     @Test
-    public void twoContentTwoDoctypesExplicitIndexingInSameIndexingCluster()
-            throws IOException, SAXException, ParseException {
+    public void twoContentTwoDoctypesExplicitIndexingInSameIndexingCluster() {
         final String MUSIC = "musiccluster";
         SearchClusterSpec musicCluster = new SearchClusterSpec(MUSIC, "dpcluster", null);
         musicCluster.searchDefs.add(new SearchDefSpec("music", "artist", "album"));
@@ -140,8 +134,8 @@ public class IndexingAndDocprocRoutingTest extends ContentBaseTest {
         SearchClusterSpec booksCluster = new SearchClusterSpec(BOOKS, "dpcluster", null);
         booksCluster.searchDefs.add(new SearchDefSpec("book", "author", "title"));
 
-        VespaModel model = getIndexedContentVespaModel(Arrays.asList(new DocprocClusterSpec("dpcluster")),
-                                                       Arrays.asList(musicCluster, booksCluster));
+        VespaModel model = getIndexedContentVespaModel(List.of(new DocprocClusterSpec("dpcluster")),
+                                                       List.of(musicCluster, booksCluster));
 
         assertIndexing(model, new DocprocClusterSpec("dpcluster", new DocprocChainSpec("dpcluster/chain.indexing")));
         assertFeedingRoute(model, MUSIC, "dpcluster/chain.indexing");
@@ -165,14 +159,12 @@ public class IndexingAndDocprocRoutingTest extends ContentBaseTest {
                         "</services>\n";
 
         List<String> sds = ApplicationPackageUtils.generateSchemas("music", "title", "artist");
-        VespaModel model = new VespaModelCreatorWithMockPkg(getHosts(),
-                services, sds).create();
+        VespaModel model = new VespaModelCreatorWithMockPkg(getHosts(), services, sds).create();
         assertIndexing(model, new DocprocClusterSpec("dokprok"));
     }
 
     @Test
-    public void twoContentTwoDoctypesExplicitIndexingInDifferentIndexingClustersExplicitChain()
-            throws IOException, SAXException, ParseException {
+    public void twoContentTwoDoctypesExplicitIndexingInDifferentIndexingClustersExplicitChain() {
         final String MUSIC = "musiccluster";
         SearchClusterSpec musicCluster = new SearchClusterSpec(MUSIC, "dpmusiccluster", "dpmusicchain");
         musicCluster.searchDefs.add(new SearchDefSpec("music", "artist", "album"));
@@ -183,12 +175,8 @@ public class IndexingAndDocprocRoutingTest extends ContentBaseTest {
 
         DocprocClusterSpec dpMusicCluster = new DocprocClusterSpec("dpmusiccluster", new DocprocChainSpec("dpmusicchain", "indexing"));
         DocprocClusterSpec dpBooksCluster = new DocprocClusterSpec("dpbookscluster", new DocprocChainSpec("dpbookschain", "indexing"));
-        VespaModel model = getIndexedContentVespaModel(Arrays.asList(
-                dpMusicCluster,
-                dpBooksCluster),
-                                                       Arrays.asList(
-                                                               musicCluster,
-                                                               booksCluster));
+        VespaModel model = getIndexedContentVespaModel(List.of(dpMusicCluster, dpBooksCluster),
+                                                       List.of(musicCluster, booksCluster));
 
         //after we generated model, add indexing chains for validation:
         dpMusicCluster.chains.clear();
@@ -204,52 +192,52 @@ public class IndexingAndDocprocRoutingTest extends ContentBaseTest {
         assertFeedingRoute(model, BOOKS, "dpbookscluster/chain.dpbookschain");
     }
 
-    @Test(expected = IllegalArgumentException.class)
-    public void twoContentTwoDoctypesExplicitIndexingInDifferentIndexingClustersExplicitChainIncorrectInheritance()
-            throws IOException, SAXException, ParseException {
-        final String MUSIC = "musiccluster";
-        SearchClusterSpec musicCluster = new SearchClusterSpec(MUSIC, "dpmusiccluster", "dpmusicchain");
-        musicCluster.searchDefs.add(new SearchDefSpec("music", "artist", "album"));
-
-        final String BOOKS = "bookscluster";
-        SearchClusterSpec booksCluster = new SearchClusterSpec(BOOKS, "dpbookscluster", "dpbookschain");
-        booksCluster.searchDefs.add(new SearchDefSpec("book", "author", "title"));
-
-        DocprocClusterSpec dpMusicCluster = new DocprocClusterSpec("dpmusiccluster", new DocprocChainSpec("dpmusicchain"));
-        DocprocClusterSpec dpBooksCluster = new DocprocClusterSpec("dpbookscluster", new DocprocChainSpec("dpbookschain"));
-        VespaModel model = getIndexedContentVespaModel(Arrays.asList(
-                dpMusicCluster,
-                dpBooksCluster),
-                                                       Arrays.asList(
-                                                               musicCluster,
-                                                               booksCluster));
-
-        //after we generated model, add indexing chains for validation:
-        dpMusicCluster.chains.clear();
-        dpMusicCluster.chains.add(new DocprocChainSpec("dpmusiccluster/chain.indexing"));
-        dpMusicCluster.chains.add(new DocprocChainSpec("dpmusiccluster/chain.dpmusicchain"));
-
-        dpBooksCluster.chains.clear();
-        dpBooksCluster.chains.add(new DocprocChainSpec("dpbookscluster/chain.indexing"));
-        dpBooksCluster.chains.add(new DocprocChainSpec("dpbookscluster/chain.dpbookschain"));
+    @Test
+    public void requiresIndexingInheritance() {
+        try {
+            SearchClusterSpec musicCluster = new SearchClusterSpec("musiccluster",
+                                                                   "dpmusiccluster",
+                                                                   "dpmusicchain");
+            musicCluster.searchDefs.add(new SearchDefSpec("music", "artist", "album"));
+
+            DocprocClusterSpec dpMusicCluster = new DocprocClusterSpec("dpmusiccluster", new DocprocChainSpec("dpmusicchain"));
+            getIndexedContentVespaModel(List.of(dpMusicCluster), List.of(musicCluster));
+            fail("Expected exception");
+        }
+        catch (IllegalArgumentException e) {
+            assertEquals("Docproc chain 'dpmusicchain' must inherit from the 'indexing' chain", e.getMessage());
+        }
+    }
 
-        assertIndexing(model, dpMusicCluster, dpBooksCluster);
-        assertFeedingRoute(model, MUSIC, "dpmusiccluster/chain.dpmusicchain");
-        assertFeedingRoute(model, BOOKS, "dpbookscluster/chain.dpbookschain");
+    @Test
+    public void indexingChainShouldNotBeTheDefaultChain() {
+        try {
+            SearchClusterSpec musicCluster = new SearchClusterSpec("musiccluster",
+                                                                   "dpmusiccluster",
+                                                                   "default");
+            musicCluster.searchDefs.add(new SearchDefSpec("music", "artist", "album"));
+
+            DocprocClusterSpec dpMusicCluster = new DocprocClusterSpec("dpmusiccluster", new DocprocChainSpec("default", "indexing"));
+            getIndexedContentVespaModel(List.of(dpMusicCluster), List.of(musicCluster));
+            fail("Expected exception");
+        }
+        catch (IllegalArgumentException e) {
+            assertTrue(e.getMessage().startsWith("Indexing cluster 'musiccluster' specifies the chain 'default' as indexing chain"));
+        }
     }
 
     private void assertIndexing(VespaModel model, DocprocClusterSpec... expectedDocprocClusters) {
         Map<String, ContainerCluster> docprocClusters = getDocprocClusters(model);
-        assertThat(docprocClusters.size(), is(expectedDocprocClusters.length));
+        assertEquals(expectedDocprocClusters.length, docprocClusters.size());
 
         for (DocprocClusterSpec expectedDocprocCluster : expectedDocprocClusters) {
             ContainerCluster docprocCluster = docprocClusters.get(expectedDocprocCluster.name);
-            assertThat(docprocCluster, not(nullValue()));
-            assertThat(docprocCluster.getName(), is(expectedDocprocCluster.name));
+            assertNotNull(docprocCluster);
+            assertEquals(expectedDocprocCluster.name, docprocCluster.getName());
             ContainerDocproc containerDocproc = docprocCluster.getDocproc();
-            assertThat(containerDocproc, not(nullValue()));
+            assertNotNull(containerDocproc);
             List<DocprocChain> chains = containerDocproc.getChains().allChains().allComponents();
-            assertThat(chains.size(), is(expectedDocprocCluster.chains.size()));
+            assertEquals(expectedDocprocCluster.chains.size(),  chains.size());
             List<String> actualDocprocChains = new ArrayList<>();
             for (DocprocChain chain : chains) {
                 actualDocprocChains.add(chain.getServiceName());
@@ -373,7 +361,8 @@ public class IndexingAndDocprocRoutingTest extends ContentBaseTest {
         return retval.toString();
     }
 
-    private String createVespaServicesWithContent(List<DocprocClusterSpec> docprocClusterSpecs, List<SearchClusterSpec> searchClusterSpecs) {
+    private String createVespaServicesWithContent(List<DocprocClusterSpec> docprocClusterSpecs,
+                                                  List<SearchClusterSpec> searchClusterSpecs) {
         String mainPre =
                 "<?xml version='1.0' encoding='utf-8' ?>\n" +
                         "<services version='1.0'>\n" +
@@ -393,7 +382,7 @@ public class IndexingAndDocprocRoutingTest extends ContentBaseTest {
             String docprocCluster = "";
             docprocCluster += "  <container version='1.0' id='" + docprocClusterSpec.name + "'>\n";
 
-            if (docprocClusterSpec.chains != null && docprocClusterSpec.chains.size() > 0) {
+            if (docprocClusterSpec.chains.size() > 0) {
                 docprocCluster += "    <document-processing>\n";
                 for (DocprocChainSpec chain : docprocClusterSpec.chains) {
                     if (chain.inherits.isEmpty()) {
@@ -465,11 +454,12 @@ public class IndexingAndDocprocRoutingTest extends ContentBaseTest {
                 createVespaServicesWithContent(docprocClusterSpecs, searchClusterSpecs), sds).create();
     }
 
-    private class SearchClusterSpec {
+    private static class SearchClusterSpec {
+
         private final String name;
-        private List<SearchDefSpec> searchDefs = new ArrayList<>(2);
-        private String indexingClusterName;
-        private String indexingChainName;
+        private final List<SearchDefSpec> searchDefs = new ArrayList<>(2);
+        private final String indexingClusterName;
+        private final String indexingChainName;
 
         private SearchClusterSpec(String name, String indexingClusterName, String indexingChainName) {
             this.name = name;
@@ -478,10 +468,11 @@ public class IndexingAndDocprocRoutingTest extends ContentBaseTest {
         }
     }
 
-    private class SearchDefSpec {
-        private String typeName;
-        private String field1Name;
-        private String field2Name;
+    private static class SearchDefSpec {
+
+        private final String typeName;
+        private final String field1Name;
+        private final String field2Name;
 
         private SearchDefSpec(String typeName, String field1Name, String field2Name) {
             this.typeName = typeName;
@@ -491,6 +482,7 @@ public class IndexingAndDocprocRoutingTest extends ContentBaseTest {
     }
 
     private class DocprocClusterSpec {
+
         private final String name;
         private final List<DocprocChainSpec> chains = new ArrayList<>();
 
@@ -500,7 +492,8 @@ public class IndexingAndDocprocRoutingTest extends ContentBaseTest {
         }
     }
 
-    private class DocprocChainSpec {
+    private static class DocprocChainSpec {
+
         private final String name;
         private final List<String> inherits = new ArrayList<>();
 
@@ -509,4 +502,5 @@ public class IndexingAndDocprocRoutingTest extends ContentBaseTest {
             this.inherits.addAll(Arrays.asList(inherits));
         }
     }
+
 }