Fix check for branch enablement on event
When checking if an event is enabled for a specific branch, the wrong
name was being used. For example, branch "master" would fail when
checking against refPattern "refs/heads/master". Instead of using the
branch name, use the full refName to match.
Bug: issue 3099
Change-Id: I29a1b051e4cf7ddc944f0b1dc4111d81f8eb893e
diff --git a/src/main/java/com/googlesource/gerrit/plugins/hooks/its/ItsConfig.java b/src/main/java/com/googlesource/gerrit/plugins/hooks/its/ItsConfig.java
index 83ed535..89bccf4 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/hooks/its/ItsConfig.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/hooks/its/ItsConfig.java
@@ -52,19 +52,19 @@
public boolean isEnabled(Event event) {
if (event instanceof PatchSetCreatedEvent) {
PatchSetCreatedEvent e = (PatchSetCreatedEvent) event;
- return isEnabled(e.change.project, e.change.branch);
+ return isEnabled(e.change.project, e.getRefName());
} else if (event instanceof CommentAddedEvent) {
CommentAddedEvent e = (CommentAddedEvent) event;
- return isEnabled(e.change.project, e.change.branch);
+ return isEnabled(e.change.project, e.getRefName());
} else if (event instanceof ChangeMergedEvent) {
ChangeMergedEvent e = (ChangeMergedEvent) event;
- return isEnabled(e.change.project, e.change.branch);
+ return isEnabled(e.change.project, e.getRefName());
} else if (event instanceof ChangeAbandonedEvent) {
ChangeAbandonedEvent e = (ChangeAbandonedEvent) event;
- return isEnabled(e.change.project, e.change.branch);
+ return isEnabled(e.change.project, e.getRefName());
} else if (event instanceof ChangeRestoredEvent) {
ChangeRestoredEvent e = (ChangeRestoredEvent) event;
- return isEnabled(e.change.project, e.change.branch);
+ return isEnabled(e.change.project, e.getRefName());
} else if (event instanceof RefUpdatedEvent) {
RefUpdatedEvent e = (RefUpdatedEvent) event;
return isEnabled(e.refUpdate.project, e.refUpdate.refName);
@@ -74,7 +74,7 @@
}
}
- public boolean isEnabled(String project, String branch) {
+ public boolean isEnabled(String project, String refName) {
ProjectState projectState = projectCache.get(new Project.NameKey(project));
if (projectState == null) {
log.error("Failed to check if " + pluginName + " is enabled for project "
@@ -86,17 +86,17 @@
PluginConfig parentCfg =
pluginCfgFactory.getFromProjectConfig(parentState, pluginName);
if ("enforced".equals(parentCfg.getString("enabled"))
- && isEnabledForBranch(parentState, branch)) {
+ && isEnabledForBranch(parentState, refName)) {
return true;
}
}
return pluginCfgFactory.getFromProjectConfigWithInheritance(
projectState, pluginName).getBoolean("enabled", false)
- && isEnabledForBranch(projectState, branch);
+ && isEnabledForBranch(projectState, refName);
}
- private boolean isEnabledForBranch(ProjectState project, String branch) {
+ private boolean isEnabledForBranch(ProjectState project, String refName) {
String[] refPatterns =
pluginCfgFactory.getFromProjectConfigWithInheritance(project,
pluginName).getStringList("branch");
@@ -104,14 +104,14 @@
return true;
}
for (String refPattern : refPatterns) {
- if (RefConfigSection.isValid(refPattern) && match(branch, refPattern)) {
+ if (RefConfigSection.isValid(refPattern) && match(refName, refPattern)) {
return true;
}
}
return false;
}
- private boolean match(String branch, String refPattern) {
- return RefPatternMatcher.getMatcher(refPattern).match(branch, null);
+ private boolean match(String refName, String refPattern) {
+ return RefPatternMatcher.getMatcher(refPattern).match(refName, null);
}
}
diff --git a/src/main/resources/Documentation/config-common.md b/src/main/resources/Documentation/config-common.md
index 4ad3c97..13720b4 100644
--- a/src/main/resources/Documentation/config-common.md
+++ b/src/main/resources/Documentation/config-common.md
@@ -90,7 +90,7 @@
The issue tracker system integration can be limited to specific
branches by setting `plugin.@PLUGIN@.branch`. The branches may be
-configured using explicit branch names, ref patterns, or regular
+configured using explicit ref names, ref patterns, or regular
expressions. Multiple branches may be specified.
E.g. to limit the issue tracker system integration to the `master`
diff --git a/src/test/java/com/googlesource/gerrit/plugins/hooks/its/ItsConfigTest.java b/src/test/java/com/googlesource/gerrit/plugins/hooks/its/ItsConfigTest.java
new file mode 100644
index 0000000..72d229c
--- /dev/null
+++ b/src/test/java/com/googlesource/gerrit/plugins/hooks/its/ItsConfigTest.java
@@ -0,0 +1,145 @@
+// Copyright (C) 2015 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.hooks.its;
+
+import static org.easymock.EasyMock.expect;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.config.PluginConfig;
+import com.google.gerrit.server.config.PluginConfigFactory;
+import com.google.gerrit.server.data.ChangeAttribute;
+import com.google.gerrit.server.events.PatchSetCreatedEvent;
+import com.google.gerrit.server.project.ProjectCache;
+import com.google.gerrit.server.project.ProjectState;
+
+import org.easymock.EasyMockSupport;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.ArrayList;
+
+public class ItsConfigTest {
+ private EasyMockSupport easyMock;
+ private String pluginName = "its-base";
+ private PluginConfig pluginConfig;
+ private PluginConfig pluginConfigWithInheritance;
+ private ItsConfig itsConfig;
+ private PatchSetCreatedEvent pscEvent;
+
+ @Before
+ public void setup() {
+ easyMock = new EasyMockSupport();
+ ProjectCache projectCache = easyMock.createNiceMock(ProjectCache.class);
+ PluginConfigFactory pluginCfgFactory = easyMock.createNiceMock(PluginConfigFactory.class);
+ ProjectState projectState = easyMock.createNiceMock(ProjectState.class);
+ pluginConfig = easyMock.createNiceMock(PluginConfig.class);
+ pluginConfigWithInheritance = easyMock.createNiceMock(PluginConfig.class);
+ itsConfig = new ItsConfig(pluginName, projectCache, pluginCfgFactory);
+
+ pscEvent = new PatchSetCreatedEvent();
+ pscEvent.change = new ChangeAttribute();
+ pscEvent.change.project = "testProject";
+ pscEvent.change.branch = "testBranch";
+ ArrayList<ProjectState> listProjectState = new ArrayList<>(1);
+ listProjectState.add(projectState);
+
+ expect(projectCache.get(new Project.NameKey("testProject"))).andReturn(
+ projectState).once();
+ expect(projectState.treeInOrder()).andReturn(listProjectState);
+ expect(pluginCfgFactory.getFromProjectConfig(projectState, pluginName))
+ .andStubReturn(pluginConfig);
+ expect(
+ pluginCfgFactory.getFromProjectConfigWithInheritance(projectState,
+ pluginName)).andStubReturn(pluginConfigWithInheritance);
+ }
+
+ @Test
+ public void testEnforcedWithRegex() {
+ String[] refPatterns = {"^refs/heads/test.*"};
+ setUpEnforced(refPatterns);
+
+ assertTrue(itsConfig.isEnabled(pscEvent));
+ easyMock.verifyAll();
+ }
+
+ @Test
+ public void testEnforcedWithExact() {
+ String[] refPatterns = {"refs/heads/testBranch"};
+ setUpEnforced(refPatterns);
+
+ assertTrue(itsConfig.isEnabled(pscEvent));
+ easyMock.verifyAll();
+ }
+
+ @Test
+ public void testEnforcedForAll() {
+ String[] refPatterns = new String[0];
+ setUpEnforced(refPatterns);
+
+ assertTrue(itsConfig.isEnabled(pscEvent));
+ easyMock.verifyAll();
+ }
+
+ @Test
+ public void testEnabledWithRegex() {
+ String[] refPatterns = {"^refs/heads/test.*"};
+ setUpEnabled(refPatterns);
+
+ assertTrue(itsConfig.isEnabled(pscEvent));
+ easyMock.verifyAll();
+ }
+
+ @Test
+ public void testEnabledWithExact() {
+ String[] refPatterns = {"refs/heads/testBranch"};
+ setUpEnabled(refPatterns);
+
+ assertTrue(itsConfig.isEnabled(pscEvent));
+ easyMock.verifyAll();
+ }
+
+ @Test
+ public void testDisabled() {
+ String[] refPatterns = {"refs/heads/testBranch1"};
+ setUpEnabled(refPatterns);
+
+ assertFalse(itsConfig.isEnabled(pscEvent));
+ easyMock.verifyAll();
+ }
+
+ @Test
+ public void testInvalidPattern() {
+ String[] refPatterns = {"testBranch"};
+ setUpEnabled(refPatterns);
+
+ assertFalse(itsConfig.isEnabled(pscEvent));
+ easyMock.verifyAll();
+ }
+
+ private void setUpEnforced(String[] refPatterns) {
+ expect(pluginConfig.getString("enabled")).andReturn("enforced").once();
+ expect(pluginConfigWithInheritance.getStringList("branch")).andReturn(refPatterns);
+ easyMock.replayAll();
+ }
+
+ private void setUpEnabled(String[] refPatterns) {
+ expect(pluginConfig.getString("enabled")).andReturn("true");
+ expect(pluginConfigWithInheritance.getBoolean("enabled", false)).andReturn(true);
+ expect(pluginConfigWithInheritance.getStringList("branch")).andReturn(refPatterns);
+ easyMock.replayAll();
+ }
+}
