Merge pull request #27368 from vespa-engine/revert-27367-hmusum/revert-validate-semantic-rulesv8.175.37

Validate semantic rules

9 files changed, 99 insertions, 30 deletions

diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/search/SemanticRuleBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/container/search/SemanticRuleBuilder.java
index ce999603439..4923c41224e 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/search/SemanticRuleBuilder.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/search/SemanticRuleBuilder.java
@@ -4,11 +4,18 @@ package com.yahoo.vespa.model.container.search;
 import com.yahoo.config.application.api.ApplicationPackage;
 import com.yahoo.io.IOUtils;
 import com.yahoo.io.reader.NamedReader;
+import com.yahoo.language.simple.SimpleLinguistics;
+import com.yahoo.prelude.semantics.RuleBase;
+import com.yahoo.prelude.semantics.RuleImporter;
+import com.yahoo.prelude.semantics.SemanticRulesConfig;
+import com.yahoo.prelude.semantics.parser.ParseException;
 
 import java.io.File;
 import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.List;
-import java.util.stream.Collectors;
+import java.util.Map;
 
 /**
  * Reads the semantic rules from the application package by delegating to SemanticRules.
@@ -18,16 +25,22 @@ import java.util.stream.Collectors;
 // TODO: Move into SemanticRules
 public class SemanticRuleBuilder {
 
-    /** Build the set of semantic rules for an application package */
+    /** Builds the semantic rules for an application package and validates them */
     public SemanticRules build(ApplicationPackage applicationPackage) {
-        List<NamedReader> ruleBaseFiles = null;
+        var ruleFiles = applicationPackage.getFiles(ApplicationPackage.RULES_DIR, "sr");
+        var rules = new SemanticRules(ruleFiles.stream().map(this::toRuleBaseConfigView).toList());
+
+        // Create config to make sure rules are valid, config is validated in call to toMap() below
+        var builder = new SemanticRulesConfig.Builder();
+        rules.getConfig(builder);
+        SemanticRulesConfig config = builder.build();
         try {
-            ruleBaseFiles = applicationPackage.getFiles(ApplicationPackage.RULES_DIR, "sr");
-            return new SemanticRules(ruleBaseFiles.stream().map(this::toRuleBaseConfigView).toList());
-        }
-        finally {
-            NamedReader.closeAll(ruleBaseFiles);
+            toMap(config);  // validates config
+            ensureZeroOrOneDefaultRule(config);
+        } catch (ParseException | IOException e) {
+            throw new RuntimeException(e);
         }
+        return rules;
     }
 
     private SemanticRules.RuleBase toRuleBaseConfigView(NamedReader reader) {
@@ -43,7 +56,32 @@ public class SemanticRuleBuilder {
 
     private String toName(String fileName) {
         String shortName = new File(fileName).getName();
-        return shortName.substring(0, shortName.length()-".sr".length());
+        return shortName.substring(0, shortName.length() - ".sr".length());
+    }
+
+    private void ensureZeroOrOneDefaultRule(SemanticRulesConfig config) {
+        String defaultName = null;
+        for (SemanticRulesConfig.Rulebase ruleBase : config.rulebase()) {
+            if (defaultName != null && ruleBase.isdefault()) {
+                List<String> defaultRules = new ArrayList<>(List.of(defaultName, ruleBase.name()));
+                defaultRules.sort(String::compareTo);
+                throw new IllegalArgumentException("Rules " + defaultRules + " are both marked as the default rule, there can only be one");
+            }
+            if (ruleBase.isdefault())
+                defaultName = ruleBase.name();
+        }
+    }
+
+    static Map<String, RuleBase> toMap(SemanticRulesConfig config) throws ParseException, IOException {
+        RuleImporter ruleImporter = new RuleImporter(config, true, new SimpleLinguistics());
+        Map<String, RuleBase> ruleBaseMap = new HashMap<>();
+        for (SemanticRulesConfig.Rulebase ruleBaseConfig : config.rulebase()) {
+            RuleBase ruleBase = ruleImporter.importConfig(ruleBaseConfig);
+            if (ruleBaseConfig.isdefault())
+                ruleBase.setDefault(true);
+            ruleBaseMap.put(ruleBase.getName(), ruleBase);
+        }
+        return ruleBaseMap;
     }
 
 }
diff --git a/config-model/src/main/java/com/yahoo/vespa/model/container/search/SemanticRules.java b/config-model/src/main/java/com/yahoo/vespa/model/container/search/SemanticRules.java
index 46c8577c6e8..0083fc6b886 100644
--- a/config-model/src/main/java/com/yahoo/vespa/model/container/search/SemanticRules.java
+++ b/config-model/src/main/java/com/yahoo/vespa/model/container/search/SemanticRules.java
@@ -6,14 +6,14 @@ import java.io.Serializable;
 import java.util.List;
 
 /**
- * Returns the semantic rules config form a set of rule bases.
+ * Returns the semantic rules config from a set of rule bases.
  * Owned by a container cluster
  *
  * @author bratseth
  */
 public class SemanticRules implements Serializable, SemanticRulesConfig.Producer {
 
-    private List<RuleBase> ruleBases;
+    private final List<RuleBase> ruleBases;
 
     public SemanticRules(List<RuleBase> ruleBases) {
         this.ruleBases = ruleBases;
@@ -46,7 +46,6 @@ public class SemanticRules implements Serializable, SemanticRulesConfig.Producer
             return ruleBaseBuilder;
         }
 
-
     }
 
 }
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/search/SemanticRulesTest.java b/config-model/src/test/java/com/yahoo/vespa/model/container/search/SemanticRulesTest.java
index d9e2ae59ef6..e20e14f9465 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/container/search/SemanticRulesTest.java
+++ b/config-model/src/test/java/com/yahoo/vespa/model/container/search/SemanticRulesTest.java
@@ -2,52 +2,66 @@
 package com.yahoo.vespa.model.container.search;
 
 import com.yahoo.config.model.application.provider.FilesApplicationPackage;
