From 87904e3da81fa4917578d18f6a1812f1f1c465e1 Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Tue, 10 Dec 2019 15:56:56 +0100 Subject: [PATCH 01/35] Stop fetching commits when this takes too long This is a first step to create results in big repositories. Next step should be querying the commit messages in the background and update cached results for further requests. --- .../scm/repository/spi/GitBrowseCommand.java | 89 +++++++++++-------- 1 file changed, 54 insertions(+), 35 deletions(-) diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java index aa362b8ec6..753802d3eb 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java @@ -69,6 +69,11 @@ import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import static java.util.Optional.empty; import static sonia.scm.ContextEntry.ContextBuilder.entity; @@ -94,6 +99,9 @@ public class GitBrowseCommand extends AbstractGitCommand LoggerFactory.getLogger(GitBrowseCommand.class); private final LfsBlobStoreFactory lfsBlobStoreFactory; + private boolean fetchCommits = true; + private ExecutorService executorService; + //~--- constructors --------------------------------------------------------- /** @@ -114,42 +122,36 @@ public class GitBrowseCommand extends AbstractGitCommand @SuppressWarnings("unchecked") public BrowserResult getBrowserResult(BrowseCommandRequest request) throws IOException { - logger.debug("try to create browse result for {}", request); + executorService = Executors.newSingleThreadExecutor(); + try { + logger.debug("try to create browse result for {}", request); - BrowserResult result; + org.eclipse.jgit.lib.Repository repo = open(); + ObjectId revId = computeRevIdToBrowse(request, repo); - org.eclipse.jgit.lib.Repository repo = open(); - ObjectId revId; - - if (Util.isEmpty(request.getRevision())) - { - revId = getDefaultBranch(repo); - } - else - { - revId = GitUtil.getRevisionId(repo, request.getRevision()); + if (revId != null) { + return new BrowserResult(revId.getName(), request.getRevision(), getEntry(repo, request, revId)); + } else { + logger.warn("could not find head of repository {}, empty?", repository.getNamespaceAndName()); + return new BrowserResult(Constants.HEAD, request.getRevision(), createEmtpyRoot()); + } + } finally { + executorService.shutdown(); } + } - if (revId != null) - { - result = new BrowserResult(revId.getName(), request.getRevision(), getEntry(repo, request, revId)); - } - else - { - if (Util.isNotEmpty(request.getRevision())) - { + private ObjectId computeRevIdToBrowse(BrowseCommandRequest request, org.eclipse.jgit.lib.Repository repo) throws IOException { + + if (Util.isEmpty(request.getRevision())) { + return getDefaultBranch(repo); + } else { + ObjectId revId = GitUtil.getRevisionId(repo, request.getRevision()); + if (revId == null) { logger.error("could not find revision {}", request.getRevision()); throw notFound(entity("Revision", request.getRevision()).in(this.repository)); } - else if (logger.isWarnEnabled()) - { - logger.warn("could not find head of repository, empty?"); - } - - result = new BrowserResult(Constants.HEAD, request.getRevision(), createEmtpyRoot()); + return revId; } - - return result; } //~--- methods -------------------------------------------------------------- @@ -207,8 +209,19 @@ public class GitBrowseCommand extends AbstractGitCommand // don't show message and date for directories to improve performance if (!file.isDirectory() &&!request.isDisableLastCommit()) { - logger.trace("fetch last commit for {} at {}", path, revId.getName()); - RevCommit commit = getLatestCommit(repo, revId, path); + RevCommit commit = null; + if (fetchCommits) { + logger.trace("fetch last commit for {} at {}", path, revId.getName()); + Future f = executorService.submit(() -> getLatestCommit(repo, revId, path)); + try { + commit = f.get(100, TimeUnit.MILLISECONDS); + } catch (TimeoutException e) { + logger.info("lookup of latest commit took too much time for {} at {}; disabling lookup on {}", path, revId.name(), repository.getNamespaceAndName()); + fetchCommits = false; + } catch (Exception e) { + logger.warn("got exception while computing last commit for {} on {}", path, revId.name(), e); + } + } Optional lfsPointer = commit == null? empty(): GitUtil.getLfsPointer(repo, path, commit, treeWalk); @@ -231,7 +244,7 @@ public class GitBrowseCommand extends AbstractGitCommand file.setLastModified(GitUtil.getCommitTime(commit)); file.setDescription(commit.getShortMessage()); } - else if (logger.isWarnEnabled()) + else if (fetchCommits) { logger.warn("could not find latest commit for {} on {}", path, revId); @@ -243,6 +256,8 @@ public class GitBrowseCommand extends AbstractGitCommand //~--- get methods ---------------------------------------------------------- +private RevWalk commitWalk = null; + /** * Method description * @@ -263,8 +278,12 @@ public class GitBrowseCommand extends AbstractGitCommand try { walk = new RevWalk(repo); - walk.setTreeFilter(AndTreeFilter.create(PathFilter.create(path), - TreeFilter.ANY_DIFF)); + walk.setTreeFilter(AndTreeFilter.create( + TreeFilter.ANY_DIFF + , + PathFilter.create(path) + ) + ); RevCommit commit = walk.parseCommit(revId); @@ -404,8 +423,8 @@ public class GitBrowseCommand extends AbstractGitCommand } catch (NotFoundException ex) { - logger.trace("could not find .gitmodules", ex); - subRepositories = Collections.EMPTY_MAP; + logger.trace("could not find .gitmodules: {}", ex.getMessage()); + subRepositories = Collections.emptyMap(); } return subRepositories; From 6f4074c21cc628c2500bffc540c37591751fb86c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Tue, 10 Dec 2019 18:10:11 +0100 Subject: [PATCH 02/35] Update browser result after computation --- .../repository/api/BrowseCommandBuilder.java | 11 ++- .../repository/spi/BrowseCommandRequest.java | 20 +++++ .../scm/repository/spi/GitBrowseCommand.java | 77 ++++++++++--------- .../repository/spi/GitBrowseCommandTest.java | 2 +- 4 files changed, 70 insertions(+), 40 deletions(-) diff --git a/scm-core/src/main/java/sonia/scm/repository/api/BrowseCommandBuilder.java b/scm-core/src/main/java/sonia/scm/repository/api/BrowseCommandBuilder.java index d482c04ea4..563557f0c1 100644 --- a/scm-core/src/main/java/sonia/scm/repository/api/BrowseCommandBuilder.java +++ b/scm-core/src/main/java/sonia/scm/repository/api/BrowseCommandBuilder.java @@ -199,7 +199,7 @@ public final class BrowseCommandBuilder return this; } - + /** * Disabling the last commit means that every call to * {@link FileObject#getDescription()} and @@ -300,6 +300,13 @@ public final class BrowseCommandBuilder return this; } + private void updateCache(BrowserResult updatedResult) { + if (!disableCache) { + CacheKey key = new CacheKey(repository, request); + cache.put(key, updatedResult); + } + } + //~--- inner classes -------------------------------------------------------- /** @@ -416,5 +423,5 @@ public final class BrowseCommandBuilder private final Repository repository; /** request for the command */ - private final BrowseCommandRequest request = new BrowseCommandRequest(); + private final BrowseCommandRequest request = new BrowseCommandRequest(this::updateCache); } diff --git a/scm-core/src/main/java/sonia/scm/repository/spi/BrowseCommandRequest.java b/scm-core/src/main/java/sonia/scm/repository/spi/BrowseCommandRequest.java index 39da9a9ace..0c82c71331 100644 --- a/scm-core/src/main/java/sonia/scm/repository/spi/BrowseCommandRequest.java +++ b/scm-core/src/main/java/sonia/scm/repository/spi/BrowseCommandRequest.java @@ -37,6 +37,10 @@ package sonia.scm.repository.spi; import com.google.common.base.MoreObjects; import com.google.common.base.Objects; +import sonia.scm.repository.BrowserResult; + +import java.util.function.Consumer; + /** * * @author Sebastian Sdorra @@ -48,6 +52,14 @@ public final class BrowseCommandRequest extends FileBaseCommandRequest /** Field description */ private static final long serialVersionUID = 7956624623516803183L; + public BrowseCommandRequest() { + this(null); + } + + public BrowseCommandRequest(Consumer updater) { + this.updater = updater; + } + //~--- methods -------------------------------------------------------------- /** @@ -220,6 +232,12 @@ public final class BrowseCommandRequest extends FileBaseCommandRequest return recursive; } + public void updateCache(BrowserResult update) { + if (updater != null) { + updater.accept(update); + } + } + //~--- fields --------------------------------------------------------------- /** disable last commit */ @@ -230,4 +248,6 @@ public final class BrowseCommandRequest extends FileBaseCommandRequest /** browse file objects recursive */ private boolean recursive = false; + + private final Consumer updater; } diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java index 753802d3eb..6b02ab5fd7 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java @@ -56,6 +56,7 @@ import sonia.scm.repository.BrowserResult; import sonia.scm.repository.FileObject; import sonia.scm.repository.GitSubModuleParser; import sonia.scm.repository.GitUtil; +import sonia.scm.repository.InternalRepositoryException; import sonia.scm.repository.Repository; import sonia.scm.repository.SubRepository; import sonia.scm.store.Blob; @@ -71,9 +72,7 @@ import java.util.Map; import java.util.Optional; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; -import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; import static java.util.Optional.empty; import static sonia.scm.ContextEntry.ContextBuilder.entity; @@ -99,7 +98,6 @@ public class GitBrowseCommand extends AbstractGitCommand LoggerFactory.getLogger(GitBrowseCommand.class); private final LfsBlobStoreFactory lfsBlobStoreFactory; - private boolean fetchCommits = true; private ExecutorService executorService; //~--- constructors --------------------------------------------------------- @@ -130,13 +128,25 @@ public class GitBrowseCommand extends AbstractGitCommand ObjectId revId = computeRevIdToBrowse(request, repo); if (revId != null) { - return new BrowserResult(revId.getName(), request.getRevision(), getEntry(repo, request, revId)); + BrowserResult browserResult = new BrowserResult(revId.getName(), request.getRevision(), getEntry(repo, request, revId)); + executorService.execute(() -> { + request.updateCache(browserResult); + logger.info("updated browser result for repository {}", repository.getNamespaceAndName()); + }); + return browserResult; } else { logger.warn("could not find head of repository {}, empty?", repository.getNamespaceAndName()); return new BrowserResult(Constants.HEAD, request.getRevision(), createEmtpyRoot()); } } finally { executorService.shutdown(); + try { + if (!executorService.awaitTermination(10, TimeUnit.SECONDS)) { + logger.info("lookup of all commits took too long in repo {}", repository.getNamespaceAndName()); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } } } @@ -209,46 +219,39 @@ public class GitBrowseCommand extends AbstractGitCommand // don't show message and date for directories to improve performance if (!file.isDirectory() &&!request.isDisableLastCommit()) { - RevCommit commit = null; - if (fetchCommits) { + executorService.execute(() -> { logger.trace("fetch last commit for {} at {}", path, revId.getName()); - Future f = executorService.submit(() -> getLatestCommit(repo, revId, path)); + RevCommit commit = getLatestCommit(repo, revId, path); + + Optional lfsPointer; try { - commit = f.get(100, TimeUnit.MILLISECONDS); - } catch (TimeoutException e) { - logger.info("lookup of latest commit took too much time for {} at {}; disabling lookup on {}", path, revId.name(), repository.getNamespaceAndName()); - fetchCommits = false; - } catch (Exception e) { - logger.warn("got exception while computing last commit for {} on {}", path, revId.name(), e); + lfsPointer = commit == null ? empty() : GitUtil.getLfsPointer(repo, path, commit, treeWalk); + } catch (IOException e) { + throw new InternalRepositoryException(repository, "could not read lfs pointer", e); } - } - Optional lfsPointer = commit == null? empty(): GitUtil.getLfsPointer(repo, path, commit, treeWalk); - - if (lfsPointer.isPresent()) { - BlobStore lfsBlobStore = lfsBlobStoreFactory.getLfsBlobStore(repository); - String oid = lfsPointer.get().getOid().getName(); - Blob blob = lfsBlobStore.get(oid); - if (blob == null) { - logger.error("lfs blob for lob id {} not found in lfs store of repository {}", oid, repository.getNamespaceAndName()); - file.setLength(-1); + if (lfsPointer.isPresent()) { + BlobStore lfsBlobStore = lfsBlobStoreFactory.getLfsBlobStore(repository); + String oid = lfsPointer.get().getOid().getName(); + Blob blob = lfsBlobStore.get(oid); + if (blob == null) { + logger.error("lfs blob for lob id {} not found in lfs store of repository {}", oid, repository.getNamespaceAndName()); + file.setLength(-1); + } else { + file.setLength(blob.getSize()); + } } else { - file.setLength(blob.getSize()); + file.setLength(loader.getSize()); } - } else { - file.setLength(loader.getSize()); - } - if (commit != null) - { - file.setLastModified(GitUtil.getCommitTime(commit)); - file.setDescription(commit.getShortMessage()); - } - else if (fetchCommits) - { - logger.warn("could not find latest commit for {} on {}", path, - revId); - } + if (commit != null) { + file.setLastModified(GitUtil.getCommitTime(commit)); + file.setDescription(commit.getShortMessage()); + } else { + logger.warn("could not find latest commit for {} on {}", path, + revId); + } + }); } } return file; diff --git a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBrowseCommandTest.java b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBrowseCommandTest.java index 4b854f6209..61d877bd4a 100644 --- a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBrowseCommandTest.java +++ b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBrowseCommandTest.java @@ -142,7 +142,7 @@ public class GitBrowseCommandTest extends AbstractGitCommandTestBase { } @Test - public void testRecusive() throws IOException { + public void testRecursive() throws IOException { BrowseCommandRequest request = new BrowseCommandRequest(); request.setRecursive(true); From 8d0249b70882870cee76efbd89dcf6b2c6a1cbc8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Tue, 10 Dec 2019 18:22:40 +0100 Subject: [PATCH 03/35] Make timeout configurable --- .../repository/api/BrowseCommandBuilder.java | 4 ++++ .../repository/spi/FileBaseCommandRequest.java | 17 ++++++++++++++++- .../scm/repository/spi/GitBrowseCommand.java | 6 ++---- 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/scm-core/src/main/java/sonia/scm/repository/api/BrowseCommandBuilder.java b/scm-core/src/main/java/sonia/scm/repository/api/BrowseCommandBuilder.java index 563557f0c1..437e3f5fa0 100644 --- a/scm-core/src/main/java/sonia/scm/repository/api/BrowseCommandBuilder.java +++ b/scm-core/src/main/java/sonia/scm/repository/api/BrowseCommandBuilder.java @@ -300,6 +300,10 @@ public final class BrowseCommandBuilder return this; } + public void setComputationTimeoutMilliSeconds(long computationTimeoutMilliSeconds) { + request.setComputationTimeoutMilliSeconds(computationTimeoutMilliSeconds); + } + private void updateCache(BrowserResult updatedResult) { if (!disableCache) { CacheKey key = new CacheKey(repository, request); diff --git a/scm-core/src/main/java/sonia/scm/repository/spi/FileBaseCommandRequest.java b/scm-core/src/main/java/sonia/scm/repository/spi/FileBaseCommandRequest.java index 9f563345fd..c6683905dd 100644 --- a/scm-core/src/main/java/sonia/scm/repository/spi/FileBaseCommandRequest.java +++ b/scm-core/src/main/java/sonia/scm/repository/spi/FileBaseCommandRequest.java @@ -147,6 +147,10 @@ public abstract class FileBaseCommandRequest this.revision = revision; } + public void setComputationTimeoutMilliSeconds(long computationTimeoutMilliSeconds) { + this.computationTimeoutMilliSeconds = computationTimeoutMilliSeconds; + } + //~--- get methods ---------------------------------------------------------- /** @@ -171,7 +175,14 @@ public abstract class FileBaseCommandRequest return revision; } - //~--- methods -------------------------------------------------------------- + public boolean isDisableCommitValues() { + return disableCommitValues; + } + + public long getComputationTimeoutMilliSeconds() { + return computationTimeoutMilliSeconds; + } +//~--- methods -------------------------------------------------------------- /** * Method description @@ -208,4 +219,8 @@ public abstract class FileBaseCommandRequest /** Field description */ private String revision; + + private boolean disableCommitValues = false; + + private long computationTimeoutMilliSeconds = 1000; } diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java index 6b02ab5fd7..a82a592497 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java @@ -141,8 +141,8 @@ public class GitBrowseCommand extends AbstractGitCommand } finally { executorService.shutdown(); try { - if (!executorService.awaitTermination(10, TimeUnit.SECONDS)) { - logger.info("lookup of all commits took too long in repo {}", repository.getNamespaceAndName()); + if (!executorService.awaitTermination(request.getComputationTimeoutMilliSeconds(), TimeUnit.MILLISECONDS)) { + logger.info("lookup of all commits aborted after {}ms in repo {}", request.getComputationTimeoutMilliSeconds(), repository.getNamespaceAndName()); } } catch (InterruptedException e) { Thread.currentThread().interrupt(); @@ -259,8 +259,6 @@ public class GitBrowseCommand extends AbstractGitCommand //~--- get methods ---------------------------------------------------------- -private RevWalk commitWalk = null; - /** * Method description * From ce15b116bd140f99d275766c7f562e40f714e0ef Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Wed, 11 Dec 2019 10:10:56 +0100 Subject: [PATCH 04/35] Introduce SyncAsyncExecutor --- .../scm/repository/spi/SyncAsyncExecutor.java | 43 +++++++++++++++++ .../spi/SyncAsyncExecutorProvider.java | 29 +++++++++++ .../scm/repository/spi/GitBrowseCommand.java | 48 +++++++------------ .../spi/GitRepositoryServiceProvider.java | 7 ++- .../spi/GitRepositoryServiceResolver.java | 6 ++- .../repository/spi/GitBrowseCommandTest.java | 3 +- .../repository/spi/SyncAsyncExecutors.java | 10 ++++ 7 files changed, 111 insertions(+), 35 deletions(-) create mode 100644 scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutor.java create mode 100644 scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutorProvider.java create mode 100644 scm-test/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutors.java diff --git a/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutor.java b/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutor.java new file mode 100644 index 0000000000..86d37814da --- /dev/null +++ b/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutor.java @@ -0,0 +1,43 @@ +package sonia.scm.repository.spi; + +import java.time.Instant; +import java.util.concurrent.Executor; +import java.util.function.Consumer; + +import static sonia.scm.repository.spi.SyncAsyncExecutor.ExecutionType.ASYNCHRONOUS; +import static sonia.scm.repository.spi.SyncAsyncExecutor.ExecutionType.SYNCHRONOUS; + +public class SyncAsyncExecutor { + + private final Executor executor; + private final Instant switchToAsyncTime; + private boolean executedAllSynchronously = true; + + SyncAsyncExecutor(Executor executor, Instant switchToAsyncTime) { + this.executor = executor; + this.switchToAsyncTime = switchToAsyncTime; + } + + public ExecutionType execute(Runnable runnable) { + return execute(ignored -> runnable.run()); + } + + public ExecutionType execute(Consumer runnable) { + if (Instant.now().isAfter(switchToAsyncTime)) { + executor.execute(() -> runnable.accept(ASYNCHRONOUS)); + executedAllSynchronously = false; + return ASYNCHRONOUS; + } else { + runnable.accept(SYNCHRONOUS); + return SYNCHRONOUS; + } + } + + public boolean hasExecutedAllSynchronously() { + return executedAllSynchronously; + } + + public enum ExecutionType { + SYNCHRONOUS, ASYNCHRONOUS + } +} diff --git a/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutorProvider.java b/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutorProvider.java new file mode 100644 index 0000000000..2c576f814f --- /dev/null +++ b/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutorProvider.java @@ -0,0 +1,29 @@ +package sonia.scm.repository.spi; + +import java.time.Instant; +import java.time.temporal.ChronoUnit; +import java.util.concurrent.Executor; +import java.util.concurrent.Executors; + +public class SyncAsyncExecutorProvider { + + private static final int DEFAULT_TIMEOUT_SECONDS = 2; + + private final Executor executor; + + public SyncAsyncExecutorProvider() { + this(Executors.newFixedThreadPool(4)); + } + + public SyncAsyncExecutorProvider(Executor executor) { + this.executor = executor; + } + + public SyncAsyncExecutor createExecutorWithDefaultTimeout() { + return createExecutorWithSecondsToTimeout(DEFAULT_TIMEOUT_SECONDS); + } + + public SyncAsyncExecutor createExecutorWithSecondsToTimeout(int seconds) { + return new SyncAsyncExecutor(executor, Instant.now().plus(seconds, ChronoUnit.SECONDS)); + } +} diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java index a82a592497..670b43dd5c 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java @@ -70,9 +70,6 @@ import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Optional; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; -import java.util.concurrent.TimeUnit; import static java.util.Optional.empty; import static sonia.scm.ContextEntry.ContextBuilder.entity; @@ -98,55 +95,46 @@ public class GitBrowseCommand extends AbstractGitCommand LoggerFactory.getLogger(GitBrowseCommand.class); private final LfsBlobStoreFactory lfsBlobStoreFactory; - private ExecutorService executorService; + private final SyncAsyncExecutor executor; //~--- constructors --------------------------------------------------------- /** * Constructs ... - * @param context + * @param context * @param repository * @param lfsBlobStoreFactory + * @param executor */ - public GitBrowseCommand(GitContext context, Repository repository, LfsBlobStoreFactory lfsBlobStoreFactory) + public GitBrowseCommand(GitContext context, Repository repository, LfsBlobStoreFactory lfsBlobStoreFactory, SyncAsyncExecutor executor) { super(context, repository); this.lfsBlobStoreFactory = lfsBlobStoreFactory; + this.executor = executor; } //~--- get methods ---------------------------------------------------------- @Override - @SuppressWarnings("unchecked") public BrowserResult getBrowserResult(BrowseCommandRequest request) throws IOException { - executorService = Executors.newSingleThreadExecutor(); - try { - logger.debug("try to create browse result for {}", request); + logger.debug("try to create browse result for {}", request); - org.eclipse.jgit.lib.Repository repo = open(); - ObjectId revId = computeRevIdToBrowse(request, repo); + org.eclipse.jgit.lib.Repository repo = open(); + ObjectId revId = computeRevIdToBrowse(request, repo); - if (revId != null) { - BrowserResult browserResult = new BrowserResult(revId.getName(), request.getRevision(), getEntry(repo, request, revId)); - executorService.execute(() -> { + if (revId != null) { + BrowserResult browserResult = new BrowserResult(revId.getName(), request.getRevision(), getEntry(repo, request, revId)); + executor.execute(executionType -> { + if (executionType == SyncAsyncExecutor.ExecutionType.ASYNCHRONOUS) { request.updateCache(browserResult); logger.info("updated browser result for repository {}", repository.getNamespaceAndName()); - }); - return browserResult; - } else { - logger.warn("could not find head of repository {}, empty?", repository.getNamespaceAndName()); - return new BrowserResult(Constants.HEAD, request.getRevision(), createEmtpyRoot()); - } - } finally { - executorService.shutdown(); - try { - if (!executorService.awaitTermination(request.getComputationTimeoutMilliSeconds(), TimeUnit.MILLISECONDS)) { - logger.info("lookup of all commits aborted after {}ms in repo {}", request.getComputationTimeoutMilliSeconds(), repository.getNamespaceAndName()); } - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - } + }); + return browserResult; + } else { + logger.warn("could not find head of repository {}, empty?", repository.getNamespaceAndName()); + return new BrowserResult(Constants.HEAD, request.getRevision(), createEmtpyRoot()); } } @@ -219,7 +207,7 @@ public class GitBrowseCommand extends AbstractGitCommand // don't show message and date for directories to improve performance if (!file.isDirectory() &&!request.isDisableLastCommit()) { - executorService.execute(() -> { + executor.execute(() -> { logger.trace("fetch last commit for {} at {}", path, revId.getName()); RevCommit commit = getLatestCommit(repo, revId, path); diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitRepositoryServiceProvider.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitRepositoryServiceProvider.java index e1ae58ada5..fa54ca6007 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitRepositoryServiceProvider.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitRepositoryServiceProvider.java @@ -80,12 +80,13 @@ public class GitRepositoryServiceProvider extends RepositoryServiceProvider //~--- constructors --------------------------------------------------------- - public GitRepositoryServiceProvider(GitRepositoryHandler handler, Repository repository, GitRepositoryConfigStoreProvider storeProvider, LfsBlobStoreFactory lfsBlobStoreFactory, HookContextFactory hookContextFactory, ScmEventBus eventBus) { + public GitRepositoryServiceProvider(GitRepositoryHandler handler, Repository repository, GitRepositoryConfigStoreProvider storeProvider, LfsBlobStoreFactory lfsBlobStoreFactory, HookContextFactory hookContextFactory, ScmEventBus eventBus, SyncAsyncExecutorProvider executorProvider) { this.handler = handler; this.repository = repository; this.lfsBlobStoreFactory = lfsBlobStoreFactory; this.hookContextFactory = hookContextFactory; this.eventBus = eventBus; + this.executorProvider = executorProvider; this.context = new GitContext(handler.getDirectory(repository.getId()), repository, storeProvider); } @@ -150,7 +151,7 @@ public class GitRepositoryServiceProvider extends RepositoryServiceProvider @Override public BrowseCommand getBrowseCommand() { - return new GitBrowseCommand(context, repository, lfsBlobStoreFactory); + return new GitBrowseCommand(context, repository, lfsBlobStoreFactory, executorProvider.createExecutorWithDefaultTimeout()); } /** @@ -301,4 +302,6 @@ public class GitRepositoryServiceProvider extends RepositoryServiceProvider private final HookContextFactory hookContextFactory; private final ScmEventBus eventBus; + + private final SyncAsyncExecutorProvider executorProvider; } diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitRepositoryServiceResolver.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitRepositoryServiceResolver.java index 7fc5fb27c4..1bb7e84b92 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitRepositoryServiceResolver.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitRepositoryServiceResolver.java @@ -55,14 +55,16 @@ public class GitRepositoryServiceResolver implements RepositoryServiceResolver { private final LfsBlobStoreFactory lfsBlobStoreFactory; private final HookContextFactory hookContextFactory; private final ScmEventBus eventBus; + private final SyncAsyncExecutorProvider executorProvider; @Inject - public GitRepositoryServiceResolver(GitRepositoryHandler handler, GitRepositoryConfigStoreProvider storeProvider, LfsBlobStoreFactory lfsBlobStoreFactory, HookContextFactory hookContextFactory, ScmEventBus eventBus) { + public GitRepositoryServiceResolver(GitRepositoryHandler handler, GitRepositoryConfigStoreProvider storeProvider, LfsBlobStoreFactory lfsBlobStoreFactory, HookContextFactory hookContextFactory, ScmEventBus eventBus, SyncAsyncExecutorProvider executorProvider) { this.handler = handler; this.storeProvider = storeProvider; this.lfsBlobStoreFactory = lfsBlobStoreFactory; this.hookContextFactory = hookContextFactory; this.eventBus = eventBus; + this.executorProvider = executorProvider; } @Override @@ -70,7 +72,7 @@ public class GitRepositoryServiceResolver implements RepositoryServiceResolver { GitRepositoryServiceProvider provider = null; if (GitRepositoryHandler.TYPE_NAME.equalsIgnoreCase(repository.getType())) { - provider = new GitRepositoryServiceProvider(handler, repository, storeProvider, lfsBlobStoreFactory, hookContextFactory, eventBus); + provider = new GitRepositoryServiceProvider(handler, repository, storeProvider, lfsBlobStoreFactory, hookContextFactory, eventBus, executorProvider); } return provider; diff --git a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBrowseCommandTest.java b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBrowseCommandTest.java index 61d877bd4a..40dfdf2159 100644 --- a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBrowseCommandTest.java +++ b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBrowseCommandTest.java @@ -44,6 +44,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; +import static sonia.scm.repository.spi.SyncAsyncExecutors.synchronousExecutor; /** * Unit tests for {@link GitBrowseCommand}. @@ -171,6 +172,6 @@ public class GitBrowseCommandTest extends AbstractGitCommandTestBase { } private GitBrowseCommand createCommand() { - return new GitBrowseCommand(createContext(), repository, null); + return new GitBrowseCommand(createContext(), repository, null, synchronousExecutor()); } } diff --git a/scm-test/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutors.java b/scm-test/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutors.java new file mode 100644 index 0000000000..9f64a39474 --- /dev/null +++ b/scm-test/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutors.java @@ -0,0 +1,10 @@ +package sonia.scm.repository.spi; + +import java.time.Instant; + +public final class SyncAsyncExecutors { + + public static SyncAsyncExecutor synchronousExecutor() { + return new SyncAsyncExecutor(Runnable::run, Instant.MAX); + } +} From 9a8f0a4ee742a380dda186188d0107dcb6823c93 Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Wed, 11 Dec 2019 12:42:21 +0100 Subject: [PATCH 05/35] Use interfaces for executor classes --- .../scm/repository/spi/SyncAsyncExecutor.java | 37 +++-------------- .../spi/SyncAsyncExecutorProvider.java | 25 ++--------- .../modules/ApplicationModuleProvider.java | 2 + .../repository/DefaultSyncAsyncExecutor.java | 41 +++++++++++++++++++ .../DefaultSyncAsyncExecutorProvider.java | 32 +++++++++++++++ .../sonia/scm/repository/ExecutorModule.java | 12 ++++++ 6 files changed, 96 insertions(+), 53 deletions(-) create mode 100644 scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutor.java create mode 100644 scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutorProvider.java create mode 100644 scm-webapp/src/main/java/sonia/scm/repository/ExecutorModule.java diff --git a/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutor.java b/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutor.java index 86d37814da..8ba7af3155 100644 --- a/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutor.java +++ b/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutor.java @@ -1,43 +1,16 @@ package sonia.scm.repository.spi; -import java.time.Instant; -import java.util.concurrent.Executor; import java.util.function.Consumer; -import static sonia.scm.repository.spi.SyncAsyncExecutor.ExecutionType.ASYNCHRONOUS; -import static sonia.scm.repository.spi.SyncAsyncExecutor.ExecutionType.SYNCHRONOUS; +public interface SyncAsyncExecutor { -public class SyncAsyncExecutor { + ExecutionType execute(Runnable runnable); - private final Executor executor; - private final Instant switchToAsyncTime; - private boolean executedAllSynchronously = true; + ExecutionType execute(Consumer runnable); - SyncAsyncExecutor(Executor executor, Instant switchToAsyncTime) { - this.executor = executor; - this.switchToAsyncTime = switchToAsyncTime; - } + boolean hasExecutedAllSynchronously(); - public ExecutionType execute(Runnable runnable) { - return execute(ignored -> runnable.run()); - } - - public ExecutionType execute(Consumer runnable) { - if (Instant.now().isAfter(switchToAsyncTime)) { - executor.execute(() -> runnable.accept(ASYNCHRONOUS)); - executedAllSynchronously = false; - return ASYNCHRONOUS; - } else { - runnable.accept(SYNCHRONOUS); - return SYNCHRONOUS; - } - } - - public boolean hasExecutedAllSynchronously() { - return executedAllSynchronously; - } - - public enum ExecutionType { + enum ExecutionType { SYNCHRONOUS, ASYNCHRONOUS } } diff --git a/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutorProvider.java b/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutorProvider.java index 2c576f814f..2d21129eb2 100644 --- a/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutorProvider.java +++ b/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutorProvider.java @@ -1,29 +1,12 @@ package sonia.scm.repository.spi; -import java.time.Instant; -import java.time.temporal.ChronoUnit; -import java.util.concurrent.Executor; -import java.util.concurrent.Executors; +public interface SyncAsyncExecutorProvider { -public class SyncAsyncExecutorProvider { + int DEFAULT_TIMEOUT_SECONDS = 2; - private static final int DEFAULT_TIMEOUT_SECONDS = 2; - - private final Executor executor; - - public SyncAsyncExecutorProvider() { - this(Executors.newFixedThreadPool(4)); - } - - public SyncAsyncExecutorProvider(Executor executor) { - this.executor = executor; - } - - public SyncAsyncExecutor createExecutorWithDefaultTimeout() { + default SyncAsyncExecutor createExecutorWithDefaultTimeout() { return createExecutorWithSecondsToTimeout(DEFAULT_TIMEOUT_SECONDS); } - public SyncAsyncExecutor createExecutorWithSecondsToTimeout(int seconds) { - return new SyncAsyncExecutor(executor, Instant.now().plus(seconds, ChronoUnit.SECONDS)); - } + SyncAsyncExecutor createExecutorWithSecondsToTimeout(int seconds); } diff --git a/scm-webapp/src/main/java/sonia/scm/lifecycle/modules/ApplicationModuleProvider.java b/scm-webapp/src/main/java/sonia/scm/lifecycle/modules/ApplicationModuleProvider.java index 62793da0f6..d5ccb73c7d 100644 --- a/scm-webapp/src/main/java/sonia/scm/lifecycle/modules/ApplicationModuleProvider.java +++ b/scm-webapp/src/main/java/sonia/scm/lifecycle/modules/ApplicationModuleProvider.java @@ -12,6 +12,7 @@ import sonia.scm.debug.DebugModule; import sonia.scm.filter.WebElementModule; import sonia.scm.plugin.ExtensionProcessor; import sonia.scm.plugin.PluginLoader; +import sonia.scm.repository.ExecutorModule; import javax.servlet.ServletContext; import java.util.ArrayList; @@ -51,6 +52,7 @@ public class ApplicationModuleProvider implements ModuleProvider { moduleList.add(new DebugModule()); } moduleList.add(new MapperModule()); + moduleList.add(new ExecutorModule()); return moduleList; } diff --git a/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutor.java b/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutor.java new file mode 100644 index 0000000000..232435aec5 --- /dev/null +++ b/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutor.java @@ -0,0 +1,41 @@ +package sonia.scm.repository; + +import sonia.scm.repository.spi.SyncAsyncExecutor; + +import java.time.Instant; +import java.util.concurrent.Executor; +import java.util.function.Consumer; + +import static sonia.scm.repository.spi.SyncAsyncExecutor.ExecutionType.ASYNCHRONOUS; +import static sonia.scm.repository.spi.SyncAsyncExecutor.ExecutionType.SYNCHRONOUS; + +public class DefaultSyncAsyncExecutor implements SyncAsyncExecutor { + + private final Executor executor; + private final Instant switchToAsyncTime; + private boolean executedAllSynchronously = true; + + DefaultSyncAsyncExecutor(Executor executor, Instant switchToAsyncTime) { + this.executor = executor; + this.switchToAsyncTime = switchToAsyncTime; + } + + public ExecutionType execute(Runnable runnable) { + return execute(ignored -> runnable.run()); + } + + public ExecutionType execute(Consumer runnable) { + if (Instant.now().isAfter(switchToAsyncTime)) { + executor.execute(() -> runnable.accept(ASYNCHRONOUS)); + executedAllSynchronously = false; + return ASYNCHRONOUS; + } else { + runnable.accept(SYNCHRONOUS); + return SYNCHRONOUS; + } + } + + public boolean hasExecutedAllSynchronously() { + return executedAllSynchronously; + } +} diff --git a/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutorProvider.java b/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutorProvider.java new file mode 100644 index 0000000000..0be7d38234 --- /dev/null +++ b/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutorProvider.java @@ -0,0 +1,32 @@ +package sonia.scm.repository; + +import sonia.scm.repository.spi.SyncAsyncExecutor; +import sonia.scm.repository.spi.SyncAsyncExecutorProvider; + +import java.io.Closeable; +import java.time.Instant; +import java.time.temporal.ChronoUnit; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; + +public class DefaultSyncAsyncExecutorProvider implements SyncAsyncExecutorProvider, Closeable { + + private final ExecutorService executor; + + public DefaultSyncAsyncExecutorProvider() { + this(Executors.newFixedThreadPool(4)); + } + + public DefaultSyncAsyncExecutorProvider(ExecutorService executor) { + this.executor = executor; + } + + public SyncAsyncExecutor createExecutorWithSecondsToTimeout(int seconds) { + return new DefaultSyncAsyncExecutor(executor, Instant.now().plus(seconds, ChronoUnit.SECONDS)); + } + + @Override + public void close() { + executor.shutdownNow(); + } +} diff --git a/scm-webapp/src/main/java/sonia/scm/repository/ExecutorModule.java b/scm-webapp/src/main/java/sonia/scm/repository/ExecutorModule.java new file mode 100644 index 0000000000..4e494b6ba6 --- /dev/null +++ b/scm-webapp/src/main/java/sonia/scm/repository/ExecutorModule.java @@ -0,0 +1,12 @@ +package sonia.scm.repository; + +import com.google.inject.AbstractModule; +import sonia.scm.lifecycle.modules.CloseableModule; +import sonia.scm.repository.spi.SyncAsyncExecutorProvider; + +public class ExecutorModule extends AbstractModule { + @Override + protected void configure() { + bind(SyncAsyncExecutorProvider.class).to(DefaultSyncAsyncExecutorProvider.class); + } +} From a37df2c20b396611f36fde20cd9738330422e51b Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Wed, 11 Dec 2019 12:58:01 +0100 Subject: [PATCH 06/35] Fix test helper --- .../scm/repository/spi/SyncAsyncExecutor.java | 4 +++- .../scm/repository/spi/SyncAsyncExecutors.java | 17 +++++++++++++++-- .../repository/DefaultSyncAsyncExecutor.java | 4 ---- 3 files changed, 18 insertions(+), 7 deletions(-) diff --git a/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutor.java b/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutor.java index 8ba7af3155..55fdbcacdb 100644 --- a/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutor.java +++ b/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutor.java @@ -4,7 +4,9 @@ import java.util.function.Consumer; public interface SyncAsyncExecutor { - ExecutionType execute(Runnable runnable); + default ExecutionType execute(Runnable runnable) { + return execute(ignored -> runnable.run()); + } ExecutionType execute(Consumer runnable); diff --git a/scm-test/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutors.java b/scm-test/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutors.java index 9f64a39474..15a1bef046 100644 --- a/scm-test/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutors.java +++ b/scm-test/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutors.java @@ -1,10 +1,23 @@ package sonia.scm.repository.spi; -import java.time.Instant; +import java.util.function.Consumer; + +import static sonia.scm.repository.spi.SyncAsyncExecutor.ExecutionType.SYNCHRONOUS; public final class SyncAsyncExecutors { public static SyncAsyncExecutor synchronousExecutor() { - return new SyncAsyncExecutor(Runnable::run, Instant.MAX); + return new SyncAsyncExecutor() { + @Override + public ExecutionType execute(Consumer runnable) { + runnable.accept(SYNCHRONOUS); + return SYNCHRONOUS; + } + + @Override + public boolean hasExecutedAllSynchronously() { + return true; + } + }; } } diff --git a/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutor.java b/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutor.java index 232435aec5..326b329218 100644 --- a/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutor.java +++ b/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutor.java @@ -20,10 +20,6 @@ public class DefaultSyncAsyncExecutor implements SyncAsyncExecutor { this.switchToAsyncTime = switchToAsyncTime; } - public ExecutionType execute(Runnable runnable) { - return execute(ignored -> runnable.run()); - } - public ExecutionType execute(Consumer runnable) { if (Instant.now().isAfter(switchToAsyncTime)) { executor.execute(() -> runnable.accept(ASYNCHRONOUS)); From 7c0eb9251ae60c1e681c5b6197f89fa11598a10a Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Wed, 11 Dec 2019 13:00:07 +0100 Subject: [PATCH 07/35] Add unit test --- .../DefaultSyncAsyncExecutorTest.java | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 scm-webapp/src/test/java/sonia/scm/repository/DefaultSyncAsyncExecutorTest.java diff --git a/scm-webapp/src/test/java/sonia/scm/repository/DefaultSyncAsyncExecutorTest.java b/scm-webapp/src/test/java/sonia/scm/repository/DefaultSyncAsyncExecutorTest.java new file mode 100644 index 0000000000..bd4148606b --- /dev/null +++ b/scm-webapp/src/test/java/sonia/scm/repository/DefaultSyncAsyncExecutorTest.java @@ -0,0 +1,38 @@ +package sonia.scm.repository; + +import org.junit.jupiter.api.Test; +import sonia.scm.repository.spi.SyncAsyncExecutor.ExecutionType; + +import java.time.Instant; + +import static java.time.temporal.ChronoUnit.MILLIS; +import static org.assertj.core.api.Assertions.assertThat; +import static sonia.scm.repository.spi.SyncAsyncExecutor.ExecutionType.ASYNCHRONOUS; +import static sonia.scm.repository.spi.SyncAsyncExecutor.ExecutionType.SYNCHRONOUS; + +class DefaultSyncAsyncExecutorTest { + + ExecutionType calledWithType = null; + + @Test + void shouldExecuteSynchronouslyBeforeTimeout() { + DefaultSyncAsyncExecutor executor = new DefaultSyncAsyncExecutor(Runnable::run, Instant.MAX); + + ExecutionType result = executor.execute(type -> calledWithType = type); + + assertThat(result).isEqualTo(SYNCHRONOUS); + assertThat(calledWithType).isEqualTo(SYNCHRONOUS); + assertThat(executor.hasExecutedAllSynchronously()).isTrue(); + } + + @Test + void shouldExecuteAsynchronouslyAfterTimeout() { + DefaultSyncAsyncExecutor executor = new DefaultSyncAsyncExecutor(Runnable::run, Instant.now().minus(1, MILLIS)); + + ExecutionType result = executor.execute(type -> calledWithType = type); + + assertThat(result).isEqualTo(ASYNCHRONOUS); + assertThat(calledWithType).isEqualTo(ASYNCHRONOUS); + assertThat(executor.hasExecutedAllSynchronously()).isFalse(); + } +} From 093c0abb0213e08fd7de30105e8acbc6feafd81a Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Wed, 11 Dec 2019 13:32:05 +0100 Subject: [PATCH 08/35] Add unit test for asynchronous browse --- .../repository/spi/GitBrowseCommandTest.java | 25 +++++++ .../repository/spi/SyncAsyncExecutors.java | 71 +++++++++++++++++++ 2 files changed, 96 insertions(+) diff --git a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBrowseCommandTest.java b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBrowseCommandTest.java index 40dfdf2159..0e36c39499 100644 --- a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBrowseCommandTest.java +++ b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBrowseCommandTest.java @@ -35,6 +35,7 @@ import org.junit.Test; import sonia.scm.repository.BrowserResult; import sonia.scm.repository.FileObject; import sonia.scm.repository.GitRepositoryConfig; +import sonia.scm.repository.spi.SyncAsyncExecutors.AsyncExecutorStepper; import java.io.IOException; import java.util.Collection; @@ -43,7 +44,9 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; +import static sonia.scm.repository.spi.SyncAsyncExecutors.stepperAsynchronousExecutor; import static sonia.scm.repository.spi.SyncAsyncExecutors.synchronousExecutor; /** @@ -112,6 +115,28 @@ public class GitBrowseCommandTest extends AbstractGitCommandTestBase { assertEquals("c", c.getPath()); } + @Test + public void testAsynchronousBrowse() throws IOException { + try (AsyncExecutorStepper executor = stepperAsynchronousExecutor()) { + GitBrowseCommand command = new GitBrowseCommand(createContext(), repository, null, executor); + FileObject root = command.getBrowserResult(new BrowseCommandRequest()).getFile(); + assertNotNull(root); + + Collection foList = root.getChildren(); + + FileObject a = findFile(foList, "a.txt"); + + assertFalse(a.isDirectory()); + assertNull("expected empty name before commit could have been read", a.getDescription()); + assertNull("expected empty date before commit could have been read", a.getLastModified()); + + executor.next(); + + assertNotNull("expected correct name after commit could have been read", a.getDescription()); + assertNotNull("expected correct date after commit could have been read", a.getLastModified()); + } + } + @Test public void testBrowseSubDirectory() throws IOException { BrowseCommandRequest request = new BrowseCommandRequest(); diff --git a/scm-test/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutors.java b/scm-test/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutors.java index 15a1bef046..9dbe26403b 100644 --- a/scm-test/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutors.java +++ b/scm-test/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutors.java @@ -1,7 +1,12 @@ package sonia.scm.repository.spi; +import java.io.Closeable; +import java.util.concurrent.Executor; +import java.util.concurrent.Executors; +import java.util.concurrent.Semaphore; import java.util.function.Consumer; +import static sonia.scm.repository.spi.SyncAsyncExecutor.ExecutionType.ASYNCHRONOUS; import static sonia.scm.repository.spi.SyncAsyncExecutor.ExecutionType.SYNCHRONOUS; public final class SyncAsyncExecutors { @@ -20,4 +25,70 @@ public final class SyncAsyncExecutors { } }; } + + public static SyncAsyncExecutor asynchronousExecutor() { + + Executor executor = Executors.newSingleThreadExecutor(); + + return new SyncAsyncExecutor() { + @Override + public ExecutionType execute(Consumer runnable) { + executor.execute(() -> runnable.accept(ASYNCHRONOUS)); + return ASYNCHRONOUS; + } + + @Override + public boolean hasExecutedAllSynchronously() { + return true; + } + }; + } + + public static AsyncExecutorStepper stepperAsynchronousExecutor() { + + Executor executor = Executors.newSingleThreadExecutor(); + Semaphore enterSemaphore = new Semaphore(0); + Semaphore exitSemaphore = new Semaphore(0); + + return new AsyncExecutorStepper() { + @Override + public void close() { + enterSemaphore.release(Integer.MAX_VALUE); + exitSemaphore.release(Integer.MAX_VALUE); + } + + @Override + public ExecutionType execute(Consumer runnable) { + executor.execute(() -> { + try { + enterSemaphore.acquire(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + runnable.accept(ASYNCHRONOUS); + exitSemaphore.release(); + }); + return ASYNCHRONOUS; + } + + @Override + public void next() { + enterSemaphore.release(); + try { + exitSemaphore.acquire(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } + + @Override + public boolean hasExecutedAllSynchronously() { + return true; + } + }; + } + + public interface AsyncExecutorStepper extends SyncAsyncExecutor, Closeable { + void next(); + } } From 9caf6c09846f07146adfc00378d9fc49ccddb3ed Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Wed, 11 Dec 2019 14:01:17 +0100 Subject: [PATCH 09/35] Update cache after each file --- .../scm/repository/spi/GitBrowseCommand.java | 13 ++++++++++--- .../repository/spi/GitBrowseCommandTest.java | 19 +++++++++++++++++-- .../repository/spi/SyncAsyncExecutors.java | 4 ++-- 3 files changed, 29 insertions(+), 7 deletions(-) diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java index 670b43dd5c..bdeb425d15 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java @@ -74,6 +74,7 @@ import java.util.Optional; import static java.util.Optional.empty; import static sonia.scm.ContextEntry.ContextBuilder.entity; import static sonia.scm.NotFoundException.notFound; +import static sonia.scm.repository.spi.SyncAsyncExecutor.ExecutionType.ASYNCHRONOUS; //~--- JDK imports ------------------------------------------------------------ @@ -97,6 +98,8 @@ public class GitBrowseCommand extends AbstractGitCommand private final SyncAsyncExecutor executor; + private BrowserResult browserResult; + //~--- constructors --------------------------------------------------------- /** @@ -124,9 +127,9 @@ public class GitBrowseCommand extends AbstractGitCommand ObjectId revId = computeRevIdToBrowse(request, repo); if (revId != null) { - BrowserResult browserResult = new BrowserResult(revId.getName(), request.getRevision(), getEntry(repo, request, revId)); + browserResult = new BrowserResult(revId.getName(), request.getRevision(), getEntry(repo, request, revId)); executor.execute(executionType -> { - if (executionType == SyncAsyncExecutor.ExecutionType.ASYNCHRONOUS) { + if (executionType == ASYNCHRONOUS) { request.updateCache(browserResult); logger.info("updated browser result for repository {}", repository.getNamespaceAndName()); } @@ -207,7 +210,7 @@ public class GitBrowseCommand extends AbstractGitCommand // don't show message and date for directories to improve performance if (!file.isDirectory() &&!request.isDisableLastCommit()) { - executor.execute(() -> { + executor.execute(executionType -> { logger.trace("fetch last commit for {} at {}", path, revId.getName()); RevCommit commit = getLatestCommit(repo, revId, path); @@ -235,6 +238,10 @@ public class GitBrowseCommand extends AbstractGitCommand if (commit != null) { file.setLastModified(GitUtil.getCommitTime(commit)); file.setDescription(commit.getShortMessage()); + if (executionType == ASYNCHRONOUS && browserResult != null) { + request.updateCache(browserResult); + logger.info("updated browser result for repository {}", repository.getNamespaceAndName()); + } } else { logger.warn("could not find latest commit for {} on {}", path, revId); diff --git a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBrowseCommandTest.java b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBrowseCommandTest.java index 0e36c39499..03f1e761c7 100644 --- a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBrowseCommandTest.java +++ b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBrowseCommandTest.java @@ -39,6 +39,8 @@ import sonia.scm.repository.spi.SyncAsyncExecutors.AsyncExecutorStepper; import java.io.IOException; import java.util.Collection; +import java.util.LinkedList; +import java.util.List; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.assertEquals; @@ -119,21 +121,34 @@ public class GitBrowseCommandTest extends AbstractGitCommandTestBase { public void testAsynchronousBrowse() throws IOException { try (AsyncExecutorStepper executor = stepperAsynchronousExecutor()) { GitBrowseCommand command = new GitBrowseCommand(createContext(), repository, null, executor); - FileObject root = command.getBrowserResult(new BrowseCommandRequest()).getFile(); + List updatedResults = new LinkedList<>(); + BrowseCommandRequest request = new BrowseCommandRequest(updatedResults::add); + FileObject root = command.getBrowserResult(request).getFile(); assertNotNull(root); Collection foList = root.getChildren(); FileObject a = findFile(foList, "a.txt"); + FileObject b = findFile(foList, "b.txt"); - assertFalse(a.isDirectory()); assertNull("expected empty name before commit could have been read", a.getDescription()); assertNull("expected empty date before commit could have been read", a.getLastModified()); + assertNull("expected empty name before commit could have been read", b.getDescription()); + assertNull("expected empty date before commit could have been read", b.getLastModified()); executor.next(); + assertEquals(1, updatedResults.size()); assertNotNull("expected correct name after commit could have been read", a.getDescription()); assertNotNull("expected correct date after commit could have been read", a.getLastModified()); + assertNull("expected empty name before commit could have been read", b.getDescription()); + assertNull("expected empty date before commit could have been read", b.getLastModified()); + + executor.next(); + + assertEquals(2, updatedResults.size()); + assertNotNull("expected correct name after commit could have been read", b.getDescription()); + assertNotNull("expected correct date after commit could have been read", b.getLastModified()); } } diff --git a/scm-test/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutors.java b/scm-test/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutors.java index 9dbe26403b..aa7e7bb954 100644 --- a/scm-test/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutors.java +++ b/scm-test/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutors.java @@ -53,8 +53,8 @@ public final class SyncAsyncExecutors { return new AsyncExecutorStepper() { @Override public void close() { - enterSemaphore.release(Integer.MAX_VALUE); - exitSemaphore.release(Integer.MAX_VALUE); + enterSemaphore.release(Integer.MAX_VALUE/2); + exitSemaphore.release(Integer.MAX_VALUE/2); } @Override From 8129f2fad05746305c0b7f2806dffb5deb987a9d Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Wed, 11 Dec 2019 14:41:42 +0100 Subject: [PATCH 10/35] Add 'partial' flag to files --- .../src/main/java/sonia/scm/repository/FileObject.java | 10 ++++++++++ .../sonia/scm/repository/spi/GitBrowseCommand.java | 2 ++ .../sonia/scm/repository/spi/GitBrowseCommandTest.java | 5 +++++ 3 files changed, 17 insertions(+) diff --git a/scm-core/src/main/java/sonia/scm/repository/FileObject.java b/scm-core/src/main/java/sonia/scm/repository/FileObject.java index 7dedebb13a..2f3241bf40 100644 --- a/scm-core/src/main/java/sonia/scm/repository/FileObject.java +++ b/scm-core/src/main/java/sonia/scm/repository/FileObject.java @@ -222,6 +222,10 @@ public class FileObject implements LastModifiedAware, Serializable return directory; } + public boolean isPartialResult() { + return partialResult; + } + //~--- set methods ---------------------------------------------------------- /** @@ -302,6 +306,10 @@ public class FileObject implements LastModifiedAware, Serializable this.subRepository = subRepository; } + public void setPartialResult(boolean partialResult) { + this.partialResult = partialResult; + } + public Collection getChildren() { return unmodifiableCollection(children); } @@ -338,6 +346,8 @@ public class FileObject implements LastModifiedAware, Serializable /** file path */ private String path; + private boolean partialResult = false; + /** sub repository informations */ @XmlElement(name = "subrepository") private SubRepository subRepository; diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java index bdeb425d15..06b724d820 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java @@ -210,6 +210,7 @@ public class GitBrowseCommand extends AbstractGitCommand // don't show message and date for directories to improve performance if (!file.isDirectory() &&!request.isDisableLastCommit()) { + file.setPartialResult(true); executor.execute(executionType -> { logger.trace("fetch last commit for {} at {}", path, revId.getName()); RevCommit commit = getLatestCommit(repo, revId, path); @@ -235,6 +236,7 @@ public class GitBrowseCommand extends AbstractGitCommand file.setLength(loader.getSize()); } + file.setPartialResult(false); if (commit != null) { file.setLastModified(GitUtil.getCommitTime(commit)); file.setDescription(commit.getShortMessage()); diff --git a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBrowseCommandTest.java b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBrowseCommandTest.java index 03f1e761c7..9b45cda16f 100644 --- a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBrowseCommandTest.java +++ b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBrowseCommandTest.java @@ -131,22 +131,27 @@ public class GitBrowseCommandTest extends AbstractGitCommandTestBase { FileObject a = findFile(foList, "a.txt"); FileObject b = findFile(foList, "b.txt"); + assertTrue(a.isPartialResult()); assertNull("expected empty name before commit could have been read", a.getDescription()); assertNull("expected empty date before commit could have been read", a.getLastModified()); + assertTrue(b.isPartialResult()); assertNull("expected empty name before commit could have been read", b.getDescription()); assertNull("expected empty date before commit could have been read", b.getLastModified()); executor.next(); assertEquals(1, updatedResults.size()); + assertFalse(a.isPartialResult()); assertNotNull("expected correct name after commit could have been read", a.getDescription()); assertNotNull("expected correct date after commit could have been read", a.getLastModified()); + assertTrue(b.isPartialResult()); assertNull("expected empty name before commit could have been read", b.getDescription()); assertNull("expected empty date before commit could have been read", b.getLastModified()); executor.next(); assertEquals(2, updatedResults.size()); + assertFalse(b.isPartialResult()); assertNotNull("expected correct name after commit could have been read", b.getDescription()); assertNotNull("expected correct date after commit could have been read", b.getLastModified()); } From 58cff0797b215c8f236e819a518a068fa45dea9b Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Wed, 11 Dec 2019 15:09:53 +0100 Subject: [PATCH 11/35] Remove redundant mapper FileObjectToFileObjectDtoMapper#map and BrowserResultToFileObjectDtoMapper#fileObjectToDto had the same mapstruct implementation. --- .../BrowserResultToFileObjectDtoMapper.java | 15 +-- .../FileObjectToFileObjectDtoMapper.java | 22 ---- .../scm/api/v2/resources/MapperModule.java | 1 - ...rowserResultToFileObjectDtoMapperTest.java | 5 - .../FileObjectToFileObjectDtoMapperTest.java | 121 ------------------ .../v2/resources/SourceRootResourceTest.java | 7 +- 6 files changed, 2 insertions(+), 169 deletions(-) delete mode 100644 scm-webapp/src/main/java/sonia/scm/api/v2/resources/FileObjectToFileObjectDtoMapper.java delete mode 100644 scm-webapp/src/test/java/sonia/scm/api/v2/resources/FileObjectToFileObjectDtoMapperTest.java diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/BrowserResultToFileObjectDtoMapper.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/BrowserResultToFileObjectDtoMapper.java index b8e34f8101..866c2cc0b9 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/BrowserResultToFileObjectDtoMapper.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/BrowserResultToFileObjectDtoMapper.java @@ -1,6 +1,5 @@ package sonia.scm.api.v2.resources; -import com.google.common.annotations.VisibleForTesting; import de.otto.edison.hal.Embedded; import de.otto.edison.hal.Links; import org.mapstruct.Context; @@ -20,14 +19,6 @@ import java.lang.annotation.Target; @Mapper public abstract class BrowserResultToFileObjectDtoMapper extends BaseFileObjectDtoMapper { - @Inject - private FileObjectToFileObjectDtoMapper childrenMapper; - - @VisibleForTesting - void setChildrenMapper(FileObjectToFileObjectDtoMapper childrenMapper) { - this.childrenMapper = childrenMapper; - } - FileObjectDto map(BrowserResult browserResult, @Context NamespaceAndName namespaceAndName) { FileObjectDto fileObjectDto = fileObjectToDto(browserResult.getFile(), namespaceAndName, browserResult); fileObjectDto.setRevision(browserResult.getRevision()); @@ -36,12 +27,8 @@ public abstract class BrowserResultToFileObjectDtoMapper extends BaseFileObjectD @Mapping(target = "attributes", ignore = true) // We do not map HAL attributes @Mapping(target = "children", qualifiedBy = Children.class) - protected abstract FileObjectDto fileObjectToDto(FileObject fileObject, @Context NamespaceAndName namespaceAndName, @Context BrowserResult browserResult); - @Children - protected FileObjectDto childrenToDto(FileObject fileObject, @Context NamespaceAndName namespaceAndName, @Context BrowserResult browserResult) { - return childrenMapper.map(fileObject, namespaceAndName, browserResult); - } + protected abstract FileObjectDto fileObjectToDto(FileObject fileObject, @Context NamespaceAndName namespaceAndName, @Context BrowserResult browserResult); @Override void applyEnrichers(Links.Builder links, Embedded.Builder embeddedBuilder, NamespaceAndName namespaceAndName, BrowserResult browserResult, FileObject fileObject) { diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/FileObjectToFileObjectDtoMapper.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/FileObjectToFileObjectDtoMapper.java deleted file mode 100644 index c2884a460f..0000000000 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/FileObjectToFileObjectDtoMapper.java +++ /dev/null @@ -1,22 +0,0 @@ -package sonia.scm.api.v2.resources; - -import de.otto.edison.hal.Embedded; -import de.otto.edison.hal.Links; -import org.mapstruct.Context; -import org.mapstruct.Mapper; -import org.mapstruct.Mapping; -import sonia.scm.repository.BrowserResult; -import sonia.scm.repository.FileObject; -import sonia.scm.repository.NamespaceAndName; - -@Mapper -public abstract class FileObjectToFileObjectDtoMapper extends BaseFileObjectDtoMapper { - - @Mapping(target = "attributes", ignore = true) // We do not map HAL attributes - protected abstract FileObjectDto map(FileObject fileObject, @Context NamespaceAndName namespaceAndName, @Context BrowserResult browserResult); - - @Override - void applyEnrichers(Links.Builder links, Embedded.Builder embeddedBuilder, NamespaceAndName namespaceAndName, BrowserResult browserResult, FileObject fileObject) { - applyEnrichers(new EdisonHalAppender(links, embeddedBuilder), fileObject, namespaceAndName, browserResult, browserResult.getRevision()); - } -} diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/MapperModule.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/MapperModule.java index 48de63969d..c7d88e7c0a 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/MapperModule.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/MapperModule.java @@ -37,7 +37,6 @@ public class MapperModule extends AbstractModule { bind(TagToTagDtoMapper.class).to(Mappers.getMapper(TagToTagDtoMapper.class).getClass()); - bind(FileObjectToFileObjectDtoMapper.class).to(Mappers.getMapper(FileObjectToFileObjectDtoMapper.class).getClass()); bind(BrowserResultToFileObjectDtoMapper.class).to(Mappers.getMapper(BrowserResultToFileObjectDtoMapper.class).getClass()); bind(ModificationsToDtoMapper.class).to(Mappers.getMapper(ModificationsToDtoMapper.class).getClass()); diff --git a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/BrowserResultToFileObjectDtoMapperTest.java b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/BrowserResultToFileObjectDtoMapperTest.java index 162a90d45a..27ef82834b 100644 --- a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/BrowserResultToFileObjectDtoMapperTest.java +++ b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/BrowserResultToFileObjectDtoMapperTest.java @@ -8,7 +8,6 @@ import org.junit.After; import org.junit.Before; import org.junit.Test; import org.mapstruct.factory.Mappers; -import org.mockito.InjectMocks; import sonia.scm.repository.BrowserResult; import sonia.scm.repository.FileObject; import sonia.scm.repository.NamespaceAndName; @@ -24,9 +23,6 @@ public class BrowserResultToFileObjectDtoMapperTest { private final URI baseUri = URI.create("http://example.com/base/"); private final ResourceLinks resourceLinks = ResourceLinksMock.createMock(baseUri); - @InjectMocks - private FileObjectToFileObjectDtoMapperImpl fileObjectToFileObjectDtoMapper; - private BrowserResultToFileObjectDtoMapper mapper; private final Subject subject = mock(Subject.class); @@ -40,7 +36,6 @@ public class BrowserResultToFileObjectDtoMapperTest { public void init() { initMocks(this); mapper = Mappers.getMapper(BrowserResultToFileObjectDtoMapper.class); - mapper.setChildrenMapper(fileObjectToFileObjectDtoMapper); mapper.setResourceLinks(resourceLinks); subjectThreadState.bind(); diff --git a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/FileObjectToFileObjectDtoMapperTest.java b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/FileObjectToFileObjectDtoMapperTest.java deleted file mode 100644 index de357848c5..0000000000 --- a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/FileObjectToFileObjectDtoMapperTest.java +++ /dev/null @@ -1,121 +0,0 @@ -package sonia.scm.api.v2.resources; - -import org.apache.shiro.subject.Subject; -import org.apache.shiro.subject.support.SubjectThreadState; -import org.apache.shiro.util.ThreadContext; -import org.apache.shiro.util.ThreadState; -import org.junit.After; -import org.junit.Before; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.mockito.InjectMocks; -import org.mockito.junit.MockitoJUnitRunner; -import sonia.scm.repository.BrowserResult; -import sonia.scm.repository.FileObject; -import sonia.scm.repository.NamespaceAndName; -import sonia.scm.repository.SubRepository; - -import java.net.URI; - -import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.mock; - -@RunWith(MockitoJUnitRunner.Silent.class) -public class FileObjectToFileObjectDtoMapperTest { - - private final URI baseUri = URI.create("http://example.com/base/"); - @SuppressWarnings("unused") // Is injected - private final ResourceLinks resourceLinks = ResourceLinksMock.createMock(baseUri); - - @InjectMocks - private FileObjectToFileObjectDtoMapperImpl mapper; - - private final Subject subject = mock(Subject.class); - private final ThreadState subjectThreadState = new SubjectThreadState(subject); - - private URI expectedBaseUri; - - @Before - public void init() { - expectedBaseUri = baseUri.resolve(RepositoryRootResource.REPOSITORIES_PATH_V2 + "/"); - subjectThreadState.bind(); - ThreadContext.bind(subject); - } - - @After - public void unbind() { - ThreadContext.unbindSubject(); - } - - @Test - public void shouldMapAttributesCorrectly() { - FileObject fileObject = createFileObject(); - FileObjectDto dto = mapper.map(fileObject, new NamespaceAndName("namespace", "name"), new BrowserResult("revision", fileObject)); - - assertEqualAttributes(fileObject, dto); - } - - @Test - public void shouldHaveCorrectSelfLinkForDirectory() { - FileObject fileObject = createDirectoryObject(); - FileObjectDto dto = mapper.map(fileObject, new NamespaceAndName("namespace", "name"), new BrowserResult("revision", fileObject)); - - assertThat(dto.getLinks().getLinkBy("self").get().getHref()).isEqualTo(expectedBaseUri.resolve("namespace/name/sources/revision/foo/bar").toString()); - } - - @Test - public void shouldHaveCorrectContentLink() { - FileObject fileObject = createFileObject(); - fileObject.setDirectory(false); - FileObjectDto dto = mapper.map(fileObject, new NamespaceAndName("namespace", "name"), new BrowserResult("revision", fileObject)); - - assertThat(dto.getLinks().getLinkBy("self").get().getHref()).isEqualTo(expectedBaseUri.resolve("namespace/name/content/revision/foo/bar").toString()); - } - - @Test - public void shouldAppendLinks() { - HalEnricherRegistry registry = new HalEnricherRegistry(); - registry.register(FileObject.class, (ctx, appender) -> { - NamespaceAndName repository = ctx.oneRequireByType(NamespaceAndName.class); - FileObject fo = ctx.oneRequireByType(FileObject.class); - String rev = ctx.oneRequireByType(String.class); - - appender.appendLink("hog", "http://" + repository.logString() + "/" + fo.getName() + "/" + rev); - }); - mapper.setRegistry(registry); - - FileObject fileObject = createFileObject(); - FileObjectDto dto = mapper.map(fileObject, new NamespaceAndName("hitchhiker", "hog"), new BrowserResult("42", fileObject)); - - assertThat(dto.getLinks().getLinkBy("hog").get().getHref()).isEqualTo("http://hitchhiker/hog/foo/42"); - } - - private FileObject createDirectoryObject() { - FileObject fileObject = createFileObject(); - fileObject.setDirectory(true); - return fileObject; - } - - private FileObject createFileObject() { - FileObject fileObject = new FileObject(); - fileObject.setName("foo"); - fileObject.setDescription("bar"); - fileObject.setPath("foo/bar"); - fileObject.setDirectory(false); - fileObject.setLength(100); - fileObject.setLastModified(123L); - - fileObject.setSubRepository(new SubRepository("repo.url")); - return fileObject; - } - - private void assertEqualAttributes(FileObject fileObject, FileObjectDto dto) { - assertThat(dto.getName()).isEqualTo(fileObject.getName()); - assertThat(dto.getDescription()).isEqualTo(fileObject.getDescription()); - assertThat(dto.getPath()).isEqualTo(fileObject.getPath()); - assertThat(dto.isDirectory()).isEqualTo(fileObject.isDirectory()); - assertThat(dto.getLength()).isEqualTo(fileObject.getLength()); - assertThat(dto.getLastModified().toEpochMilli()).isEqualTo((long) fileObject.getLastModified()); - assertThat(dto.getSubRepository().getBrowserUrl()).isEqualTo(fileObject.getSubRepository().getBrowserUrl()); - } -} diff --git a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/SourceRootResourceTest.java b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/SourceRootResourceTest.java index 37c7659b1c..7b205c732a 100644 --- a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/SourceRootResourceTest.java +++ b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/SourceRootResourceTest.java @@ -7,7 +7,6 @@ import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.mapstruct.factory.Mappers; -import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.junit.MockitoJUnitRunner; import sonia.scm.NotFoundException; @@ -41,16 +40,12 @@ public class SourceRootResourceTest extends RepositoryTestBase { @Mock private BrowseCommandBuilder browseCommandBuilder; - @InjectMocks - private FileObjectToFileObjectDtoMapperImpl fileObjectToFileObjectDtoMapper; - private BrowserResultToFileObjectDtoMapper browserResultToFileObjectDtoMapper; @Before - public void prepareEnvironment() throws Exception { + public void prepareEnvironment() { browserResultToFileObjectDtoMapper = Mappers.getMapper(BrowserResultToFileObjectDtoMapper.class); - browserResultToFileObjectDtoMapper.setChildrenMapper(fileObjectToFileObjectDtoMapper); browserResultToFileObjectDtoMapper.setResourceLinks(resourceLinks); when(serviceFactory.create(new NamespaceAndName("space", "repo"))).thenReturn(service); when(service.getBrowseCommand()).thenReturn(browseCommandBuilder); From f7dc89ee81d1e8c51cefc443c2e5ed6beb8abb1a Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Wed, 11 Dec 2019 15:49:33 +0100 Subject: [PATCH 12/35] Reload partial results --- scm-ui/ui-types/src/Sources.ts | 1 + .../src/repos/sources/modules/sources.ts | 18 ++++++++++++++++-- .../scm/api/v2/resources/FileObjectDto.java | 1 + 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/scm-ui/ui-types/src/Sources.ts b/scm-ui/ui-types/src/Sources.ts index 4e0e7fe188..7b80b9df94 100644 --- a/scm-ui/ui-types/src/Sources.ts +++ b/scm-ui/ui-types/src/Sources.ts @@ -16,6 +16,7 @@ export type File = { length: number; lastModified?: string; subRepository?: SubRepository; // TODO + partialResult: boolean; _links: Links; _embedded: { children: File[] | null | undefined; diff --git a/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts b/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts index 826b32f1f6..b989e18b34 100644 --- a/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts +++ b/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts @@ -10,13 +10,27 @@ export const FETCH_SOURCES_SUCCESS = `${FETCH_SOURCES}_${types.SUCCESS_SUFFIX}`; export const FETCH_SOURCES_FAILURE = `${FETCH_SOURCES}_${types.FAILURE_SUFFIX}`; export function fetchSources(repository: Repository, revision: string, path: string) { + return fetchSourcesWithoutOptionalLoadingState(repository, revision, path, true); +} + +export function fetchSourcesWithoutOptionalLoadingState( + repository: Repository, + revision: string, + path: string, + dispatchLoading: boolean +) { return function(dispatch: any) { - dispatch(fetchSourcesPending(repository, revision, path)); + if (dispatchLoading) { + dispatch(fetchSourcesPending(repository, revision, path)); + } return apiClient .get(createUrl(repository, revision, path)) .then(response => response.json()) - .then(sources => { + .then((sources: File) => { dispatch(fetchSourcesSuccess(repository, revision, path, sources)); + if (sources._embedded.children && sources._embedded.children.find(c => c.partialResult)) { + setTimeout(() => dispatch(fetchSourcesWithoutOptionalLoadingState(repository, revision, path, false)), 1000); + } }) .catch(err => { dispatch(fetchSourcesFailure(repository, revision, path, err)); diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/FileObjectDto.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/FileObjectDto.java index 0bce564e35..626af4bac5 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/FileObjectDto.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/FileObjectDto.java @@ -27,6 +27,7 @@ public class FileObjectDto extends HalRepresentation { private SubRepositoryDto subRepository; @JsonInclude(JsonInclude.Include.NON_EMPTY) private String revision; + private boolean partialResult; public FileObjectDto(Links links, Embedded embedded) { super(links, embedded); From 8df43e7b4e352851972a34d19b124fce181f9830 Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Thu, 12 Dec 2019 11:47:03 +0100 Subject: [PATCH 13/35] Let background computations abort for browse command --- .../java/sonia/scm/repository/FileObject.java | 10 +++++ .../scm/repository/spi/SyncAsyncExecutor.java | 8 +++- .../scm/repository/spi/GitBrowseCommand.java | 7 +++- .../repository/spi/SyncAsyncExecutors.java | 33 ++++++++++++----- scm-ui/ui-types/src/Sources.ts | 1 + scm-ui/ui-webapp/public/locales/de/repos.json | 4 +- scm-ui/ui-webapp/public/locales/en/repos.json | 4 +- .../repos/sources/components/FileTreeLeaf.tsx | 37 +++++++++++++++---- .../src/repos/sources/modules/sources.ts | 2 +- .../scm/api/v2/resources/FileObjectDto.java | 1 + .../repository/DefaultSyncAsyncExecutor.java | 26 +++++++++++-- .../DefaultSyncAsyncExecutorTest.java | 18 ++++++++- 12 files changed, 122 insertions(+), 29 deletions(-) diff --git a/scm-core/src/main/java/sonia/scm/repository/FileObject.java b/scm-core/src/main/java/sonia/scm/repository/FileObject.java index 2f3241bf40..acb7559094 100644 --- a/scm-core/src/main/java/sonia/scm/repository/FileObject.java +++ b/scm-core/src/main/java/sonia/scm/repository/FileObject.java @@ -226,6 +226,10 @@ public class FileObject implements LastModifiedAware, Serializable return partialResult; } + public boolean isComputationAborted() { + return computationAborted; + } + //~--- set methods ---------------------------------------------------------- /** @@ -310,6 +314,10 @@ public class FileObject implements LastModifiedAware, Serializable this.partialResult = partialResult; } + public void setComputationAborted(boolean computationAborted) { + this.computationAborted = computationAborted; + } + public Collection getChildren() { return unmodifiableCollection(children); } @@ -348,6 +356,8 @@ public class FileObject implements LastModifiedAware, Serializable private boolean partialResult = false; + private boolean computationAborted = false; + /** sub repository informations */ @XmlElement(name = "subrepository") private SubRepository subRepository; diff --git a/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutor.java b/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutor.java index 55fdbcacdb..0cd3200736 100644 --- a/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutor.java +++ b/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutor.java @@ -5,10 +5,14 @@ import java.util.function.Consumer; public interface SyncAsyncExecutor { default ExecutionType execute(Runnable runnable) { - return execute(ignored -> runnable.run()); + return execute(ignored -> runnable.run(), () -> {}); } - ExecutionType execute(Consumer runnable); + default ExecutionType execute(Runnable runnable, Runnable abortionFallback) { + return execute(ignored -> runnable.run(), abortionFallback); + } + + ExecutionType execute(Consumer runnable, Runnable abortionFallback); boolean hasExecutedAllSynchronously(); diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java index 06b724d820..de479a4250 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java @@ -133,7 +133,7 @@ public class GitBrowseCommand extends AbstractGitCommand request.updateCache(browserResult); logger.info("updated browser result for repository {}", repository.getNamespaceAndName()); } - }); + }, () -> {}); return browserResult; } else { logger.warn("could not find head of repository {}, empty?", repository.getNamespaceAndName()); @@ -248,6 +248,11 @@ public class GitBrowseCommand extends AbstractGitCommand logger.warn("could not find latest commit for {} on {}", path, revId); } + }, () -> { + file.setPartialResult(false); + file.setComputationAborted(true); + request.updateCache(browserResult); + logger.info("updated browser result for repository {}", repository.getNamespaceAndName()); }); } } diff --git a/scm-test/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutors.java b/scm-test/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutors.java index aa7e7bb954..6c24e72b69 100644 --- a/scm-test/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutors.java +++ b/scm-test/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutors.java @@ -14,7 +14,7 @@ public final class SyncAsyncExecutors { public static SyncAsyncExecutor synchronousExecutor() { return new SyncAsyncExecutor() { @Override - public ExecutionType execute(Consumer runnable) { + public ExecutionType execute(Consumer runnable, Runnable abortionFallback) { runnable.accept(SYNCHRONOUS); return SYNCHRONOUS; } @@ -32,7 +32,7 @@ public final class SyncAsyncExecutors { return new SyncAsyncExecutor() { @Override - public ExecutionType execute(Consumer runnable) { + public ExecutionType execute(Consumer runnable, Runnable abortionFallback) { executor.execute(() -> runnable.accept(ASYNCHRONOUS)); return ASYNCHRONOUS; } @@ -45,12 +45,13 @@ public final class SyncAsyncExecutors { } public static AsyncExecutorStepper stepperAsynchronousExecutor() { - - Executor executor = Executors.newSingleThreadExecutor(); - Semaphore enterSemaphore = new Semaphore(0); - Semaphore exitSemaphore = new Semaphore(0); - return new AsyncExecutorStepper() { + + Executor executor = Executors.newSingleThreadExecutor(); + Semaphore enterSemaphore = new Semaphore(0); + Semaphore exitSemaphore = new Semaphore(0); + boolean timedOut = false; + @Override public void close() { enterSemaphore.release(Integer.MAX_VALUE/2); @@ -58,15 +59,19 @@ public final class SyncAsyncExecutors { } @Override - public ExecutionType execute(Consumer runnable) { + public ExecutionType execute(Consumer runnable, Runnable abortionFallback) { executor.execute(() -> { try { enterSemaphore.acquire(); } catch (InterruptedException e) { Thread.currentThread().interrupt(); } - runnable.accept(ASYNCHRONOUS); - exitSemaphore.release(); + if (timedOut) { + abortionFallback.run(); + } else { + runnable.accept(ASYNCHRONOUS); + exitSemaphore.release(); + } }); return ASYNCHRONOUS; } @@ -81,6 +86,12 @@ public final class SyncAsyncExecutors { } } + @Override + public void timeout() { + timedOut = true; + close(); + } + @Override public boolean hasExecutedAllSynchronously() { return true; @@ -90,5 +101,7 @@ public final class SyncAsyncExecutors { public interface AsyncExecutorStepper extends SyncAsyncExecutor, Closeable { void next(); + + void timeout(); } } diff --git a/scm-ui/ui-types/src/Sources.ts b/scm-ui/ui-types/src/Sources.ts index 7b80b9df94..f5fca71d65 100644 --- a/scm-ui/ui-types/src/Sources.ts +++ b/scm-ui/ui-types/src/Sources.ts @@ -17,6 +17,7 @@ export type File = { lastModified?: string; subRepository?: SubRepository; // TODO partialResult: boolean; + computationAborted: boolean; _links: Links; _embedded: { children: File[] | null | undefined; diff --git a/scm-ui/ui-webapp/public/locales/de/repos.json b/scm-ui/ui-webapp/public/locales/de/repos.json index df6c534ece..5a357efd95 100644 --- a/scm-ui/ui-webapp/public/locales/de/repos.json +++ b/scm-ui/ui-webapp/public/locales/de/repos.json @@ -101,7 +101,9 @@ "length": "Größe", "lastModified": "Zuletzt bearbeitet", "description": "Beschreibung", - "branch": "Branch" + "branch": "Branch", + "notYetComputed": "Noch nicht berechnet; Der Wert wird in Kürze aktualisiert", + "computationAborted": "Die Berechnung dauert zu lange und wurde abgebrochen" }, "content": { "historyButton": "History", diff --git a/scm-ui/ui-webapp/public/locales/en/repos.json b/scm-ui/ui-webapp/public/locales/en/repos.json index 103e30b825..c1e7a835cd 100644 --- a/scm-ui/ui-webapp/public/locales/en/repos.json +++ b/scm-ui/ui-webapp/public/locales/en/repos.json @@ -101,7 +101,9 @@ "length": "Length", "lastModified": "Last modified", "description": "Description", - "branch": "Branch" + "branch": "Branch", + "notYetComputed": "Not yet computed, will be updated in a short while", + "computationAborted": "The computation took too long and was aborted" }, "content": { "historyButton": "History", diff --git a/scm-ui/ui-webapp/src/repos/sources/components/FileTreeLeaf.tsx b/scm-ui/ui-webapp/src/repos/sources/components/FileTreeLeaf.tsx index 961d65c043..037d8f7c45 100644 --- a/scm-ui/ui-webapp/src/repos/sources/components/FileTreeLeaf.tsx +++ b/scm-ui/ui-webapp/src/repos/sources/components/FileTreeLeaf.tsx @@ -4,10 +4,12 @@ import classNames from "classnames"; import styled from "styled-components"; import { binder, ExtensionPoint } from "@scm-manager/ui-extensions"; import { File } from "@scm-manager/ui-types"; -import { DateFromNow, FileSize } from "@scm-manager/ui-components"; +import { DateFromNow, FileSize, Tooltip } from "@scm-manager/ui-components"; import FileIcon from "./FileIcon"; +import { Icon } from "@scm-manager/ui-components/src"; +import { WithTranslation, withTranslation } from "react-i18next"; -type Props = { +type Props = WithTranslation & { file: File; baseUrl: string; }; @@ -31,7 +33,7 @@ export function createLink(base: string, file: File) { return link; } -export default class FileTreeLeaf extends React.Component { +class FileTreeLeaf extends React.Component { createLink = (file: File) => { return createLink(this.props.baseUrl, file); }; @@ -58,6 +60,25 @@ export default class FileTreeLeaf extends React.Component { return {file.name}; }; + contentIfPresent = (file: File, content: any) => { + const { t } = this.props; + if (file.computationAborted) { + return ( + + + + ); + } else if (file.partialResult) { + return ( + + + + ); + } else { + return content; + } + }; + render() { const { file } = this.props; @@ -68,10 +89,10 @@ export default class FileTreeLeaf extends React.Component { {this.createFileIcon(file)} {this.createFileName(file)} {fileSize} - - - - {file.description} + {this.contentIfPresent(file, )} + + {this.contentIfPresent(file, file.description)} + {binder.hasExtension("repos.sources.tree.row.right") && ( {!file.directory && ( @@ -89,3 +110,5 @@ export default class FileTreeLeaf extends React.Component { ); } } + +export default withTranslation("repos")(FileTreeLeaf); diff --git a/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts b/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts index b989e18b34..7d2b8c94a7 100644 --- a/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts +++ b/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts @@ -29,7 +29,7 @@ export function fetchSourcesWithoutOptionalLoadingState( .then((sources: File) => { dispatch(fetchSourcesSuccess(repository, revision, path, sources)); if (sources._embedded.children && sources._embedded.children.find(c => c.partialResult)) { - setTimeout(() => dispatch(fetchSourcesWithoutOptionalLoadingState(repository, revision, path, false)), 1000); + setTimeout(() => dispatch(fetchSourcesWithoutOptionalLoadingState(repository, revision, path, false)), 3000); } }) .catch(err => { diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/FileObjectDto.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/FileObjectDto.java index 626af4bac5..4676a0fb03 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/FileObjectDto.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/FileObjectDto.java @@ -28,6 +28,7 @@ public class FileObjectDto extends HalRepresentation { @JsonInclude(JsonInclude.Include.NON_EMPTY) private String revision; private boolean partialResult; + private boolean computationAborted; public FileObjectDto(Links links, Embedded embedded) { super(links, embedded); diff --git a/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutor.java b/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutor.java index 326b329218..6bdfa2d39f 100644 --- a/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutor.java +++ b/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutor.java @@ -4,6 +4,7 @@ import sonia.scm.repository.spi.SyncAsyncExecutor; import java.time.Instant; import java.util.concurrent.Executor; +import java.util.concurrent.atomic.AtomicLong; import java.util.function.Consumer; import static sonia.scm.repository.spi.SyncAsyncExecutor.ExecutionType.ASYNCHRONOUS; @@ -11,18 +12,35 @@ import static sonia.scm.repository.spi.SyncAsyncExecutor.ExecutionType.SYNCHRONO public class DefaultSyncAsyncExecutor implements SyncAsyncExecutor { + public static final long DEFAULT_MAX_ASYNC_RUNTIME = 60 * 1000L; + private final Executor executor; private final Instant switchToAsyncTime; + private final long maxAsyncRuntime; + private AtomicLong asyncRuntime = new AtomicLong(0L); private boolean executedAllSynchronously = true; DefaultSyncAsyncExecutor(Executor executor, Instant switchToAsyncTime) { - this.executor = executor; - this.switchToAsyncTime = switchToAsyncTime; + this(executor, switchToAsyncTime, DEFAULT_MAX_ASYNC_RUNTIME); } - public ExecutionType execute(Consumer runnable) { + DefaultSyncAsyncExecutor(Executor executor, Instant switchToAsyncTime, long maxAsyncRuntime) { + this.executor = executor; + this.switchToAsyncTime = switchToAsyncTime; + this.maxAsyncRuntime = maxAsyncRuntime; + } + + public ExecutionType execute(Consumer runnable, Runnable abortionFallback) { if (Instant.now().isAfter(switchToAsyncTime)) { - executor.execute(() -> runnable.accept(ASYNCHRONOUS)); + executor.execute(() -> { + if (asyncRuntime.get() < maxAsyncRuntime) { + long chunkStartTime = System.currentTimeMillis(); + runnable.accept(ASYNCHRONOUS); + asyncRuntime.addAndGet(System.currentTimeMillis() - chunkStartTime); + } else { + abortionFallback.run(); + } + }); executedAllSynchronously = false; return ASYNCHRONOUS; } else { diff --git a/scm-webapp/src/test/java/sonia/scm/repository/DefaultSyncAsyncExecutorTest.java b/scm-webapp/src/test/java/sonia/scm/repository/DefaultSyncAsyncExecutorTest.java index bd4148606b..6baca6204e 100644 --- a/scm-webapp/src/test/java/sonia/scm/repository/DefaultSyncAsyncExecutorTest.java +++ b/scm-webapp/src/test/java/sonia/scm/repository/DefaultSyncAsyncExecutorTest.java @@ -13,26 +13,40 @@ import static sonia.scm.repository.spi.SyncAsyncExecutor.ExecutionType.SYNCHRONO class DefaultSyncAsyncExecutorTest { ExecutionType calledWithType = null; + boolean aborted = false; @Test void shouldExecuteSynchronouslyBeforeTimeout() { DefaultSyncAsyncExecutor executor = new DefaultSyncAsyncExecutor(Runnable::run, Instant.MAX); - ExecutionType result = executor.execute(type -> calledWithType = type); + ExecutionType result = executor.execute(type -> calledWithType = type, () -> aborted = true); assertThat(result).isEqualTo(SYNCHRONOUS); assertThat(calledWithType).isEqualTo(SYNCHRONOUS); assertThat(executor.hasExecutedAllSynchronously()).isTrue(); + assertThat(aborted).isFalse(); } @Test void shouldExecuteAsynchronouslyAfterTimeout() { DefaultSyncAsyncExecutor executor = new DefaultSyncAsyncExecutor(Runnable::run, Instant.now().minus(1, MILLIS)); - ExecutionType result = executor.execute(type -> calledWithType = type); + ExecutionType result = executor.execute(type -> calledWithType = type, () -> aborted = true); assertThat(result).isEqualTo(ASYNCHRONOUS); assertThat(calledWithType).isEqualTo(ASYNCHRONOUS); assertThat(executor.hasExecutedAllSynchronously()).isFalse(); + assertThat(aborted).isFalse(); + } + + @Test + void shouldCallFallbackAfterAbortion() { + DefaultSyncAsyncExecutor executor = new DefaultSyncAsyncExecutor(Runnable::run, Instant.now().minus(1, MILLIS), 0L); + + ExecutionType result = executor.execute(type -> calledWithType = type, () -> aborted = true); + + assertThat(result).isEqualTo(ASYNCHRONOUS); + assertThat(calledWithType).isNull(); + assertThat(aborted).isTrue(); } } From 6c8820719ecbe681bbc32519e683e1fb8057c966 Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Thu, 12 Dec 2019 11:52:44 +0100 Subject: [PATCH 14/35] File size is unknown when not computed --- scm-ui/ui-webapp/src/repos/sources/components/FileTreeLeaf.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scm-ui/ui-webapp/src/repos/sources/components/FileTreeLeaf.tsx b/scm-ui/ui-webapp/src/repos/sources/components/FileTreeLeaf.tsx index 037d8f7c45..9190a02dd6 100644 --- a/scm-ui/ui-webapp/src/repos/sources/components/FileTreeLeaf.tsx +++ b/scm-ui/ui-webapp/src/repos/sources/components/FileTreeLeaf.tsx @@ -88,7 +88,7 @@ class FileTreeLeaf extends React.Component { {this.createFileIcon(file)} {this.createFileName(file)} - {fileSize} + {this.contentIfPresent(file, fileSize)} {this.contentIfPresent(file, )} {this.contentIfPresent(file, file.description)} From 4fd2a0dd23888126a32830450fd8821d7a65e956 Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Thu, 12 Dec 2019 16:13:36 +0100 Subject: [PATCH 15/35] Small API changes --- .../scm/repository/spi/SyncAsyncExecutor.java | 71 +++++++++++++++++-- .../spi/SyncAsyncExecutorProvider.java | 50 ++++++++++++- .../repository/DefaultSyncAsyncExecutor.java | 26 +++---- .../DefaultSyncAsyncExecutorProvider.java | 25 ++++++- .../DefaultSyncAsyncExecutorTest.java | 8 ++- 5 files changed, 150 insertions(+), 30 deletions(-) diff --git a/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutor.java b/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutor.java index 0cd3200736..a9e7d85dcc 100644 --- a/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutor.java +++ b/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutor.java @@ -2,18 +2,79 @@ package sonia.scm.repository.spi; import java.util.function.Consumer; +/** + * Tasks submitted to this executor will be run synchronously up to a given time, after which they will be queued and + * processed asynchronously. After a maximum amount of time consumed by these tasks, they will be skipped. Note that + * this only works for short-living tasks. + *

