From dcfc689ea28052b7378805f805e3922b91d9e376 Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Sat, 10 Jun 2023 11:43:59 +0200 Subject: Revert "Make error message deterministic by sorting rule names" This reverts commit 7f13594d6595b81a291018f902c438d3aaea057b. --- .../vespa/model/container/search/SemanticRuleBuilder.java | 10 +++------- .../yahoo/vespa/model/container/search/SemanticRulesTest.java | 2 +- 2 files changed, 4 insertions(+), 8 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..b184861c102 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 @@ -12,9 +12,7 @@ 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.Map; /** @@ -62,11 +60,9 @@ public class SemanticRuleBuilder { 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 (defaultName != null && ruleBase.isdefault()) + throw new IllegalArgumentException("Both '" + defaultName + "' and '" + ruleBase.name() + + "' is marked as default rule, there can only be one"); if (ruleBase.isdefault()) defaultName = ruleBase.name(); } 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 e20e14f9465..f0e28f9df09 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 @@ -60,7 +60,7 @@ public class SemanticRulesTest { 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()); + assertEquals("Both 'one' and 'other' is marked as default rule, there can only be one", e.getMessage()); } } -- cgit v1.2.3 From 4abbbe29cf9c38104be31ff6820f0118fddb3557 Mon Sep 17 00:00:00 2001 From: Harald Musum Date: Sat, 10 Jun 2023 11:44:14 +0200 Subject: Revert "Validate semantic rules when building config model" This reverts commit fa0044d92067bfe91a104d0a6bb6b085c0b9439e. --- .../container/search/SemanticRuleBuilder.java | 54 ++++------------------ .../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, 31 insertions(+), 95 deletions(-) delete mode 100644 config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_duplicate_default_rule/rules/one.sr delete mode 100644 config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_duplicate_default_rule/rules/other.sr delete 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 b184861c102..ce999603439 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,16 +4,11 @@ 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.HashMap; -import java.util.Map; +import java.util.List; +import java.util.stream.Collectors; /** * Reads the semantic rules from the application package by delegating to SemanticRules. @@ -23,22 +18,16 @@ import java.util.Map; // TODO: Move into SemanticRules public class SemanticRuleBuilder { - /** Build the set of semantic rules for an application package and validates them */ + /** Build the set of semantic rules for an application package */ public SemanticRules build(ApplicationPackage applicationPackage) { - 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(); + List ruleBaseFiles = null; try { - toMap(config); // validates config - ensureZeroOrOneDefaultRule(config); - } catch (ParseException | IOException e) { - throw new RuntimeException(e); + ruleBaseFiles = applicationPackage.getFiles(ApplicationPackage.RULES_DIR, "sr"); + return new SemanticRules(ruleBaseFiles.stream().map(this::toRuleBaseConfigView).toList()); + } + finally { + NamedReader.closeAll(ruleBaseFiles); } - return rules; } private SemanticRules.RuleBase toRuleBaseConfigView(NamedReader reader) { @@ -54,30 +43,7 @@ public class SemanticRuleBuilder { private String toName(String fileName) { String shortName = new File(fileName).getName(); - 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()) - throw new IllegalArgumentException("Both '" + defaultName + "' and '" + ruleBase.name() + - "' is marked as 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; + return shortName.substring(0, shortName.length()-".sr".length()); } } 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 0083fc6b886..46c8577c6e8 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 from a set of rule bases. + * Returns the semantic rules config form a set of rule bases. * Owned by a container cluster * * @author bratseth */ public class SemanticRules implements Serializable, SemanticRulesConfig.Producer { - private final List ruleBases; + private List ruleBases; public SemanticRules(List ruleBases) { this.ruleBases = ruleBases; @@ -46,6 +46,7 @@ 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 f0e28f9df09..d9e2ae59ef6 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,66 +2,52 @@ 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 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"; + private final static String root = "src/test/java/com/yahoo/vespa/model/container/search/semanticrules"; @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 = SemanticRuleBuilder.toMap(config); + Map ruleBases = 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")); } - @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("Both 'one' and 'other' is marked as default rule, there can only be one", e.getMessage()); + 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); } + 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 deleted file mode 100644 index 4f2271e91ba..00000000000 --- a/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_duplicate_default_rule/rules/one.sr +++ /dev/null @@ -1,5 +0,0 @@ -# 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 deleted file mode 100644 index 29f7e85967f..00000000000 --- a/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_duplicate_default_rule/rules/other.sr +++ /dev/null @@ -1,5 +0,0 @@ -# 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 deleted file mode 100644 index 9d89cab7e31..00000000000 --- a/config-model/src/test/java/com/yahoo/vespa/model/container/search/semanticrules_with_errors/rules/invalid.sr +++ /dev/null @@ -1,7 +0,0 @@ -# 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 d8e521ed940..71fb907ffe2 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,7 +3,6 @@ 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 2e3299f7723..b040a44ad90 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,3 +1,4 @@ +compatibility false rulebase[1] rulebase[0].name "cjk" rulebase[0].isdefault false -- cgit v1.2.3