From 80f98c387af5b1a08228b4f19ac411f3d5b22ae5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Vyhl=C3=ADdka?= Date: Mon, 17 Sep 2018 15:49:08 +0200 Subject: [PATCH] Fix branch not to be updated when refreshRate used (#1136) (#1138) --- .../JGitEnvironmentRepository.java | 33 ++++++---- ...EnvironmentRepositoryIntegrationTests.java | 60 +++++++++++++++++++ .../JGitEnvironmentRepositoryTests.java | 56 ++++++++++++++++- 3 files changed, 137 insertions(+), 12 deletions(-) diff --git a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java index 64d792e3..41d325a3 100644 --- a/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java +++ b/spring-cloud-config-server/src/main/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepository.java @@ -239,20 +239,13 @@ public class JGitEnvironmentRepository extends AbstractScmEnvironmentRepository } // checkout after fetch so we can get any new branches, tags, ect. checkout(git, label); - if (isBranch(git, label)) { - // merge results from fetch - merge(git, label); - if (!isClean(git)) { - logger.warn( - "The local repository is dirty. Resetting it to origin/" - + label + "."); - resetHard(git, label, LOCAL_BRANCH_REF_PREFIX + label); - } - } + tryMerge(git, label); } else { - // nothing to update so just checkout + // nothing to update so just checkout and merge. + // Merge because remote branch could have been updated before checkout(git, label); + tryMerge(git, label); } // always return what is currently HEAD as the version return git.getRepository().getRef("HEAD").getObjectId().getName(); @@ -281,6 +274,24 @@ public class JGitEnvironmentRepository extends AbstractScmEnvironmentRepository } } } + + private void tryMerge(Git git, String label) { + try { + if (isBranch(git, label)) { + // merge results from fetch + merge(git, label); + if (!isClean(git)) { + logger.warn("The local repository is dirty or ahead of origin. Resetting" + + " it to origin/" + label + "."); + resetHard(git, label, LOCAL_BRANCH_REF_PREFIX + label); + } + } + } + catch (GitAPIException e) { + throw new NoSuchRepositoryException( + "Cannot clone or checkout repository: " + getUri(), e); + } + } /** * Clones the remote repository and then opens a connection to it. diff --git a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryIntegrationTests.java b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryIntegrationTests.java index 6e23de52..2899aa46 100644 --- a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryIntegrationTests.java +++ b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryIntegrationTests.java @@ -64,6 +64,7 @@ import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; /** * @author Dave Syer @@ -458,6 +459,65 @@ public class JGitEnvironmentRepositoryIntegrationTests { fooProperty = ConfigServerTestUtils.getProperty(environment, "bar.properties", "foo"); assertEquals(fooProperty, "bar"); } + + @Test + /** + * In this scenario there is set the refresh rate so the remote repository is not fetched for every configuration read. + * + * There is more than one label queried - test and master - but only one such branch exists - master. + * + * There is a new commit to master branch but when client loads new configuration, the "test" label is queried first. + */ + public void testNewCommitIDWithRefreshRate() throws Exception { + JGitConfigServerTestData testData = JGitConfigServerTestData + .prepareClonedGitRepository(TestConfiguration.class); + + // get our starting versions + String startingRemoteVersion = getCommitID(testData.getServerGit().getGit(), "master"); + + //Ask test label configuration first + try { + testData.getRepository().findOne("bar", "staging", "test"); + fail("Should have thrown NoSuchLabelException."); + } catch (NoSuchLabelException ex) { + // OK + } + + // make sure we get the right version out of the gate + Environment environment = testData.getRepository().findOne("bar", "staging", "master"); + assertEquals(environment.getVersion(), startingRemoteVersion); + + // update the remote repo + FileOutputStream out = new FileOutputStream( + new File(testData.getServerGit().getGitWorkingDirectory(), "bar.properties")); + StreamUtils.copy("foo: barNewCommit", Charset.defaultCharset(), out); + testData.getServerGit().getGit().add().addFilepattern("bar.properties").call(); + testData.getServerGit().getGit().commit().setMessage("Updated for pull").call(); + String updatedRemoteVersion = getCommitID(testData.getServerGit().getGit(), "master"); + + // Set refresh rate to 60 seconds (now it will fetch the remote repo only once) + testData.getRepository().setRefreshRate(60); + + //Ask test label configuration first + try { + testData.getRepository().findOne("bar", "staging", "test"); + fail("Should have thrown NoSuchLabelException."); + } catch (NoSuchLabelException ex) { + // OK + } + + // do a normal request and verify we get the new version + environment = testData.getRepository().findOne("bar", "staging", "master"); + assertEquals(environment.getVersion(), updatedRemoteVersion); + Object fooProperty = ConfigServerTestUtils.getProperty(environment, "bar.properties", "foo"); + assertEquals(fooProperty, "barNewCommit"); + + // request the prior commit ID and make sure we get it + environment = testData.getRepository().findOne("bar", "staging", startingRemoteVersion); + assertEquals(environment.getVersion(), startingRemoteVersion); + fooProperty = ConfigServerTestUtils.getProperty(environment, "bar.properties", "foo"); + assertEquals(fooProperty, "bar"); + } @Test(expected = NoSuchLabelException.class) public void testUnknownLabelWithRemote() throws Exception { diff --git a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryTests.java b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryTests.java index bc9490d8..9594e0f7 100644 --- a/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryTests.java +++ b/spring-cloud-config-server/src/test/java/org/springframework/cloud/config/server/environment/JGitEnvironmentRepositoryTests.java @@ -19,6 +19,7 @@ package org.springframework.cloud.config.server.environment; import java.io.File; import java.io.IOException; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.List; @@ -56,7 +57,7 @@ import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; - +import org.mockito.Mockito; import org.springframework.cloud.config.environment.Environment; import org.springframework.cloud.config.server.support.AwsCodeCommitCredentialProvider; import org.springframework.cloud.config.server.support.GitCredentialsProviderFactory; @@ -513,6 +514,59 @@ public class JGitEnvironmentRepositoryTests { verify(git, times(0)).branchDelete(); } + + @Test + public void testRefreshWithoutFetch() throws Exception { + Git git = mock(Git.class); + + CloneCommand cloneCommand = mock(CloneCommand.class); + when(cloneCommand.setURI(anyString())).thenReturn(cloneCommand); + when(cloneCommand.setDirectory(any(File.class))).thenReturn(cloneCommand); + when(cloneCommand.call()).thenReturn(git); + + MockGitFactory factory = new MockGitFactory(git, cloneCommand); + + StatusCommand statusCommand = mock(StatusCommand.class); + CheckoutCommand checkoutCommand = mock(CheckoutCommand.class); + Status status = mock(Status.class); + Repository repository = mock(Repository.class, Mockito.RETURNS_DEEP_STUBS); + StoredConfig storedConfig = mock(StoredConfig.class); + Ref ref = mock(Ref.class); + ListBranchCommand listBranchCommand = mock(ListBranchCommand.class); + FetchCommand fetchCommand = mock(FetchCommand.class); + FetchResult fetchResult = mock(FetchResult.class); + Ref branch1Ref = mock(Ref.class); + + when(git.branchList()).thenReturn(listBranchCommand); + when(git.status()).thenReturn(statusCommand); + when(git.getRepository()).thenReturn(repository); + when(git.checkout()).thenReturn(checkoutCommand); + when(git.fetch()).thenReturn(fetchCommand); + when(git.merge()).thenReturn(mock(MergeCommand.class, Mockito.RETURNS_DEEP_STUBS)); + when(repository.getConfig()).thenReturn(storedConfig); + when(storedConfig.getString("remote", "origin", "url")).thenReturn("http://example/git"); + when(statusCommand.call()).thenReturn(status); + when(checkoutCommand.call()).thenReturn(ref); + when(listBranchCommand.call()).thenReturn(Arrays.asList(branch1Ref)); + when(fetchCommand.call()).thenReturn(fetchResult); + when(branch1Ref.getName()).thenReturn("origin/master"); + when(status.isClean()).thenReturn(true); + + JGitEnvironmentRepository repo = new JGitEnvironmentRepository(this.environment); + repo.setGitFactory(factory); + repo.setUri("http://somegitserver/somegitrepo"); + repo.setBasedir(this.basedir); + + // Set the refresh rate to 2 seconds and last update before 100ms. There should be no remote repo fetch. + repo.setLastRefresh(System.currentTimeMillis() - 100); + repo.setRefreshRate(2); + + repo.refresh("master"); + + // Verify no fetch but merge only. + verify(git, times(0)).fetch(); + verify(git).merge(); + } @Test public void testResetHardException() throws Exception {