From 6a7987481abe7449a5e5ae1c8fe6ff01bee90d46 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= <rene.pfeuffer@cloudogu.com>
Date: Fri, 17 Aug 2018 09:33:45 +0200
Subject: [PATCH] Peer review

---
 .../sonia/scm/it/RepositoryAccessITCase.java  | 32 +++++++++++++++++++
 .../java/sonia/scm/repository/SvnUtil.java    | 12 +++----
 .../api/v2/resources/BrowserResultDto.java    |  2 ++
 .../api/v2/resources/BrowserResultMapper.java |  6 +++-
 .../api/v2/resources/FileObjectMapper.java    | 11 ++++---
 .../scm/api/v2/resources/ResourceLinks.java   | 10 ++++--
 .../api/v2/resources/SourceRootResource.java  | 22 +++++++------
 .../v2/resources/FileObjectMapperTest.java    |  6 ++--
 8 files changed, 73 insertions(+), 28 deletions(-)

diff --git a/scm-it/src/test/java/sonia/scm/it/RepositoryAccessITCase.java b/scm-it/src/test/java/sonia/scm/it/RepositoryAccessITCase.java
index a461e40dea..1a132711c8 100644
--- a/scm-it/src/test/java/sonia/scm/it/RepositoryAccessITCase.java
+++ b/scm-it/src/test/java/sonia/scm/it/RepositoryAccessITCase.java
@@ -11,6 +11,8 @@ import org.junit.runners.Parameterized;
 import java.io.IOException;
 import java.util.Collection;
 
+import static java.lang.Thread.sleep;
+import static org.hamcrest.Matchers.equalTo;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assume.assumeFalse;
 import static sonia.scm.it.RestUtil.given;
@@ -64,4 +66,34 @@ public class RepositoryAccessITCase {
 
     assertNotNull(branchName);
   }
+
+  @Test
+  public void shouldReadContent() throws IOException, InterruptedException {
+    repositoryUtil.createAndCommitFile("a.txt", "a");
+
+    sleep(1000);
+
+    String sourcesUrl = given()
+      .when()
+      .get(TestData.getDefaultRepositoryUrl(repositoryType))
+      .then()
+      .statusCode(HttpStatus.SC_OK)
+      .extract()
+      .path("_links.sources.href");
+
+    String contentUrl = given()
+      .when()
+      .get(sourcesUrl)
+      .then()
+      .statusCode(HttpStatus.SC_OK)
+      .extract()
+      .path("files[0]._links.content.href");
+
+    given()
+      .when()
+      .get(contentUrl)
+      .then()
+      .statusCode(HttpStatus.SC_OK)
+      .body(equalTo("a"));
+  }
 }
diff --git a/scm-plugins/scm-svn-plugin/src/main/java/sonia/scm/repository/SvnUtil.java b/scm-plugins/scm-svn-plugin/src/main/java/sonia/scm/repository/SvnUtil.java
index 6cc5eb5da9..63fabaf4e9 100644
--- a/scm-plugins/scm-svn-plugin/src/main/java/sonia/scm/repository/SvnUtil.java
+++ b/scm-plugins/scm-svn-plugin/src/main/java/sonia/scm/repository/SvnUtil.java
@@ -38,10 +38,8 @@ package sonia.scm.repository;
 import com.google.common.base.Strings;
 import com.google.common.collect.Lists;
 import com.google.common.io.Closeables;
-
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
-
 import org.tmatesoft.svn.core.SVNErrorCode;
 import org.tmatesoft.svn.core.SVNLogEntry;
 import org.tmatesoft.svn.core.SVNLogEntryPath;
@@ -52,20 +50,17 @@ import org.tmatesoft.svn.core.internal.util.SVNXMLUtil;
 import org.tmatesoft.svn.core.io.SVNRepository;
 import org.tmatesoft.svn.core.wc.SVNClientManager;
 import org.tmatesoft.svn.core.wc.admin.SVNChangeEntry;
-
 import sonia.scm.util.HttpUtil;
 import sonia.scm.util.Util;
 
-//~--- JDK imports ------------------------------------------------------------
-
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
 import java.io.IOException;
 import java.io.PrintWriter;
