RuleBase: Extract reading rules to a separate class Logic to read rules from a configuration file is extracted to a separate class. This has two benefits: * Simplify the RuleBase class and its corresponding test class * Prepare the way to introduce reading rules from project configuration which is done in a follow-up commit. Change-Id: I6c0e1ef43da0f34d2ad0ba7001d205a492ecfe87
diff --git a/src/main/java/com/googlesource/gerrit/plugins/its/base/workflow/RuleBase.java b/src/main/java/com/googlesource/gerrit/plugins/its/base/workflow/RuleBase.java index fc705ad..fffb7a0 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/its/base/workflow/RuleBase.java +++ b/src/main/java/com/googlesource/gerrit/plugins/its/base/workflow/RuleBase.java
@@ -14,6 +14,7 @@ package com.googlesource.gerrit.plugins.its.base.workflow; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import com.google.inject.Inject; import com.googlesource.gerrit.plugins.its.base.GlobalRulesFileName; @@ -23,6 +24,7 @@ import java.io.IOException; import java.nio.file.Path; import java.util.Collection; +import java.util.Collections; import java.util.Map; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.storage.file.FileBasedConfig; @@ -34,18 +36,8 @@ public class RuleBase { private static final Logger log = LoggerFactory.getLogger(RuleBase.class); - /** The section for rules within rulebases */ - private static final String RULE_SECTION = "rule"; - - /** The key for actions within rulebases */ - private static final String ACTION_KEY = "action"; - - private final Path itsPath; - private final Rule.Factory ruleFactory; - private final Condition.Factory conditionFactory; - private final ActionRequest.Factory actionRequestFactory; - private final String globalRulesFileName; - private final String pluginRulesFileName; + private final File globalRuleFile; + private final File itsSpecificRuleFile; private Collection<Rule> rules; @@ -56,82 +48,40 @@ @Inject public RuleBase( @ItsPath Path itsPath, - Rule.Factory ruleFactory, - Condition.Factory conditionFactory, - ActionRequest.Factory actionRequestFactory, @GlobalRulesFileName String globalRulesFileName, - @PluginRulesFileName String pluginRulesFileName) { - this.itsPath = itsPath; - this.ruleFactory = ruleFactory; - this.conditionFactory = conditionFactory; - this.actionRequestFactory = actionRequestFactory; - this.globalRulesFileName = globalRulesFileName; - this.pluginRulesFileName = pluginRulesFileName; - reloadRules(); + @PluginRulesFileName String pluginRulesFileName, + RulesConfigReader rulesConfigReader) { + this.globalRuleFile = itsPath.resolve(globalRulesFileName).toFile(); + this.itsSpecificRuleFile = itsPath.resolve(pluginRulesFileName).toFile(); + this.rules = + new ImmutableList.Builder<Rule>() + .addAll(getRulesFromFile(rulesConfigReader, globalRuleFile)) + .addAll(getRulesFromFile(rulesConfigReader, itsSpecificRuleFile)) + .build(); } /** - * Adds rules from a file to the RuleBase. + * Gets rules from a file. * * <p>If the given file does not exist, it is silently ignored * + * @param rulesConfigReader The rules reader * @param ruleFile File from which to read the rules + * @return A collection of rules or an empty collection if the file does not exist or contains an + * invalid configuration */ - private void addRulesFromFile(File ruleFile) { + private static Collection<Rule> getRulesFromFile( + RulesConfigReader rulesConfigReader, File ruleFile) { if (ruleFile.exists()) { FileBasedConfig cfg = new FileBasedConfig(ruleFile, FS.DETECTED); try { cfg.load(); + return rulesConfigReader.getRulesFromConfig(cfg); } catch (IOException | ConfigInvalidException e) { log.error("Invalid ITS action configuration", e); - return; - } - - for (String subsection : cfg.getSubsections(RULE_SECTION)) { - Rule rule = ruleFactory.create(subsection); - for (String key : cfg.getNames(RULE_SECTION, subsection)) { - String[] values = cfg.getStringList(RULE_SECTION, subsection, key); - if (ACTION_KEY.equals(key)) { - addActions(rule, values); - } else { - addConditions(rule, key, values); - } - } - rules.add(rule); } } - } - - private void addActions(Rule rule, String[] values) { - for (String value : values) { - rule.addActionRequest(actionRequestFactory.create(value)); - } - } - - private void addConditions(Rule rule, String key, String[] values) { - for (String value : values) { - rule.addCondition(conditionFactory.create(key, value)); - } - } - - /** Loads the rules for the RuleBase. */ - private void reloadRules() { - rules = Lists.newArrayList(); - - // Add global rules - File globalRuleFile = itsPath.resolve(globalRulesFileName).toFile(); - addRulesFromFile(globalRuleFile); - - // Add its-specific rules - File itsSpecificRuleFile = itsPath.resolve(pluginRulesFileName).toFile(); - addRulesFromFile(itsSpecificRuleFile); - - if (!globalRuleFile.exists() && !itsSpecificRuleFile.exists()) { - log.warn( - "Neither global rule file {} nor Its specific rule file {} exist. Please configure rules.", - globalRuleFile, - itsSpecificRuleFile); - } + return Collections.emptyList(); } /**
diff --git a/src/main/java/com/googlesource/gerrit/plugins/its/base/workflow/RulesConfigReader.java b/src/main/java/com/googlesource/gerrit/plugins/its/base/workflow/RulesConfigReader.java new file mode 100644 index 0000000..e4c0f0c --- /dev/null +++ b/src/main/java/com/googlesource/gerrit/plugins/its/base/workflow/RulesConfigReader.java
@@ -0,0 +1,72 @@ +// Copyright (C) 2018 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.googlesource.gerrit.plugins.its.base.workflow; + +import com.google.inject.Inject; +import java.util.ArrayList; +import java.util.Collection; +import org.eclipse.jgit.lib.Config; + +public class RulesConfigReader { + + /** The section for rules within rulebases */ + static final String RULE_SECTION = "rule"; + + /** The key for actions within rulebases */ + static final String ACTION_KEY = "action"; + + private final Rule.Factory ruleFactory; + private final Condition.Factory conditionFactory; + private final ActionRequest.Factory actionRequestFactory; + + @Inject + RulesConfigReader( + Rule.Factory ruleFactory, + Condition.Factory conditionFactory, + ActionRequest.Factory actionRequestFactory) { + this.ruleFactory = ruleFactory; + this.conditionFactory = conditionFactory; + this.actionRequestFactory = actionRequestFactory; + } + + Collection<Rule> getRulesFromConfig(Config cfg) { + Collection<Rule> rules = new ArrayList<>(); + for (String subsection : cfg.getSubsections(RULE_SECTION)) { + Rule rule = ruleFactory.create(subsection); + for (String key : cfg.getNames(RULE_SECTION, subsection)) { + String[] values = cfg.getStringList(RULE_SECTION, subsection, key); + if (ACTION_KEY.equals(key)) { + addActions(rule, values); + } else { + addConditions(rule, key, values); + } + } + rules.add(rule); + } + return rules; + } + + private void addActions(Rule rule, String[] values) { + for (String value : values) { + rule.addActionRequest(actionRequestFactory.create(value)); + } + } + + private void addConditions(Rule rule, String key, String[] values) { + for (String value : values) { + rule.addCondition(conditionFactory.create(key, value)); + } + } +}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/its/base/workflow/RuleBaseTest.java b/src/test/java/com/googlesource/gerrit/plugins/its/base/workflow/RuleBaseTest.java index 1002a92..367d2b3 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/its/base/workflow/RuleBaseTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/its/base/workflow/RuleBaseTest.java
@@ -14,7 +14,9 @@ package com.googlesource.gerrit.plugins.its.base.workflow; import static org.easymock.EasyMock.expect; +import static org.easymock.EasyMock.isA; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; import com.google.gerrit.extensions.annotations.PluginName; @@ -33,15 +35,15 @@ import java.util.List; import java.util.Map; import java.util.UUID; +import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.util.FileUtils; public class RuleBaseTest extends LoggingMockingTestCase { + private Injector injector; private Path itsPath; - private Rule.Factory ruleFactory; - private Condition.Factory conditionFactory; - private ActionRequest.Factory actionRequestFactory; + private RulesConfigReader rulesConfigReader; private boolean cleanupSitePath; @@ -56,105 +58,12 @@ } } - public void testWarnNonExistingRuleBase() { - replayMocks(); - - createRuleBase(); - - assertLogMessageContains("Neither global"); - } - - public void testEmptyRuleBase() throws IOException { - injectRuleBase(""); - - replayMocks(); - - createRuleBase(); - } - - public void testSimpleRuleBase() throws IOException { - injectRuleBase("[rule \"rule1\"]\n" + "\tconditionA = value1\n" + "\taction = action1"); - - Rule rule1 = createMock(Rule.class); - expect(ruleFactory.create("rule1")).andReturn(rule1); - - Condition condition1 = createMock(Condition.class); - expect(conditionFactory.create("conditionA", "value1")).andReturn(condition1); - rule1.addCondition(condition1); - - ActionRequest actionRequest1 = createMock(ActionRequest.class); - expect(actionRequestFactory.create("action1")).andReturn(actionRequest1); - rule1.addActionRequest(actionRequest1); - - replayMocks(); - - createRuleBase(); - } - - public void testBasicRuleBase() throws IOException { - injectRuleBase( - "[rule \"rule1\"]\n" - + "\tconditionA = value1,value2\n" - + "\tconditionA = value3,value of 4\n" - + "\tconditionB = value5\n" - + "\taction = action1\n" - + "\taction = action2 param\n" - + "\n" - + "[ruleXZ \"nonrule\"]\n" - + "\tconditionA = value1\n" - + "\taction = action2\n" - + "[rule \"rule2\"]\n" - + "\tconditionC = value6\n" - + "\taction = action3"); - - Rule rule1 = createMock(Rule.class); - expect(ruleFactory.create("rule1")).andReturn(rule1); - - Condition condition1 = createMock(Condition.class); - expect(conditionFactory.create("conditionA", "value1,value2")).andReturn(condition1); - rule1.addCondition(condition1); - - Condition condition2 = createMock(Condition.class); - expect(conditionFactory.create("conditionA", "value3,value of 4")).andReturn(condition2); - rule1.addCondition(condition2); - - Condition condition3 = createMock(Condition.class); - expect(conditionFactory.create("conditionB", "value5")).andReturn(condition3); - rule1.addCondition(condition3); - - ActionRequest actionRequest1 = createMock(ActionRequest.class); - expect(actionRequestFactory.create("action1")).andReturn(actionRequest1); - rule1.addActionRequest(actionRequest1); - - ActionRequest actionRequest2 = createMock(ActionRequest.class); - expect(actionRequestFactory.create("action2 param")).andReturn(actionRequest2); - rule1.addActionRequest(actionRequest2); - - Rule rule2 = createMock(Rule.class); - expect(ruleFactory.create("rule2")).andReturn(rule2); - - Condition condition4 = createMock(Condition.class); - expect(conditionFactory.create("conditionC", "value6")).andReturn(condition4); - rule2.addCondition(condition4); - - ActionRequest actionRequest3 = createMock(ActionRequest.class); - expect(actionRequestFactory.create("action3")).andReturn(actionRequest3); - rule2.addActionRequest(actionRequest3); - - replayMocks(); - - createRuleBase(); - } - public void testActionRequestsForSimple() throws IOException { - injectRuleBase("[rule \"rule1\"]\n" + "\taction = action1"); + String rules = "[rule \"rule1\"]\n\taction = action1\n"; + injectRuleBase(rules); Rule rule1 = createMock(Rule.class); - expect(ruleFactory.create("rule1")).andReturn(rule1); - ActionRequest actionRequest1 = createMock(ActionRequest.class); - expect(actionRequestFactory.create("action1")).andReturn(actionRequest1); - rule1.addActionRequest(actionRequest1); Map<String, String> properties = ImmutableMap.of(); @@ -162,6 +71,10 @@ rule1Match.add(actionRequest1); expect(rule1.actionRequestsFor(properties)).andReturn(rule1Match); + expect(rulesConfigReader.getRulesFromConfig(isA(Config.class))) + .andReturn(ImmutableList.of(rule1)) + .once(); + replayMocks(); RuleBase ruleBase = createRuleBase(); @@ -180,108 +93,69 @@ + "\taction = action2\n" + "\n" + "[rule \"rule2\"]\n" - + "\taction = action3"); + + "\taction = action3\n"); Rule rule1 = createMock(Rule.class); - expect(ruleFactory.create("rule1")).andReturn(rule1); - ActionRequest actionRequest1 = createMock(ActionRequest.class); - expect(actionRequestFactory.create("action1")).andReturn(actionRequest1); - rule1.addActionRequest(actionRequest1); - ActionRequest actionRequest2 = createMock(ActionRequest.class); - expect(actionRequestFactory.create("action2")).andReturn(actionRequest2); - rule1.addActionRequest(actionRequest2); Rule rule2 = createMock(Rule.class); - expect(ruleFactory.create("rule2")).andReturn(rule2); - ActionRequest actionRequest3 = createMock(ActionRequest.class); - expect(actionRequestFactory.create("action3")).andReturn(actionRequest3); - rule2.addActionRequest(actionRequest3); - Map<String, String> properties = ImmutableMap.of("sample", "property"); + Map<String, String> properties = ImmutableMap.of(); - List<ActionRequest> rule1Match = Lists.newArrayListWithCapacity(2); - rule1Match.add(actionRequest1); - rule1Match.add(actionRequest2); - expect(rule1.actionRequestsFor(properties)).andReturn(rule1Match); + List<ActionRequest> rule1Match = ImmutableList.of(actionRequest1, actionRequest2); + expect(rule1.actionRequestsFor(properties)).andReturn(rule1Match).anyTimes(); - List<ActionRequest> rule2Match = Lists.newArrayListWithCapacity(1); - rule2Match.add(actionRequest3); - expect(rule2.actionRequestsFor(properties)).andReturn(rule2Match); + List<ActionRequest> rule2Match = ImmutableList.of(actionRequest3); + expect(rule2.actionRequestsFor(properties)).andReturn(rule2Match).anyTimes(); + + expect(rulesConfigReader.getRulesFromConfig(isA(Config.class))) + .andReturn(ImmutableList.of(rule1, rule2)) + .andReturn(ImmutableList.of()) + .anyTimes(); replayMocks(); RuleBase ruleBase = createRuleBase(); Collection<ActionRequest> actual = ruleBase.actionRequestsFor(properties); - List<ActionRequest> expected = Lists.newArrayListWithCapacity(3); - expected.add(actionRequest1); - expected.add(actionRequest2); - expected.add(actionRequest3); + List<ActionRequest> expected = ImmutableList.of(actionRequest1, actionRequest2, actionRequest3); assertEquals("Matched actionRequests do not match", expected, actual); } - public void testSimpleItsRuleBase() throws IOException { - injectRuleBase( - "[rule \"rule1\"]\n" + "\tconditionA = value1\n" + "\taction = action1", RuleBaseKind.ITS); - - Rule rule1 = createMock(Rule.class); - expect(ruleFactory.create("rule1")).andReturn(rule1); - - Condition condition1 = createMock(Condition.class); - expect(conditionFactory.create("conditionA", "value1")).andReturn(condition1); - rule1.addCondition(condition1); - - ActionRequest actionRequest1 = createMock(ActionRequest.class); - expect(actionRequestFactory.create("action1")).andReturn(actionRequest1); - rule1.addActionRequest(actionRequest1); - - replayMocks(); - - createRuleBase(); - } - public void testAllRuleBaseFilesAreLoaded() throws IOException { - injectRuleBase("[rule \"rule2\"]\n" + "\taction = action2", RuleBaseKind.GLOBAL); + injectRuleBase("[rule \"rule2\"]\n\taction = action2", RuleBaseKind.GLOBAL); - injectRuleBase("[rule \"rule3\"]\n" + "\taction = action3", RuleBaseKind.ITS); + injectRuleBase("[rule \"rule3\"]\n\taction = action3", RuleBaseKind.ITS); - Map<String, String> properties = ImmutableMap.of("sample", "property"); + Map<String, String> properties = ImmutableMap.of(); Rule rule2 = createMock(Rule.class); - expect(ruleFactory.create("rule2")).andReturn(rule2); - ActionRequest actionRequest2 = createMock(ActionRequest.class); - expect(actionRequestFactory.create("action2")).andReturn(actionRequest2); - rule2.addActionRequest(actionRequest2); - List<ActionRequest> rule2Match = Lists.newArrayListWithCapacity(1); - rule2Match.add(actionRequest2); + List<ActionRequest> rule2Match = ImmutableList.of(actionRequest2); expect(rule2.actionRequestsFor(properties)).andReturn(rule2Match); Rule rule3 = createMock(Rule.class); - expect(ruleFactory.create("rule3")).andReturn(rule3); - ActionRequest actionRequest3 = createMock(ActionRequest.class); - expect(actionRequestFactory.create("action3")).andReturn(actionRequest3); - rule3.addActionRequest(actionRequest3); - List<ActionRequest> rule3Match = Lists.newArrayListWithCapacity(1); - rule3Match.add(actionRequest3); + List<ActionRequest> rule3Match = ImmutableList.of(actionRequest3); expect(rule3.actionRequestsFor(properties)).andReturn(rule3Match); + expect(rulesConfigReader.getRulesFromConfig(isA(Config.class))) + .andReturn(ImmutableList.of(rule2, rule3)) + .andReturn(ImmutableList.of()) + .anyTimes(); + replayMocks(); RuleBase ruleBase = createRuleBase(); Collection<ActionRequest> actual = ruleBase.actionRequestsFor(properties); - List<ActionRequest> expected = Lists.newArrayListWithCapacity(3); - expected.add(actionRequest2); - expected.add(actionRequest3); + List<ActionRequest> expected = ImmutableList.of(actionRequest2, actionRequest3); assertEquals("Matched actionRequests do not match", expected, actual); } @@ -333,6 +207,9 @@ bind(Path.class).annotatedWith(ItsPath.class).toInstance(itsPath); + rulesConfigReader = createMock(RulesConfigReader.class); + bind(RulesConfigReader.class).toInstance(rulesConfigReader); + bind(String.class) .annotatedWith(GlobalRulesFileName.class) .toInstance(RuleBaseKind.GLOBAL.fileName); @@ -340,15 +217,6 @@ bind(String.class) .annotatedWith(PluginRulesFileName.class) .toInstance(RuleBaseKind.ITS.fileName); - - ruleFactory = createMock(Rule.Factory.class); - bind(Rule.Factory.class).toInstance(ruleFactory); - - conditionFactory = createMock(Condition.Factory.class); - bind(Condition.Factory.class).toInstance(conditionFactory); - - actionRequestFactory = createMock(ActionRequest.Factory.class); - bind(ActionRequest.Factory.class).toInstance(actionRequestFactory); } } }
diff --git a/src/test/java/com/googlesource/gerrit/plugins/its/base/workflow/RulesConfigReaderTest.java b/src/test/java/com/googlesource/gerrit/plugins/its/base/workflow/RulesConfigReaderTest.java new file mode 100644 index 0000000..96fb869 --- /dev/null +++ b/src/test/java/com/googlesource/gerrit/plugins/its/base/workflow/RulesConfigReaderTest.java
@@ -0,0 +1,84 @@ +// Copyright (C) 2018 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.googlesource.gerrit.plugins.its.base.workflow; + +import static com.googlesource.gerrit.plugins.its.base.workflow.RulesConfigReader.ACTION_KEY; +import static com.googlesource.gerrit.plugins.its.base.workflow.RulesConfigReader.RULE_SECTION; +import static org.easymock.EasyMock.expect; + +import com.google.common.collect.ImmutableList; +import com.google.gerrit.extensions.config.FactoryModule; +import com.google.inject.Guice; +import com.google.inject.Injector; +import com.googlesource.gerrit.plugins.its.base.testutil.LoggingMockingTestCase; +import java.util.Collection; +import org.eclipse.jgit.lib.Config; + +public class RulesConfigReaderTest extends LoggingMockingTestCase { + + private class TestModule extends FactoryModule { + @Override + protected void configure() { + ruleFactory = createMock(Rule.Factory.class); + bind(Rule.Factory.class).toInstance(ruleFactory); + + conditionFactory = createMock(Condition.Factory.class); + bind(Condition.Factory.class).toInstance(conditionFactory); + + actionRequestFactory = createMock(ActionRequest.Factory.class); + bind(ActionRequest.Factory.class).toInstance(actionRequestFactory); + } + } + + private static final String ACTION_1 = "action1"; + private static final String CONDITION_KEY = "condition"; + private static final String RULE_1 = "rule1"; + private static final String VALUE_1 = "value1"; + + private ActionRequest.Factory actionRequestFactory; + private Condition.Factory conditionFactory; + private Rule.Factory ruleFactory; + private Injector injector; + + @Override + public void setUp() throws Exception { + super.setUp(); + injector = Guice.createInjector(new TestModule()); + } + + public void testGetRulesFromConfig() { + Config cfg = new Config(); + cfg.setString(RULE_SECTION, RULE_1, CONDITION_KEY, VALUE_1); + cfg.setString(RULE_SECTION, RULE_1, ACTION_KEY, ACTION_1); + + Rule rule1 = createMock(Rule.class); + expect(ruleFactory.create(RULE_1)).andReturn(rule1); + + ActionRequest actionRequest1 = createMock(ActionRequest.class); + expect(actionRequestFactory.create(ACTION_1)).andReturn(actionRequest1); + rule1.addActionRequest(actionRequest1); + + Condition condition1 = createMock(Condition.class); + expect(conditionFactory.create(CONDITION_KEY, VALUE_1)).andReturn(condition1); + rule1.addCondition(condition1); + + replayMocks(); + + Collection<Rule> expected = ImmutableList.of(rule1); + + RulesConfigReader rulesConfigReader = injector.getInstance(RulesConfigReader.class); + assertEquals("Rules do not match", expected, rulesConfigReader.getRulesFromConfig(cfg)); + } +}