+ * Get instances of this using a {@link SyncAsyncExecutorProvider}. + */ public interface SyncAsyncExecutor { - default ExecutionType execute(Runnable runnable) { - return execute(ignored -> runnable.run(), () -> {}); + /** + * Execute the given task (either synchronously or asynchronously). If this task is skipped due to + * timeouts, nothing will be done. + * + * @param task The {@link Runnable} to be executed. + * @return Either {@link ExecutionType#SYNCHRONOUS} when the given {@link Runnable} has been executed immediately or + * {@link ExecutionType#ASYNCHRONOUS}, when the task was queued to be executed asynchronously in the future. + */ + default ExecutionType execute(Runnable task) { + return execute( + ignored -> task.run(), + () -> {} + ); } - default ExecutionType execute(Runnable runnable, Runnable abortionFallback) { - return execute(ignored -> runnable.run(), abortionFallback); + /** + * Execute the given task (either synchronously or asynchronously). If this task is + * skipped due to timeouts, the abortionFallback will be called. + * + * @param task The {@link Runnable} to be executed. + * @param abortionFallback This will only be run, when this and all remaining tasks are aborted. This task should + * only consume a negligible amount of time. + * @return Either {@link ExecutionType#SYNCHRONOUS} when the given {@link Runnable} has been executed immediately or + * {@link ExecutionType#ASYNCHRONOUS}, when the task was queued to be executed asynchronously in the future. + */ + default ExecutionType execute(Runnable task, Runnable abortionFallback) { + return execute(ignored -> task.run(), abortionFallback); } - ExecutionType execute(Consumer runnable, Runnable abortionFallback); + /** + * Execute the given task (either synchronously or asynchronously). If this task is skipped due to + * timeouts, nothing will be done. + * + * @param task The {@link Consumer} to be executed. The parameter given to this is either + * {@link ExecutionType#SYNCHRONOUS} when the given {@link Consumer} is executed immediately + * or {@link ExecutionType#ASYNCHRONOUS}, when the task had been queued and now is executed + * asynchronously. + * @return Either {@link ExecutionType#SYNCHRONOUS} when the given {@link Runnable} has been executed immediately or + * {@link ExecutionType#ASYNCHRONOUS}, when the task was queued to be executed asynchronously in the future. + */ + default ExecutionType execute(Consumer task) { + return execute(task, () -> {}); + } + /** + * Execute the given task (either synchronously or asynchronously). If this task is + * skipped due to timeouts, the abortionFallback will be called. + * + * @param task The {@link Consumer} to be executed. The parameter given to this is either + * {@link ExecutionType#SYNCHRONOUS} when the given {@link Consumer} is executed immediately + * or {@link ExecutionType#ASYNCHRONOUS}, when the task had been queued and now is executed + * asynchronously. + * @param abortionFallback This will only be run, when this and all remaining tasks are aborted. This task should + * only consume a negligible amount of time. + * @return Either {@link ExecutionType#SYNCHRONOUS} when the given {@link Runnable} has been executed immediately or + * {@link ExecutionType#ASYNCHRONOUS}, when the task was queued to be executed asynchronously in the future. + */ + ExecutionType execute(Consumer task, Runnable abortionFallback); + + /** + * When all submitted tasks have been executed synchronously, this will return true. If at least one task + * has been enqueued to be executed asynchronously, this returns false (even when none of the enqueued + * tasks have been run, yet). + */ boolean hasExecutedAllSynchronously(); enum ExecutionType { diff --git a/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutorProvider.java b/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutorProvider.java index 2d21129eb2..5f417f324e 100644 --- a/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutorProvider.java +++ b/scm-core/src/main/java/sonia/scm/repository/spi/SyncAsyncExecutorProvider.java @@ -1,12 +1,56 @@ package sonia.scm.repository.spi; +/** + * Use this provider to get {@link SyncAsyncExecutor} instances to execute a number of normally short-lived tasks, that + * should be run asynchronously (or even be skipped) whenever they take too long in summary. + *

+ * The goal of this is a "best effort" approach: The submitted tasks are run immediately when they are submitted, unless + * a given timespan (switchToAsyncInSeconds) has passed. From this moment on the tasks are put into a queue to be + * processed asynchronously. If even then they take too long and their accumulated asynchronous runtime exceeds another + * limit (maxAsyncAbortSeconds), the tasks are skipped. + *

+ * Note that whenever a task has been started either synchronously or asynchronously it will neither be terminated nor + * switched from foreground to background execution, so this will only work well for short-living tasks. A long running + * task can still block this for longer than the configured amount of seconds. + */ public interface SyncAsyncExecutorProvider { - int DEFAULT_TIMEOUT_SECONDS = 2; + int DEFAULT_SWITCH_TO_ASYNC_IN_SECONDS = 2; + /** + * Creates an {@link SyncAsyncExecutor} that will run tasks synchronously for + * {@link #DEFAULT_SWITCH_TO_ASYNC_IN_SECONDS} seconds. The limit of asynchronous runtime is implementation dependant. + * + * @return The executor. + */ default SyncAsyncExecutor createExecutorWithDefaultTimeout() { - return createExecutorWithSecondsToTimeout(DEFAULT_TIMEOUT_SECONDS); + return createExecutorWithSecondsToTimeout(DEFAULT_SWITCH_TO_ASYNC_IN_SECONDS); } - SyncAsyncExecutor createExecutorWithSecondsToTimeout(int seconds); + /** + * Creates an {@link SyncAsyncExecutor} that will run tasks synchronously for + * switchToAsyncInSeconds seconds. The limit of asynchronous runtime is implementation dependant. + * + * @param switchToAsyncInSeconds The amount of seconds submitted tasks will be run synchronously. After this time, + * further tasks will be run asynchronously. To run all tasks asynchronously no matter + * what, set this to 0. + * @return The executor. + */ + SyncAsyncExecutor createExecutorWithSecondsToTimeout(int switchToAsyncInSeconds); + + /** + * Creates an {@link SyncAsyncExecutor} that will run tasks synchronously for + * switchToAsyncInSeconds seconds and will abort tasks after they ran + * maxAsyncAbortSeconds asynchronously. + * + * @param switchToAsyncInSeconds The amount of seconds submitted tasks will be run synchronously. After this time, + * further tasks will be run asynchronously. To run all tasks asynchronously no matter + * what, set this to 0. + * @param maxAsyncAbortSeconds The amount of seconds, tasks that were started asynchronously may run in summary + * before remaining tasks will not be executed at all anymore. To abort all tasks that + * are submitted after switchToAsyncInSeconds immediately, set this to + * 0. + * @return The executor. + */ + SyncAsyncExecutor createExecutorWithSecondsToTimeout(int switchToAsyncInSeconds, int maxAsyncAbortSeconds); } diff --git a/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutor.java b/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutor.java index 6bdfa2d39f..de9e27ecda 100644 --- a/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutor.java +++ b/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutor.java @@ -12,31 +12,25 @@ import static sonia.scm.repository.spi.SyncAsyncExecutor.ExecutionType.SYNCHRONO public class DefaultSyncAsyncExecutor implements SyncAsyncExecutor { - public static final long DEFAULT_MAX_ASYNC_RUNTIME = 60 * 1000L; - private final Executor executor; private final Instant switchToAsyncTime; - private final long maxAsyncRuntime; - private AtomicLong asyncRuntime = new AtomicLong(0L); + private final long maxAsyncAbortMilliseconds; + private AtomicLong accumulatedAsyncRuntime = new AtomicLong(0L); private boolean executedAllSynchronously = true; - DefaultSyncAsyncExecutor(Executor executor, Instant switchToAsyncTime) { - this(executor, switchToAsyncTime, DEFAULT_MAX_ASYNC_RUNTIME); - } - - DefaultSyncAsyncExecutor(Executor executor, Instant switchToAsyncTime, long maxAsyncRuntime) { + DefaultSyncAsyncExecutor(Executor executor, Instant switchToAsyncTime, int maxAsyncAbortSeconds) { this.executor = executor; this.switchToAsyncTime = switchToAsyncTime; - this.maxAsyncRuntime = maxAsyncRuntime; + this.maxAsyncAbortMilliseconds = maxAsyncAbortSeconds * 1000L; } - public ExecutionType execute(Consumer runnable, Runnable abortionFallback) { - if (Instant.now().isAfter(switchToAsyncTime)) { + public ExecutionType execute(Consumer task, Runnable abortionFallback) { + if (switchToAsyncTime.isBefore(Instant.now())) { executor.execute(() -> { - if (asyncRuntime.get() < maxAsyncRuntime) { + if (accumulatedAsyncRuntime.get() < maxAsyncAbortMilliseconds) { long chunkStartTime = System.currentTimeMillis(); - runnable.accept(ASYNCHRONOUS); - asyncRuntime.addAndGet(System.currentTimeMillis() - chunkStartTime); + task.accept(ASYNCHRONOUS); + accumulatedAsyncRuntime.addAndGet(System.currentTimeMillis() - chunkStartTime); } else { abortionFallback.run(); } @@ -44,7 +38,7 @@ public class DefaultSyncAsyncExecutor implements SyncAsyncExecutor { executedAllSynchronously = false; return ASYNCHRONOUS; } else { - runnable.accept(SYNCHRONOUS); + task.accept(SYNCHRONOUS); return SYNCHRONOUS; } } diff --git a/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutorProvider.java b/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutorProvider.java index 0be7d38234..4c1f0429da 100644 --- a/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutorProvider.java +++ b/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutorProvider.java @@ -11,22 +11,41 @@ import java.util.concurrent.Executors; public class DefaultSyncAsyncExecutorProvider implements SyncAsyncExecutorProvider, Closeable { + public static final int DEFAULT_MAX_ASYNC_ABORT_SECONDS = 60; + public static final String MAX_ASYNC_ABORT_SECONDS_PROPERTY = "scm.maxAsyncAbortSeconds"; + + public static final int DEFAULT_NUMBER_OF_THREADS = 4; + public static final String NUMBER_OF_THREADS_PROPERTY = "scm.asyncThreads"; + private final ExecutorService executor; + private final int defaultMaxAsyncAbortSeconds; public DefaultSyncAsyncExecutorProvider() { - this(Executors.newFixedThreadPool(4)); + this(Executors.newFixedThreadPool(getProperty(NUMBER_OF_THREADS_PROPERTY, DEFAULT_NUMBER_OF_THREADS))); } public DefaultSyncAsyncExecutorProvider(ExecutorService executor) { this.executor = executor; + this.defaultMaxAsyncAbortSeconds = getProperty(MAX_ASYNC_ABORT_SECONDS_PROPERTY, DEFAULT_MAX_ASYNC_ABORT_SECONDS); } - public SyncAsyncExecutor createExecutorWithSecondsToTimeout(int seconds) { - return new DefaultSyncAsyncExecutor(executor, Instant.now().plus(seconds, ChronoUnit.SECONDS)); + public SyncAsyncExecutor createExecutorWithSecondsToTimeout(int switchToAsyncInSeconds) { + return createExecutorWithSecondsToTimeout(switchToAsyncInSeconds, DEFAULT_MAX_ASYNC_ABORT_SECONDS); + } + + public SyncAsyncExecutor createExecutorWithSecondsToTimeout(int switchToAsyncInSeconds, int maxAsyncAbortSeconds) { + return new DefaultSyncAsyncExecutor( + executor, + Instant.now().plus(switchToAsyncInSeconds, ChronoUnit.SECONDS), + maxAsyncAbortSeconds); } @Override public void close() { executor.shutdownNow(); } + + private static int getProperty(String key, int defaultValue) { + return Integer.parseInt(System.getProperty(key, Integer.toString(defaultValue))); + } } diff --git a/scm-webapp/src/test/java/sonia/scm/repository/DefaultSyncAsyncExecutorTest.java b/scm-webapp/src/test/java/sonia/scm/repository/DefaultSyncAsyncExecutorTest.java index 6baca6204e..7e4ad60157 100644 --- a/scm-webapp/src/test/java/sonia/scm/repository/DefaultSyncAsyncExecutorTest.java +++ b/scm-webapp/src/test/java/sonia/scm/repository/DefaultSyncAsyncExecutorTest.java @@ -5,6 +5,8 @@ import sonia.scm.repository.spi.SyncAsyncExecutor.ExecutionType; import java.time.Instant; +import static java.lang.Integer.MAX_VALUE; +import static java.time.Instant.MAX; import static java.time.temporal.ChronoUnit.MILLIS; import static org.assertj.core.api.Assertions.assertThat; import static sonia.scm.repository.spi.SyncAsyncExecutor.ExecutionType.ASYNCHRONOUS; @@ -17,7 +19,7 @@ class DefaultSyncAsyncExecutorTest { @Test void shouldExecuteSynchronouslyBeforeTimeout() { - DefaultSyncAsyncExecutor executor = new DefaultSyncAsyncExecutor(Runnable::run, Instant.MAX); + DefaultSyncAsyncExecutor executor = new DefaultSyncAsyncExecutor(Runnable::run, MAX, MAX_VALUE); ExecutionType result = executor.execute(type -> calledWithType = type, () -> aborted = true); @@ -29,7 +31,7 @@ class DefaultSyncAsyncExecutorTest { @Test void shouldExecuteAsynchronouslyAfterTimeout() { - DefaultSyncAsyncExecutor executor = new DefaultSyncAsyncExecutor(Runnable::run, Instant.now().minus(1, MILLIS)); + DefaultSyncAsyncExecutor executor = new DefaultSyncAsyncExecutor(Runnable::run, Instant.now().minus(1, MILLIS), MAX_VALUE); ExecutionType result = executor.execute(type -> calledWithType = type, () -> aborted = true); @@ -41,7 +43,7 @@ class DefaultSyncAsyncExecutorTest { @Test void shouldCallFallbackAfterAbortion() { - DefaultSyncAsyncExecutor executor = new DefaultSyncAsyncExecutor(Runnable::run, Instant.now().minus(1, MILLIS), 0L); + DefaultSyncAsyncExecutor executor = new DefaultSyncAsyncExecutor(Runnable::run, Instant.now().minus(1, MILLIS), 0); ExecutionType result = executor.execute(type -> calledWithType = type, () -> aborted = true); From 211aa15399cd03596a50a35b2b029df305481b69 Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Thu, 12 Dec 2019 16:29:42 +0100 Subject: [PATCH 16/35] Make tasks explicit --- .../scm/repository/spi/GitBrowseCommand.java | 138 ++++++++++++------ 1 file changed, 95 insertions(+), 43 deletions(-) diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java index de479a4250..18b8b628f1 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java @@ -70,6 +70,7 @@ import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.function.Consumer; import static java.util.Optional.empty; import static sonia.scm.ContextEntry.ContextBuilder.entity; @@ -211,49 +212,10 @@ public class GitBrowseCommand extends AbstractGitCommand if (!file.isDirectory() &&!request.isDisableLastCommit()) { file.setPartialResult(true); - executor.execute(executionType -> { - logger.trace("fetch last commit for {} at {}", path, revId.getName()); - RevCommit commit = getLatestCommit(repo, revId, path); - - Optional lfsPointer; - try { - lfsPointer = commit == null ? empty() : GitUtil.getLfsPointer(repo, path, commit, treeWalk); - } catch (IOException e) { - throw new InternalRepositoryException(repository, "could not read lfs pointer", e); - } - - if (lfsPointer.isPresent()) { - BlobStore lfsBlobStore = lfsBlobStoreFactory.getLfsBlobStore(repository); - String oid = lfsPointer.get().getOid().getName(); - Blob blob = lfsBlobStore.get(oid); - if (blob == null) { - logger.error("lfs blob for lob id {} not found in lfs store of repository {}", oid, repository.getNamespaceAndName()); - file.setLength(-1); - } else { - file.setLength(blob.getSize()); - } - } else { - file.setLength(loader.getSize()); - } - - file.setPartialResult(false); - if (commit != null) { - file.setLastModified(GitUtil.getCommitTime(commit)); - file.setDescription(commit.getShortMessage()); - if (executionType == ASYNCHRONOUS && browserResult != null) { - request.updateCache(browserResult); - logger.info("updated browser result for repository {}", repository.getNamespaceAndName()); - } - } else { - logger.warn("could not find latest commit for {} on {}", path, - revId); - } - }, () -> { - file.setPartialResult(false); - file.setComputationAborted(true); - request.updateCache(browserResult); - logger.info("updated browser result for repository {}", repository.getNamespaceAndName()); - }); + executor.execute( + new CompleteFileInformation(path, revId, repo, treeWalk, file, loader, request), + new AbortFileInformation(file, request) + ); } } return file; @@ -458,4 +420,94 @@ public class GitBrowseCommand extends AbstractGitCommand /** sub repository cache */ private final Map> subrepositoryCache = Maps.newHashMap(); + + private class CompleteFileInformation implements Consumer { + private final String path; + private final ObjectId revId; + private final org.eclipse.jgit.lib.Repository repo; + private final TreeWalk treeWalk; + private final FileObject file; + private final ObjectLoader loader; + private final BrowseCommandRequest request; + + public CompleteFileInformation(String path, ObjectId revId, org.eclipse.jgit.lib.Repository repo, TreeWalk treeWalk, FileObject file, ObjectLoader loader, BrowseCommandRequest request) { + this.path = path; + this.revId = revId; + this.repo = repo; + this.treeWalk = treeWalk; + this.file = file; + this.loader = loader; + this.request = request; + } + + @Override + public void accept(SyncAsyncExecutor.ExecutionType executionType) { + logger.trace("fetch last commit for {} at {}", path, revId.getName()); + RevCommit commit = GitBrowseCommand.this.getLatestCommit(repo, revId, path); + + Optional lfsPointer = getLfsPointer(commit); + + if (lfsPointer.isPresent()) { + setFileLengthFromLfsBlob(lfsPointer.get()); + } else { + file.setLength(loader.getSize()); + } + + file.setPartialResult(false); + if (commit != null) { + applyValuesFromCommit(executionType, commit); + } else { + logger.warn("could not find latest commit for {} on {}", path, revId); + } + } + + private void setFileLengthFromLfsBlob(LfsPointer lfsPointer) { + BlobStore lfsBlobStore = lfsBlobStoreFactory.getLfsBlobStore(repository); + String oid = lfsPointer.getOid().getName(); + Blob blob = lfsBlobStore.get(oid); + if (blob == null) { + logger.error("lfs blob for lob id {} not found in lfs store of repository {}", oid, repository.getNamespaceAndName()); + file.setLength(-1); + } else { + file.setLength(blob.getSize()); + } + } + + private void applyValuesFromCommit(SyncAsyncExecutor.ExecutionType executionType, RevCommit commit) { + file.setLastModified(GitUtil.getCommitTime(commit)); + file.setDescription(commit.getShortMessage()); + if (executionType == ASYNCHRONOUS && browserResult != null) { + request.updateCache(browserResult); + logger.info("updated browser result for repository {}", repository.getNamespaceAndName()); + } + } + + private Optional getLfsPointer(RevCommit commit) { + Optional lfsPointer; + try { + lfsPointer = commit == null ? empty() : GitUtil.getLfsPointer(repo, path, commit, treeWalk); + } catch (IOException e) { + throw new InternalRepositoryException(repository, "could not read lfs pointer", e); + } + return lfsPointer; + } + } + + private class AbortFileInformation implements Runnable { + private final FileObject file; + private final BrowseCommandRequest request; + + public AbortFileInformation(FileObject file, BrowseCommandRequest request) { + this.file = file; + this.request = request; + } + + @Override + public void run() { + file.setPartialResult(false); + file.setComputationAborted(true); + request.updateCache(browserResult); + logger.info("updated browser result for repository {}", repository.getNamespaceAndName()); + } + } } From 1beaf9d53a97c7e0f562c5a1082db98982898a4c Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Thu, 12 Dec 2019 17:03:37 +0100 Subject: [PATCH 17/35] Cleanup --- .../scm/repository/spi/GitBrowseCommand.java | 142 +++++------------- 1 file changed, 35 insertions(+), 107 deletions(-) diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java index 18b8b628f1..7927d38b2c 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java @@ -73,6 +73,7 @@ import java.util.Optional; import java.util.function.Consumer; import static java.util.Optional.empty; +import static java.util.Optional.of; import static sonia.scm.ContextEntry.ContextBuilder.entity; import static sonia.scm.NotFoundException.notFound; import static sonia.scm.repository.spi.SyncAsyncExecutor.ExecutionType.ASYNCHRONOUS; @@ -101,24 +102,12 @@ public class GitBrowseCommand extends AbstractGitCommand private BrowserResult browserResult; - //~--- constructors --------------------------------------------------------- - - /** - * Constructs ... - * @param context - * @param repository - * @param lfsBlobStoreFactory - * @param executor - */ - public GitBrowseCommand(GitContext context, Repository repository, LfsBlobStoreFactory lfsBlobStoreFactory, SyncAsyncExecutor executor) - { + public GitBrowseCommand(GitContext context, Repository repository, LfsBlobStoreFactory lfsBlobStoreFactory, SyncAsyncExecutor executor) { super(context, repository); this.lfsBlobStoreFactory = lfsBlobStoreFactory; this.executor = executor; } - //~--- get methods ---------------------------------------------------------- - @Override public BrowserResult getBrowserResult(BrowseCommandRequest request) throws IOException { @@ -131,8 +120,7 @@ public class GitBrowseCommand extends AbstractGitCommand browserResult = new BrowserResult(revId.getName(), request.getRevision(), getEntry(repo, request, revId)); executor.execute(executionType -> { if (executionType == ASYNCHRONOUS) { - request.updateCache(browserResult); - logger.info("updated browser result for repository {}", repository.getNamespaceAndName()); + updateCache(request); } }, () -> {}); return browserResult; @@ -156,8 +144,6 @@ public class GitBrowseCommand extends AbstractGitCommand } } - //~--- methods -------------------------------------------------------------- - private FileObject createEmtpyRoot() { FileObject fileObject = new FileObject(); fileObject.setName(""); @@ -166,18 +152,6 @@ public class GitBrowseCommand extends AbstractGitCommand return fileObject; } - /** - * Method description - * - * @param repo - * @param request - * @param revId - * @param treeWalk - * - * @return - * - * @throws IOException - */ private FileObject createFileObject(org.eclipse.jgit.lib.Repository repo, BrowseCommandRequest request, ObjectId revId, TreeWalk treeWalk) throws IOException { @@ -221,97 +195,41 @@ public class GitBrowseCommand extends AbstractGitCommand return file; } - //~--- get methods ---------------------------------------------------------- - - /** - * Method description - * - * - * - * @param repo - * @param revId - * @param path - * - * @return - */ - private RevCommit getLatestCommit(org.eclipse.jgit.lib.Repository repo, - ObjectId revId, String path) - { - RevCommit result = null; - RevWalk walk = null; - - try - { - walk = new RevWalk(repo); - walk.setTreeFilter(AndTreeFilter.create( - TreeFilter.ANY_DIFF - , - PathFilter.create(path) - ) - ); - - RevCommit commit = walk.parseCommit(revId); - - walk.markStart(commit); - result = Util.getFirst(walk); - } - catch (IOException ex) - { - logger.error("could not parse commit for file", ex); - } - finally - { - GitUtil.release(walk); - } - - return result; + private void updateCache(BrowseCommandRequest request) { + request.updateCache(browserResult); + logger.info("updated browser result for repository {}", repository.getNamespaceAndName()); } private FileObject getEntry(org.eclipse.jgit.lib.Repository repo, BrowseCommandRequest request, ObjectId revId) throws IOException { - RevWalk revWalk = null; - TreeWalk treeWalk = null; - FileObject result; - - try { + try (RevWalk revWalk = new RevWalk(repo); TreeWalk treeWalk = new TreeWalk(repo)) { logger.debug("load repository browser for revision {}", revId.name()); - treeWalk = new TreeWalk(repo); if (!isRootRequest(request)) { treeWalk.setFilter(PathFilter.create(request.getPath())); } - revWalk = new RevWalk(repo); RevTree tree = revWalk.parseTree(revId); - if (tree != null) - { + if (tree != null) { treeWalk.addTree(tree); - } - else - { + } else { throw new IllegalStateException("could not find tree for " + revId.name()); } if (isRootRequest(request)) { - result = createEmtpyRoot(); + FileObject result = createEmtpyRoot(); findChildren(result, repo, request, revId, treeWalk); + return result; } else { - result = findFirstMatch(repo, request, revId, treeWalk); + FileObject result = findFirstMatch(repo, request, revId, treeWalk); if ( result.isDirectory() ) { treeWalk.enterSubtree(); findChildren(result, repo, request, revId, treeWalk); } + return result; } - } - finally - { - GitUtil.release(revWalk); - GitUtil.release(treeWalk); - } - - return result; } private boolean isRootRequest(BrowseCommandRequest request) { @@ -368,7 +286,6 @@ public class GitBrowseCommand extends AbstractGitCommand throw notFound(entity("File", request.getPath()).in("Revision", revId.getName()).in(this.repository)); } - @SuppressWarnings("unchecked") private Map getSubRepositories(org.eclipse.jgit.lib.Repository repo, ObjectId revision) @@ -443,9 +360,9 @@ public class GitBrowseCommand extends AbstractGitCommand @Override public void accept(SyncAsyncExecutor.ExecutionType executionType) { logger.trace("fetch last commit for {} at {}", path, revId.getName()); - RevCommit commit = GitBrowseCommand.this.getLatestCommit(repo, revId, path); + Optional commit = getLatestCommit(repo, revId, path); - Optional lfsPointer = getLfsPointer(commit); + Optional lfsPointer = commit.flatMap(this::getLfsPointer); if (lfsPointer.isPresent()) { setFileLengthFromLfsBlob(lfsPointer.get()); @@ -454,13 +371,28 @@ public class GitBrowseCommand extends AbstractGitCommand } file.setPartialResult(false); - if (commit != null) { - applyValuesFromCommit(executionType, commit); + if (commit.isPresent()) { + applyValuesFromCommit(executionType, commit.get()); } else { logger.warn("could not find latest commit for {} on {}", path, revId); } } + private Optional getLatestCommit(org.eclipse.jgit.lib.Repository repo, + ObjectId revId, String path) { + try (RevWalk walk = new RevWalk(repo)) { + walk.setTreeFilter(AndTreeFilter.create(TreeFilter.ANY_DIFF, PathFilter.create(path))); + + RevCommit commit = walk.parseCommit(revId); + + walk.markStart(commit); + return of(Util.getFirst(walk)); + } catch (IOException ex) { + logger.error("could not parse commit for file", ex); + return empty(); + } + } + private void setFileLengthFromLfsBlob(LfsPointer lfsPointer) { BlobStore lfsBlobStore = lfsBlobStoreFactory.getLfsBlobStore(repository); String oid = lfsPointer.getOid().getName(); @@ -477,19 +409,16 @@ public class GitBrowseCommand extends AbstractGitCommand file.setLastModified(GitUtil.getCommitTime(commit)); file.setDescription(commit.getShortMessage()); if (executionType == ASYNCHRONOUS && browserResult != null) { - request.updateCache(browserResult); - logger.info("updated browser result for repository {}", repository.getNamespaceAndName()); + updateCache(request); } } private Optional getLfsPointer(RevCommit commit) { - Optional lfsPointer; try { - lfsPointer = commit == null ? empty() : GitUtil.getLfsPointer(repo, path, commit, treeWalk); + return GitUtil.getLfsPointer(repo, path, commit, treeWalk); } catch (IOException e) { throw new InternalRepositoryException(repository, "could not read lfs pointer", e); } - return lfsPointer; } } @@ -506,8 +435,7 @@ public class GitBrowseCommand extends AbstractGitCommand public void run() { file.setPartialResult(false); file.setComputationAborted(true); - request.updateCache(browserResult); - logger.info("updated browser result for repository {}", repository.getNamespaceAndName()); + updateCache(request); } } } From ee0972ef34adeb3149a7ba61db74f1cb23564d44 Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Thu, 12 Dec 2019 17:31:55 +0100 Subject: [PATCH 18/35] synchronize cache updates --- .../scm/repository/spi/GitBrowseCommand.java | 43 +++++++++++-------- 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java index 7927d38b2c..75c8631975 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java @@ -96,6 +96,8 @@ public class GitBrowseCommand extends AbstractGitCommand */ private static final Logger logger = LoggerFactory.getLogger(GitBrowseCommand.class); + private static final Object asyncMonitor = new Object(); + private final LfsBlobStoreFactory lfsBlobStoreFactory; private final SyncAsyncExecutor executor; @@ -118,11 +120,6 @@ public class GitBrowseCommand extends AbstractGitCommand if (revId != null) { browserResult = new BrowserResult(revId.getName(), request.getRevision(), getEntry(repo, request, revId)); - executor.execute(executionType -> { - if (executionType == ASYNCHRONOUS) { - updateCache(request); - } - }, () -> {}); return browserResult; } else { logger.warn("could not find head of repository {}, empty?", repository.getNamespaceAndName()); @@ -364,17 +361,19 @@ public class GitBrowseCommand extends AbstractGitCommand Optional lfsPointer = commit.flatMap(this::getLfsPointer); - if (lfsPointer.isPresent()) { - setFileLengthFromLfsBlob(lfsPointer.get()); - } else { - file.setLength(loader.getSize()); - } + synchronized (asyncMonitor) { + if (lfsPointer.isPresent()) { + setFileLengthFromLfsBlob(lfsPointer.get()); + } else { + file.setLength(loader.getSize()); + } - file.setPartialResult(false); - if (commit.isPresent()) { - applyValuesFromCommit(executionType, commit.get()); - } else { - logger.warn("could not find latest commit for {} on {}", path, revId); + file.setPartialResult(false); + if (commit.isPresent()) { + applyValuesFromCommit(executionType, commit.get()); + } else { + logger.warn("could not find latest commit for {} on {}", path, revId); + } } } @@ -433,9 +432,17 @@ public class GitBrowseCommand extends AbstractGitCommand @Override public void run() { - file.setPartialResult(false); - file.setComputationAborted(true); - updateCache(request); + synchronized (asyncMonitor) { + if (browserResult.getFile().getChildren().stream().anyMatch(FileObject::isPartialResult)) { + browserResult.getFile().getChildren().stream().filter(FileObject::isPartialResult).forEach( + f -> { + f.setPartialResult(false); + f.setComputationAborted(true); + } + ); + updateCache(request); + } + } } } } From c8a115a37352ec2537e2575e1468b2469e451425 Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Fri, 13 Dec 2019 08:20:26 +0100 Subject: [PATCH 19/35] Mark files aborted recursively --- .../scm/repository/spi/GitBrowseCommand.java | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java index 75c8631975..c95fa13d95 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java @@ -185,7 +185,7 @@ public class GitBrowseCommand extends AbstractGitCommand file.setPartialResult(true); executor.execute( new CompleteFileInformation(path, revId, repo, treeWalk, file, loader, request), - new AbortFileInformation(file, request) + new AbortFileInformation(request) ); } } @@ -422,27 +422,32 @@ public class GitBrowseCommand extends AbstractGitCommand } private class AbortFileInformation implements Runnable { - private final FileObject file; private final BrowseCommandRequest request; - public AbortFileInformation(FileObject file, BrowseCommandRequest request) { - this.file = file; + public AbortFileInformation(BrowseCommandRequest request) { this.request = request; } @Override public void run() { synchronized (asyncMonitor) { - if (browserResult.getFile().getChildren().stream().anyMatch(FileObject::isPartialResult)) { - browserResult.getFile().getChildren().stream().filter(FileObject::isPartialResult).forEach( - f -> { - f.setPartialResult(false); - f.setComputationAborted(true); - } - ); + if (markPartialAsAborted(browserResult.getFile())) { updateCache(request); } } } + + private boolean markPartialAsAborted(FileObject file) { + boolean changed = false; + if (file.isPartialResult()) { + file.setPartialResult(false); + file.setComputationAborted(true); + changed = true; + } + for (FileObject child : file.getChildren()) { + changed |= markPartialAsAborted(child); + } + return changed; + } } } From 0374aad7a749764d04ee19570aec9f8e5ca3edc4 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Fri, 13 Dec 2019 13:46:20 +0100 Subject: [PATCH 20/35] DefaultSyncAsyncExecutorProvider should be a Singleton --- .../scm/repository/DefaultSyncAsyncExecutorProvider.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutorProvider.java b/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutorProvider.java index 4c1f0429da..1727fff970 100644 --- a/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutorProvider.java +++ b/scm-webapp/src/main/java/sonia/scm/repository/DefaultSyncAsyncExecutorProvider.java @@ -3,12 +3,15 @@ package sonia.scm.repository; import sonia.scm.repository.spi.SyncAsyncExecutor; import sonia.scm.repository.spi.SyncAsyncExecutorProvider; +import javax.inject.Inject; +import javax.inject.Singleton; import java.io.Closeable; import java.time.Instant; import java.time.temporal.ChronoUnit; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +@Singleton public class DefaultSyncAsyncExecutorProvider implements SyncAsyncExecutorProvider, Closeable { public static final int DEFAULT_MAX_ASYNC_ABORT_SECONDS = 60; @@ -20,6 +23,7 @@ public class DefaultSyncAsyncExecutorProvider implements SyncAsyncExecutorProvid private final ExecutorService executor; private final int defaultMaxAsyncAbortSeconds; + @Inject public DefaultSyncAsyncExecutorProvider() { this(Executors.newFixedThreadPool(getProperty(NUMBER_OF_THREADS_PROPERTY, DEFAULT_NUMBER_OF_THREADS))); } @@ -30,7 +34,7 @@ public class DefaultSyncAsyncExecutorProvider implements SyncAsyncExecutorProvid } public SyncAsyncExecutor createExecutorWithSecondsToTimeout(int switchToAsyncInSeconds) { - return createExecutorWithSecondsToTimeout(switchToAsyncInSeconds, DEFAULT_MAX_ASYNC_ABORT_SECONDS); + return createExecutorWithSecondsToTimeout(switchToAsyncInSeconds, defaultMaxAsyncAbortSeconds); } public SyncAsyncExecutor createExecutorWithSecondsToTimeout(int switchToAsyncInSeconds, int maxAsyncAbortSeconds) { From 8410e7e679dcb160ce05edae9069e3efcaf59071 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Fri, 13 Dec 2019 13:52:08 +0100 Subject: [PATCH 21/35] Log duration of commit message calculation and do not synchronize over all instances --- .../scm/repository/spi/GitBrowseCommand.java | 69 ++++++++----------- 1 file changed, 27 insertions(+), 42 deletions(-) diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java index c95fa13d95..b0ec131b9c 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java @@ -35,6 +35,7 @@ package sonia.scm.repository.spi; //~--- non-JDK imports -------------------------------------------------------- +import com.google.common.base.Stopwatch; import com.google.common.base.Strings; import com.google.common.collect.Lists; import com.google.common.collect.Maps; @@ -94,9 +95,12 @@ public class GitBrowseCommand extends AbstractGitCommand /** * the logger for GitBrowseCommand */ - private static final Logger logger = - LoggerFactory.getLogger(GitBrowseCommand.class); - private static final Object asyncMonitor = new Object(); + private static final Logger logger = LoggerFactory.getLogger(GitBrowseCommand.class); + + /** sub repository cache */ + private final Map> subrepositoryCache = Maps.newHashMap(); + + private final Object asyncMonitor = new Object(); private final LfsBlobStoreFactory lfsBlobStoreFactory; @@ -123,12 +127,11 @@ public class GitBrowseCommand extends AbstractGitCommand return browserResult; } else { logger.warn("could not find head of repository {}, empty?", repository.getNamespaceAndName()); - return new BrowserResult(Constants.HEAD, request.getRevision(), createEmtpyRoot()); + return new BrowserResult(Constants.HEAD, request.getRevision(), createEmptyRoot()); } } private ObjectId computeRevIdToBrowse(BrowseCommandRequest request, org.eclipse.jgit.lib.Repository repo) throws IOException { - if (Util.isEmpty(request.getRevision())) { return getDefaultBranch(repo); } else { @@ -141,7 +144,7 @@ public class GitBrowseCommand extends AbstractGitCommand } } - private FileObject createEmtpyRoot() { + private FileObject createEmptyRoot() { FileObject fileObject = new FileObject(); fileObject.setName(""); fileObject.setPath(""); @@ -198,7 +201,6 @@ public class GitBrowseCommand extends AbstractGitCommand } private FileObject getEntry(org.eclipse.jgit.lib.Repository repo, BrowseCommandRequest request, ObjectId revId) throws IOException { - try (RevWalk revWalk = new RevWalk(repo); TreeWalk treeWalk = new TreeWalk(repo)) { logger.debug("load repository browser for revision {}", revId.name()); @@ -215,7 +217,7 @@ public class GitBrowseCommand extends AbstractGitCommand } if (isRootRequest(request)) { - FileObject result = createEmtpyRoot(); + FileObject result = createEmptyRoot(); findChildren(result, repo, request, revId, treeWalk); return result; } else { @@ -283,58 +285,36 @@ public class GitBrowseCommand extends AbstractGitCommand throw notFound(entity("File", request.getPath()).in("Revision", revId.getName()).in(this.repository)); } - private Map getSubRepositories(org.eclipse.jgit.lib.Repository repo, - ObjectId revision) + private Map getSubRepositories(org.eclipse.jgit.lib.Repository repo, ObjectId revision) throws IOException { - if (logger.isDebugEnabled()) - { - logger.debug("read submodules of {} at {}", repository.getName(), - revision); - } - Map subRepositories; - try ( ByteArrayOutputStream baos = new ByteArrayOutputStream() ) - { + logger.debug("read submodules of {} at {}", repository.getName(), revision); + + try ( ByteArrayOutputStream baos = new ByteArrayOutputStream() ) { new GitCatCommand(context, repository, lfsBlobStoreFactory).getContent(repo, revision, PATH_MODULES, baos); - subRepositories = GitSubModuleParser.parse(baos.toString()); - } - catch (NotFoundException ex) - { + return GitSubModuleParser.parse(baos.toString()); + } catch (NotFoundException ex) { logger.trace("could not find .gitmodules: {}", ex.getMessage()); - subRepositories = Collections.emptyMap(); + return Collections.emptyMap(); } - - return subRepositories; } - private SubRepository getSubRepository(org.eclipse.jgit.lib.Repository repo, - ObjectId revId, String path) + private SubRepository getSubRepository(org.eclipse.jgit.lib.Repository repo, ObjectId revId, String path) throws IOException { Map subRepositories = subrepositoryCache.get(revId); - if (subRepositories == null) - { + if (subRepositories == null) { subRepositories = getSubRepositories(repo, revId); subrepositoryCache.put(revId, subRepositories); } - SubRepository sub = null; - - if (subRepositories != null) - { - sub = subRepositories.get(path); + if (subRepositories != null) { + return subRepositories.get(path); } - - return sub; + return null; } - //~--- fields --------------------------------------------------------------- - - /** sub repository cache */ - private final Map> subrepositoryCache = Maps.newHashMap(); - private class CompleteFileInformation implements Consumer { private final String path; private final ObjectId revId; @@ -357,6 +337,9 @@ public class GitBrowseCommand extends AbstractGitCommand @Override public void accept(SyncAsyncExecutor.ExecutionType executionType) { logger.trace("fetch last commit for {} at {}", path, revId.getName()); + + Stopwatch sw = Stopwatch.createStarted(); + Optional commit = getLatestCommit(repo, revId, path); Optional lfsPointer = commit.flatMap(this::getLfsPointer); @@ -375,6 +358,8 @@ public class GitBrowseCommand extends AbstractGitCommand logger.warn("could not find latest commit for {} on {}", path, revId); } } + + logger.trace("finished loading of last commit {} of {} in {}", revId.getName(), path, sw.stop()); } private Optional getLatestCommit(org.eclipse.jgit.lib.Repository repo, From 5e47ef0323cbc9445cf7177b3af2275f88b598aa Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Fri, 13 Dec 2019 14:41:36 +0100 Subject: [PATCH 22/35] Compute LFS attributes on top commit In the previous version, the LFS attributes were read for the latest commit of the file. This is not the way, a git client handles LFS files. Therefore we switch to the way, the native git client works and read the attributes from the commit of the command. --- .../java/sonia/scm/repository/GitUtil.java | 4 ++ .../scm/repository/spi/GitBrowseCommand.java | 72 ++++++++++--------- 2 files changed, 42 insertions(+), 34 deletions(-) diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/GitUtil.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/GitUtil.java index d726b992ca..a93c1b5d81 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/GitUtil.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/GitUtil.java @@ -745,6 +745,10 @@ public final class GitUtil public static Optional getLfsPointer(org.eclipse.jgit.lib.Repository repo, String path, RevCommit commit, TreeWalk treeWalk) throws IOException { Attributes attributes = LfsFactory.getAttributesForPath(repo, path, commit); + return getLfsPointer(repo, treeWalk, attributes); + } + + public static Optional getLfsPointer(org.eclipse.jgit.lib.Repository repo, TreeWalk treeWalk, Attributes attributes) throws IOException { Attribute filter = attributes.get("filter"); if (filter != null && "lfs".equals(filter.getValue())) { ObjectId blobId = treeWalk.getObjectId(0); diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java index b0ec131b9c..d13991558a 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java @@ -39,6 +39,7 @@ import com.google.common.base.Stopwatch; import com.google.common.base.Strings; import com.google.common.collect.Lists; import com.google.common.collect.Maps; +import org.eclipse.jgit.attributes.Attributes; import org.eclipse.jgit.lfs.LfsPointer; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; @@ -50,6 +51,7 @@ import org.eclipse.jgit.treewalk.TreeWalk; import org.eclipse.jgit.treewalk.filter.AndTreeFilter; import org.eclipse.jgit.treewalk.filter.PathFilter; import org.eclipse.jgit.treewalk.filter.TreeFilter; +import org.eclipse.jgit.util.LfsFactory; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import sonia.scm.NotFoundException; @@ -186,8 +188,20 @@ public class GitBrowseCommand extends AbstractGitCommand if (!file.isDirectory() &&!request.isDisableLastCommit()) { file.setPartialResult(true); + RevCommit commit; + try (RevWalk walk = new RevWalk(repo)) { + commit = walk.parseCommit(revId); + } + Optional lfsPointer = getLfsPointer(repo, path, commit, treeWalk); + + if (lfsPointer.isPresent()) { + setFileLengthFromLfsBlob(lfsPointer.get(), file); + } else { + file.setLength(loader.getSize()); + } + executor.execute( - new CompleteFileInformation(path, revId, repo, treeWalk, file, loader, request), + new CompleteFileInformation(path, revId, repo, file, request), new AbortFileInformation(request) ); } @@ -315,22 +329,40 @@ public class GitBrowseCommand extends AbstractGitCommand return null; } + private Optional getLfsPointer(org.eclipse.jgit.lib.Repository repo, String path, RevCommit commit, TreeWalk treeWalk) { + try { + Attributes attributes = LfsFactory.getAttributesForPath(repo, path, commit); + + return GitUtil.getLfsPointer(repo, treeWalk, attributes); + } catch (IOException e) { + throw new InternalRepositoryException(repository, "could not read lfs pointer", e); + } + } + + private void setFileLengthFromLfsBlob(LfsPointer lfsPointer, FileObject file) { + BlobStore lfsBlobStore = lfsBlobStoreFactory.getLfsBlobStore(repository); + String oid = lfsPointer.getOid().getName(); + Blob blob = lfsBlobStore.get(oid); + if (blob == null) { + logger.error("lfs blob for lob id {} not found in lfs store of repository {}", oid, repository.getNamespaceAndName()); + file.setLength(-1); + } else { + file.setLength(blob.getSize()); + } + } + private class CompleteFileInformation implements Consumer { private final String path; private final ObjectId revId; private final org.eclipse.jgit.lib.Repository repo; - private final TreeWalk treeWalk; private final FileObject file; - private final ObjectLoader loader; private final BrowseCommandRequest request; - public CompleteFileInformation(String path, ObjectId revId, org.eclipse.jgit.lib.Repository repo, TreeWalk treeWalk, FileObject file, ObjectLoader loader, BrowseCommandRequest request) { + public CompleteFileInformation(String path, ObjectId revId, org.eclipse.jgit.lib.Repository repo, FileObject file, BrowseCommandRequest request) { this.path = path; this.revId = revId; this.repo = repo; - this.treeWalk = treeWalk; this.file = file; - this.loader = loader; this.request = request; } @@ -342,15 +374,7 @@ public class GitBrowseCommand extends AbstractGitCommand Optional commit = getLatestCommit(repo, revId, path); - Optional lfsPointer = commit.flatMap(this::getLfsPointer); - synchronized (asyncMonitor) { - if (lfsPointer.isPresent()) { - setFileLengthFromLfsBlob(lfsPointer.get()); - } else { - file.setLength(loader.getSize()); - } - file.setPartialResult(false); if (commit.isPresent()) { applyValuesFromCommit(executionType, commit.get()); @@ -377,18 +401,6 @@ public class GitBrowseCommand extends AbstractGitCommand } } - private void setFileLengthFromLfsBlob(LfsPointer lfsPointer) { - BlobStore lfsBlobStore = lfsBlobStoreFactory.getLfsBlobStore(repository); - String oid = lfsPointer.getOid().getName(); - Blob blob = lfsBlobStore.get(oid); - if (blob == null) { - logger.error("lfs blob for lob id {} not found in lfs store of repository {}", oid, repository.getNamespaceAndName()); - file.setLength(-1); - } else { - file.setLength(blob.getSize()); - } - } - private void applyValuesFromCommit(SyncAsyncExecutor.ExecutionType executionType, RevCommit commit) { file.setLastModified(GitUtil.getCommitTime(commit)); file.setDescription(commit.getShortMessage()); @@ -396,14 +408,6 @@ public class GitBrowseCommand extends AbstractGitCommand updateCache(request); } } - - private Optional getLfsPointer(RevCommit commit) { - try { - return GitUtil.getLfsPointer(repo, path, commit, treeWalk); - } catch (IOException e) { - throw new InternalRepositoryException(repository, "could not read lfs pointer", e); - } - } } private class AbortFileInformation implements Runnable { From 7bd48c91bc35b5787f34aa5572a6972f906dc0ba Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Fri, 13 Dec 2019 14:41:55 +0100 Subject: [PATCH 23/35] Fix error for empty repositories --- scm-ui/ui-webapp/src/repos/sources/modules/sources.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts b/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts index 7d2b8c94a7..7977d9f3e0 100644 --- a/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts +++ b/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts @@ -28,7 +28,7 @@ export function fetchSourcesWithoutOptionalLoadingState( .then(response => response.json()) .then((sources: File) => { dispatch(fetchSourcesSuccess(repository, revision, path, sources)); - if (sources._embedded.children && sources._embedded.children.find(c => c.partialResult)) { + if (sources._embedded && sources._embedded.children && sources._embedded.children.find(c => c.partialResult)) { setTimeout(() => dispatch(fetchSourcesWithoutOptionalLoadingState(repository, revision, path, false)), 3000); } }) From 4ef97816e0c61cb72c3338c263e8fc4e20a326f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Tue, 17 Dec 2019 10:25:53 +0100 Subject: [PATCH 24/35] Trigger update of sources from component The old trigger in the dispatcher function led to updates even when the component was no longer mounted (aka displayed). --- .../src/repos/sources/components/FileTree.tsx | 21 ++++++++- .../src/repos/sources/modules/sources.ts | 45 +++++++++++++------ 2 files changed, 50 insertions(+), 16 deletions(-) diff --git a/scm-ui/ui-webapp/src/repos/sources/components/FileTree.tsx b/scm-ui/ui-webapp/src/repos/sources/components/FileTree.tsx index 150e4d1eb6..419171b301 100644 --- a/scm-ui/ui-webapp/src/repos/sources/components/FileTree.tsx +++ b/scm-ui/ui-webapp/src/repos/sources/components/FileTree.tsx @@ -7,7 +7,7 @@ import styled from "styled-components"; import { binder } from "@scm-manager/ui-extensions"; import { Repository, File } from "@scm-manager/ui-types"; import { ErrorNotification, Loading, Notification } from "@scm-manager/ui-components"; -import { getFetchSourcesFailure, isFetchSourcesPending, getSources } from "../modules/sources"; +import { getFetchSourcesFailure, isFetchSourcesPending, getSources, fetchSources } from "../modules/sources"; import FileTreeLeaf from "./FileTreeLeaf"; type Props = WithTranslation & { @@ -19,6 +19,8 @@ type Props = WithTranslation & { path: string; baseUrl: string; + updateSources: () => void; + // context props match: any; }; @@ -40,6 +42,13 @@ export function findParent(path: string) { } class FileTree extends React.Component { + componentDidUpdate(prevProps: Readonly, prevState: Readonly<{}>, snapshot?: any): void { + const { tree, updateSources } = this.props; + if (tree?._embedded?.children && tree._embedded.children.find(c => c.partialResult)) { + setTimeout(updateSources, 3000); + } + } + render() { const { error, loading, tree } = this.props; @@ -123,6 +132,14 @@ class FileTree extends React.Component { } } +const mapDispatchToProps = (dispatch: any, ownProps: Props) => { + const { repository, revision, path } = ownProps; + + const updateSources = () => dispatch(fetchSources(repository, revision, path, false)); + + return { updateSources }; +}; + const mapStateToProps = (state: any, ownProps: Props) => { const { repository, revision, path } = ownProps; @@ -141,5 +158,5 @@ const mapStateToProps = (state: any, ownProps: Props) => { export default compose( withRouter, - connect(mapStateToProps) + connect(mapStateToProps, mapDispatchToProps) )(withTranslation("repos")(FileTree)); diff --git a/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts b/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts index 7d2b8c94a7..e6ab2836b4 100644 --- a/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts +++ b/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts @@ -9,28 +9,29 @@ export const FETCH_SOURCES_PENDING = `${FETCH_SOURCES}_${types.PENDING_SUFFIX}`; export const FETCH_SOURCES_SUCCESS = `${FETCH_SOURCES}_${types.SUCCESS_SUFFIX}`; export const FETCH_SOURCES_FAILURE = `${FETCH_SOURCES}_${types.FAILURE_SUFFIX}`; -export function fetchSources(repository: Repository, revision: string, path: string) { - return fetchSourcesWithoutOptionalLoadingState(repository, revision, path, true); -} - -export function fetchSourcesWithoutOptionalLoadingState( +export function fetchSources( repository: Repository, revision: string, path: string, - dispatchLoading: boolean + initialLoad = true ) { - return function(dispatch: any) { - if (dispatchLoading) { + return function(dispatch: any, getState: () => any) { + const state = getState(); + if (isFetchSourcesPending(state, repository, revision, path) + || isUpdateSourcePending(state, repository, revision, path)) { + return; + } + + if (initialLoad) { dispatch(fetchSourcesPending(repository, revision, path)); + } else { + dispatch(updateSourcesPending(repository, revision, path)) } return apiClient .get(createUrl(repository, revision, path)) .then(response => response.json()) .then((sources: File) => { dispatch(fetchSourcesSuccess(repository, revision, path, sources)); - if (sources._embedded.children && sources._embedded.children.find(c => c.partialResult)) { - setTimeout(() => dispatch(fetchSourcesWithoutOptionalLoadingState(repository, revision, path, false)), 3000); - } }) .catch(err => { dispatch(fetchSourcesFailure(repository, revision, path, err)); @@ -56,10 +57,17 @@ export function fetchSourcesPending(repository: Repository, revision: string, pa }; } +export function updateSourcesPending(repository: Repository, revision: string, path: string): Action { + return { + type: "UPDATE_PENDING", + itemId: createItemId(repository, revision, path) + }; +} + export function fetchSourcesSuccess(repository: Repository, revision: string, path: string, sources: File) { return { type: FETCH_SOURCES_SUCCESS, - payload: sources, + payload: { updatePending: false, sources }, itemId: createItemId(repository, revision, path) }; } @@ -91,6 +99,11 @@ export default function reducer( ...state, [action.itemId]: action.payload }; + } else if (action.itemId && action.type === "UPDATE_PENDING") { + return { + ...state, + [action.itemId]: { updatePending: true }} + ; } return state; } @@ -113,13 +126,17 @@ export function getSources( path: string ): File | null | undefined { if (state.sources) { - return state.sources[createItemId(repository, revision, path)]; + return state.sources[createItemId(repository, revision, path)]?.sources; } return null; } export function isFetchSourcesPending(state: any, repository: Repository, revision: string, path: string): boolean { - return isPending(state, FETCH_SOURCES, createItemId(repository, revision, path)); + return state && isPending(state, FETCH_SOURCES, createItemId(repository, revision, path)); +} + +function isUpdateSourcePending(state: any, repository: Repository, revision: string, path: string): boolean { + return state?.sources[createItemId(repository, revision, path)]?.updatePending; } export function getFetchSourcesFailure( From c72bcb04c1e62f769314352ffd1ef3850ca22c6a Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Tue, 17 Dec 2019 12:14:43 +0100 Subject: [PATCH 25/35] Cancel update call is component will be unmounted --- .../src/repos/sources/components/FileTree.tsx | 29 +++++++++++++++---- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/scm-ui/ui-webapp/src/repos/sources/components/FileTree.tsx b/scm-ui/ui-webapp/src/repos/sources/components/FileTree.tsx index 419171b301..7983fdd32e 100644 --- a/scm-ui/ui-webapp/src/repos/sources/components/FileTree.tsx +++ b/scm-ui/ui-webapp/src/repos/sources/components/FileTree.tsx @@ -25,6 +25,10 @@ type Props = WithTranslation & { match: any; }; +type State = { + stoppableUpdateHandler?: number; +} + const FixedWidthTh = styled.th` width: 16px; `; @@ -41,11 +45,26 @@ export function findParent(path: string) { return ""; } -class FileTree extends React.Component { - componentDidUpdate(prevProps: Readonly, prevState: Readonly<{}>, snapshot?: any): void { - const { tree, updateSources } = this.props; - if (tree?._embedded?.children && tree._embedded.children.find(c => c.partialResult)) { - setTimeout(updateSources, 3000); +class FileTree extends React.Component { + + constructor(props: Props) { + super(props); + this.state = {}; + } + + componentDidUpdate(prevProps: Readonly, prevState: Readonly): void { + if (prevState.stoppableUpdateHandler === this.state.stoppableUpdateHandler) { + const {tree, updateSources} = this.props; + if (tree?._embedded?.children && tree._embedded.children.find(c => c.partialResult)) { + const stoppableUpdateHandler = setTimeout(updateSources, 3000); + this.setState({stoppableUpdateHandler: stoppableUpdateHandler}); + } + } + } + + componentWillUnmount(): void { + if (this.state.stoppableUpdateHandler) { + clearTimeout(this.state.stoppableUpdateHandler); } } From 7da7a6881739e504c671f23443535d258b63f226 Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Tue, 17 Dec 2019 12:36:58 +0100 Subject: [PATCH 26/35] Prevent "flicker" of tree while loading update --- .../ui-webapp/src/repos/sources/modules/sources.ts | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts b/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts index e6ab2836b4..f976928e9f 100644 --- a/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts +++ b/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts @@ -25,7 +25,7 @@ export function fetchSources( if (initialLoad) { dispatch(fetchSourcesPending(repository, revision, path)); } else { - dispatch(updateSourcesPending(repository, revision, path)) + dispatch(updateSourcesPending(repository, revision, path, getSources(state, repository, revision, path))); } return apiClient .get(createUrl(repository, revision, path)) @@ -57,9 +57,10 @@ export function fetchSourcesPending(repository: Repository, revision: string, pa }; } -export function updateSourcesPending(repository: Repository, revision: string, path: string): Action { +export function updateSourcesPending(repository: Repository, revision: string, path: string, currentSources: any): Action { return { type: "UPDATE_PENDING", + payload: { updatePending: true, sources: currentSources}, itemId: createItemId(repository, revision, path) }; } @@ -94,16 +95,11 @@ export default function reducer( type: "UNKNOWN" } ): any { - if (action.itemId && action.type === FETCH_SOURCES_SUCCESS) { + if (action.itemId && (action.type === FETCH_SOURCES_SUCCESS || action.type === "UPDATE_PENDING")) { return { ...state, [action.itemId]: action.payload }; - } else if (action.itemId && action.type === "UPDATE_PENDING") { - return { - ...state, - [action.itemId]: { updatePending: true }} - ; } return state; } From f257a8eeb86f4c71edbc01ae62af6ac1fa40fb3f Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Tue, 17 Dec 2019 12:42:05 +0100 Subject: [PATCH 27/35] Fix formatting --- .../src/repos/sources/components/FileTree.tsx | 12 +++++----- .../src/repos/sources/modules/sources.ts | 22 ++++++++++--------- 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/scm-ui/ui-webapp/src/repos/sources/components/FileTree.tsx b/scm-ui/ui-webapp/src/repos/sources/components/FileTree.tsx index 7983fdd32e..fee6f985cd 100644 --- a/scm-ui/ui-webapp/src/repos/sources/components/FileTree.tsx +++ b/scm-ui/ui-webapp/src/repos/sources/components/FileTree.tsx @@ -27,7 +27,7 @@ type Props = WithTranslation & { type State = { stoppableUpdateHandler?: number; -} +}; const FixedWidthTh = styled.th` width: 16px; @@ -46,7 +46,6 @@ export function findParent(path: string) { } class FileTree extends React.Component { - constructor(props: Props) { super(props); this.state = {}; @@ -54,10 +53,10 @@ class FileTree extends React.Component { componentDidUpdate(prevProps: Readonly, prevState: Readonly): void { if (prevState.stoppableUpdateHandler === this.state.stoppableUpdateHandler) { - const {tree, updateSources} = this.props; + const { tree, updateSources } = this.props; if (tree?._embedded?.children && tree._embedded.children.find(c => c.partialResult)) { const stoppableUpdateHandler = setTimeout(updateSources, 3000); - this.setState({stoppableUpdateHandler: stoppableUpdateHandler}); + this.setState({ stoppableUpdateHandler: stoppableUpdateHandler }); } } } @@ -177,5 +176,8 @@ const mapStateToProps = (state: any, ownProps: Props) => { export default compose( withRouter, - connect(mapStateToProps, mapDispatchToProps) + connect( + mapStateToProps, + mapDispatchToProps + ) )(withTranslation("repos")(FileTree)); diff --git a/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts b/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts index f976928e9f..8af2cc33cc 100644 --- a/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts +++ b/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts @@ -9,16 +9,13 @@ export const FETCH_SOURCES_PENDING = `${FETCH_SOURCES}_${types.PENDING_SUFFIX}`; export const FETCH_SOURCES_SUCCESS = `${FETCH_SOURCES}_${types.SUCCESS_SUFFIX}`; export const FETCH_SOURCES_FAILURE = `${FETCH_SOURCES}_${types.FAILURE_SUFFIX}`; -export function fetchSources( - repository: Repository, - revision: string, - path: string, - initialLoad = true -) { +`export function fetchSources(repository: Repository, revision: string, path: string, initialLoad = true) { return function(dispatch: any, getState: () => any) { const state = getState(); - if (isFetchSourcesPending(state, repository, revision, path) - || isUpdateSourcePending(state, repository, revision, path)) { + if ( + isFetchSourcesPending(state, repository, revision, path) || + isUpdateSourcePending(state, repository, revision, path) + ) { return; } @@ -57,10 +54,15 @@ export function fetchSourcesPending(repository: Repository, revision: string, pa }; } -export function updateSourcesPending(repository: Repository, revision: string, path: string, currentSources: any): Action { +export function updateSourcesPending( + repository: Repository, + revision: string, + path: string, + currentSources: any +): Action { return { type: "UPDATE_PENDING", - payload: { updatePending: true, sources: currentSources}, + payload: { updatePending: true, sources: currentSources }, itemId: createItemId(repository, revision, path) }; } From a61799723b834846120e7a5b17459f62615ccb9f Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Tue, 17 Dec 2019 12:42:31 +0100 Subject: [PATCH 28/35] Fix typo --- scm-ui/ui-webapp/src/repos/sources/modules/sources.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts b/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts index 8af2cc33cc..8a04dcdc76 100644 --- a/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts +++ b/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts @@ -9,7 +9,7 @@ export const FETCH_SOURCES_PENDING = `${FETCH_SOURCES}_${types.PENDING_SUFFIX}`; export const FETCH_SOURCES_SUCCESS = `${FETCH_SOURCES}_${types.SUCCESS_SUFFIX}`; export const FETCH_SOURCES_FAILURE = `${FETCH_SOURCES}_${types.FAILURE_SUFFIX}`; -`export function fetchSources(repository: Repository, revision: string, path: string, initialLoad = true) { +export function fetchSources(repository: Repository, revision: string, path: string, initialLoad = true) { return function(dispatch: any, getState: () => any) { const state = getState(); if ( From 45b989cd1afc061e2395a98e38f1ab37111d082a Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Tue, 17 Dec 2019 13:03:05 +0100 Subject: [PATCH 29/35] Always show file size The file size is always present and not updated in the background --- scm-ui/ui-webapp/src/repos/sources/components/FileTreeLeaf.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scm-ui/ui-webapp/src/repos/sources/components/FileTreeLeaf.tsx b/scm-ui/ui-webapp/src/repos/sources/components/FileTreeLeaf.tsx index 9190a02dd6..037d8f7c45 100644 --- a/scm-ui/ui-webapp/src/repos/sources/components/FileTreeLeaf.tsx +++ b/scm-ui/ui-webapp/src/repos/sources/components/FileTreeLeaf.tsx @@ -88,7 +88,7 @@ class FileTreeLeaf extends React.Component { {this.createFileIcon(file)} {this.createFileName(file)} - {this.contentIfPresent(file, fileSize)} + {fileSize} {this.contentIfPresent(file, )} {this.contentIfPresent(file, file.description)} From 5ea149c2622b1d4f8c4f4c4932f0b88dc9198b7d Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Tue, 17 Dec 2019 13:34:28 +0100 Subject: [PATCH 30/35] Adapt tests to new state layout --- .../src/repos/sources/modules/sources.test.ts | 18 +++++++++++------- .../src/repos/sources/modules/sources.ts | 2 +- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/scm-ui/ui-webapp/src/repos/sources/modules/sources.test.ts b/scm-ui/ui-webapp/src/repos/sources/modules/sources.test.ts index 55737f7ee0..fd676d7c57 100644 --- a/scm-ui/ui-webapp/src/repos/sources/modules/sources.test.ts +++ b/scm-ui/ui-webapp/src/repos/sources/modules/sources.test.ts @@ -127,7 +127,7 @@ describe("sources fetch", () => { { type: FETCH_SOURCES_SUCCESS, itemId: "scm/core/_/", - payload: collection + payload: { updatePending: false, sources: collection } } ]; @@ -148,7 +148,7 @@ describe("sources fetch", () => { { type: FETCH_SOURCES_SUCCESS, itemId: "scm/core/abc/src", - payload: collection + payload: { updatePending: false, sources: collection } } ]; @@ -182,14 +182,14 @@ describe("reducer tests", () => { it("should store the collection, without revision and path", () => { const expectedState = { - "scm/core/_/": collection + "scm/core/_/": { updatePending: false, sources: collection } }; expect(reducer({}, fetchSourcesSuccess(repository, "", "", collection))).toEqual(expectedState); }); it("should store the collection, with revision and path", () => { const expectedState = { - "scm/core/abc/src/main": collection + "scm/core/abc/src/main": { updatePending: false, sources: collection } }; expect(reducer({}, fetchSourcesSuccess(repository, "abc", "src/main", collection))).toEqual(expectedState); }); @@ -200,7 +200,7 @@ describe("selector tests", () => { const state = { sources: { "scm/core/abc/src/main/package.json": { - noDirectory + sources: {noDirectory} } } }; @@ -223,7 +223,9 @@ describe("selector tests", () => { it("should return the source collection without revision and path", () => { const state = { sources: { - "scm/core/_/": collection + "scm/core/_/": { + sources: collection + } } }; expect(getSources(state, repository, "", "")).toBe(collection); @@ -232,7 +234,9 @@ describe("selector tests", () => { it("should return the source collection with revision and path", () => { const state = { sources: { - "scm/core/abc/src/main": collection + "scm/core/abc/src/main": { + sources: collection + } } }; expect(getSources(state, repository, "abc", "src/main")).toBe(collection); diff --git a/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts b/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts index 8a04dcdc76..8abdec9fd5 100644 --- a/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts +++ b/scm-ui/ui-webapp/src/repos/sources/modules/sources.ts @@ -134,7 +134,7 @@ export function isFetchSourcesPending(state: any, repository: Repository, revisi } function isUpdateSourcePending(state: any, repository: Repository, revision: string, path: string): boolean { - return state?.sources[createItemId(repository, revision, path)]?.updatePending; + return state?.sources && state.sources[createItemId(repository, revision, path)]?.updatePending; } export function getFetchSourcesFailure( From cf9d1edb701a868e5125fa9ff1573698058c1472 Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Wed, 18 Dec 2019 11:48:17 +0100 Subject: [PATCH 31/35] Make partial file attributes explicit. --- .../java/sonia/scm/repository/FileObject.java | 140 +++++++++++++----- .../scm/repository/spi/GitBrowseCommand.java | 4 +- .../repository/spi/GitBrowseCommandTest.java | 34 ++--- .../spi/javahg/HgFileviewCommand.java | 4 +- .../repository/spi/HgBrowseCommandTest.java | 24 +-- .../scm/repository/spi/SvnBrowseCommand.java | 2 +- .../repository/spi/SvnBrowseCommandTest.java | 24 +-- scm-ui/ui-types/src/Sources.ts | 4 +- .../src/repos/sources/components/FileTree.tsx | 2 +- .../repos/sources/components/FileTreeLeaf.tsx | 17 ++- .../src/repos/sources/containers/Content.tsx | 2 +- .../src/repos/sources/modules/sources.test.ts | 4 +- .../BrowserResultToFileObjectDtoMapper.java | 11 ++ .../scm/api/v2/resources/FileObjectDto.java | 8 +- ...rowserResultToFileObjectDtoMapperTest.java | 9 +- .../v2/resources/SourceRootResourceTest.java | 4 +- 16 files changed, 190 insertions(+), 103 deletions(-) diff --git a/scm-core/src/main/java/sonia/scm/repository/FileObject.java b/scm-core/src/main/java/sonia/scm/repository/FileObject.java index acb7559094..8f1cf298de 100644 --- a/scm-core/src/main/java/sonia/scm/repository/FileObject.java +++ b/scm-core/src/main/java/sonia/scm/repository/FileObject.java @@ -46,8 +46,11 @@ import java.io.Serializable; import java.util.ArrayList; import java.util.Collection; import java.util.List; +import java.util.Optional; +import java.util.OptionalLong; import static java.util.Collections.unmodifiableCollection; +import static java.util.Optional.ofNullable; /** * The FileObject represents a file or a directory in a repository. @@ -90,7 +93,9 @@ public class FileObject implements LastModifiedAware, Serializable && Objects.equal(description, other.description) && Objects.equal(length, other.length) && Objects.equal(subRepository, other.subRepository) - && Objects.equal(lastModified, other.lastModified); + && Objects.equal(commitDate, other.commitDate) + && Objects.equal(partialResult, other.partialResult) + && Objects.equal(computationAborted, other.computationAborted); //J+ } @@ -100,8 +105,16 @@ public class FileObject implements LastModifiedAware, Serializable @Override public int hashCode() { - return Objects.hashCode(name, path, directory, description, length, - subRepository, lastModified); + return Objects.hashCode( + name, + path, + directory, + description, + length, + subRepository, + commitDate, + partialResult, + computationAborted); } /** @@ -118,7 +131,9 @@ public class FileObject implements LastModifiedAware, Serializable .add("description", description) .add("length", length) .add("subRepository", subRepository) - .add("lastModified", lastModified) + .add("commitDate", commitDate) + .add("partialResult", partialResult) + .add("computationAborted", computationAborted) .toString(); //J+ } @@ -130,35 +145,44 @@ public class FileObject implements LastModifiedAware, Serializable * if the repository provider is not able to get the last commit for the path. * * - * @return last commit message + * @return Last commit message or null, when this value has not been computed + * (see {@link #isPartialResult()}). */ - public String getDescription() + public Optional getDescription() { - return description; + return ofNullable(description); } /** * Returns the last commit date for this. The method will return null, - * if the repository provider is not able to get the last commit for the path. + * if the repository provider is not able to get the last commit for the path + * or it has not been computed. * * * @return last commit date */ @Override - public Long getLastModified() - { - return lastModified; + public Long getLastModified() { + return this.isPartialResult()? null: this.commitDate; } /** - * Returns the length of the file. - * - * - * @return length of file + * Returns the last commit date for this. The method will return {@link OptionalLong#empty()}, + * if the repository provider is not able to get the last commit for the path or if this value has not been computed + * (see {@link #isPartialResult()} and {@link #isComputationAborted()}). */ - public long getLength() + public OptionalLong getCommitDate() { - return length; + return commitDate == null? OptionalLong.empty(): OptionalLong.of(commitDate); + } + + /** + * Returns the length of the file or {@link OptionalLong#empty()}, when this value has not been computed + * (see {@link #isPartialResult()} and {@link #isComputationAborted()}). + */ + public OptionalLong getLength() + { + return length == null? OptionalLong.empty(): OptionalLong.of(length); } /** @@ -200,7 +224,7 @@ public class FileObject implements LastModifiedAware, Serializable } /** - * Return sub repository informations or null if the file is not + * Return sub repository information or null if the file is not * sub repository. * * @since 1.10 @@ -222,10 +246,38 @@ public class FileObject implements LastModifiedAware, Serializable return directory; } + /** + * Returns the children of this file. + * + * @return The children of this file if it is a directory. + */ + public Collection getChildren() { + return children == null? null: unmodifiableCollection(children); + } + + /** + * If this is true, some values for this object have not been computed, yet. These values (like + * {@link #getLength()}, {@link #getDescription()} or {@link #getCommitDate()}) + * will return {@link Optional#empty()} (or {@link OptionalLong#empty()} respectively), unless they are computed. + * There may be an asynchronous task running, that will set these values in the future. + * + * @since 2.0.0 + * + * @return true, whenever some values of this object have not been computed, yet. + */ public boolean isPartialResult() { return partialResult; } + /** + * If this is true, some values for this object have not been computed and will not be computed. These + * values (like {@link #getLength()}, {@link #getDescription()} or {@link #getCommitDate()}) + * will return {@link Optional#empty()} (or {@link OptionalLong#empty()} respectively), unless they are computed. + * + * @since 2.0.0 + * + * @return true, whenever some values of this object finally are not computed. + */ public boolean isComputationAborted() { return computationAborted; } @@ -255,14 +307,14 @@ public class FileObject implements LastModifiedAware, Serializable } /** - * Sets the last modified date of the file. + * Sets the commit date of the file. * * - * @param lastModified last modified date + * @param commitDate commit date */ - public void setLastModified(Long lastModified) + public void setCommitDate(Long commitDate) { - this.lastModified = lastModified; + this.commitDate = commitDate; } /** @@ -271,7 +323,7 @@ public class FileObject implements LastModifiedAware, Serializable * * @param length file length */ - public void setLength(long length) + public void setLength(Long length) { this.length = length; } @@ -310,30 +362,47 @@ public class FileObject implements LastModifiedAware, Serializable this.subRepository = subRepository; } + /** + * Set marker, that some values for this object are not computed, yet. + * + * @since 2.0.0 + * + * @param partialResult Set this to true, whenever some values of this object are not computed, yet. + */ public void setPartialResult(boolean partialResult) { this.partialResult = partialResult; } + /** + * Set marker, that computation of some values for this object has been aborted. + * + * @since 2.0.0 + * + * @param computationAborted Set this to true, whenever some values of this object are not computed and + * will not be computed in the future. + */ public void setComputationAborted(boolean computationAborted) { this.computationAborted = computationAborted; } - public Collection getChildren() { - return unmodifiableCollection(children); - } - + /** + * Set the children for this file. + * + * @param children The new childre. + */ public void setChildren(List children) { this.children = new ArrayList<>(children); } + /** + * Adds a child to the list of children . + * + * @param child The additional child. + */ public void addChild(FileObject child) { this.children.add(child); } - public boolean hasChildren() { - return !children.isEmpty(); - } - //~--- fields --------------------------------------------------------------- /** file description */ @@ -342,11 +411,11 @@ public class FileObject implements LastModifiedAware, Serializable /** directory indicator */ private boolean directory; - /** last modified date */ - private Long lastModified; + /** commit date */ + private Long commitDate; /** file length */ - private long length; + private Long length; /** filename */ private String name; @@ -354,13 +423,16 @@ public class FileObject implements LastModifiedAware, Serializable /** file path */ private String path; + /** Marker for partial result. */ private boolean partialResult = false; + /** Marker for aborted computation. */ private boolean computationAborted = false; /** sub repository informations */ @XmlElement(name = "subrepository") private SubRepository subRepository; + /** Children of this file (aka directory). */ private Collection children = new ArrayList<>(); } diff --git a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java index d13991558a..e8ef5a7a33 100644 --- a/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java +++ b/scm-plugins/scm-git-plugin/src/main/java/sonia/scm/repository/spi/GitBrowseCommand.java @@ -345,7 +345,7 @@ public class GitBrowseCommand extends AbstractGitCommand Blob blob = lfsBlobStore.get(oid); if (blob == null) { logger.error("lfs blob for lob id {} not found in lfs store of repository {}", oid, repository.getNamespaceAndName()); - file.setLength(-1); + file.setLength(null); } else { file.setLength(blob.getSize()); } @@ -402,7 +402,7 @@ public class GitBrowseCommand extends AbstractGitCommand } private void applyValuesFromCommit(SyncAsyncExecutor.ExecutionType executionType, RevCommit commit) { - file.setLastModified(GitUtil.getCommitTime(commit)); + file.setCommitDate(GitUtil.getCommitTime(commit)); file.setDescription(commit.getShortMessage()); if (executionType == ASYNCHRONOUS && browserResult != null) { updateCache(request); diff --git a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBrowseCommandTest.java b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBrowseCommandTest.java index 9b45cda16f..39066f0a9d 100644 --- a/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBrowseCommandTest.java +++ b/scm-plugins/scm-git-plugin/src/test/java/sonia/scm/repository/spi/GitBrowseCommandTest.java @@ -108,9 +108,9 @@ public class GitBrowseCommandTest extends AbstractGitCommandTestBase { assertFalse(a.isDirectory()); assertEquals("a.txt", a.getName()); assertEquals("a.txt", a.getPath()); - assertEquals("added new line for blame", a.getDescription()); - assertTrue(a.getLength() > 0); - checkDate(a.getLastModified()); + assertEquals("added new line for blame", a.getDescription().get()); + assertTrue(a.getLength().getAsLong() > 0); + checkDate(a.getCommitDate().getAsLong()); assertTrue(c.isDirectory()); assertEquals("c", c.getName()); @@ -132,28 +132,28 @@ public class GitBrowseCommandTest extends AbstractGitCommandTestBase { FileObject b = findFile(foList, "b.txt"); assertTrue(a.isPartialResult()); - assertNull("expected empty name before commit could have been read", a.getDescription()); - assertNull("expected empty date before commit could have been read", a.getLastModified()); + assertFalse("expected empty name before commit could have been read", a.getDescription().isPresent()); + assertFalse("expected empty date before commit could have been read", a.getCommitDate().isPresent()); assertTrue(b.isPartialResult()); - assertNull("expected empty name before commit could have been read", b.getDescription()); - assertNull("expected empty date before commit could have been read", b.getLastModified()); + assertFalse("expected empty name before commit could have been read", b.getDescription().isPresent()); + assertFalse("expected empty date before commit could have been read", b.getCommitDate().isPresent()); executor.next(); assertEquals(1, updatedResults.size()); assertFalse(a.isPartialResult()); assertNotNull("expected correct name after commit could have been read", a.getDescription()); - assertNotNull("expected correct date after commit could have been read", a.getLastModified()); + assertTrue("expected correct date after commit could have been read", a.getCommitDate().isPresent()); assertTrue(b.isPartialResult()); - assertNull("expected empty name before commit could have been read", b.getDescription()); - assertNull("expected empty date before commit could have been read", b.getLastModified()); + assertFalse("expected empty name before commit could have been read", b.getDescription().isPresent()); + assertFalse("expected empty date before commit could have been read", b.getCommitDate().isPresent()); executor.next(); assertEquals(2, updatedResults.size()); assertFalse(b.isPartialResult()); assertNotNull("expected correct name after commit could have been read", b.getDescription()); - assertNotNull("expected correct date after commit could have been read", b.getLastModified()); + assertTrue("expected correct date after commit could have been read", b.getCommitDate().isPresent()); } } @@ -175,16 +175,16 @@ public class GitBrowseCommandTest extends AbstractGitCommandTestBase { assertFalse(d.isDirectory()); assertEquals("d.txt", d.getName()); assertEquals("c/d.txt", d.getPath()); - assertEquals("added file d and e in folder c", d.getDescription()); - assertTrue(d.getLength() > 0); - checkDate(d.getLastModified()); + assertEquals("added file d and e in folder c", d.getDescription().get()); + assertTrue(d.getLength().getAsLong() > 0); + checkDate(d.getCommitDate().getAsLong()); assertFalse(e.isDirectory()); assertEquals("e.txt", e.getName()); assertEquals("c/e.txt", e.getPath()); - assertEquals("added file d and e in folder c", e.getDescription()); - assertTrue(e.getLength() > 0); - checkDate(e.getLastModified()); + assertEquals("added file d and e in folder c", e.getDescription().get()); + assertTrue(e.getLength().getAsLong() > 0); + checkDate(e.getCommitDate().getAsLong()); } @Test diff --git a/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/spi/javahg/HgFileviewCommand.java b/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/spi/javahg/HgFileviewCommand.java index 0897a191a1..4d5d5e8646 100644 --- a/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/spi/javahg/HgFileviewCommand.java +++ b/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/spi/javahg/HgFileviewCommand.java @@ -231,13 +231,13 @@ public class HgFileviewCommand extends AbstractCommand file.setName(getNameFromPath(path)); file.setPath(path); file.setDirectory(false); - file.setLength(stream.decimalIntUpTo(' ')); + file.setLength((long) stream.decimalIntUpTo(' ')); DateTime timestamp = stream.dateTimeUpTo(' '); String description = stream.textUpTo('\0'); if (!disableLastCommit) { - file.setLastModified(timestamp.getDate().getTime()); + file.setCommitDate(timestamp.getDate().getTime()); file.setDescription(description); } diff --git a/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/spi/HgBrowseCommandTest.java b/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/spi/HgBrowseCommandTest.java index 2116d06a7a..92a05a05a0 100644 --- a/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/spi/HgBrowseCommandTest.java +++ b/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/spi/HgBrowseCommandTest.java @@ -61,7 +61,7 @@ public class HgBrowseCommandTest extends AbstractHgCommandTestBase { FileObject file = new HgBrowseCommand(cmdContext, repository).getBrowserResult(request).getFile(); assertEquals("a.txt", file.getName()); assertFalse(file.isDirectory()); - assertTrue(file.getChildren().isEmpty()); + assertTrue(file.getChildren() == null || file.getChildren().isEmpty()); } @Test @@ -73,9 +73,9 @@ public class HgBrowseCommandTest extends AbstractHgCommandTestBase { assertFalse(a.isDirectory()); assertEquals("a.txt", a.getName()); assertEquals("a.txt", a.getPath()); - assertEquals("added new line for blame", a.getDescription()); - assertTrue(a.getLength() > 0); - checkDate(a.getLastModified()); + assertEquals("added new line for blame", a.getDescription().get()); + assertTrue(a.getLength().getAsLong() > 0); + checkDate(a.getCommitDate().getAsLong()); assertTrue(c.isDirectory()); assertEquals("c", c.getName()); assertEquals("c", c.getPath()); @@ -132,16 +132,16 @@ public class HgBrowseCommandTest extends AbstractHgCommandTestBase { assertFalse(d.isDirectory()); assertEquals("d.txt", d.getName()); assertEquals("c/d.txt", d.getPath()); - assertEquals("added file d and e in folder c", d.getDescription()); - assertTrue(d.getLength() > 0); - checkDate(d.getLastModified()); + assertEquals("added file d and e in folder c", d.getDescription().get()); + assertTrue(d.getLength().getAsLong() > 0); + checkDate(d.getCommitDate().getAsLong()); assertNotNull(e); assertFalse(e.isDirectory()); assertEquals("e.txt", e.getName()); assertEquals("c/e.txt", e.getPath()); - assertEquals("added file d and e in folder c", e.getDescription()); - assertTrue(e.getLength() > 0); - checkDate(e.getLastModified()); + assertEquals("added file d and e in folder c", e.getDescription().get()); + assertTrue(e.getLength().getAsLong() > 0); + checkDate(e.getCommitDate().getAsLong()); } @Test @@ -154,8 +154,8 @@ public class HgBrowseCommandTest extends AbstractHgCommandTestBase { FileObject a = getFileObject(foList, "a.txt"); - assertNull(a.getDescription()); - assertNull(a.getLastModified()); + assertFalse(a.getDescription().isPresent()); + assertFalse(a.getCommitDate().isPresent()); } @Test diff --git a/scm-plugins/scm-svn-plugin/src/main/java/sonia/scm/repository/spi/SvnBrowseCommand.java b/scm-plugins/scm-svn-plugin/src/main/java/sonia/scm/repository/spi/SvnBrowseCommand.java index 99dae0e77b..e4a32c8ca6 100644 --- a/scm-plugins/scm-svn-plugin/src/main/java/sonia/scm/repository/spi/SvnBrowseCommand.java +++ b/scm-plugins/scm-svn-plugin/src/main/java/sonia/scm/repository/spi/SvnBrowseCommand.java @@ -173,7 +173,7 @@ public class SvnBrowseCommand extends AbstractSvnCommand { if (entry.getDate() != null) { - fileObject.setLastModified(entry.getDate().getTime()); + fileObject.setCommitDate(entry.getDate().getTime()); } fileObject.setDescription(entry.getCommitMessage()); diff --git a/scm-plugins/scm-svn-plugin/src/test/java/sonia/scm/repository/spi/SvnBrowseCommandTest.java b/scm-plugins/scm-svn-plugin/src/test/java/sonia/scm/repository/spi/SvnBrowseCommandTest.java index d3e6a98558..980d486b5c 100644 --- a/scm-plugins/scm-svn-plugin/src/test/java/sonia/scm/repository/spi/SvnBrowseCommandTest.java +++ b/scm-plugins/scm-svn-plugin/src/test/java/sonia/scm/repository/spi/SvnBrowseCommandTest.java @@ -60,7 +60,7 @@ public class SvnBrowseCommandTest extends AbstractSvnCommandTestBase FileObject file = createCommand().getBrowserResult(request).getFile(); assertEquals("a.txt", file.getName()); assertFalse(file.isDirectory()); - assertTrue(file.getChildren().isEmpty()); + assertTrue(file.getChildren() == null || file.getChildren().isEmpty()); } @Test @@ -73,9 +73,9 @@ public class SvnBrowseCommandTest extends AbstractSvnCommandTestBase assertFalse(a.isDirectory()); assertEquals("a.txt", a.getName()); assertEquals("a.txt", a.getPath()); - assertEquals("added line for blame test", a.getDescription()); - assertTrue(a.getLength() > 0); - checkDate(a.getLastModified()); + assertEquals("added line for blame test", a.getDescription().get()); + assertTrue(a.getLength().getAsLong() > 0); + checkDate(a.getCommitDate().getAsLong()); assertTrue(c.isDirectory()); assertEquals("c", c.getName()); assertEquals("c", c.getPath()); @@ -122,16 +122,16 @@ public class SvnBrowseCommandTest extends AbstractSvnCommandTestBase assertFalse(d.isDirectory()); assertEquals("d.txt", d.getName()); assertEquals("c/d.txt", d.getPath()); - assertEquals("added d and e in folder c", d.getDescription()); - assertTrue(d.getLength() > 0); - checkDate(d.getLastModified()); + assertEquals("added d and e in folder c", d.getDescription().get()); + assertTrue(d.getLength().getAsLong() > 0); + checkDate(d.getCommitDate().getAsLong()); assertNotNull(e); assertFalse(e.isDirectory()); assertEquals("e.txt", e.getName()); assertEquals("c/e.txt", e.getPath()); - assertEquals("added d and e in folder c", e.getDescription()); - assertTrue(e.getLength() > 0); - checkDate(e.getLastModified()); + assertEquals("added d and e in folder c", e.getDescription().get()); + assertTrue(e.getLength().getAsLong() > 0); + checkDate(e.getCommitDate().getAsLong()); } @Test @@ -144,8 +144,8 @@ public class SvnBrowseCommandTest extends AbstractSvnCommandTestBase FileObject a = getFileObject(foList, "a.txt"); - assertNull(a.getDescription()); - assertNull(a.getLastModified()); + assertFalse(a.getDescription().isPresent()); + assertFalse(a.getCommitDate().isPresent()); } @Test diff --git a/scm-ui/ui-types/src/Sources.ts b/scm-ui/ui-types/src/Sources.ts index f5fca71d65..dce6947622 100644 --- a/scm-ui/ui-types/src/Sources.ts +++ b/scm-ui/ui-types/src/Sources.ts @@ -13,8 +13,8 @@ export type File = { directory: boolean; description?: string; revision: string; - length: number; - lastModified?: string; + length?: number; + commitDate?: string; subRepository?: SubRepository; // TODO partialResult: boolean; computationAborted: boolean; diff --git a/scm-ui/ui-webapp/src/repos/sources/components/FileTree.tsx b/scm-ui/ui-webapp/src/repos/sources/components/FileTree.tsx index 9782b69e0d..c1384b5192 100644 --- a/scm-ui/ui-webapp/src/repos/sources/components/FileTree.tsx +++ b/scm-ui/ui-webapp/src/repos/sources/components/FileTree.tsx @@ -133,7 +133,7 @@ class FileTree extends React.Component { {t("sources.file-tree.name")} {t("sources.file-tree.length")} - {t("sources.file-tree.lastModified")} + {t("sources.file-tree.commitDate")} {t("sources.file-tree.description")} {binder.hasExtension("repos.sources.tree.row.right") && } diff --git a/scm-ui/ui-webapp/src/repos/sources/components/FileTreeLeaf.tsx b/scm-ui/ui-webapp/src/repos/sources/components/FileTreeLeaf.tsx index 8a84fc5e1e..cd6baea395 100644 --- a/scm-ui/ui-webapp/src/repos/sources/components/FileTreeLeaf.tsx +++ b/scm-ui/ui-webapp/src/repos/sources/components/FileTreeLeaf.tsx @@ -64,9 +64,11 @@ class FileTreeLeaf extends React.Component { return {file.name}; }; - contentIfPresent = (file: File, content: any) => { + contentIfPresent = (file: File, attribute: string, content: (file: File) => any) => { const { t } = this.props; - if (file.computationAborted) { + if (file.hasOwnProperty(attribute)) { + return content(file); + } else if (file.computationAborted) { return ( @@ -79,23 +81,24 @@ class FileTreeLeaf extends React.Component { ); } else { - return content; + return content(file); } }; render() { const { file } = this.props; - const fileSize = file.directory ? "" : ; + const renderFileSize = (file: File) => ; + const renderCommitDate = (file: File) => ; return ( {this.createFileIcon(file)} {this.createFileName(file)} - {fileSize} - {this.contentIfPresent(file, )} + {file.directory ? "" : this.contentIfPresent(file, "length", renderFileSize)} + {this.contentIfPresent(file, "commitDate", renderCommitDate)} - {this.contentIfPresent(file, file.description)} + {this.contentIfPresent(file, "description", file => file.description)} {binder.hasExtension("repos.sources.tree.row.right") && ( diff --git a/scm-ui/ui-webapp/src/repos/sources/containers/Content.tsx b/scm-ui/ui-webapp/src/repos/sources/containers/Content.tsx index adb56ecd97..34c07e0be1 100644 --- a/scm-ui/ui-webapp/src/repos/sources/containers/Content.tsx +++ b/scm-ui/ui-webapp/src/repos/sources/containers/Content.tsx @@ -115,7 +115,7 @@ class Content extends React.Component { showMoreInformation() { const collapsed = this.state.collapsed; const { file, revision, t, repository } = this.props; - const date = ; + const date = ; const description = file.description ? (

{file.description.split("\n").map((item, key) => { diff --git a/scm-ui/ui-webapp/src/repos/sources/modules/sources.test.ts b/scm-ui/ui-webapp/src/repos/sources/modules/sources.test.ts index fd676d7c57..2d05c5814e 100644 --- a/scm-ui/ui-webapp/src/repos/sources/modules/sources.test.ts +++ b/scm-ui/ui-webapp/src/repos/sources/modules/sources.test.ts @@ -49,10 +49,8 @@ const collection = { name: "src", path: "src", directory: true, - description: "", length: 176, revision: "76aae4bb4ceacf0e88938eb5b6832738b7d537b4", - lastModified: "", subRepository: undefined, _links: { self: { @@ -71,7 +69,7 @@ const collection = { description: "bump version", length: 780, revision: "76aae4bb4ceacf0e88938eb5b6832738b7d537b4", - lastModified: "2017-07-31T11:17:19Z", + commitDate: "2017-07-31T11:17:19Z", subRepository: undefined, _links: { self: { diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/BrowserResultToFileObjectDtoMapper.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/BrowserResultToFileObjectDtoMapper.java index 866c2cc0b9..f9304881e7 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/BrowserResultToFileObjectDtoMapper.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/BrowserResultToFileObjectDtoMapper.java @@ -15,6 +15,9 @@ import java.lang.annotation.ElementType; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; +import java.time.Instant; +import java.util.Optional; +import java.util.OptionalLong; @Mapper public abstract class BrowserResultToFileObjectDtoMapper extends BaseFileObjectDtoMapper { @@ -39,6 +42,14 @@ public abstract class BrowserResultToFileObjectDtoMapper extends BaseFileObjectD applyEnrichers(appender, fileObject, namespaceAndName, browserResult, browserResult.getRevision()); } + Optional mapOptionalInstant(OptionalLong optionalLong) { + if (optionalLong.isPresent()) { + return Optional.of(Instant.ofEpochMilli(optionalLong.getAsLong())); + } else { + return Optional.empty(); + } + } + @Qualifier @Target(ElementType.METHOD) @Retention(RetentionPolicy.CLASS) diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/FileObjectDto.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/FileObjectDto.java index 4676a0fb03..b273f241dc 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/FileObjectDto.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/FileObjectDto.java @@ -10,6 +10,8 @@ import lombok.Setter; import java.time.Instant; import java.util.List; +import java.util.Optional; +import java.util.OptionalLong; @Getter @Setter @@ -19,10 +21,10 @@ public class FileObjectDto extends HalRepresentation { private String path; private boolean directory; @JsonInclude(JsonInclude.Include.NON_EMPTY) - private String description; - private long length; + private Optional description; + private OptionalLong length; @JsonInclude(JsonInclude.Include.NON_EMPTY) - private Instant lastModified; + private Optional commitDate; @JsonInclude(JsonInclude.Include.NON_EMPTY) private SubRepositoryDto subRepository; @JsonInclude(JsonInclude.Include.NON_EMPTY) diff --git a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/BrowserResultToFileObjectDtoMapperTest.java b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/BrowserResultToFileObjectDtoMapperTest.java index 27ef82834b..273cc25018 100644 --- a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/BrowserResultToFileObjectDtoMapperTest.java +++ b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/BrowserResultToFileObjectDtoMapperTest.java @@ -30,6 +30,7 @@ public class BrowserResultToFileObjectDtoMapperTest { private FileObject fileObject1 = new FileObject(); private FileObject fileObject2 = new FileObject(); + private FileObject partialFileObject = new FileObject(); @Before @@ -42,15 +43,15 @@ public class BrowserResultToFileObjectDtoMapperTest { ThreadContext.bind(subject); fileObject1.setName("FO 1"); - fileObject1.setLength(100); - fileObject1.setLastModified(0L); + fileObject1.setLength(100L); + fileObject1.setCommitDate(0L); fileObject1.setPath("/path/object/1"); fileObject1.setDescription("description of file object 1"); fileObject1.setDirectory(false); fileObject2.setName("FO 2"); - fileObject2.setLength(100); - fileObject2.setLastModified(101L); + fileObject2.setLength(100L); + fileObject2.setCommitDate(101L); fileObject2.setPath("/path/object/2"); fileObject2.setDescription("description of file object 2"); fileObject2.setDirectory(true); diff --git a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/SourceRootResourceTest.java b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/SourceRootResourceTest.java index 7b205c732a..0023ea4556 100644 --- a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/SourceRootResourceTest.java +++ b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/SourceRootResourceTest.java @@ -122,7 +122,7 @@ public class SourceRootResourceTest extends RepositoryTestBase { fileObject1.setDescription("File object 1"); fileObject1.setPath("/foo/bar/fo1"); fileObject1.setLength(1024L); - fileObject1.setLastModified(0L); + fileObject1.setCommitDate(0L); parent.addChild(fileObject1); FileObject fileObject2 = new FileObject(); @@ -131,7 +131,7 @@ public class SourceRootResourceTest extends RepositoryTestBase { fileObject2.setDescription("File object 2"); fileObject2.setPath("/foo/bar/fo2"); fileObject2.setLength(4096L); - fileObject2.setLastModified(1234L); + fileObject2.setCommitDate(1234L); parent.addChild(fileObject2); return parent; From 42d8d844c0b616cbcbd9e7a88f5927843865d93d Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Wed, 18 Dec 2019 11:52:08 +0100 Subject: [PATCH 32/35] Remove unused functions --- .../scm/repository/api/BrowseCommandBuilder.java | 4 ---- .../scm/repository/spi/FileBaseCommandRequest.java | 11 +---------- 2 files changed, 1 insertion(+), 14 deletions(-) diff --git a/scm-core/src/main/java/sonia/scm/repository/api/BrowseCommandBuilder.java b/scm-core/src/main/java/sonia/scm/repository/api/BrowseCommandBuilder.java index 437e3f5fa0..563557f0c1 100644 --- a/scm-core/src/main/java/sonia/scm/repository/api/BrowseCommandBuilder.java +++ b/scm-core/src/main/java/sonia/scm/repository/api/BrowseCommandBuilder.java @@ -300,10 +300,6 @@ public final class BrowseCommandBuilder return this; } - public void setComputationTimeoutMilliSeconds(long computationTimeoutMilliSeconds) { - request.setComputationTimeoutMilliSeconds(computationTimeoutMilliSeconds); - } - private void updateCache(BrowserResult updatedResult) { if (!disableCache) { CacheKey key = new CacheKey(repository, request); diff --git a/scm-core/src/main/java/sonia/scm/repository/spi/FileBaseCommandRequest.java b/scm-core/src/main/java/sonia/scm/repository/spi/FileBaseCommandRequest.java index c6683905dd..0455e65a95 100644 --- a/scm-core/src/main/java/sonia/scm/repository/spi/FileBaseCommandRequest.java +++ b/scm-core/src/main/java/sonia/scm/repository/spi/FileBaseCommandRequest.java @@ -147,10 +147,6 @@ public abstract class FileBaseCommandRequest this.revision = revision; } - public void setComputationTimeoutMilliSeconds(long computationTimeoutMilliSeconds) { - this.computationTimeoutMilliSeconds = computationTimeoutMilliSeconds; - } - //~--- get methods ---------------------------------------------------------- /** @@ -179,10 +175,7 @@ public abstract class FileBaseCommandRequest return disableCommitValues; } - public long getComputationTimeoutMilliSeconds() { - return computationTimeoutMilliSeconds; - } -//~--- methods -------------------------------------------------------------- + //~--- methods -------------------------------------------------------------- /** * Method description @@ -221,6 +214,4 @@ public abstract class FileBaseCommandRequest private String revision; private boolean disableCommitValues = false; - - private long computationTimeoutMilliSeconds = 1000; } From a5dc2828ad05077104970161c2e4e70015f2b573 Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Wed, 18 Dec 2019 12:11:57 +0100 Subject: [PATCH 33/35] Remove unused function --- .../sonia/scm/repository/spi/FileBaseCommandRequest.java | 6 ------ 1 file changed, 6 deletions(-) diff --git a/scm-core/src/main/java/sonia/scm/repository/spi/FileBaseCommandRequest.java b/scm-core/src/main/java/sonia/scm/repository/spi/FileBaseCommandRequest.java index 0455e65a95..9f563345fd 100644 --- a/scm-core/src/main/java/sonia/scm/repository/spi/FileBaseCommandRequest.java +++ b/scm-core/src/main/java/sonia/scm/repository/spi/FileBaseCommandRequest.java @@ -171,10 +171,6 @@ public abstract class FileBaseCommandRequest return revision; } - public boolean isDisableCommitValues() { - return disableCommitValues; - } - //~--- methods -------------------------------------------------------------- /** @@ -212,6 +208,4 @@ public abstract class FileBaseCommandRequest /** Field description */ private String revision; - - private boolean disableCommitValues = false; } From edcc036f62b1ec4d135231b99a3088fcf83ad969 Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Wed, 18 Dec 2019 12:48:11 +0100 Subject: [PATCH 34/35] Make reverse channel transient --- .../java/sonia/scm/repository/spi/BrowseCommandRequest.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/scm-core/src/main/java/sonia/scm/repository/spi/BrowseCommandRequest.java b/scm-core/src/main/java/sonia/scm/repository/spi/BrowseCommandRequest.java index 0c82c71331..9c23fe93f2 100644 --- a/scm-core/src/main/java/sonia/scm/repository/spi/BrowseCommandRequest.java +++ b/scm-core/src/main/java/sonia/scm/repository/spi/BrowseCommandRequest.java @@ -249,5 +249,7 @@ public final class BrowseCommandRequest extends FileBaseCommandRequest /** browse file objects recursive */ private boolean recursive = false; - private final Consumer updater; + // WARNING / TODO: This field creates a reverse channel from the implementation to the API. This will break + // whenever the API runs in a different process than the SPI (for example to run explicit hosts for git repositories). + private final transient Consumer updater; } From a88dc19efa9cf59ea42ae9a270409b09c50db24f Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Wed, 18 Dec 2019 13:28:22 +0000 Subject: [PATCH 35/35] Close branch feature/git_browse_performance