-
 import java.util.List;
 import java.util.Map;
 
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
+//~--- JDK imports ------------------------------------------------------------
 
 /**
  *
@@ -366,6 +361,7 @@ public final class SvnUtil
   public static long getRevisionNumber(String revision)
     throws RepositoryException
   {
+    // REVIEW Bei SVN wird ohne Revision die -1 genommen, was zu einem Fehler führt
     long revisionNumber = -1;
 
     if (Util.isNotEmpty(revision))
diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/BrowserResultDto.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/BrowserResultDto.java
index 12c227b65c..b8ffd7ff26 100644
--- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/BrowserResultDto.java
+++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/BrowserResultDto.java
@@ -16,6 +16,7 @@ public class BrowserResultDto extends HalRepresentation implements Iterable<File
   private String revision;
   private String tag;
   private String branch;
+  // REVIEW files nicht embedded?
   private List<FileObjectDto> files;
 
   @Override
@@ -24,6 +25,7 @@ public class BrowserResultDto extends HalRepresentation implements Iterable<File
     return super.add(links);
   }
 
+  // REVIEW return null?
   @Override
   public Iterator<FileObjectDto> iterator() {
     Iterator<FileObjectDto> it = null;
diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/BrowserResultMapper.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/BrowserResultMapper.java
index 6ab054bd74..79f049a5be 100644
--- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/BrowserResultMapper.java
+++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/BrowserResultMapper.java
@@ -39,7 +39,11 @@ public class BrowserResultMapper {
   }
 
   private void addLinks(BrowserResult browserResult, BrowserResultDto dto, NamespaceAndName namespaceAndName) {
-    dto.add(Links.linkingTo().self(resourceLinks.source().self(namespaceAndName.getNamespace(), namespaceAndName.getName(), browserResult.getRevision())).build());
+    if (browserResult.getRevision() == null) {
+      dto.add(Links.linkingTo().self(resourceLinks.source().selfWithoutRevision(namespaceAndName.getNamespace(), namespaceAndName.getName())).build());
+    } else {
+      dto.add(Links.linkingTo().self(resourceLinks.source().self(namespaceAndName.getNamespace(), namespaceAndName.getName(), browserResult.getRevision())).build());
+    }
   }
 
 
diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/FileObjectMapper.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/FileObjectMapper.java
index 4363a8a9b1..ebabf79342 100644
--- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/FileObjectMapper.java
+++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/FileObjectMapper.java
@@ -25,9 +25,12 @@ public abstract class FileObjectMapper extends BaseMapper<FileObject, FileObject
 
   @AfterMapping
   void addLinks(FileObject fileObject, @MappingTarget FileObjectDto dto, @Context NamespaceAndName namespaceAndName, @Context String revision) {
-      dto.add(Links.linkingTo()
-        .self(resourceLinks.source().sourceWithPath(namespaceAndName.getNamespace(), namespaceAndName.getName(), revision, fileObject.getName()))
-        .single(link("content", resourceLinks.source().content(namespaceAndName.getNamespace(), namespaceAndName.getName(), revision, fileObject.getName())))
-        .build());
+    Links.Builder links = Links.linkingTo()
+      .self(resourceLinks.source().sourceWithPath(namespaceAndName.getNamespace(), namespaceAndName.getName(), revision, fileObject.getPath()));
+    if (!dto.isDirectory()) {
+      links.single(link("content", resourceLinks.source().content(namespaceAndName.getNamespace(), namespaceAndName.getName(), revision, fileObject.getPath())));
+    }
+
+    dto.add(links.build());
   }
 }
diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/ResourceLinks.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/ResourceLinks.java
index c4aa579c80..0d88995281 100644
--- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/ResourceLinks.java
+++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/ResourceLinks.java
@@ -291,11 +291,11 @@ class ResourceLinks {
     }
 
     String self(String namespace, String name, String revision) {
-      return sourceLinkBuilder.method("getRepositoryResource").parameters(namespace, name, revision).method("sources").parameters().method("getAll").parameters("").href();
+      return sourceLinkBuilder.method("getRepositoryResource").parameters(namespace, name).method("sources").parameters().method("getAll").parameters(revision).href();
     }
 
     String selfWithoutRevision(String namespace, String name) {
-      return sourceLinkBuilder.method("getRepositoryResource").parameters(namespace, name).method("sources").parameters().method("getAll").parameters("").href();
+      return sourceLinkBuilder.method("getRepositoryResource").parameters(namespace, name).method("sources").parameters().method("getAllWithoutRevision").parameters().href();
     }
 
     public String source(String namespace, String name, String revision) {
@@ -303,7 +303,11 @@ class ResourceLinks {
     }
 
     public String sourceWithPath(String namespace, String name, String revision, String path) {
-      return sourceLinkBuilder.method("getRepositoryResource").parameters(namespace, name).method("sources").parameters().method("get").parameters(revision, path).href();
+      if (revision == null) {
+        return sourceLinkBuilder.method("getRepositoryResource").parameters(namespace, name).method("sources").parameters().method("get").parameters(null, path).href();
+      } else {
+        return sourceLinkBuilder.method("getRepositoryResource").parameters(namespace, name).method("sources").parameters().method("get").parameters(revision, path).href();
+      }
     }
 
     public String content(String namespace, String name, String revision, String path) {
diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/SourceRootResource.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/SourceRootResource.java
index 9f2709cb43..3e0ab31ef7 100644
--- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/SourceRootResource.java
+++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/SourceRootResource.java
@@ -31,7 +31,14 @@ public class SourceRootResource {
 
   @GET
   @Produces(VndMediaType.SOURCE)
-  @Path("{revision : (\\w+)?}")
+  @Path("")
+  public Response getAllWithoutRevision(@PathParam("namespace") String namespace, @PathParam("name") String name) {
+    return getSource(namespace, name, "/", null);
+  }
+
+  @GET
+  @Produces(VndMediaType.SOURCE)
+  @Path("{revision}")
   public Response getAll(@PathParam("namespace") String namespace, @PathParam("name") String name, @PathParam("revision") String revision) {
     return getSource(namespace, name, "/", revision);
   }
@@ -44,8 +51,6 @@ public class SourceRootResource {
   }
 
   private Response getSource(String namespace, String repoName, String path, String revision) {
-    BrowserResult browserResult;
-    Response response;
     NamespaceAndName namespaceAndName = new NamespaceAndName(namespace, repoName);
     try (RepositoryService repositoryService = serviceFactory.create(namespaceAndName)) {
       BrowseCommandBuilder browseCommand = repositoryService.getBrowseCommand();
@@ -53,19 +58,18 @@ public class SourceRootResource {
       if (revision != null && !revision.isEmpty()) {
         browseCommand.setRevision(revision);
       }
-      browserResult = browseCommand.getBrowserResult();
+      BrowserResult browserResult = browseCommand.getBrowserResult();
 
       if (browserResult != null) {
-        response = Response.ok(browserResultMapper.map(browserResult, namespaceAndName)).build();
+        return Response.ok(browserResultMapper.map(browserResult, namespaceAndName)).build();
       } else {
-        response = Response.status(Response.Status.NOT_FOUND).build();
+        return Response.status(Response.Status.NOT_FOUND).build();
       }
 
     } catch (RepositoryNotFoundException e) {
-      response = Response.status(Response.Status.NOT_FOUND).build();
+      return Response.status(Response.Status.NOT_FOUND).build();
     } catch (RepositoryException | IOException e) {
-      response = Response.status(Response.Status.INTERNAL_SERVER_ERROR).build();
+      return Response.status(Response.Status.INTERNAL_SERVER_ERROR).build();
     }
-    return response;
   }
 }
diff --git a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/FileObjectMapperTest.java b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/FileObjectMapperTest.java
index 63d1050797..2feb5bc8e1 100644
--- a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/FileObjectMapperTest.java
+++ b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/FileObjectMapperTest.java
@@ -54,7 +54,7 @@ public class FileObjectMapperTest {
     FileObject fileObject = createFileObject();
     FileObjectDto dto = mapper.map(fileObject, new NamespaceAndName("namespace", "name"), "revision");
 
-    assertThat(dto.getLinks().getLinkBy("self").get().getHref()).isEqualTo(expectedBaseUri.resolve("namespace/name/sources/revision/foo").toString());
+    assertThat(dto.getLinks().getLinkBy("self").get().getHref()).isEqualTo(expectedBaseUri.resolve("namespace/name/sources/revision/foo%2Fbar").toString());
   }
 
   @Test
@@ -62,14 +62,14 @@ public class FileObjectMapperTest {
     FileObject fileObject = createFileObject();
     FileObjectDto dto = mapper.map(fileObject, new NamespaceAndName("namespace", "name"), "revision");
 
-    assertThat(dto.getLinks().getLinkBy("content").get().getHref()).isEqualTo(expectedBaseUri.resolve("namespace/name/content/revision/foo").toString());
+    assertThat(dto.getLinks().getLinkBy("content").get().getHref()).isEqualTo(expectedBaseUri.resolve("namespace/name/content/revision/foo%2Fbar").toString());
   }
 
   private FileObject createFileObject() {
     FileObject fileObject = new FileObject();
     fileObject.setName("foo");
     fileObject.setDescription("bar");
-    fileObject.setPath("/foo/bar");
+    fileObject.setPath("foo/bar");
     fileObject.setDirectory(false);
     fileObject.setLength(100);
     fileObject.setLastModified(123L);