From 4a275c445e3e75963c44644d780c069f645f2505 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Mon, 22 Jul 2019 09:44:03 +0200 Subject: [PATCH 1/5] removed unnecessary request provider --- .../sonia/scm/web/protocol/HttpProtocolServlet.java | 10 ++-------- .../scm/web/protocol/HttpProtocolServletTest.java | 13 ++----------- 2 files changed, 4 insertions(+), 19 deletions(-) diff --git a/scm-webapp/src/main/java/sonia/scm/web/protocol/HttpProtocolServlet.java b/scm-webapp/src/main/java/sonia/scm/web/protocol/HttpProtocolServlet.java index 250846a8cc..7aae2f036c 100644 --- a/scm-webapp/src/main/java/sonia/scm/web/protocol/HttpProtocolServlet.java +++ b/scm-webapp/src/main/java/sonia/scm/web/protocol/HttpProtocolServlet.java @@ -16,7 +16,6 @@ import sonia.scm.repository.spi.HttpScmProtocol; import sonia.scm.web.UserAgent; import sonia.scm.web.UserAgentParser; -import javax.inject.Provider; import javax.servlet.ServletException; import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; @@ -33,17 +32,13 @@ public class HttpProtocolServlet extends HttpServlet { public static final String PATTERN = PATH + "/*"; private final RepositoryServiceFactory serviceFactory; - - private final Provider requestProvider; - private final PushStateDispatcher dispatcher; private final UserAgentParser userAgentParser; @Inject - public HttpProtocolServlet(RepositoryServiceFactory serviceFactory, Provider requestProvider, PushStateDispatcher dispatcher, UserAgentParser userAgentParser) { + public HttpProtocolServlet(RepositoryServiceFactory serviceFactory, PushStateDispatcher dispatcher, UserAgentParser userAgentParser) { this.serviceFactory = serviceFactory; - this.requestProvider = requestProvider; this.dispatcher = dispatcher; this.userAgentParser = userAgentParser; } @@ -55,7 +50,6 @@ public class HttpProtocolServlet extends HttpServlet { log.trace("dispatch browser request for user agent {}", userAgent); dispatcher.dispatch(request, response, request.getRequestURI()); } else { - String pathInfo = request.getPathInfo(); Optional namespaceAndName = NamespaceAndNameFromPathExtractor.fromUri(pathInfo); if (namespaceAndName.isPresent()) { @@ -69,7 +63,7 @@ public class HttpProtocolServlet extends HttpServlet { private void service(HttpServletRequest req, HttpServletResponse resp, NamespaceAndName namespaceAndName) throws IOException, ServletException { try (RepositoryService repositoryService = serviceFactory.create(namespaceAndName)) { - requestProvider.get().setAttribute(DefaultRepositoryProvider.ATTRIBUTE_NAME, repositoryService.getRepository()); + req.setAttribute(DefaultRepositoryProvider.ATTRIBUTE_NAME, repositoryService.getRepository()); HttpScmProtocol protocol = repositoryService.getProtocol(HttpScmProtocol.class); protocol.serve(req, resp, getServletConfig()); } catch (NotFoundException e) { diff --git a/scm-webapp/src/test/java/sonia/scm/web/protocol/HttpProtocolServletTest.java b/scm-webapp/src/test/java/sonia/scm/web/protocol/HttpProtocolServletTest.java index 1bd6358c95..82088c6499 100644 --- a/scm-webapp/src/test/java/sonia/scm/web/protocol/HttpProtocolServletTest.java +++ b/scm-webapp/src/test/java/sonia/scm/web/protocol/HttpProtocolServletTest.java @@ -15,7 +15,6 @@ import sonia.scm.repository.spi.HttpScmProtocol; import sonia.scm.web.UserAgent; import sonia.scm.web.UserAgentParser; -import javax.inject.Provider; import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -23,24 +22,17 @@ import java.io.IOException; import static org.mockito.AdditionalMatchers.not; import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.doReturn; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; import static org.mockito.MockitoAnnotations.initMocks; public class HttpProtocolServletTest { - @Mock private RepositoryServiceFactory serviceFactory; @Mock - private HttpServletRequest httpServletRequest; - @Mock private PushStateDispatcher dispatcher; @Mock private UserAgentParser userAgentParser; - @Mock - private Provider requestProvider; @InjectMocks private HttpProtocolServlet servlet; @@ -65,7 +57,6 @@ public class HttpProtocolServletTest { NamespaceAndName existingRepo = new NamespaceAndName("space", "repo"); when(serviceFactory.create(not(eq(existingRepo)))).thenThrow(new NotFoundException("Test", "a")); when(serviceFactory.create(existingRepo)).thenReturn(repositoryService); - when(requestProvider.get()).thenReturn(httpServletRequest); } @Test @@ -107,7 +98,7 @@ public class HttpProtocolServletTest { servlet.service(request, response); - verify(httpServletRequest).setAttribute(DefaultRepositoryProvider.ATTRIBUTE_NAME, repository); + verify(request).setAttribute(DefaultRepositoryProvider.ATTRIBUTE_NAME, repository); verify(protocol).serve(request, response, null); verify(repositoryService).close(); } From 56a683c7c5b75059b59e53acde165b48c7020b65 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Mon, 22 Jul 2019 13:00:49 +0200 Subject: [PATCH 2/5] fix checkout of repositories with dots in the names --- .../scm/web/protocol/HttpProtocolServlet.java | 6 +- .../NamespaceAndNameFromPathExtractor.java | 28 ++++++-- ...NamespaceAndNameFromPathExtractorTest.java | 66 +++++++++++++++++-- 3 files changed, 84 insertions(+), 16 deletions(-) diff --git a/scm-webapp/src/main/java/sonia/scm/web/protocol/HttpProtocolServlet.java b/scm-webapp/src/main/java/sonia/scm/web/protocol/HttpProtocolServlet.java index 7aae2f036c..623be728c1 100644 --- a/scm-webapp/src/main/java/sonia/scm/web/protocol/HttpProtocolServlet.java +++ b/scm-webapp/src/main/java/sonia/scm/web/protocol/HttpProtocolServlet.java @@ -32,13 +32,15 @@ public class HttpProtocolServlet extends HttpServlet { public static final String PATTERN = PATH + "/*"; private final RepositoryServiceFactory serviceFactory; + private final NamespaceAndNameFromPathExtractor pathExtractor; private final PushStateDispatcher dispatcher; private final UserAgentParser userAgentParser; @Inject - public HttpProtocolServlet(RepositoryServiceFactory serviceFactory, PushStateDispatcher dispatcher, UserAgentParser userAgentParser) { + public HttpProtocolServlet(RepositoryServiceFactory serviceFactory, NamespaceAndNameFromPathExtractor pathExtractor, PushStateDispatcher dispatcher, UserAgentParser userAgentParser) { this.serviceFactory = serviceFactory; + this.pathExtractor = pathExtractor; this.dispatcher = dispatcher; this.userAgentParser = userAgentParser; } @@ -51,7 +53,7 @@ public class HttpProtocolServlet extends HttpServlet { dispatcher.dispatch(request, response, request.getRequestURI()); } else { String pathInfo = request.getPathInfo(); - Optional namespaceAndName = NamespaceAndNameFromPathExtractor.fromUri(pathInfo); + Optional namespaceAndName = pathExtractor.fromUri(pathInfo); if (namespaceAndName.isPresent()) { service(request, response, namespaceAndName.get()); } else { diff --git a/scm-webapp/src/main/java/sonia/scm/web/protocol/NamespaceAndNameFromPathExtractor.java b/scm-webapp/src/main/java/sonia/scm/web/protocol/NamespaceAndNameFromPathExtractor.java index 22e2433561..2c1eeb1b90 100644 --- a/scm-webapp/src/main/java/sonia/scm/web/protocol/NamespaceAndNameFromPathExtractor.java +++ b/scm-webapp/src/main/java/sonia/scm/web/protocol/NamespaceAndNameFromPathExtractor.java @@ -1,18 +1,31 @@ package sonia.scm.web.protocol; +import sonia.scm.Type; import sonia.scm.repository.NamespaceAndName; +import sonia.scm.repository.RepositoryManager; import sonia.scm.util.HttpUtil; +import javax.inject.Inject; import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; import static java.util.Optional.empty; import static java.util.Optional.of; final class NamespaceAndNameFromPathExtractor { - private NamespaceAndNameFromPathExtractor() {} + private final Set types; - static Optional fromUri(String uri) { + @Inject + public NamespaceAndNameFromPathExtractor(RepositoryManager repositoryManager) { + this.types = repositoryManager.getConfiguredTypes() + .stream() + .map(Type::getName) + .collect(Collectors.toSet()); + } + + Optional fromUri(String uri) { if (uri.startsWith(HttpUtil.SEPARATOR_PATH)) { uri = uri.substring(1); } @@ -30,12 +43,13 @@ final class NamespaceAndNameFromPathExtractor { } String name = uri.substring(endOfNamespace + 1, nameIndex); - - int nameDotIndex = name.indexOf('.'); + int nameDotIndex = name.lastIndexOf('.'); if (nameDotIndex >= 0) { - return of(new NamespaceAndName(namespace, name.substring(0, nameDotIndex))); - } else { - return of(new NamespaceAndName(namespace, name)); + String suffix = name.substring(nameDotIndex + 1); + if (types.contains(suffix)) { + name = name.substring(0, nameDotIndex); + } } + return of(new NamespaceAndName(namespace, name)); } } diff --git a/scm-webapp/src/test/java/sonia/scm/web/protocol/NamespaceAndNameFromPathExtractorTest.java b/scm-webapp/src/test/java/sonia/scm/web/protocol/NamespaceAndNameFromPathExtractorTest.java index 0998010069..5ac8f37c01 100644 --- a/scm-webapp/src/test/java/sonia/scm/web/protocol/NamespaceAndNameFromPathExtractorTest.java +++ b/scm-webapp/src/test/java/sonia/scm/web/protocol/NamespaceAndNameFromPathExtractorTest.java @@ -1,17 +1,48 @@ package sonia.scm.web.protocol; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DynamicNode; import org.junit.jupiter.api.DynamicTest; +import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestFactory; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; import sonia.scm.repository.NamespaceAndName; +import sonia.scm.repository.RepositoryManager; +import sonia.scm.repository.RepositoryType; +import javax.inject.Inject; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; import java.util.Optional; import java.util.stream.Stream; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.DynamicTest.dynamicTest; +import static org.mockito.Mockito.when; + +@ExtendWith(MockitoExtension.class) +class NamespaceAndNameFromPathExtractorTest { + + @Mock + private RepositoryManager repositoryManager; + + private NamespaceAndNameFromPathExtractor extractor; + + @BeforeEach + void setUpObjectUnderTest() { + List types = Arrays.asList( + new RepositoryType("git", "Git", Collections.emptySet()), + new RepositoryType("hg", "Mercurial", Collections.emptySet()), + new RepositoryType("svn", "Subversion", Collections.emptySet()) + ); + when(repositoryManager.getConfiguredTypes()).thenReturn(types); + extractor = new NamespaceAndNameFromPathExtractor(repositoryManager); + } -public class NamespaceAndNameFromPathExtractorTest { @TestFactory Stream shouldExtractCorrectNamespaceAndName() { return Stream.of( @@ -26,21 +57,26 @@ public class NamespaceAndNameFromPathExtractorTest { } @TestFactory - Stream shouldHandleTrailingDotSomethings() { + Stream shouldHandleTypeSuffix() { return Stream.of( "/space/repo.git", - "/space/repo.and.more", - "/space/repo." + "/space/repo.hg", + "/space/repo.svn", + "/space/repo" ).map(this::createCorrectTest); } private DynamicTest createCorrectTest(String path) { + return createCorrectTest(path, new NamespaceAndName("space", "repo")); + } + + private DynamicTest createCorrectTest(String path, NamespaceAndName expected) { return dynamicTest( "should extract correct namespace and name for path " + path, () -> { - Optional namespaceAndName = NamespaceAndNameFromPathExtractor.fromUri(path); + Optional namespaceAndName = extractor.fromUri(path); - assertThat(namespaceAndName.get()).isEqualTo(new NamespaceAndName("space", "repo")); + assertThat(namespaceAndName.get()).isEqualTo(expected); } ); } @@ -59,10 +95,26 @@ public class NamespaceAndNameFromPathExtractorTest { return dynamicTest( "should not fail for wrong path " + path, () -> { - Optional namespaceAndName = NamespaceAndNameFromPathExtractor.fromUri(path); + Optional namespaceAndName = extractor.fromUri(path); assertThat(namespaceAndName.isPresent()).isFalse(); } ); } + + @TestFactory + Stream shouldHandleDots() { + return Stream.of( + "/space/repo.with.dots.git", + "/space/repo.with.dots.hg", + "/space/repo.with.dots.svn", + "/space/repo.with.dots" + ).map(path -> createCorrectTest(path, new NamespaceAndName("space", "repo.with.dots"))); + } + + @Test + void shouldNotFailOnEndingDot() { + Optional namespaceAndName = extractor.fromUri("/space/repo."); + assertThat(namespaceAndName).contains(new NamespaceAndName("space", "repo.")); + } } From 405ffcf164c1fce0e3aa933043c147a17e460d7d Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Mon, 22 Jul 2019 14:26:15 +0200 Subject: [PATCH 3/5] migrate tests to junit 5 --- .../web/protocol/HttpProtocolServletTest.java | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/scm-webapp/src/test/java/sonia/scm/web/protocol/HttpProtocolServletTest.java b/scm-webapp/src/test/java/sonia/scm/web/protocol/HttpProtocolServletTest.java index 82088c6499..3c1009106a 100644 --- a/scm-webapp/src/test/java/sonia/scm/web/protocol/HttpProtocolServletTest.java +++ b/scm-webapp/src/test/java/sonia/scm/web/protocol/HttpProtocolServletTest.java @@ -1,9 +1,11 @@ package sonia.scm.web.protocol; -import org.junit.Before; -import org.junit.Test; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.InjectMocks; import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; import sonia.scm.NotFoundException; import sonia.scm.PushStateDispatcher; import sonia.scm.repository.DefaultRepositoryProvider; @@ -23,9 +25,9 @@ import java.io.IOException; import static org.mockito.AdditionalMatchers.not; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.*; -import static org.mockito.MockitoAnnotations.initMocks; -public class HttpProtocolServletTest { +@ExtendWith(MockitoExtension.class) +class HttpProtocolServletTest { @Mock private RepositoryServiceFactory serviceFactory; @@ -49,9 +51,8 @@ public class HttpProtocolServletTest { @Mock private HttpScmProtocol protocol; - @Before - public void init() { - initMocks(this); + @BeforeEach + void init() { when(userAgentParser.parse(request)).thenReturn(userAgent); when(userAgent.isBrowser()).thenReturn(false); NamespaceAndName existingRepo = new NamespaceAndName("space", "repo"); @@ -60,7 +61,7 @@ public class HttpProtocolServletTest { } @Test - public void shouldDispatchBrowserRequests() throws ServletException, IOException { + void shouldDispatchBrowserRequests() throws ServletException, IOException { when(userAgent.isBrowser()).thenReturn(true); when(request.getRequestURI()).thenReturn("uri"); @@ -70,7 +71,7 @@ public class HttpProtocolServletTest { } @Test - public void shouldHandleBadPaths() throws IOException, ServletException { + void shouldHandleBadPaths() throws IOException, ServletException { when(request.getPathInfo()).thenReturn("/illegal"); servlet.service(request, response); @@ -79,7 +80,7 @@ public class HttpProtocolServletTest { } @Test - public void shouldHandleNotExistingRepository() throws IOException, ServletException { + void shouldHandleNotExistingRepository() throws IOException, ServletException { when(request.getPathInfo()).thenReturn("/not/exists"); servlet.service(request, response); @@ -88,7 +89,7 @@ public class HttpProtocolServletTest { } @Test - public void shouldDelegateToProvider() throws IOException, ServletException { + void shouldDelegateToProvider() throws IOException, ServletException { when(request.getPathInfo()).thenReturn("/space/name"); NamespaceAndName namespaceAndName = new NamespaceAndName("space", "name"); doReturn(repositoryService).when(serviceFactory).create(namespaceAndName); From 4ba94374104e2d7593af6f3586c7d2096f72cba8 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Mon, 22 Jul 2019 14:41:12 +0200 Subject: [PATCH 4/5] fixed broken tests --- .../web/protocol/HttpProtocolServletTest.java | 112 +++++++++++------- 1 file changed, 71 insertions(+), 41 deletions(-) diff --git a/scm-webapp/src/test/java/sonia/scm/web/protocol/HttpProtocolServletTest.java b/scm-webapp/src/test/java/sonia/scm/web/protocol/HttpProtocolServletTest.java index 3c1009106a..59b29c8eda 100644 --- a/scm-webapp/src/test/java/sonia/scm/web/protocol/HttpProtocolServletTest.java +++ b/scm-webapp/src/test/java/sonia/scm/web/protocol/HttpProtocolServletTest.java @@ -1,6 +1,7 @@ package sonia.scm.web.protocol; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.InjectMocks; @@ -11,6 +12,7 @@ import sonia.scm.PushStateDispatcher; import sonia.scm.repository.DefaultRepositoryProvider; import sonia.scm.repository.NamespaceAndName; import sonia.scm.repository.Repository; +import sonia.scm.repository.RepositoryTestData; import sonia.scm.repository.api.RepositoryService; import sonia.scm.repository.api.RepositoryServiceFactory; import sonia.scm.repository.spi.HttpScmProtocol; @@ -21,18 +23,23 @@ import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import java.io.IOException; +import java.util.Optional; -import static org.mockito.AdditionalMatchers.not; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.*; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) class HttpProtocolServletTest { @Mock private RepositoryServiceFactory serviceFactory; + + @Mock + private NamespaceAndNameFromPathExtractor extractor; + @Mock private PushStateDispatcher dispatcher; + @Mock private UserAgentParser userAgentParser; @@ -41,66 +48,89 @@ class HttpProtocolServletTest { @Mock private RepositoryService repositoryService; + @Mock private UserAgent userAgent; @Mock private HttpServletRequest request; + @Mock private HttpServletResponse response; + @Mock private HttpScmProtocol protocol; - @BeforeEach - void init() { - when(userAgentParser.parse(request)).thenReturn(userAgent); - when(userAgent.isBrowser()).thenReturn(false); - NamespaceAndName existingRepo = new NamespaceAndName("space", "repo"); - when(serviceFactory.create(not(eq(existingRepo)))).thenThrow(new NotFoundException("Test", "a")); - when(serviceFactory.create(existingRepo)).thenReturn(repositoryService); + @Nested + class Browser { + + @BeforeEach + void prepareMocks() { + when(userAgentParser.parse(request)).thenReturn(userAgent); + when(userAgent.isBrowser()).thenReturn(true); + when(request.getRequestURI()).thenReturn("uri"); + } + + @Test + void shouldDispatchBrowserRequests() throws ServletException, IOException { + when(userAgent.isBrowser()).thenReturn(true); + when(request.getRequestURI()).thenReturn("uri"); + + servlet.service(request, response); + + verify(dispatcher).dispatch(request, response, "uri"); + } + } - @Test - void shouldDispatchBrowserRequests() throws ServletException, IOException { - when(userAgent.isBrowser()).thenReturn(true); - when(request.getRequestURI()).thenReturn("uri"); + @Nested + class ScmClient { - servlet.service(request, response); + @BeforeEach + void prepareMocks() { + when(userAgentParser.parse(request)).thenReturn(userAgent); + when(userAgent.isBrowser()).thenReturn(false); + } - verify(dispatcher).dispatch(request, response, "uri"); - } + @Test + void shouldHandleBadPaths() throws IOException, ServletException { + when(request.getPathInfo()).thenReturn("/illegal"); - @Test - void shouldHandleBadPaths() throws IOException, ServletException { - when(request.getPathInfo()).thenReturn("/illegal"); + servlet.service(request, response); - servlet.service(request, response); + verify(response).setStatus(400); + } - verify(response).setStatus(400); - } + @Test + void shouldHandleNotExistingRepository() throws IOException, ServletException { + when(request.getPathInfo()).thenReturn("/not/exists"); - @Test - void shouldHandleNotExistingRepository() throws IOException, ServletException { - when(request.getPathInfo()).thenReturn("/not/exists"); + NamespaceAndName repo = new NamespaceAndName("not", "exists"); + when(extractor.fromUri("/not/exists")).thenReturn(Optional.of(repo)); + when(serviceFactory.create(repo)).thenThrow(new NotFoundException("Test", "a")); - servlet.service(request, response); + servlet.service(request, response); - verify(response).setStatus(404); - } + verify(response).setStatus(404); + } - @Test - void shouldDelegateToProvider() throws IOException, ServletException { - when(request.getPathInfo()).thenReturn("/space/name"); - NamespaceAndName namespaceAndName = new NamespaceAndName("space", "name"); - doReturn(repositoryService).when(serviceFactory).create(namespaceAndName); - Repository repository = new Repository(); - when(repositoryService.getRepository()).thenReturn(repository); - when(repositoryService.getProtocol(HttpScmProtocol.class)).thenReturn(protocol); + @Test + void shouldDelegateToProvider() throws IOException, ServletException { + NamespaceAndName repo = new NamespaceAndName("space", "name"); + when(extractor.fromUri("/space/name")).thenReturn(Optional.of(repo)); + when(serviceFactory.create(repo)).thenReturn(repositoryService); - servlet.service(request, response); + when(request.getPathInfo()).thenReturn("/space/name"); + Repository repository = RepositoryTestData.createHeartOfGold(); + when(repositoryService.getRepository()).thenReturn(repository); + when(repositoryService.getProtocol(HttpScmProtocol.class)).thenReturn(protocol); + + servlet.service(request, response); + + verify(request).setAttribute(DefaultRepositoryProvider.ATTRIBUTE_NAME, repository); + verify(protocol).serve(request, response, null); + verify(repositoryService).close(); + } - verify(request).setAttribute(DefaultRepositoryProvider.ATTRIBUTE_NAME, repository); - verify(protocol).serve(request, response, null); - verify(repositoryService).close(); } } From cd03d47a3e9dd1a3241654c90f552d69654a6032 Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Tue, 23 Jul 2019 07:41:48 +0000 Subject: [PATCH 5/5] Close branch bugfix/repository_names_with_dot