Refactor directories in RepositoryPath and dao

scm-core/src/main/java/sonia/scm/repository/InitialRepositoryLocationResolver.java

@@ -19,7 +19,7 @@ import java.io.IOException;
  * @author Mohamed Karray
  * @since 2.0.0
  */
-public final class InitialRepositoryLocationResolver {
+public class InitialRepositoryLocationResolver {
 
   private static final String DEFAULT_REPOSITORY_PATH = "repositories";
   public static final String REPOSITORIES_NATIVE_DIRECTORY = "data";
@@ -38,24 +38,21 @@ public final class InitialRepositoryLocationResolver {
   }
 
   public File createDirectory(Repository repository) {
-    File initialRepoFolder = getDirectory(getDefaultRepositoryPath(), repository);
+    String initialRepoFolder = getRelativeRepositoryPath(repository);
     try {
-      fileSystem.create(initialRepoFolder);
+      File directory = new File(context.getBaseDirectory(), initialRepoFolder);
+      fileSystem.create(directory);
+      return directory;
     } catch (IOException e) {
-      throw new InternalRepositoryException(repository, "Cannot create repository directory for "+repository.getNamespaceAndName(),  e);
+      throw new InternalRepositoryException(repository, "Cannot create repository directory for " + repository.getNamespaceAndName(), e);
     }
-    return initialRepoFolder;
   }
 
-  public File getDirectory(String defaultRepositoryRelativePath, Repository repository) {
+  public String getRelativeRepositoryPath(Repository repository) {
-    return new File(context.getBaseDirectory(), defaultRepositoryRelativePath + File.separator + repository.getId());
+    return getDefaultRepositoryPath() + File.separator + repository.getId();
   }
 
   public String getDefaultRepositoryPath() {
-    return DEFAULT_REPOSITORY_PATH ;
+    return DEFAULT_REPOSITORY_PATH;
-  }
-
-  public String getRelativePath(String absolutePath) {
-    return absolutePath.replaceFirst(context.getBaseDirectory().getAbsolutePath()+"/", "");
   }
 }

scm-core/src/test/java/sonia/scm/repository/InitialRepositoryLocationResolverTest.java

@@ -0,0 +1,43 @@
+package sonia.scm.repository;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+import sonia.scm.SCMContextProvider;
+import sonia.scm.io.DefaultFileSystem;
+
+import java.io.File;
+import java.io.IOException;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.Mockito.when;
+
+@RunWith(MockitoJUnitRunner.class)
+public class InitialRepositoryLocationResolverTest {
+
+  @Mock
+  private SCMContextProvider context;
+
+  @Rule
+  public TemporaryFolder temporaryFolder = new TemporaryFolder();
+
+  @Before
+  public void init() throws IOException {
+    when(context.getBaseDirectory()).thenReturn(temporaryFolder.newFolder());
+  }
+
+  @Test
+  public void x() {
+    InitialRepositoryLocationResolver resolver = new InitialRepositoryLocationResolver(context, new DefaultFileSystem());
+    Repository repository = new Repository();
+    repository.setId("ABC");
+    File directory = resolver.createDirectory(repository);
+
+    assertThat(directory).isEqualTo(new File(context.getBaseDirectory(), "repositories/ABC"));
+    assertThat(context.getBaseDirectory().exists()).isTrue();
+  }
+}

scm-dao-xml/src/main/java/sonia/scm/repository/xml/RepositoryPath.java

@@ -21,9 +21,6 @@ public class RepositoryPath implements ModelObject {
   @XmlTransient
   private Repository repository;
 
-  @XmlTransient
-  private String absolutePath;
-
   @XmlTransient
   private boolean toBeSynchronized;
 
@@ -33,9 +30,8 @@ public class RepositoryPath implements ModelObject {
   public RepositoryPath() {
   }
 
-  public RepositoryPath(String path, String absolutePath, String id, Repository repository) {
+  public RepositoryPath(String path, String id, Repository repository) {
     this.path = path;
-    this.absolutePath = absolutePath;
     this.id = id;
     this.repository = repository;
   }
@@ -102,12 +98,4 @@ public class RepositoryPath implements ModelObject {
   public void setToBeSynchronized(boolean toBeSynchronized) {
     this.toBeSynchronized = toBeSynchronized;
   }
-
-  public String getAbsolutePath() {
-    return absolutePath;
-  }
-
-  public void setAbsolutePath(String absolutePath) {
-    this.absolutePath = absolutePath;
-  }
 }

scm-dao-xml/src/main/java/sonia/scm/repository/xml/XmlRepositoryDAO.java

@@ -35,19 +35,18 @@ package sonia.scm.repository.xml;
 
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
-import sonia.scm.SCMContext;
+import sonia.scm.SCMContextProvider;
 import sonia.scm.repository.InitialRepositoryLocationResolver;
+import sonia.scm.repository.InternalRepositoryException;
 import sonia.scm.repository.NamespaceAndName;
 import sonia.scm.repository.PathBasedRepositoryDAO;
 import sonia.scm.repository.Repository;
 import sonia.scm.store.ConfigurationStoreFactory;
 import sonia.scm.xml.AbstractXmlDAO;
 
-import java.io.File;
 import java.nio.file.Path;
-import java.nio.file.Paths;
 import java.util.Collection;
-import java.util.function.Supplier;
+import java.util.Optional;
 
 /**
  * @author Sebastian Sdorra
@@ -59,6 +58,7 @@ public class XmlRepositoryDAO
 
   public static final String STORE_NAME = "repositories";
   private InitialRepositoryLocationResolver initialRepositoryLocationResolver;
+  private final SCMContextProvider context;
 
   //~--- constructors ---------------------------------------------------------
 
@@ -66,11 +66,13 @@ public class XmlRepositoryDAO
    * Constructs ...
    *
    * @param storeFactory
+   * @param context
    */
   @Inject
-  public XmlRepositoryDAO(ConfigurationStoreFactory storeFactory, InitialRepositoryLocationResolver initialRepositoryLocationResolver) {
+  public XmlRepositoryDAO(ConfigurationStoreFactory storeFactory, InitialRepositoryLocationResolver initialRepositoryLocationResolver, SCMContextProvider context) {
     super(storeFactory.getStore(XmlRepositoryDatabase.class, STORE_NAME));
     this.initialRepositoryLocationResolver = initialRepositoryLocationResolver;
+    this.context = context;
   }
 
   //~--- methods --------------------------------------------------------------
@@ -92,22 +94,17 @@ public class XmlRepositoryDAO
 
   @Override
   public void modify(Repository repository) {
-    String path = getPath(repository).toAbsolutePath().toString();
+    RepositoryPath repositoryPath = findExistingRepositoryPath(repository).orElseThrow(() -> new InternalRepositoryException(repository, "path object for repository not found"));
-    db.remove(repository.getId());
+    repositoryPath.setRepository(repository);
-    RepositoryPath repositoryPath = new RepositoryPath(initialRepositoryLocationResolver.getRelativePath(path), path, repository.getId(), repository.clone());
     repositoryPath.setToBeSynchronized(true);
-    add(repositoryPath);
+    storeDB();
   }
 
   @Override
   public void add(Repository repository) {
-    String path = getPath(repository).toAbsolutePath().toString();
+    String relativeRepositoryPath = initialRepositoryLocationResolver.getRelativeRepositoryPath(repository);
-    RepositoryPath repositoryPath = new RepositoryPath(initialRepositoryLocationResolver.getRelativePath(path),path, repository.getId(), repository.clone());
+    RepositoryPath repositoryPath = new RepositoryPath(relativeRepositoryPath, repository.getId(), repository.clone());
     repositoryPath.setToBeSynchronized(true);
-    add(repositoryPath);
-  }
-
-  public void add(RepositoryPath repositoryPath) {
     synchronized (store) {
       db.add(repositoryPath);
       storeDB();
@@ -151,20 +148,15 @@ public class XmlRepositoryDAO
 
   @Override
   public Path getPath(Repository repository) {
-    return db.getPaths().stream()
+    return context
-      .filter(repoPath -> repoPath.getId().equals(repository.getId()))
+      .getBaseDirectory()
-      .findFirst()
+      .toPath()
-      .map(RepositoryPath::getPath)
+      .resolve(findExistingRepositoryPath(repository).map(RepositoryPath::getPath).orElse(initialRepositoryLocationResolver.getRelativeRepositoryPath(repository)));
-      .map(relativePath -> new File(SCMContext.getContext().getBaseDirectory(), relativePath).toPath())
-      .orElseGet(createRepositoryPath(repository));
   }
 
-  private Supplier<? extends Path> createRepositoryPath(Repository repository) {
+  private Optional<RepositoryPath> findExistingRepositoryPath(Repository repository) {
-    return () -> {
+    return db.getPaths().stream()
-      if (db.getDefaultDirectory() == null) {
+      .filter(repoPath -> repoPath.getId().equals(repository.getId()))
-        db.setDefaultDirectory(initialRepositoryLocationResolver.getDefaultRepositoryPath());
+      .findFirst();
-      }
-      return Paths.get(initialRepositoryLocationResolver.getDirectory(db.getDefaultDirectory(), repository).toURI());
-    };
   }
 }

scm-dao-xml/src/main/java/sonia/scm/repository/xml/XmlRepositoryDatabase.java

@@ -60,8 +60,6 @@ public class XmlRepositoryDatabase implements XmlDatabase<RepositoryPath> {
 
   private Long lastModified;
 
-  private String defaultDirectory;
-
   @XmlJavaTypeAdapter(XmlRepositoryMapAdapter.class)
   @XmlElement(name = "repositories")
   private Map<String, RepositoryPath> repositoryPathMap = new LinkedHashMap<>();
@@ -202,12 +200,4 @@ public class XmlRepositoryDatabase implements XmlDatabase<RepositoryPath> {
   {
     this.lastModified = lastModified;
   }
-
-  public String getDefaultDirectory() {
-    return defaultDirectory;
-  }
-
-  public void setDefaultDirectory(String defaultDirectory) {
-    this.defaultDirectory = defaultDirectory;
-  }
 }

scm-dao-xml/src/main/java/sonia/scm/repository/xml/XmlRepositoryMapAdapter.java

@@ -33,10 +33,10 @@ package sonia.scm.repository.xml;
 
 import sonia.scm.SCMContext;
 import sonia.scm.SCMContextProvider;
+import sonia.scm.repository.InternalRepositoryException;
 import sonia.scm.repository.Repository;
 import sonia.scm.store.StoreConstants;
 import sonia.scm.store.StoreException;
-import sonia.scm.util.IOUtil;
 
 import javax.xml.bind.JAXBContext;
 import javax.xml.bind.JAXBException;
@@ -44,6 +44,8 @@ import javax.xml.bind.Marshaller;
 import javax.xml.bind.Unmarshaller;
 import javax.xml.bind.annotation.adapters.XmlAdapter;
 import java.io.File;
+import java.nio.file.Files;
+import java.nio.file.Path;
 import java.util.LinkedHashMap;
 import java.util.Map;
 
@@ -66,11 +68,13 @@ public class XmlRepositoryMapAdapter extends XmlAdapter<XmlRepositoryList, Map<S
       for (RepositoryPath repositoryPath : repositoryPaths.getRepositoryPaths()) {
         if (repositoryPath.toBeSynchronized()) {
 
-          File dir = new File(repositoryPath.getAbsolutePath());
+          File baseDirectory = SCMContext.getContext().getBaseDirectory();
-          if (!dir.exists()) {
+          Path dir = baseDirectory.toPath().resolve(repositoryPath.getPath());
-            IOUtil.mkdirs(dir);
+
+          if (!Files.isDirectory(dir)) {
+            throw new InternalRepositoryException(repositoryPath.getRepository(), "repository path not found");
           }
-          marshaller.marshal(repositoryPath.getRepository(), getRepositoryMetadataFile(dir));
+          marshaller.marshal(repositoryPath.getRepository(), getRepositoryMetadataFile(dir.toFile()));
           repositoryPath.setToBeSynchronized(false);
         }
       }
@@ -95,7 +99,8 @@ public class XmlRepositoryMapAdapter extends XmlAdapter<XmlRepositoryList, Map<S
       for (RepositoryPath repositoryPath : repositoryPaths) {
         SCMContextProvider contextProvider = SCMContext.getContext();
         File baseDirectory = contextProvider.getBaseDirectory();
-        Repository repository = (Repository) unmarshaller.unmarshal(getRepositoryMetadataFile(new File(baseDirectory, repositoryPath.getPath())));
+        Repository repository = (Repository) unmarshaller.unmarshal(getRepositoryMetadataFile(baseDirectory.toPath().resolve(repositoryPath.getPath()).toFile()));
+
         repositoryPath.setRepository(repository);
         repositoryPathMap.put(XmlRepositoryDatabase.createKey(repository), repositoryPath);
       }

scm-dao-xml/src/test/java/sonia/scm/repository/xml/XmlRepositoryDAOTest.java

@@ -0,0 +1,117 @@
+package sonia.scm.repository.xml;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+import sonia.scm.SCMContextProvider;
+import sonia.scm.repository.InitialRepositoryLocationResolver;
+import sonia.scm.repository.Repository;
+import sonia.scm.store.ConfigurationStore;
+import sonia.scm.store.ConfigurationStoreFactory;
+
+import java.io.IOException;
+import java.nio.file.Path;
+
+import static java.util.Collections.emptyList;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.codehaus.groovy.runtime.InvokerHelper.asList;
+import static org.mockito.ArgumentMatchers.argThat;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+import static sonia.scm.repository.xml.XmlRepositoryDAO.STORE_NAME;
+
+@RunWith(MockitoJUnitRunner.class)
+public class XmlRepositoryDAOTest {
+
+  @Mock
+  private ConfigurationStoreFactory storeFactory;
+  @Mock
+  private ConfigurationStore<XmlRepositoryDatabase> store;
+  @Mock
+  private XmlRepositoryDatabase db;
+  @Mock
+  private SCMContextProvider context;
+
+  @Rule
+  public TemporaryFolder temporaryFolder = new TemporaryFolder();
+
+  @Before
+  public void init() throws IOException {
+    when(storeFactory.getStore(XmlRepositoryDatabase.class, STORE_NAME)).thenReturn(store);
+    when(store.get()).thenReturn(db);
+    when(context.getBaseDirectory()).thenReturn(temporaryFolder.newFolder());
+  }
+
+  @Test
+  public void addShouldCreateNewRepositoryPathWithRelativePath() {
+    InitialRepositoryLocationResolver initialRepositoryLocationResolver = new InitialRepositoryLocationResolver(context, null);
+    XmlRepositoryDAO dao = new XmlRepositoryDAO(storeFactory, initialRepositoryLocationResolver, context);
+
+    dao.add(new Repository("id", null, null, null));
+
+    verify(db).add(argThat(repositoryPath -> {
+      assertThat(repositoryPath.getId()).isEqualTo("id");
+      assertThat(repositoryPath.getPath()).isEqualTo(initialRepositoryLocationResolver.getDefaultRepositoryPath() + "/id");
+      return true;
+    }));
+    verify(store).set(db);
+  }
+
+  @Test
+  public void modifyShould() {
+    Repository oldRepository = new Repository("id", "old", null, null);
+    RepositoryPath repositoryPath = new RepositoryPath("/path", "id", oldRepository);
+    when(db.getPaths()).thenReturn(asList(repositoryPath));
+
+    XmlRepositoryDAO dao = new XmlRepositoryDAO(storeFactory, new InitialRepositoryLocationResolver(context, null), context);
+
+    Repository newRepository = new Repository("id", "new", null, null);
+    dao.modify(newRepository);
+
+    assertThat(repositoryPath.getRepository()).isSameAs(newRepository);
+    verify(store).set(db);
+  }
+
+  @Test
+  public void shouldGetPathInBaseDirectoryForRelativePath() {
+    Repository existingRepository = new Repository("id", "old", null, null);
+    RepositoryPath repositoryPath = new RepositoryPath("path", "id", existingRepository);
+    when(db.getPaths()).thenReturn(asList(repositoryPath));
+
+    XmlRepositoryDAO dao = new XmlRepositoryDAO(storeFactory, new InitialRepositoryLocationResolver(context, null), context);
+
+    Path path = dao.getPath(existingRepository);
+
+    assertThat(path.toString()).isEqualTo(context.getBaseDirectory().getPath() + "/path");
+  }
+
+  @Test
+  public void shouldGetPathInBaseDirectoryForAbsolutePath() {
+    Repository existingRepository = new Repository("id", "old", null, null);
+    RepositoryPath repositoryPath = new RepositoryPath("/tmp/path", "id", existingRepository);
+    when(db.getPaths()).thenReturn(asList(repositoryPath));
+
+    XmlRepositoryDAO dao = new XmlRepositoryDAO(storeFactory, new InitialRepositoryLocationResolver(context, null), context);
+
+    Path path = dao.getPath(existingRepository);
+
+    assertThat(path.toString()).isEqualTo("/tmp/path");
+  }
+
+  @Test
+  public void shouldGetPathForNewRepository() {
+    when(db.getPaths()).thenReturn(emptyList());
+
+    InitialRepositoryLocationResolver initialRepositoryLocationResolver = new InitialRepositoryLocationResolver(context, null);
+    XmlRepositoryDAO dao = new XmlRepositoryDAO(storeFactory, initialRepositoryLocationResolver, context);
+
+    Repository newRepository = new Repository("id", "new", null, null);
+    Path path = dao.getPath(newRepository);
+
+    assertThat(path.toString()).isEqualTo(context.getBaseDirectory().getPath() + "/" + initialRepositoryLocationResolver.getDefaultRepositoryPath() + "/id");
+  }
+}

scm-test/src/main/java/sonia/scm/ManagerTestBase.java

@@ -39,6 +39,7 @@ import org.junit.Rule;
 import org.junit.rules.TemporaryFolder;
 import sonia.scm.util.MockUtil;
 
+import java.io.File;
 import java.io.IOException;
 
 /**
@@ -56,10 +57,12 @@ public abstract class ManagerTestBase<T extends ModelObject>
   protected SCMContextProvider contextProvider;
   
   protected Manager<T> manager;
-  
+  protected File temp;
+
   @Before
   public void setUp() throws IOException {
-    contextProvider = MockUtil.getSCMContextProvider(tempFolder.newFolder());
+    temp = tempFolder.newFolder();
+    contextProvider = MockUtil.getSCMContextProvider(temp);
     manager = createManager();
     manager.init(contextProvider);
   }

scm-webapp/src/test/java/sonia/scm/repository/DefaultRepositoryManagerTest.java

@@ -39,10 +39,11 @@ import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Sets;
 import org.apache.shiro.authz.UnauthorizedException;
 import org.apache.shiro.util.ThreadContext;
-import org.junit.Ignore;
+import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
+import org.junit.rules.TemporaryFolder;
 import org.mockito.Mockito;
 import org.mockito.invocation.InvocationOnMock;
 import sonia.scm.AlreadyExistsException;
@@ -50,6 +51,7 @@ import sonia.scm.HandlerEventType;
 import sonia.scm.Manager;
 import sonia.scm.ManagerTestBase;
 import sonia.scm.NotFoundException;
+import sonia.scm.SCMContext;
 import sonia.scm.config.ScmConfiguration;
 import sonia.scm.event.ScmEventBus;
 import sonia.scm.io.DefaultFileSystem;
@@ -107,10 +109,18 @@ public class DefaultRepositoryManagerTest extends ManagerTestBase<Repository> {
   @Rule
   public ExpectedException thrown = ExpectedException.none();
 
+  @Rule
+  public TemporaryFolder temporaryFolder = new TemporaryFolder();
+
   private ScmConfiguration configuration;
 
   private String mockedNamespace = "default_namespace";
 
+  @Before
+  public void initContext() {
+    ((TempSCMContextProvider)SCMContext.getContext()).setBaseDirectory(temp);
+  }
+
   @Test
   public void testCreate() {
     Repository heartOfGold = createTestRepository();
@@ -159,7 +169,6 @@ public class DefaultRepositoryManagerTest extends ManagerTestBase<Repository> {
   }
 
   @Test
-  @Ignore
   public void testDeleteWithEnabledArchive() {
     Repository repository = createTestRepository();
 
@@ -426,7 +435,7 @@ public class DefaultRepositoryManagerTest extends ManagerTestBase<Repository> {
     Set<RepositoryHandler> handlerSet = new HashSet<>();
     ConfigurationStoreFactory factory = new JAXBConfigurationStoreFactory(contextProvider);
     InitialRepositoryLocationResolver initialRepositoryLocationResolver = new InitialRepositoryLocationResolver(contextProvider, fileSystem);
-    XmlRepositoryDAO repositoryDAO = new XmlRepositoryDAO(factory, initialRepositoryLocationResolver);
+    XmlRepositoryDAO repositoryDAO = new XmlRepositoryDAO(factory, initialRepositoryLocationResolver, contextProvider);
     RepositoryLocationResolver repositoryLocationResolver = new RepositoryLocationResolver(repositoryDAO, initialRepositoryLocationResolver);
     handlerSet.add(new DummyRepositoryHandler(factory, repositoryLocationResolver));
     handlerSet.add(new DummyRepositoryHandler(factory, repositoryLocationResolver) {