From 54be959f7f932282fb6268a77f44ae0135154a1b Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Sun, 11 Jun 2023 09:18:54 +0200 Subject: Revert "Hmusum/revert validate semantic rules" --- .../container/search/SemanticRuleBuilder.java | 56 ++++++++++++++++++---- .../model/container/search/SemanticRules.java | 5 +- .../model/container/search/SemanticRulesTest.java | 48 ++++++++++++------- .../rules/one.sr | 5 ++ .../rules/other.sr | 5 ++ .../semanticrules_with_errors/rules/invalid.sr | 7 +++ .../prelude.semantics.semantic-rules.def | 1 + .../prelude/semantics/test/rulebases/cjk-rules.cfg | 1 - 8 files changed, 98 insertions(+), 30 deletions(-) create mode 100644 config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_duplicate_default_rule/rules/one.sr create mode 100644 config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_duplicate_default_rule/rules/other.sr create mode 100644 config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_errors/rules/invalid.sr 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..4a295d49a32 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 */ + /** Build the set of semantic rules for an application package and validates them */ public SemanticRules build(ApplicationPackage applicationPackage) { - List 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 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 toMap(SemanticRulesConfig config) throws ParseException, IOException { + RuleImporter ruleImporter = new RuleImporter(config, new SimpleLinguistics()); + Map 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 ruleBases; + private final List ruleBases; public SemanticRules(List 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 ruleBases = toMap(config); + Map 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 toMap(SemanticRulesConfig config) throws ParseException, IOException { - RuleImporter ruleImporter = new RuleImporter(config, new SimpleLinguistics()); - Map 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_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 -- cgit v1.2.3 From b226cf513893863e06750a8e3d813392cc2ce002 Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Sun, 11 Jun 2023 13:10:59 +0200 Subject: Ignore automata when validating semantic rules --- .../com/yahoo/vespa/model/container/search/SemanticRuleBuilder.java | 4 ++-- .../yahoo/vespa/model/container/search/semanticrules/rules/common.sr | 1 + 2 files changed, 3 insertions(+), 2 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 4a295d49a32..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 @@ -25,7 +25,7 @@ import java.util.Map; // TODO: Move into SemanticRules public class SemanticRuleBuilder { - /** Build the set of semantic rules for an application package and validates them */ + /** Builds the semantic rules for an application package and validates them */ public SemanticRules build(ApplicationPackage applicationPackage) { var ruleFiles = applicationPackage.getFiles(ApplicationPackage.RULES_DIR, "sr"); var rules = new SemanticRules(ruleFiles.stream().map(this::toRuleBaseConfigView).toList()); @@ -73,7 +73,7 @@ public class SemanticRuleBuilder { } static Map toMap(SemanticRulesConfig config) throws ParseException, IOException { - RuleImporter ruleImporter = new RuleImporter(config, new SimpleLinguistics()); + RuleImporter ruleImporter = new RuleImporter(config, true, new SimpleLinguistics()); Map ruleBaseMap = new HashMap<>(); for (SemanticRulesConfig.Rulebase ruleBaseConfig : config.rulebase()) { RuleBase ruleBase = ruleImporter.importConfig(ruleBaseConfig); 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 -- cgit v1.2.3