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(); + } +}