From b2fa80d4cf7cb2d863ddc15894541b1250c2036a Mon Sep 17 00:00:00 2001 From: Markus Ebenhoeh Date: Sun, 22 Nov 2015 10:51:55 +1100 Subject: [PATCH 1/4] Move getBranches logic to separate class * Replace logging of job.getName with getFullname * Change cache key from job name to repo URL * Change cache mechanism to allow expriry * Cache gitlab project list Fixes #125 --- .../GitLabProjectBranchesService.java | 160 ++++++++++++++ .../gitlabjenkins/GitLabPushTrigger.java | 78 +++---- .../GitLabProjectBranchesServiceTest.java | 203 ++++++++++++++++++ 3 files changed, 388 insertions(+), 53 deletions(-) create mode 100644 src/main/java/com/dabsquared/gitlabjenkins/GitLabProjectBranchesService.java create mode 100644 src/test/java/com/dabsquared/gitlabjenkins/GitLabProjectBranchesServiceTest.java diff --git a/src/main/java/com/dabsquared/gitlabjenkins/GitLabProjectBranchesService.java b/src/main/java/com/dabsquared/gitlabjenkins/GitLabProjectBranchesService.java new file mode 100644 index 0000000..fe8a532 --- /dev/null +++ b/src/main/java/com/dabsquared/gitlabjenkins/GitLabProjectBranchesService.java @@ -0,0 +1,160 @@ +package com.dabsquared.gitlabjenkins; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.logging.Level; +import java.util.logging.Logger; + +import org.gitlab.api.GitlabAPI; +import org.gitlab.api.models.GitlabBranch; +import org.gitlab.api.models.GitlabProject; + +public class GitLabProjectBranchesService { + + private static final Logger LOGGER = Logger.getLogger(GitLabProjectBranchesService.class.getName()); + + /** + * A map of git projects' branches; this is cached for + * BRANCH_CACHE_TIME_IN_MILLISECONDS ms + */ + private final Map projectBranchCache = new HashMap(); + + /** + * length of time a git project's branch list is kept in the + * projectBranchCache for a particular source Repository + */ + protected static final long BRANCH_CACHE_TIME_IN_MILLISECONDS = 5000; + + /** + * a map of git projects; this is cached for + * PROJECT_LIST_CACHE_TIME_IN_MILLISECONDS ms + */ + private HashMap projectMapCache = new HashMap(); + + /** + * length of time the list of git project is kept without being refreshed + * the map is also refreshed when a key hasnt been found, so we can leave + * the cache time high e.g. 1 day: + */ + protected static final long PROJECT_MAP_CACHE_TIME_IN_MILLISECONDS = 24 * 3600 * 1000; + + /** + * time (epoch) the project cache will have expired + */ + private long projectCacheExpiry; + + private final GitlabAPI gitlabAPI; + + private final TimeUtility timeUtility; + + protected GitLabProjectBranchesService(GitlabAPI gitlabAPI, TimeUtility timeUtility) { + this.gitlabAPI = gitlabAPI; + this.timeUtility = timeUtility; + } + + public List getBranches(String sourceRepositoryString) throws IOException { + + synchronized (projectBranchCache) { + BranchListEntry branchListEntry = projectBranchCache.get(sourceRepositoryString); + if (branchListEntry != null && !branchListEntry.hasExpired()) { + if (LOGGER.isLoggable(Level.FINEST)) { + LOGGER.log(Level.FINEST, "found branches in cache for {0}", sourceRepositoryString); + } + return branchListEntry.branchNames; + } + + final List branchNames = new ArrayList(); + + try { + GitlabProject gitlabProject = findGitlabProjectForRepositoryUrl(sourceRepositoryString); + if (gitlabProject != null) { + final List branches = gitlabAPI.getBranches(gitlabProject); + for (final GitlabBranch branch : branches) { + branchNames.add(branch.getName()); + } + projectBranchCache.put(sourceRepositoryString, new BranchListEntry(branchNames)); + + if (LOGGER.isLoggable(Level.FINEST)) { + LOGGER.log(Level.FINEST, "found these branches for repo {0} : {1}", + new Object[] { sourceRepositoryString, branchNames.toString() }); + } + } + } catch (final Error error) { + /* WTF WTF WTF */ + final Throwable cause = error.getCause(); + if (cause instanceof IOException) { + throw (IOException) cause; + } else { + throw error; + } + } + return branchNames; + } + } + + public GitlabProject findGitlabProjectForRepositoryUrl(String sourceRepositoryString) throws IOException { + synchronized (projectMapCache) { + String repositoryUrl = sourceRepositoryString.toLowerCase(); + if (projectCacheExpiry < timeUtility.getCurrentTimeInMillis() + || !projectMapCache.containsKey(repositoryUrl)) { + + if (LOGGER.isLoggable(Level.FINEST)) { + LOGGER.log(Level.FINEST, + "refreshing repo map for {0} because expired : {1} or missing Key {2} expiry:{3} TS:{4}", + new Object[] { sourceRepositoryString, + (Boolean) (projectCacheExpiry < timeUtility.getCurrentTimeInMillis()), + (Boolean) projectMapCache.containsKey(repositoryUrl), projectCacheExpiry, + timeUtility.getCurrentTimeInMillis() }); + } + refreshGitLabProjectMap(); + } + return projectMapCache.get(repositoryUrl); + } + } + + public Map refreshGitLabProjectMap() throws IOException { + synchronized (projectMapCache) { + try { + projectMapCache.clear(); + List projects = gitlabAPI.getProjects(); + for (GitlabProject gitlabProject : projects) { + projectMapCache.put(gitlabProject.getSshUrl().toLowerCase(), gitlabProject); + projectMapCache.put(gitlabProject.getHttpUrl().toLowerCase(), gitlabProject); + } + projectCacheExpiry = timeUtility.getCurrentTimeInMillis() + PROJECT_MAP_CACHE_TIME_IN_MILLISECONDS; + } catch (final Error error) { + final Throwable cause = error.getCause(); + if (cause instanceof IOException) { + throw (IOException) cause; + } else { + throw error; + } + } + return projectMapCache; + } + } + + public class BranchListEntry { + long expireTimestamp; + List branchNames; + + public BranchListEntry(List branchNames) { + this.branchNames = branchNames; + this.expireTimestamp = timeUtility.getCurrentTimeInMillis() + BRANCH_CACHE_TIME_IN_MILLISECONDS; + } + + boolean hasExpired() { + return expireTimestamp < timeUtility.getCurrentTimeInMillis(); + } + } + + public static class TimeUtility { + public long getCurrentTimeInMillis() { + return System.currentTimeMillis(); + } + } + +} diff --git a/src/main/java/com/dabsquared/gitlabjenkins/GitLabPushTrigger.java b/src/main/java/com/dabsquared/gitlabjenkins/GitLabPushTrigger.java index 53765bc..055e988 100644 --- a/src/main/java/com/dabsquared/gitlabjenkins/GitLabPushTrigger.java +++ b/src/main/java/com/dabsquared/gitlabjenkins/GitLabPushTrigger.java @@ -44,7 +44,6 @@ import net.sf.json.JSONObject; import org.apache.commons.lang.StringUtils; import org.eclipse.jgit.transport.RemoteConfig; import org.eclipse.jgit.transport.URIish; -import org.gitlab.api.models.GitlabBranch; import org.gitlab.api.models.GitlabProject; import org.kohsuke.stapler.AncestorInPath; import org.kohsuke.stapler.DataBoundConstructor; @@ -188,11 +187,11 @@ public class GitLabPushTrigger extends Trigger> { getDescriptor().queue.execute(new Runnable() { public void run() { - LOGGER.log(Level.INFO, "{0} triggered for push.", job.getName()); + LOGGER.log(Level.INFO, "{0} triggered for push.", job.getFullName()); String name = " #" + job.getNextBuildNumber(); GitLabPushCause cause = createGitLabPushCause(req); - Action[] actions = createActions(req); + Action[] actions = createActions(req); boolean scheduled; @@ -204,11 +203,13 @@ public class GitLabPushTrigger extends Trigger> { scheduled = scheduledJob.scheduleBuild(cause); } - if (scheduled) { - LOGGER.log(Level.INFO, "GitLab Push Request detected in {0}. Triggering {1}", new String[]{job.getName(), name}); - } else { - LOGGER.log(Level.INFO, "GitLab Push Request detected in {0}. Job is already in the queue.", job.getName()); - } + if (scheduled) { + LOGGER.log(Level.INFO, "GitLab Push Request detected in {0}. Triggering {1}", + new String[] { job.getFullName(), name }); + } else { + LOGGER.log(Level.INFO, "GitLab Push Request detected in {0}. Job is already in the queue.", + job.getFullName()); + } if(addCiMessage) { req.createCommitStatus(getDescriptor().getGitlab().instance(), "pending", Jenkins.getInstance().getRootUrl() + job.getUrl()); @@ -239,7 +240,7 @@ public class GitLabPushTrigger extends Trigger> { values.put("gitlabActionType", new StringParameterValue("gitlabActionType", "PUSH")); - LOGGER.log(Level.INFO, "Trying to get name and URL for job: {0}", job.getName()); + LOGGER.log(Level.INFO, "Trying to get name and URL for job: {0}", job.getFullName()); String sourceRepoName = getDesc().getSourceRepoNameDefault(job); String sourceRepoURL = getDesc().getSourceRepoURLDefault(job).toString(); @@ -272,7 +273,6 @@ public class GitLabPushTrigger extends Trigger> { return actionsArray; } - }); } } @@ -309,7 +309,7 @@ public class GitLabPushTrigger extends Trigger> { if (triggerOnMergeRequest) { getDescriptor().queue.execute(new Runnable() { public void run() { - LOGGER.log(Level.INFO, "{0} triggered for merge request.", job.getName()); + LOGGER.log(Level.INFO, "{0} triggered for merge request.", job.getFullName()); String name = " #" + job.getNextBuildNumber(); GitLabMergeCause cause = createGitLabMergeCause(req); @@ -331,9 +331,9 @@ public class GitLabPushTrigger extends Trigger> { } if (scheduled) { - LOGGER.log(Level.INFO, "GitLab Merge Request detected in {0}. Triggering {1}", new String[]{job.getName(), name}); + LOGGER.log(Level.INFO, "GitLab Merge Request detected in {0}. Triggering {1}", new String[]{job.getFullName(), name}); } else { - LOGGER.log(Level.INFO, "GitLab Merge Request detected in {0}. Job is already in the queue.", job.getName()); + LOGGER.log(Level.INFO, "GitLab Merge Request detected in {0}. Job is already in the queue.", job.getFullName()); } if(addCiMessage) { @@ -360,7 +360,7 @@ public class GitLabPushTrigger extends Trigger> { values.put("gitlabActionType", new StringParameterValue("gitlabActionType", "MERGE")); - LOGGER.log(Level.INFO, "Trying to get name and URL for job: {0}", job.getName()); + LOGGER.log(Level.INFO, "Trying to get name and URL for job: {0}", job.getFullName()); String sourceRepoName = getDesc().getSourceRepoNameDefault(job); String sourceRepoURL = getDesc().getSourceRepoURLDefault(job).toString(); @@ -597,8 +597,8 @@ public class GitLabPushTrigger extends Trigger> { private boolean ignoreCertificateErrors = false; private transient final SequentialExecutionQueue queue = new SequentialExecutionQueue(Jenkins.MasterComputer.threadPoolForRemoting); private transient GitLab gitlab; + private transient GitLabProjectBranchesService gitLabProjectBranchesService; - private final Map> projectBranches = new HashMap>(); public DescriptorImpl() { load(); @@ -651,6 +651,7 @@ public class GitLabPushTrigger extends Trigger> { ignoreCertificateErrors = formData.getBoolean("ignoreCertificateErrors"); save(); gitlab = new GitLab(); + gitLabProjectBranchesService = new GitLabProjectBranchesService(gitlab.instance(), new GitLabProjectBranchesService.TimeUtility()); return super.configure(req, formData); } @@ -661,10 +662,6 @@ public class GitLabPushTrigger extends Trigger> { } private List getProjectBranches(final Job job) throws IOException, IllegalStateException { - if (projectBranches.containsKey(job.getName())){ - return projectBranches.get(job.getName()); - } - if (!(job instanceof AbstractProject)) { return Lists.newArrayList(); } @@ -675,36 +672,12 @@ public class GitLabPushTrigger extends Trigger> { throw new IllegalStateException(Messages.GitLabPushTrigger_NoSourceRepository()); } - try { - final List branchNames = new ArrayList(); - if (!gitlabHostUrl.isEmpty()) { - /* TODO until java-gitlab-api v1.1.5 is released, - * cannot search projects by namespace/name - * For now getting project id before getting project branches */ - final List projects = getGitlab().instance().getProjects(); - for (final GitlabProject gitlabProject : projects) { - if (gitlabProject.getSshUrl().equalsIgnoreCase(sourceRepository.toString()) - || gitlabProject.getHttpUrl().equalsIgnoreCase(sourceRepository.toString())) { - //Get all branches of project - final List branches = getGitlab().instance().getBranches(gitlabProject); - for (final GitlabBranch branch : branches) { - branchNames.add(branch.getName()); - } - break; - } - } - } - - projectBranches.put(job.getName(), branchNames); - return branchNames; - } catch (final Error error) { - /* WTF WTF WTF */ - final Throwable cause = error.getCause(); - if (cause instanceof IOException) { - throw (IOException) cause; - } else { - throw error; - } + if (!getGitlabHostUrl().isEmpty()) { + return gitLabProjectBranchesService.getBranches(sourceRepository.toString()); + } else { + LOGGER.log(Level.WARNING, "getProjectBranches: gitlabHostUrl hasn't been configured globally. Job {0}.", + job.getFullName()); + return Lists.newArrayList(); } } @@ -734,8 +707,7 @@ public class GitLabPushTrigger extends Trigger> { // show all suggestions for short strings if (query.length() < 2){ values.addAll(branches); - } - else { + } else { for (String branch : branches){ if (branch.toLowerCase().indexOf(query) > -1){ values.add(branch); @@ -743,9 +715,9 @@ public class GitLabPushTrigger extends Trigger> { } } } catch (final IllegalStateException ex) { - /* no-op */ + LOGGER.log(Level.FINEST, "Ka-Boom!", ex); } catch (final IOException ex) { - /* no-op */ + LOGGER.log(Level.FINEST, "Ka-Boom!", ex); } return ac; diff --git a/src/test/java/com/dabsquared/gitlabjenkins/GitLabProjectBranchesServiceTest.java b/src/test/java/com/dabsquared/gitlabjenkins/GitLabProjectBranchesServiceTest.java new file mode 100644 index 0000000..6b3ca25 --- /dev/null +++ b/src/test/java/com/dabsquared/gitlabjenkins/GitLabProjectBranchesServiceTest.java @@ -0,0 +1,203 @@ +package com.dabsquared.gitlabjenkins; + +import static java.util.Arrays.asList; +import static org.hamcrest.CoreMatchers.is; +import static org.junit.Assert.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; + +import org.gitlab.api.GitlabAPI; +import org.gitlab.api.models.GitlabBranch; +import org.gitlab.api.models.GitlabNamespace; +import org.gitlab.api.models.GitlabProject; +import org.junit.Before; +import org.junit.Test; + +import com.dabsquared.gitlabjenkins.GitLabProjectBranchesService.TimeUtility; + +public class GitLabProjectBranchesServiceTest { + + private GitLabProjectBranchesService branchesService; + private GitlabAPI gitlabApi; + + private GitlabProject gitlabProjectA; + private GitlabProject gitlabProjectB; + + private List branchNamesProjectA; + private List branchNamesProjectB; + private TimeUtility timeUtility; + + @Before + @SuppressWarnings("unchecked") + public void setUp() throws IOException { + + // some test data + gitlabProjectA = setupGitlabProject("groupOne", "A"); + gitlabProjectB = setupGitlabProject("groupOne", "B"); + + branchNamesProjectA = asList("master", "A-branch-1"); + branchNamesProjectB = asList("master", "B-branch-1", "B-branch-2"); + + // mock the gitlabAPI + gitlabApi = mockGitlabApi(asList(gitlabProjectA, gitlabProjectB), + asList(branchNamesProjectA, branchNamesProjectB)); + + // never expire cache for tests + timeUtility = mock(TimeUtility.class); + when(timeUtility.getCurrentTimeInMillis()).thenReturn(1L); + + branchesService = new GitLabProjectBranchesService(gitlabApi, timeUtility); + } + + @Test + public void shouldReturnProjectFromGitlabApi() throws Exception { + // when + GitlabProject gitlabProject = branchesService + .findGitlabProjectForRepositoryUrl("git@git.example.com:groupOne/A.git"); + + // then + assertThat(gitlabProject, is(gitlabProjectA)); + } + + @Test + public void shouldReturnBranchNamesFromGitlabApi() throws Exception { + // when + List actualBranchNames = branchesService.getBranches("git@git.example.com:groupOne/B.git"); + + // then + assertThat(actualBranchNames, is(branchNamesProjectB)); + } + + @Test + public void shouldNotCallGitlabApiGetProjectsWhenElementIsCached() throws Exception { + // when + branchesService.findGitlabProjectForRepositoryUrl("git@git.example.com:groupOne/A.git"); + verify(gitlabApi, times(1)).getProjects(); + branchesService.findGitlabProjectForRepositoryUrl("git@git.example.com:groupOne/B.git"); + + // then + verify(gitlabApi, times(1)).getProjects(); + } + + @Test + public void shouldCallGitlabApiGetProjectsWhenElementIsNotCached() throws Exception { + // when + branchesService.findGitlabProjectForRepositoryUrl("git@git.example.com:groupOne/A.git"); + verify(gitlabApi, times(1)).getProjects(); + branchesService.findGitlabProjectForRepositoryUrl("git@git.example.com:groupOne/DoesNotExist.git"); + + // then + verify(gitlabApi, times(2)).getProjects(); + } + + @Test + public void shoulNotCallGitlabApiGetBranchesWhenElementIsCached() throws Exception { + // when + branchesService.getBranches("git@git.example.com:groupOne/B.git"); + verify(gitlabApi, times(1)).getBranches(gitlabProjectB); + branchesService.getBranches("git@git.example.com:groupOne/B.git"); + + // then + verify(gitlabApi, times(1)).getProjects(); + } + + @Test + public void shoulNotMakeUnnecessaryCallsToGitlabApiGetBranches() throws Exception { + // when + branchesService.getBranches("git@git.example.com:groupOne/A.git"); + + // then + verify(gitlabApi, times(1)).getBranches(gitlabProjectA); + verify(gitlabApi, times(0)).getBranches(gitlabProjectB); + } + + @Test + public void shouldExpireBranchCacheAtSetTime() throws Exception { + // first call should retrieve branches from gitlabApi + branchesService.getBranches("git@git.example.com:groupOne/A.git"); + verify(gitlabApi, times(1)).getBranches(gitlabProjectA); + + long timeAfterCacheExpiry = GitLabProjectBranchesService.BRANCH_CACHE_TIME_IN_MILLISECONDS + 2; + when(timeUtility.getCurrentTimeInMillis()).thenReturn(timeAfterCacheExpiry); + branchesService.getBranches("git@git.example.com:groupOne/A.git"); + + // then + verify(gitlabApi, times(2)).getBranches(gitlabProjectA); + } + + @Test + public void shouldExpireProjectCacheAtSetTime() throws Exception { + // first call should retrieve projects from gitlabApi + branchesService.findGitlabProjectForRepositoryUrl("git@git.example.com:groupOne/A.git"); + verify(gitlabApi, times(1)).getProjects(); + + long timeAfterCacheExpiry = GitLabProjectBranchesService.PROJECT_MAP_CACHE_TIME_IN_MILLISECONDS + 2; + when(timeUtility.getCurrentTimeInMillis()).thenReturn(timeAfterCacheExpiry); + branchesService.findGitlabProjectForRepositoryUrl("git@git.example.com:groupOne/A.git"); + + // then + verify(gitlabApi, times(2)).getProjects(); + } + + /** + * mocks calls to GitlabAPI.getProjects and + * GitlabAPI.getBranches(gitlabProject) + * + * projectList has to have the size as the branchNamesList list. + * + * Each branchNamesList entry is a list of strings that is used to create a + * list of GitlabBranch elements; that list is then returned for each + * gitlabProject. + * + * @param projectList + * returned for GitlabAPI.getProjects + * @param branchNamesList + * an array of lists of branch names used to mock getBranches + * @return a mocked gitlabAPI + * @throws IOException + */ + private GitlabAPI mockGitlabApi(List projectList, List> branchNamesList) + throws IOException { + GitlabAPI api = mock(GitlabAPI.class); + + when(api.getProjects()).thenReturn(projectList); + + List branchList; + for (int i = 0; i < branchNamesList.size(); i++) { + branchList = createGitlabBranches(projectList.get(i), branchNamesList.get(1)); + when(api.getBranches(projectList.get(i))).thenReturn(branchList); + } + return api; + } + + private List createGitlabBranches(GitlabProject gitlabProject, List branchNames) { + List branches = new ArrayList(); + GitlabBranch branch; + for (String branchName : branchNames) { + branch = new GitlabBranch(); + branch.setName(branchName); + branches.add(branch); + } + return branches; + } + + private GitlabProject setupGitlabProject(String namespace, String name) { + GitlabProject project = new GitlabProject(); + project.setPathWithNamespace(namespace + "/" + name); + project.setHttpUrl("http://git.example.com/" + project.getPathWithNamespace() + ".git"); + project.setSshUrl("git@git.example.com:" + project.getPathWithNamespace() + ".git"); + project.setName(name); + GitlabNamespace gitNameSpace = new GitlabNamespace(); + gitNameSpace.setName(namespace); + gitNameSpace.setPath(namespace); + project.setNamespace(gitNameSpace); + return project; + } + +} \ No newline at end of file From eca37bcef9c57918fe9307930f84b55268df1a33 Mon Sep 17 00:00:00 2001 From: Markus Ebenhoeh Date: Sun, 22 Nov 2015 13:30:04 +1100 Subject: [PATCH 2/4] Inject GitLab in branch service instead of API Changed the implementation to awlays inject gitLab (factory) into getBranches call to allow for gitLabAPI configuration changes. Otherwise a change to the servername would require a server restart. Fixes #125 --- .../GitLabProjectBranchesService.java | 29 +++++---- .../gitlabjenkins/GitLabPushTrigger.java | 5 +- .../GitLabProjectBranchesServiceTest.java | 65 ++++++++++--------- 3 files changed, 53 insertions(+), 46 deletions(-) diff --git a/src/main/java/com/dabsquared/gitlabjenkins/GitLabProjectBranchesService.java b/src/main/java/com/dabsquared/gitlabjenkins/GitLabProjectBranchesService.java index fe8a532..8273491 100644 --- a/src/main/java/com/dabsquared/gitlabjenkins/GitLabProjectBranchesService.java +++ b/src/main/java/com/dabsquared/gitlabjenkins/GitLabProjectBranchesService.java @@ -46,16 +46,22 @@ public class GitLabProjectBranchesService { */ private long projectCacheExpiry; - private final GitlabAPI gitlabAPI; - private final TimeUtility timeUtility; - protected GitLabProjectBranchesService(GitlabAPI gitlabAPI, TimeUtility timeUtility) { - this.gitlabAPI = gitlabAPI; + private static transient GitLabProjectBranchesService gitLabProjectBranchesService; + + public static GitLabProjectBranchesService instance() { + if (gitLabProjectBranchesService == null) { + gitLabProjectBranchesService = new GitLabProjectBranchesService(new TimeUtility()); + } + return gitLabProjectBranchesService; + } + + protected GitLabProjectBranchesService(TimeUtility timeUtility) { this.timeUtility = timeUtility; } - public List getBranches(String sourceRepositoryString) throws IOException { + public List getBranches(GitLab gitLab, String sourceRepositoryString) throws IOException { synchronized (projectBranchCache) { BranchListEntry branchListEntry = projectBranchCache.get(sourceRepositoryString); @@ -69,9 +75,9 @@ public class GitLabProjectBranchesService { final List branchNames = new ArrayList(); try { - GitlabProject gitlabProject = findGitlabProjectForRepositoryUrl(sourceRepositoryString); + GitlabProject gitlabProject = findGitlabProjectForRepositoryUrl(gitLab, sourceRepositoryString); if (gitlabProject != null) { - final List branches = gitlabAPI.getBranches(gitlabProject); + final List branches = gitLab.instance().getBranches(gitlabProject); for (final GitlabBranch branch : branches) { branchNames.add(branch.getName()); } @@ -95,7 +101,8 @@ public class GitLabProjectBranchesService { } } - public GitlabProject findGitlabProjectForRepositoryUrl(String sourceRepositoryString) throws IOException { + public GitlabProject findGitlabProjectForRepositoryUrl(GitLab gitLab, String sourceRepositoryString) + throws IOException { synchronized (projectMapCache) { String repositoryUrl = sourceRepositoryString.toLowerCase(); if (projectCacheExpiry < timeUtility.getCurrentTimeInMillis() @@ -109,17 +116,17 @@ public class GitLabProjectBranchesService { (Boolean) projectMapCache.containsKey(repositoryUrl), projectCacheExpiry, timeUtility.getCurrentTimeInMillis() }); } - refreshGitLabProjectMap(); + refreshGitLabProjectMap(gitLab); } return projectMapCache.get(repositoryUrl); } } - public Map refreshGitLabProjectMap() throws IOException { + public Map refreshGitLabProjectMap(GitLab gitLab) throws IOException { synchronized (projectMapCache) { try { projectMapCache.clear(); - List projects = gitlabAPI.getProjects(); + List projects = gitLab.instance().getProjects(); for (GitlabProject gitlabProject : projects) { projectMapCache.put(gitlabProject.getSshUrl().toLowerCase(), gitlabProject); projectMapCache.put(gitlabProject.getHttpUrl().toLowerCase(), gitlabProject); diff --git a/src/main/java/com/dabsquared/gitlabjenkins/GitLabPushTrigger.java b/src/main/java/com/dabsquared/gitlabjenkins/GitLabPushTrigger.java index 055e988..268b509 100644 --- a/src/main/java/com/dabsquared/gitlabjenkins/GitLabPushTrigger.java +++ b/src/main/java/com/dabsquared/gitlabjenkins/GitLabPushTrigger.java @@ -597,8 +597,6 @@ public class GitLabPushTrigger extends Trigger> { private boolean ignoreCertificateErrors = false; private transient final SequentialExecutionQueue queue = new SequentialExecutionQueue(Jenkins.MasterComputer.threadPoolForRemoting); private transient GitLab gitlab; - private transient GitLabProjectBranchesService gitLabProjectBranchesService; - public DescriptorImpl() { load(); @@ -651,7 +649,6 @@ public class GitLabPushTrigger extends Trigger> { ignoreCertificateErrors = formData.getBoolean("ignoreCertificateErrors"); save(); gitlab = new GitLab(); - gitLabProjectBranchesService = new GitLabProjectBranchesService(gitlab.instance(), new GitLabProjectBranchesService.TimeUtility()); return super.configure(req, formData); } @@ -673,7 +670,7 @@ public class GitLabPushTrigger extends Trigger> { } if (!getGitlabHostUrl().isEmpty()) { - return gitLabProjectBranchesService.getBranches(sourceRepository.toString()); + return GitLabProjectBranchesService.instance().getBranches(getGitlab(), sourceRepository.toString()); } else { LOGGER.log(Level.WARNING, "getProjectBranches: gitlabHostUrl hasn't been configured globally. Job {0}.", job.getFullName()); diff --git a/src/test/java/com/dabsquared/gitlabjenkins/GitLabProjectBranchesServiceTest.java b/src/test/java/com/dabsquared/gitlabjenkins/GitLabProjectBranchesServiceTest.java index 6b3ca25..6650c56 100644 --- a/src/test/java/com/dabsquared/gitlabjenkins/GitLabProjectBranchesServiceTest.java +++ b/src/test/java/com/dabsquared/gitlabjenkins/GitLabProjectBranchesServiceTest.java @@ -24,14 +24,16 @@ import com.dabsquared.gitlabjenkins.GitLabProjectBranchesService.TimeUtility; public class GitLabProjectBranchesServiceTest { private GitLabProjectBranchesService branchesService; + private GitlabAPI gitlabApi; + private GitLab gitLab; + private TimeUtility timeUtility; private GitlabProject gitlabProjectA; private GitlabProject gitlabProjectB; private List branchNamesProjectA; private List branchNamesProjectB; - private TimeUtility timeUtility; @Before @SuppressWarnings("unchecked") @@ -44,22 +46,21 @@ public class GitLabProjectBranchesServiceTest { branchNamesProjectA = asList("master", "A-branch-1"); branchNamesProjectB = asList("master", "B-branch-1", "B-branch-2"); - // mock the gitlabAPI - gitlabApi = mockGitlabApi(asList(gitlabProjectA, gitlabProjectB), - asList(branchNamesProjectA, branchNamesProjectB)); + // mock the gitlab factory + gitLab = mockGitlab(asList(gitlabProjectA, gitlabProjectB), asList(branchNamesProjectA, branchNamesProjectB)); // never expire cache for tests timeUtility = mock(TimeUtility.class); when(timeUtility.getCurrentTimeInMillis()).thenReturn(1L); - branchesService = new GitLabProjectBranchesService(gitlabApi, timeUtility); + branchesService = new GitLabProjectBranchesService(timeUtility); } @Test public void shouldReturnProjectFromGitlabApi() throws Exception { // when - GitlabProject gitlabProject = branchesService - .findGitlabProjectForRepositoryUrl("git@git.example.com:groupOne/A.git"); + GitlabProject gitlabProject = branchesService.findGitlabProjectForRepositoryUrl(gitLab, + "git@git.example.com:groupOne/A.git"); // then assertThat(gitlabProject, is(gitlabProjectA)); @@ -68,7 +69,7 @@ public class GitLabProjectBranchesServiceTest { @Test public void shouldReturnBranchNamesFromGitlabApi() throws Exception { // when - List actualBranchNames = branchesService.getBranches("git@git.example.com:groupOne/B.git"); + List actualBranchNames = branchesService.getBranches(gitLab, "git@git.example.com:groupOne/B.git"); // then assertThat(actualBranchNames, is(branchNamesProjectB)); @@ -77,9 +78,9 @@ public class GitLabProjectBranchesServiceTest { @Test public void shouldNotCallGitlabApiGetProjectsWhenElementIsCached() throws Exception { // when - branchesService.findGitlabProjectForRepositoryUrl("git@git.example.com:groupOne/A.git"); + branchesService.findGitlabProjectForRepositoryUrl(gitLab, "git@git.example.com:groupOne/A.git"); verify(gitlabApi, times(1)).getProjects(); - branchesService.findGitlabProjectForRepositoryUrl("git@git.example.com:groupOne/B.git"); + branchesService.findGitlabProjectForRepositoryUrl(gitLab, "git@git.example.com:groupOne/B.git"); // then verify(gitlabApi, times(1)).getProjects(); @@ -88,9 +89,9 @@ public class GitLabProjectBranchesServiceTest { @Test public void shouldCallGitlabApiGetProjectsWhenElementIsNotCached() throws Exception { // when - branchesService.findGitlabProjectForRepositoryUrl("git@git.example.com:groupOne/A.git"); + branchesService.findGitlabProjectForRepositoryUrl(gitLab, "git@git.example.com:groupOne/A.git"); verify(gitlabApi, times(1)).getProjects(); - branchesService.findGitlabProjectForRepositoryUrl("git@git.example.com:groupOne/DoesNotExist.git"); + branchesService.findGitlabProjectForRepositoryUrl(gitLab, "git@git.example.com:groupOne/DoesNotExist.git"); // then verify(gitlabApi, times(2)).getProjects(); @@ -99,9 +100,9 @@ public class GitLabProjectBranchesServiceTest { @Test public void shoulNotCallGitlabApiGetBranchesWhenElementIsCached() throws Exception { // when - branchesService.getBranches("git@git.example.com:groupOne/B.git"); + branchesService.getBranches(gitLab, "git@git.example.com:groupOne/B.git"); verify(gitlabApi, times(1)).getBranches(gitlabProjectB); - branchesService.getBranches("git@git.example.com:groupOne/B.git"); + branchesService.getBranches(gitLab, "git@git.example.com:groupOne/B.git"); // then verify(gitlabApi, times(1)).getProjects(); @@ -110,7 +111,7 @@ public class GitLabProjectBranchesServiceTest { @Test public void shoulNotMakeUnnecessaryCallsToGitlabApiGetBranches() throws Exception { // when - branchesService.getBranches("git@git.example.com:groupOne/A.git"); + branchesService.getBranches(gitLab, "git@git.example.com:groupOne/A.git"); // then verify(gitlabApi, times(1)).getBranches(gitlabProjectA); @@ -120,12 +121,12 @@ public class GitLabProjectBranchesServiceTest { @Test public void shouldExpireBranchCacheAtSetTime() throws Exception { // first call should retrieve branches from gitlabApi - branchesService.getBranches("git@git.example.com:groupOne/A.git"); + branchesService.getBranches(gitLab, "git@git.example.com:groupOne/A.git"); verify(gitlabApi, times(1)).getBranches(gitlabProjectA); long timeAfterCacheExpiry = GitLabProjectBranchesService.BRANCH_CACHE_TIME_IN_MILLISECONDS + 2; when(timeUtility.getCurrentTimeInMillis()).thenReturn(timeAfterCacheExpiry); - branchesService.getBranches("git@git.example.com:groupOne/A.git"); + branchesService.getBranches(gitLab, "git@git.example.com:groupOne/A.git"); // then verify(gitlabApi, times(2)).getBranches(gitlabProjectA); @@ -134,26 +135,24 @@ public class GitLabProjectBranchesServiceTest { @Test public void shouldExpireProjectCacheAtSetTime() throws Exception { // first call should retrieve projects from gitlabApi - branchesService.findGitlabProjectForRepositoryUrl("git@git.example.com:groupOne/A.git"); + branchesService.findGitlabProjectForRepositoryUrl(gitLab, "git@git.example.com:groupOne/A.git"); verify(gitlabApi, times(1)).getProjects(); long timeAfterCacheExpiry = GitLabProjectBranchesService.PROJECT_MAP_CACHE_TIME_IN_MILLISECONDS + 2; when(timeUtility.getCurrentTimeInMillis()).thenReturn(timeAfterCacheExpiry); - branchesService.findGitlabProjectForRepositoryUrl("git@git.example.com:groupOne/A.git"); + branchesService.findGitlabProjectForRepositoryUrl(gitLab, "git@git.example.com:groupOne/A.git"); // then verify(gitlabApi, times(2)).getProjects(); } /** - * mocks calls to GitlabAPI.getProjects and - * GitlabAPI.getBranches(gitlabProject) + * mocks calls to GitLab.instance() and GitlabAPI.getProjects and GitlabAPI.getBranches(gitlabProject) * * projectList has to have the size as the branchNamesList list. - * - * Each branchNamesList entry is a list of strings that is used to create a - * list of GitlabBranch elements; that list is then returned for each - * gitlabProject. + * + * Each branchNamesList entry is a list of strings that is used to create a list of GitlabBranch elements; that list + * is then returned for each gitlabProject. * * @param projectList * returned for GitlabAPI.getProjects @@ -162,18 +161,22 @@ public class GitLabProjectBranchesServiceTest { * @return a mocked gitlabAPI * @throws IOException */ - private GitlabAPI mockGitlabApi(List projectList, List> branchNamesList) - throws IOException { - GitlabAPI api = mock(GitlabAPI.class); + private GitLab mockGitlab(List projectList, List> branchNamesList) throws IOException { + // mock the actual API + gitlabApi = mock(GitlabAPI.class); - when(api.getProjects()).thenReturn(projectList); + // mock the gitlab API factory + GitLab gitLab = mock(GitLab.class); + when(gitLab.instance()).thenReturn(gitlabApi); + + when(gitlabApi.getProjects()).thenReturn(projectList); List branchList; for (int i = 0; i < branchNamesList.size(); i++) { branchList = createGitlabBranches(projectList.get(i), branchNamesList.get(1)); - when(api.getBranches(projectList.get(i))).thenReturn(branchList); + when(gitlabApi.getBranches(projectList.get(i))).thenReturn(branchList); } - return api; + return gitLab; } private List createGitlabBranches(GitlabProject gitlabProject, List branchNames) { From 6cbec651840752e81d69d8d6e253a75a9283ef59 Mon Sep 17 00:00:00 2001 From: Markus Ebenhoeh Date: Sun, 22 Nov 2015 13:54:23 +1100 Subject: [PATCH 3/4] remove unused import --- .../dabsquared/gitlabjenkins/GitLabProjectBranchesService.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/com/dabsquared/gitlabjenkins/GitLabProjectBranchesService.java b/src/main/java/com/dabsquared/gitlabjenkins/GitLabProjectBranchesService.java index 8273491..39d49c9 100644 --- a/src/main/java/com/dabsquared/gitlabjenkins/GitLabProjectBranchesService.java +++ b/src/main/java/com/dabsquared/gitlabjenkins/GitLabProjectBranchesService.java @@ -8,7 +8,6 @@ import java.util.Map; import java.util.logging.Level; import java.util.logging.Logger; -import org.gitlab.api.GitlabAPI; import org.gitlab.api.models.GitlabBranch; import org.gitlab.api.models.GitlabProject; From 8d5566bc8f5229fe18b439731dfdfc7c334fdf83 Mon Sep 17 00:00:00 2001 From: Markus Ebenhoeh Date: Wed, 2 Dec 2015 18:54:26 +1100 Subject: [PATCH 4/4] Add more descriptive message to exception logs --- .../java/com/dabsquared/gitlabjenkins/GitLabPushTrigger.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/dabsquared/gitlabjenkins/GitLabPushTrigger.java b/src/main/java/com/dabsquared/gitlabjenkins/GitLabPushTrigger.java index 268b509..447f40f 100644 --- a/src/main/java/com/dabsquared/gitlabjenkins/GitLabPushTrigger.java +++ b/src/main/java/com/dabsquared/gitlabjenkins/GitLabPushTrigger.java @@ -712,9 +712,9 @@ public class GitLabPushTrigger extends Trigger> { } } } catch (final IllegalStateException ex) { - LOGGER.log(Level.FINEST, "Ka-Boom!", ex); + LOGGER.log(Level.FINEST, "Unexpected IllegalStateException. Please check the logs and your configuration.", ex); } catch (final IOException ex) { - LOGGER.log(Level.FINEST, "Ka-Boom!", ex); + LOGGER.log(Level.FINEST, "Unexpected IllegalStateException. Please check the logs and your configuration.", ex); } return ac;