-import com.yahoo.language.simple.SimpleLinguistics;
 import com.yahoo.prelude.semantics.RuleBase;
-import com.yahoo.prelude.semantics.RuleImporter;
 import com.yahoo.prelude.semantics.SemanticRulesConfig;
 import com.yahoo.prelude.semantics.parser.ParseException;
 import org.junit.jupiter.api.Test;
 
-import static org.junit.jupiter.api.Assertions.*;
-
 import java.io.File;
 import java.io.IOException;
-import java.util.HashMap;
 import java.util.Map;
 
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
+
 /**
  * @author bratseth
  */
 public class SemanticRulesTest {
 
-    private final static String root = "src/test/java/com/yahoo/vespa/model/container/search/semanticrules";
+    private static final String basePath = "src/test/java/com/yahoo/vespa/model/container/search/";
+    private static final String root = basePath + "semanticrules";
+    private static final String rootWithErrors = basePath + "semanticrules_with_errors";
+    private static final String rootWithDuplicateDefault = basePath + "semanticrules_with_duplicate_default_rule";
 
     @Test
-    void semanticRulesTest()  throws ParseException, IOException {
+    void semanticRulesTest() throws ParseException, IOException {
         SemanticRuleBuilder ruleBuilder = new SemanticRuleBuilder();
         SemanticRules rules = ruleBuilder.build(FilesApplicationPackage.fromFile(new File(root)));
         SemanticRulesConfig.Builder configBuilder = new SemanticRulesConfig.Builder();
         rules.getConfig(configBuilder);
         SemanticRulesConfig config = new SemanticRulesConfig(configBuilder);
-        Map<String, RuleBase> ruleBases = toMap(config);
+        Map<String, RuleBase> ruleBases = SemanticRuleBuilder.toMap(config);
         assertEquals(2, ruleBases.size());
         assertTrue(ruleBases.containsKey("common"));
         assertTrue(ruleBases.containsKey("other"));
         assertFalse(ruleBases.get("common").isDefault());
         assertTrue(ruleBases.get("other").isDefault());
+        assertTrue(ruleBases.get("other").includes("common"));
+        assertNotNull(ruleBases.get("other").getCondition("stopword"));
     }
 
-    private static Map<String, RuleBase> toMap(SemanticRulesConfig config) throws ParseException, IOException {
-        RuleImporter ruleImporter = new RuleImporter(config, new SimpleLinguistics());
-        Map<String, RuleBase> ruleBaseMap = new HashMap<>();
-        for (SemanticRulesConfig.Rulebase ruleBaseConfig : config.rulebase()) {
-            RuleBase ruleBase = ruleImporter.importConfig(ruleBaseConfig);
-            if (ruleBaseConfig.isdefault())
-                ruleBase.setDefault(true);
-            ruleBaseMap.put(ruleBase.getName(), ruleBase);
+    @Test
+    void rulesWithErrors() {
+        try {
+            new SemanticRuleBuilder().build(FilesApplicationPackage.fromFile(new File(rootWithErrors)));
+            fail("should fail with exception");
+        } catch (Exception e) {
+            assertEquals("com.yahoo.prelude.semantics.parser.ParseException: Could not parse 'semantic-rules.cfg'", e.getMessage());
+        }
+    }
+
+    @Test
+    void rulesWithDuplicateDefault() {
+        try {
+            new SemanticRuleBuilder().build(FilesApplicationPackage.fromFile(new File(rootWithDuplicateDefault)));
+            fail("should fail with exception");
+        } catch (Exception e) {
+            assertEquals("Rules [one, other] are both marked as the default rule, there can only be one", e.getMessage());
         }
-        return ruleBaseMap;
     }
 
 }
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules/rules/common.sr b/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules/rules/common.sr
index 3833641a56c..92b279aada4 100644
--- a/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules/rules/common.sr
+++ b/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules/rules/common.sr
@@ -1,4 +1,5 @@
 # Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+@automata(etc/vespa/fsa/stopwords.fsa)
 ## Some test rules
 
 # Spelling correction
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_duplicate_default_rule/rules/one.sr b/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_duplicate_default_rule/rules/one.sr
new file mode 100644
index 00000000000..4f2271e91ba
--- /dev/null
+++ b/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_duplicate_default_rule/rules/one.sr
@@ -0,0 +1,5 @@
+# Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+@default
+
+# Spelling correction
+bahc -> bach;
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_duplicate_default_rule/rules/other.sr b/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_duplicate_default_rule/rules/other.sr
new file mode 100644
index 00000000000..29f7e85967f
--- /dev/null
+++ b/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_duplicate_default_rule/rules/other.sr
@@ -0,0 +1,5 @@
+# Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+@default
+
+# Spelling correction
+list-> liszt;
diff --git a/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_errors/rules/invalid.sr b/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_errors/rules/invalid.sr
new file mode 100644
index 00000000000..9d89cab7e31
--- /dev/null
+++ b/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_errors/rules/invalid.sr
@@ -0,0 +1,7 @@
+# Copyright Yahoo. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+# Stopwords
+[stopword] -> ;
+[stopword] :- and, or, the, what, why, how;
+
+# Synonyms, with wrong character at end of line
+[bill] :- Bill, bill, William:
diff --git a/container-search/src/main/resources/configdefinitions/prelude.semantics.semantic-rules.def b/container-search/src/main/resources/configdefinitions/prelude.semantics.semantic-rules.def
index 71fb907ffe2..d8e521ed940 100644
--- a/container-search/src/main/resources/configdefinitions/prelude.semantics.semantic-rules.def
+++ b/container-search/src/main/resources/configdefinitions/prelude.semantics.semantic-rules.def
@@ -3,6 +3,7 @@
 namespace=prelude.semantics
 
 # Whether we should use these rule bases in pre-Vespa 2.2 compatibility mode
+# TODO: Unused, remove in Vespa 9
 compatibility bool default=false
 
 # The name of a rule base
diff --git a/container-search/src/test/java/com/yahoo/prelude/semantics/test/rulebases/cjk-rules.cfg b/container-search/src/test/java/com/yahoo/prelude/semantics/test/rulebases/cjk-rules.cfg
index b040a44ad90..2e3299f7723 100644
--- a/container-search/src/test/java/com/yahoo/prelude/semantics/test/rulebases/cjk-rules.cfg
+++ b/container-search/src/test/java/com/yahoo/prelude/semantics/test/rulebases/cjk-rules.cfg
@@ -1,4 +1,3 @@
-compatibility false
 rulebase[1]
 rulebase[0].name "cjk"
 rulebase[0].isdefault false