From f2a1effc77600dfa004da17f09e2b1b0aa8ab69e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Pfeuffer?= Date: Tue, 18 Jan 2022 09:46:10 +0100 Subject: [PATCH] Sorted autocomplete (#1918) Users, groups, repositories and repository roles have been sorted in the rest layer by default if no other sort option was given. In the layers "below" (aka the manager classes or the dao), the collections have been unsorted. This led to the effect, that the autocomplete resource, which did not sort all values beforehand, returned unsorted results. As a sideeffect, direct matches for an input could occur at a random position or not at all (as reported in #1695), when there were enough other matches. With this pull request the databases for users, groups, repositories and repository roles will use instances of TreeMap instead of LinkedHashMap internally, so that these values are sorted implicitly (by id respectively name for users, groups and repository roles and namespace/name for repositories). Due to this change the default sort applied in the rest layer could be removed. --- gradle/changelog/autocomplete_sorted.yaml | 2 + .../sonia/scm/group/xml/XmlGroupDatabase.java | 15 +-- .../scm/group/xml/XmlGroupMapAdapter.java | 13 +-- .../scm/repository/xml/XmlRepositoryDAO.java | 96 +++++++++++++------ .../xml/XmlRepositoryRoleDatabase.java | 6 +- .../xml/XmlRepositoryRoleMapAdapter.java | 6 +- .../sonia/scm/user/xml/XmlUserDatabase.java | 15 +-- .../sonia/scm/user/xml/XmlUserMapAdapter.java | 13 +-- .../java/sonia/scm/xml/AbstractXmlDAO.java | 4 +- scm-ui/ui-api/src/repositories.test.ts | 20 +--- scm-ui/ui-api/src/repositories.ts | 72 +++++++------- scm-ui/ui-components/src/repositories.test.ts | 36 +++---- .../java/sonia/scm/GenericDisplayManager.java | 4 + .../CollectionResourceManagerAdapter.java | 10 +- .../repository/DefaultRepositoryManager.java | 47 +++++++-- .../CollectionResourceManagerAdapterTest.java | 7 +- 16 files changed, 204 insertions(+), 162 deletions(-) create mode 100644 gradle/changelog/autocomplete_sorted.yaml diff --git a/gradle/changelog/autocomplete_sorted.yaml b/gradle/changelog/autocomplete_sorted.yaml new file mode 100644 index 0000000000..1058662c1e --- /dev/null +++ b/gradle/changelog/autocomplete_sorted.yaml @@ -0,0 +1,2 @@ +- type: fixed + description: Autocompletion has sorted suggestions ([#1918](https://github.com/scm-manager/scm-manager/pull/1918)) diff --git a/scm-dao-xml/src/main/java/sonia/scm/group/xml/XmlGroupDatabase.java b/scm-dao-xml/src/main/java/sonia/scm/group/xml/XmlGroupDatabase.java index 7e24a55ea5..1c5b14763f 100644 --- a/scm-dao-xml/src/main/java/sonia/scm/group/xml/XmlGroupDatabase.java +++ b/scm-dao-xml/src/main/java/sonia/scm/group/xml/XmlGroupDatabase.java @@ -21,25 +21,20 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - -package sonia.scm.group.xml; -//~--- non-JDK imports -------------------------------------------------------- +package sonia.scm.group.xml; import sonia.scm.group.Group; import sonia.scm.xml.XmlDatabase; -//~--- JDK imports ------------------------------------------------------------ - -import java.util.Collection; -import java.util.LinkedHashMap; -import java.util.Map; - import javax.xml.bind.annotation.XmlAccessType; import javax.xml.bind.annotation.XmlAccessorType; import javax.xml.bind.annotation.XmlElement; import javax.xml.bind.annotation.XmlRootElement; import javax.xml.bind.annotation.adapters.XmlJavaTypeAdapter; +import java.util.Collection; +import java.util.Map; +import java.util.TreeMap; /** * @@ -190,7 +185,7 @@ public class XmlGroupDatabase implements XmlDatabase /** Field description */ @XmlJavaTypeAdapter(XmlGroupMapAdapter.class) @XmlElement(name = "groups") - private Map groupMap = new LinkedHashMap<>(); + private Map groupMap = new TreeMap<>(); /** Field description */ private Long lastModified; diff --git a/scm-dao-xml/src/main/java/sonia/scm/group/xml/XmlGroupMapAdapter.java b/scm-dao-xml/src/main/java/sonia/scm/group/xml/XmlGroupMapAdapter.java index 5cdd0c9a69..0ff384bf53 100644 --- a/scm-dao-xml/src/main/java/sonia/scm/group/xml/XmlGroupMapAdapter.java +++ b/scm-dao-xml/src/main/java/sonia/scm/group/xml/XmlGroupMapAdapter.java @@ -21,19 +21,14 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - -package sonia.scm.group.xml; -//~--- non-JDK imports -------------------------------------------------------- +package sonia.scm.group.xml; import sonia.scm.group.Group; -//~--- JDK imports ------------------------------------------------------------ - -import java.util.LinkedHashMap; -import java.util.Map; - import javax.xml.bind.annotation.adapters.XmlAdapter; +import java.util.Map; +import java.util.TreeMap; /** * @@ -72,7 +67,7 @@ public class XmlGroupMapAdapter @Override public Map unmarshal(XmlGroupList groups) throws Exception { - Map groupMap = new LinkedHashMap<>(); + Map groupMap = new TreeMap<>(); for (Group group : groups) { diff --git a/scm-dao-xml/src/main/java/sonia/scm/repository/xml/XmlRepositoryDAO.java b/scm-dao-xml/src/main/java/sonia/scm/repository/xml/XmlRepositoryDAO.java index 2608f119ed..d319f5c542 100644 --- a/scm-dao-xml/src/main/java/sonia/scm/repository/xml/XmlRepositoryDAO.java +++ b/scm-dao-xml/src/main/java/sonia/scm/repository/xml/XmlRepositoryDAO.java @@ -41,8 +41,13 @@ import javax.inject.Inject; import java.io.IOException; import java.nio.file.Path; import java.util.Collection; +import java.util.HashMap; import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; +import java.util.TreeMap; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.function.Supplier; /** * @author Sebastian Sdorra @@ -50,7 +55,6 @@ import java.util.concurrent.ConcurrentHashMap; @Singleton public class XmlRepositoryDAO implements RepositoryDAO { - private final MetadataStore metadataStore = new MetadataStore(); private final PathBasedRepositoryLocationResolver repositoryLocationResolver; @@ -59,6 +63,7 @@ public class XmlRepositoryDAO implements RepositoryDAO { private final Map byId; private final Map byNamespaceAndName; + private final ReadWriteLock byNamespaceLock = new ReentrantReadWriteLock(); @Inject public XmlRepositoryDAO(PathBasedRepositoryLocationResolver repositoryLocationResolver, FileSystem fileSystem, RepositoryExportingCheck repositoryExportingCheck) { @@ -66,18 +71,20 @@ public class XmlRepositoryDAO implements RepositoryDAO { this.fileSystem = fileSystem; this.repositoryExportingCheck = repositoryExportingCheck; - this.byId = new ConcurrentHashMap<>(); - this.byNamespaceAndName = new ConcurrentHashMap<>(); + this.byId = new HashMap<>(); + this.byNamespaceAndName = new TreeMap<>(); init(); } private void init() { - RepositoryLocationResolver.RepositoryLocationResolverInstance pathRepositoryLocationResolverInstance = repositoryLocationResolver.create(Path.class); - pathRepositoryLocationResolverInstance.forAllLocations((repositoryId, repositoryPath) -> { - Repository repository = metadataStore.read(repositoryPath); - byNamespaceAndName.put(repository.getNamespaceAndName(), repository); - byId.put(repositoryId, repository); + withWriteLockedMaps(() -> { + RepositoryLocationResolver.RepositoryLocationResolverInstance pathRepositoryLocationResolverInstance = repositoryLocationResolver.create(Path.class); + pathRepositoryLocationResolverInstance.forAllLocations((repositoryId, repositoryPath) -> { + Repository repository = metadataStore.read(repositoryPath); + byNamespaceAndName.put(repository.getNamespaceAndName(), repository); + byId.put(repositoryId, repository); + }); }); } @@ -106,38 +113,40 @@ public class XmlRepositoryDAO implements RepositoryDAO { throw new InternalRepositoryException(repository, "failed to create filesystem", e); } - byId.put(repository.getId(), clone); - byNamespaceAndName.put(repository.getNamespaceAndName(), clone); + withWriteLockedMaps(() -> { + byId.put(repository.getId(), clone); + byNamespaceAndName.put(repository.getNamespaceAndName(), clone); + }); } @Override public boolean contains(Repository repository) { - return byId.containsKey(repository.getId()); + return withReadLockedMaps(() -> byId.containsKey(repository.getId())); } @Override public boolean contains(NamespaceAndName namespaceAndName) { - return byNamespaceAndName.containsKey(namespaceAndName); + return withReadLockedMaps(() -> byNamespaceAndName.containsKey(namespaceAndName)); } @Override public boolean contains(String id) { - return byId.containsKey(id); + return withReadLockedMaps(() -> byId.containsKey(id)); } @Override public Repository get(NamespaceAndName namespaceAndName) { - return byNamespaceAndName.get(namespaceAndName); + return withReadLockedMaps(() -> byNamespaceAndName.get(namespaceAndName)); } @Override public Repository get(String id) { - return byId.get(id); + return withReadLockedMaps(() -> byId.get(id)); } @Override public Collection getAll() { - return ImmutableList.copyOf(byNamespaceAndName.values()); + return withReadLockedMaps(() -> ImmutableList.copyOf(byNamespaceAndName.values())); } @Override @@ -147,14 +156,14 @@ public class XmlRepositoryDAO implements RepositoryDAO { throw new StoreReadOnlyException(repository); } - synchronized (this) { + withWriteLockedMaps(() -> { // remove old namespaceAndName from map, in case of rename Repository prev = byId.put(clone.getId(), clone); if (prev != null) { byNamespaceAndName.remove(prev.getNamespaceAndName()); } byNamespaceAndName.put(clone.getNamespaceAndName(), clone); - } + }); Path repositoryPath = repositoryLocationResolver .create(Path.class) @@ -164,8 +173,10 @@ public class XmlRepositoryDAO implements RepositoryDAO { } private boolean mustNotModifyRepository(Repository clone) { - return clone.isArchived() && byId.get(clone.getId()).isArchived() - || repositoryExportingCheck.isExporting(clone); + return withReadLockedMaps(() -> + clone.isArchived() && byId.get(clone.getId()).isArchived() + || repositoryExportingCheck.isExporting(clone) + ); } @Override @@ -173,14 +184,13 @@ public class XmlRepositoryDAO implements RepositoryDAO { if (repository.isArchived() || repositoryExportingCheck.isExporting(repository)) { throw new StoreReadOnlyException(repository); } - Path path; - synchronized (this) { + Path path = withWriteLockedMaps(() -> { Repository prev = byId.remove(repository.getId()); if (prev != null) { byNamespaceAndName.remove(prev.getNamespaceAndName()); } - path = repositoryLocationResolver.remove(repository.getId()); - } + return repositoryLocationResolver.remove(repository.getId()); + }); try { fileSystem.destroy(path.toFile()); @@ -201,8 +211,40 @@ public class XmlRepositoryDAO implements RepositoryDAO { public void refresh() { repositoryLocationResolver.refresh(); - byNamespaceAndName.clear(); - byId.clear(); + withWriteLockedMaps(() -> { + byNamespaceAndName.clear(); + byId.clear(); + }); init(); } + + private void withWriteLockedMaps(Runnable runnable) { + Lock lock = byNamespaceLock.writeLock(); + lock.lock(); + try { + runnable.run(); + } finally { + lock.unlock(); + } + } + + private T withWriteLockedMaps(Supplier runnable) { + Lock lock = byNamespaceLock.writeLock(); + lock.lock(); + try { + return runnable.get(); + } finally { + lock.unlock(); + } + } + + private T withReadLockedMaps(Supplier runnable) { + Lock lock = byNamespaceLock.readLock(); + lock.lock(); + try { + return runnable.get(); + } finally { + lock.unlock(); + } + } } diff --git a/scm-dao-xml/src/main/java/sonia/scm/repository/xml/XmlRepositoryRoleDatabase.java b/scm-dao-xml/src/main/java/sonia/scm/repository/xml/XmlRepositoryRoleDatabase.java index 7276540738..76c8ab7fc0 100644 --- a/scm-dao-xml/src/main/java/sonia/scm/repository/xml/XmlRepositoryRoleDatabase.java +++ b/scm-dao-xml/src/main/java/sonia/scm/repository/xml/XmlRepositoryRoleDatabase.java @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.repository.xml; import sonia.scm.repository.RepositoryRole; @@ -33,8 +33,8 @@ import javax.xml.bind.annotation.XmlElement; import javax.xml.bind.annotation.XmlRootElement; import javax.xml.bind.annotation.adapters.XmlJavaTypeAdapter; import java.util.Collection; -import java.util.LinkedHashMap; import java.util.Map; +import java.util.TreeMap; @XmlRootElement(name = "user-db") @XmlAccessorType(XmlAccessType.FIELD) @@ -45,7 +45,7 @@ public class XmlRepositoryRoleDatabase implements XmlDatabase { @XmlJavaTypeAdapter(XmlRepositoryRoleMapAdapter.class) @XmlElement(name = "roles") - private Map roleMap = new LinkedHashMap<>(); + private Map roleMap = new TreeMap<>(); public XmlRepositoryRoleDatabase() { long c = System.currentTimeMillis(); diff --git a/scm-dao-xml/src/main/java/sonia/scm/repository/xml/XmlRepositoryRoleMapAdapter.java b/scm-dao-xml/src/main/java/sonia/scm/repository/xml/XmlRepositoryRoleMapAdapter.java index 390df92e50..b0373a0238 100644 --- a/scm-dao-xml/src/main/java/sonia/scm/repository/xml/XmlRepositoryRoleMapAdapter.java +++ b/scm-dao-xml/src/main/java/sonia/scm/repository/xml/XmlRepositoryRoleMapAdapter.java @@ -21,14 +21,14 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.repository.xml; import sonia.scm.repository.RepositoryRole; import javax.xml.bind.annotation.adapters.XmlAdapter; -import java.util.LinkedHashMap; import java.util.Map; +import java.util.TreeMap; public class XmlRepositoryRoleMapAdapter extends XmlAdapter> { @@ -40,7 +40,7 @@ public class XmlRepositoryRoleMapAdapter @Override public Map unmarshal(XmlRepositoryRoleList roles) { - Map roleMap = new LinkedHashMap<>(); + Map roleMap = new TreeMap<>(); for (RepositoryRole role : roles) { roleMap.put(role.getName(), role); diff --git a/scm-dao-xml/src/main/java/sonia/scm/user/xml/XmlUserDatabase.java b/scm-dao-xml/src/main/java/sonia/scm/user/xml/XmlUserDatabase.java index aff7d8da6b..4a8a110b5d 100644 --- a/scm-dao-xml/src/main/java/sonia/scm/user/xml/XmlUserDatabase.java +++ b/scm-dao-xml/src/main/java/sonia/scm/user/xml/XmlUserDatabase.java @@ -21,25 +21,20 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - -package sonia.scm.user.xml; -//~--- non-JDK imports -------------------------------------------------------- +package sonia.scm.user.xml; import sonia.scm.user.User; import sonia.scm.xml.XmlDatabase; -//~--- JDK imports ------------------------------------------------------------ - -import java.util.Collection; -import java.util.LinkedHashMap; -import java.util.Map; - import javax.xml.bind.annotation.XmlAccessType; import javax.xml.bind.annotation.XmlAccessorType; import javax.xml.bind.annotation.XmlElement; import javax.xml.bind.annotation.XmlRootElement; import javax.xml.bind.annotation.adapters.XmlJavaTypeAdapter; +import java.util.Collection; +import java.util.Map; +import java.util.TreeMap; /** * @@ -193,5 +188,5 @@ public class XmlUserDatabase implements XmlDatabase /** Field description */ @XmlJavaTypeAdapter(XmlUserMapAdapter.class) @XmlElement(name = "users") - private Map userMap = new LinkedHashMap<>(); + private Map userMap = new TreeMap<>(); } diff --git a/scm-dao-xml/src/main/java/sonia/scm/user/xml/XmlUserMapAdapter.java b/scm-dao-xml/src/main/java/sonia/scm/user/xml/XmlUserMapAdapter.java index 1fc200a227..dd2afde6d2 100644 --- a/scm-dao-xml/src/main/java/sonia/scm/user/xml/XmlUserMapAdapter.java +++ b/scm-dao-xml/src/main/java/sonia/scm/user/xml/XmlUserMapAdapter.java @@ -21,19 +21,14 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - -package sonia.scm.user.xml; -//~--- non-JDK imports -------------------------------------------------------- +package sonia.scm.user.xml; import sonia.scm.user.User; -//~--- JDK imports ------------------------------------------------------------ - -import java.util.LinkedHashMap; -import java.util.Map; - import javax.xml.bind.annotation.adapters.XmlAdapter; +import java.util.Map; +import java.util.TreeMap; /** * @@ -72,7 +67,7 @@ public class XmlUserMapAdapter @Override public Map unmarshal(XmlUserList users) throws Exception { - Map userMap = new LinkedHashMap<>(); + Map userMap = new TreeMap<>(); for (User user : users) { diff --git a/scm-dao-xml/src/main/java/sonia/scm/xml/AbstractXmlDAO.java b/scm-dao-xml/src/main/java/sonia/scm/xml/AbstractXmlDAO.java index c319bebb75..9d69fe6d3e 100644 --- a/scm-dao-xml/src/main/java/sonia/scm/xml/AbstractXmlDAO.java +++ b/scm-dao-xml/src/main/java/sonia/scm/xml/AbstractXmlDAO.java @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.xml; //~--- non-JDK imports -------------------------------------------------------- @@ -45,7 +45,7 @@ import java.util.Collection; * @param */ public abstract class AbstractXmlDAO implements GenericDAO + T extends XmlDatabase> implements GenericDAO { /** Field description */ diff --git a/scm-ui/ui-api/src/repositories.test.ts b/scm-ui/ui-api/src/repositories.test.ts index 95ffc733c7..ee1f1852d5 100644 --- a/scm-ui/ui-api/src/repositories.test.ts +++ b/scm-ui/ui-api/src/repositories.test.ts @@ -89,11 +89,7 @@ describe("Test repository hooks", () => { it("should return repositories", async () => { const queryClient = createInfiniteCachingClient(); setIndexLink(queryClient, "repositories", "/repos"); - fetchMock.get("/api/v2/repos", repositoryCollection, { - query: { - sortBy: "namespaceAndName" - } - }); + fetchMock.get("/api/v2/repos", repositoryCollection); await expectCollection(queryClient); }); @@ -103,7 +99,6 @@ describe("Test repository hooks", () => { setIndexLink(queryClient, "repositories", "/repos"); fetchMock.get("/api/v2/repos", repositoryCollection, { query: { - sortBy: "namespaceAndName", page: "42" } }); @@ -116,11 +111,7 @@ describe("Test repository hooks", () => { it("should use repository from namespace", async () => { const queryClient = createInfiniteCachingClient(); setIndexLink(queryClient, "repositories", "/repos"); - fetchMock.get("/api/v2/spaceships", repositoryCollection, { - query: { - sortBy: "namespaceAndName" - } - }); + fetchMock.get("/api/v2/spaceships", repositoryCollection); await expectCollection(queryClient, { namespace: { @@ -139,7 +130,6 @@ describe("Test repository hooks", () => { setIndexLink(queryClient, "repositories", "/repos"); fetchMock.get("/api/v2/repos", repositoryCollection, { query: { - sortBy: "namespaceAndName", q: "heart" } }); @@ -152,11 +142,7 @@ describe("Test repository hooks", () => { it("should update repository cache", async () => { const queryClient = createInfiniteCachingClient(); setIndexLink(queryClient, "repositories", "/repos"); - fetchMock.get("/api/v2/repos", repositoryCollection, { - query: { - sortBy: "namespaceAndName" - } - }); + fetchMock.get("/api/v2/repos", repositoryCollection); await expectCollection(queryClient); diff --git a/scm-ui/ui-api/src/repositories.ts b/scm-ui/ui-api/src/repositories.ts index 077ab5656f..1e53f23c33 100644 --- a/scm-ui/ui-api/src/repositories.ts +++ b/scm-ui/ui-api/src/repositories.ts @@ -30,7 +30,7 @@ import { Repository, RepositoryCollection, RepositoryCreation, - RepositoryTypeCollection, + RepositoryTypeCollection } from "@scm-manager/ui-types"; import { useMutation, useQuery, useQueryClient } from "react-query"; import { apiClient } from "./apiclient"; @@ -55,9 +55,7 @@ export const useRepositories = (request?: UseRepositoriesRequest): ApiResult = { - sortBy: "namespaceAndName", - }; + const queryParams: Record = {}; if (request?.search) { queryParams.q = request.search; } @@ -66,7 +64,7 @@ export const useRepositories = (request?: UseRepositoriesRequest): ApiResult( ["repositories", request?.namespace?.namespace, request?.search || "", request?.page || 0], - () => apiClient.get(`${link}?${createQueryString(queryParams)}`).then((response) => response.json()), + () => apiClient.get(`${link}?${createQueryString(queryParams)}`).then(response => response.json()), { enabled: !request?.disabled, onSuccess: (repositories: RepositoryCollection) => { @@ -74,7 +72,7 @@ export const useRepositories = (request?: UseRepositoriesRequest): ApiResult { queryClient.setQueryData(["repository", repository.namespace, repository.name], repository); }); - }, + } } ); }; @@ -92,14 +90,14 @@ const createRepository = (link: string) => { } return apiClient .post(createLink, request.repository, "application/vnd.scmm-repository+json;v=2") - .then((response) => { + .then(response => { const location = response.headers.get("Location"); if (!location) { throw new Error("Server does not return required Location header"); } return apiClient.get(location); }) - .then((response) => response.json()); + .then(response => response.json()); }; }; @@ -111,10 +109,10 @@ export const useCreateRepository = () => { const { mutate, data, isLoading, error } = useMutation( createRepository(link), { - onSuccess: (repository) => { + onSuccess: repository => { queryClient.setQueryData(["repository", repository.namespace, repository.name], repository); return queryClient.invalidateQueries(["repositories"]); - }, + } } ); return { @@ -123,7 +121,7 @@ export const useCreateRepository = () => { }, isLoading, error, - repository: data, + repository: data }; }; @@ -133,7 +131,7 @@ export const useRepositoryTypes = () => useIndexJsonResource => { const link = useRequiredIndexLink("repositories"); return useQuery(["repository", namespace, name], () => - apiClient.get(concat(link, namespace, name)).then((response) => response.json()) + apiClient.get(concat(link, namespace, name)).then(response => response.json()) ); }; @@ -144,7 +142,7 @@ export type UseDeleteRepositoryOptions = { export const useDeleteRepository = (options?: UseDeleteRepositoryOptions) => { const queryClient = useQueryClient(); const { mutate, isLoading, error, data } = useMutation( - (repository) => { + repository => { const link = requiredLink(repository, "delete"); return apiClient.delete(link); }, @@ -153,23 +151,23 @@ export const useDeleteRepository = (options?: UseDeleteRepositoryOptions) => { if (options?.onSuccess) { options.onSuccess(repository); } - await queryClient.removeQueries(repoQueryKey(repository)); + queryClient.removeQueries(repoQueryKey(repository)); await queryClient.invalidateQueries(["repositories"]); - }, + } } ); return { remove: (repository: Repository) => mutate(repository), isLoading, error, - isDeleted: !!data, + isDeleted: !!data }; }; export const useUpdateRepository = () => { const queryClient = useQueryClient(); const { mutate, isLoading, error, data } = useMutation( - (repository) => { + repository => { const link = requiredLink(repository, "update"); return apiClient.put(link, repository, "application/vnd.scmm-repository+json;v=2"); }, @@ -177,21 +175,21 @@ export const useUpdateRepository = () => { onSuccess: async (_, repository) => { await queryClient.invalidateQueries(repoQueryKey(repository)); await queryClient.invalidateQueries(["repositories"]); - }, + } } ); return { update: (repository: Repository) => mutate(repository), isLoading, error, - isUpdated: !!data, + isUpdated: !!data }; }; export const useArchiveRepository = () => { const queryClient = useQueryClient(); const { mutate, isLoading, error, data } = useMutation( - (repository) => { + repository => { const link = requiredLink(repository, "archive"); return apiClient.post(link); }, @@ -199,21 +197,21 @@ export const useArchiveRepository = () => { onSuccess: async (_, repository) => { await queryClient.invalidateQueries(repoQueryKey(repository)); await queryClient.invalidateQueries(["repositories"]); - }, + } } ); return { archive: (repository: Repository) => mutate(repository), isLoading, error, - isArchived: !!data, + isArchived: !!data }; }; export const useUnarchiveRepository = () => { const queryClient = useQueryClient(); const { mutate, isLoading, error, data } = useMutation( - (repository) => { + repository => { const link = requiredLink(repository, "unarchive"); return apiClient.post(link); }, @@ -221,35 +219,35 @@ export const useUnarchiveRepository = () => { onSuccess: async (_, repository) => { await queryClient.invalidateQueries(repoQueryKey(repository)); await queryClient.invalidateQueries(["repositories"]); - }, + } } ); return { unarchive: (repository: Repository) => mutate(repository), isLoading, error, - isUnarchived: !!data, + isUnarchived: !!data }; }; export const useRunHealthCheck = () => { const queryClient = useQueryClient(); const { mutate, isLoading, error, data } = useMutation( - (repository) => { + repository => { const link = requiredLink(repository, "runHealthCheck"); return apiClient.post(link); }, { onSuccess: async (_, repository) => { await queryClient.invalidateQueries(repoQueryKey(repository)); - }, + } } ); return { runHealthCheck: (repository: Repository) => mutate(repository), isLoading, error, - isRunning: !!data, + isRunning: !!data }; }; @@ -258,7 +256,7 @@ export const useExportInfo = (repository: Repository): ApiResultWithFetching( ["repository", repository.namespace, repository.name, "exportInfo"], - () => apiClient.get(link).then((response) => response.json()), + () => apiClient.get(link).then(response => response.json()), {} ); @@ -266,7 +264,7 @@ export const useExportInfo = (repository: Repository): ApiResultWithFetching { const id = setInterval(() => { apiClient .get(infolink) - .then((r) => r.json()) + .then(r => r.json()) .then((info: ExportInfo) => { if (info._links.download) { clearInterval(id); resolve(info); } }) - .catch((e) => { + .catch(e => { clearInterval(id); reject(e); }); @@ -329,21 +327,21 @@ export const useExportRepository = () => { onSuccess: async (_, { repository }) => { await queryClient.invalidateQueries(repoQueryKey(repository)); await queryClient.invalidateQueries(["repositories"]); - }, + } } ); return { exportRepository: (repository: Repository, options: ExportOptions) => mutate({ repository, options }), isLoading, error, - data, + data }; }; export const usePaths = (repository: Repository, revision: string): ApiResult => { const link = requiredLink(repository, "paths").replace("{revision}", revision); return useQuery(repoQueryKey(repository, "paths", revision), () => - apiClient.get(link).then((response) => response.json()) + apiClient.get(link).then(response => response.json()) ); }; @@ -364,7 +362,7 @@ export const useRenameRepository = (repository: Repository) => { const { mutate, isLoading, error, data } = useMutation( ({ name, namespace }) => apiClient.post(url, { namespace, name }, "application/vnd.scmm-repository+json;v=2"), { - onSuccess: () => queryClient.removeQueries(repoQueryKey(repository)), + onSuccess: () => queryClient.removeQueries(repoQueryKey(repository)) } ); @@ -372,6 +370,6 @@ export const useRenameRepository = (repository: Repository) => { renameRepository: (namespace: string, name: string) => mutate({ namespace, name }), isLoading, error, - isRenamed: !!data, + isRenamed: !!data }; }; diff --git a/scm-ui/ui-components/src/repositories.test.ts b/scm-ui/ui-components/src/repositories.test.ts index 6a50eecf10..759e848425 100644 --- a/scm-ui/ui-components/src/repositories.test.ts +++ b/scm-ui/ui-components/src/repositories.test.ts @@ -35,10 +35,10 @@ describe("getProtocolLinkByType tests", () => { protocol: [ { name: "http", - href: "http://scm.scm-manager.org/repo/scm/core", - }, - ], - }, + href: "http://scm.scm-manager.org/repo/scm/core" + } + ] + } }; const link = getProtocolLinkByType(repository, "http"); @@ -54,14 +54,14 @@ describe("getProtocolLinkByType tests", () => { protocol: [ { name: "http", - href: "http://scm.scm-manager.org/repo/scm/core", + href: "http://scm.scm-manager.org/repo/scm/core" }, { name: "ssh", - href: "git@scm.scm-manager.org:scm/core", - }, - ], - }, + href: "git@scm.scm-manager.org:scm/core" + } + ] + } }; const link = getProtocolLinkByType(repository, "http"); @@ -76,9 +76,9 @@ describe("getProtocolLinkByType tests", () => { _links: { protocol: { name: "http", - href: "http://scm.scm-manager.org/repo/scm/core", - }, - }, + href: "http://scm.scm-manager.org/repo/scm/core" + } + } }; const link = getProtocolLinkByType(repository, "http"); @@ -94,14 +94,14 @@ describe("getProtocolLinkByType tests", () => { protocol: [ { name: "http", - href: "http://scm.scm-manager.org/repo/scm/core", + href: "http://scm.scm-manager.org/repo/scm/core" }, { name: "ssh", - href: "git@scm.scm-manager.org:scm/core", - }, - ], - }, + href: "git@scm.scm-manager.org:scm/core" + } + ] + } }; const link = getProtocolLinkByType(repository, "awesome"); @@ -113,7 +113,7 @@ describe("getProtocolLinkByType tests", () => { namespace: "scm", name: "core", type: "git", - _links: {}, + _links: {} }; const link = getProtocolLinkByType(repository, "http"); diff --git a/scm-webapp/src/main/java/sonia/scm/GenericDisplayManager.java b/scm-webapp/src/main/java/sonia/scm/GenericDisplayManager.java index 15f8871842..f9221c6e74 100644 --- a/scm-webapp/src/main/java/sonia/scm/GenericDisplayManager.java +++ b/scm-webapp/src/main/java/sonia/scm/GenericDisplayManager.java @@ -31,6 +31,7 @@ import java.util.Collection; import java.util.Optional; import java.util.function.Function; +import static java.util.Optional.empty; import static java.util.Optional.ofNullable; public abstract class GenericDisplayManager implements DisplayManager { @@ -60,6 +61,9 @@ public abstract class GenericDisplayManager imp @Override public Optional get(String id) { + if (id == null) { + return empty(); + } return ofNullable(dao.get(id)).map(transform); } } diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/CollectionResourceManagerAdapter.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/CollectionResourceManagerAdapter.java index f3eaa5e3da..8a00fa994f 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/CollectionResourceManagerAdapter.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/CollectionResourceManagerAdapter.java @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.api.v2.resources; import de.otto.edison.hal.HalRepresentation; @@ -77,12 +77,12 @@ class CollectionResourceManagerAdapter comparator = null; + if (!Util.isEmpty(sortBy)) { + comparator = createComparator(sortBy, desc); } - return manager.getPage(filter, createComparator(sortBy, desc), pageNumber, pageSize); + return manager.getPage(filter, comparator, pageNumber, pageSize); } private Comparator createComparator(String sortBy, boolean desc) { diff --git a/scm-webapp/src/main/java/sonia/scm/repository/DefaultRepositoryManager.java b/scm-webapp/src/main/java/sonia/scm/repository/DefaultRepositoryManager.java index 4564f5d36c..0b5c638cdd 100644 --- a/scm-webapp/src/main/java/sonia/scm/repository/DefaultRepositoryManager.java +++ b/scm-webapp/src/main/java/sonia/scm/repository/DefaultRepositoryManager.java @@ -41,7 +41,6 @@ import sonia.scm.event.ScmEventBus; import sonia.scm.security.AuthorizationChangedEvent; import sonia.scm.security.KeyGenerator; import sonia.scm.util.AssertUtil; -import sonia.scm.util.CollectionAppender; import sonia.scm.util.IOUtil; import sonia.scm.util.Util; @@ -51,13 +50,19 @@ import java.util.Collections; import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.function.BiConsumer; +import java.util.function.BinaryOperator; import java.util.function.Consumer; +import java.util.function.Function; import java.util.function.Predicate; +import java.util.function.Supplier; +import java.util.stream.Collector; -import static java.util.stream.Collectors.toSet; +import static java.util.Collections.emptySet; import static sonia.scm.AlreadyExistsException.alreadyExists; import static sonia.scm.ContextEntry.ContextBuilder.entity; import static sonia.scm.NotFoundException.notFound; @@ -70,6 +75,33 @@ import static sonia.scm.NotFoundException.notFound; @Singleton public class DefaultRepositoryManager extends AbstractRepositoryManager { + @SuppressWarnings("unchecked") + public static final Collector> LINKED_HASH_SET_COLLECTOR = new Collector>() { + @Override + public Supplier supplier() { + return LinkedHashSet::new; + } + + @Override + public BiConsumer accumulator() { + return (collection, value) -> ((Collection) collection).add(value); + } + + @Override + public BinaryOperator combiner() { + return (c1, c2) -> ((Collection) c1).addAll((Collection) c2); + } + + @Override + public Function> finisher() { + return collection -> (Collection) collection; + } + + @Override + public Set characteristics() { + return emptySet(); + } + }; private static final Logger logger = LoggerFactory.getLogger(DefaultRepositoryManager.class); private final Map handlerMap; private final KeyGenerator keyGenerator; @@ -340,12 +372,9 @@ public class DefaultRepositoryManager extends AbstractRepositoryManager { int start, int limit) { return Util.createSubCollection(repositoryDAO.getAll(), comparator, - new CollectionAppender() { - @Override - public void append(Collection collection, Repository item) { - if (RepositoryPermissions.read().isPermitted(item)) { - collection.add(postProcess(item)); - } + (collection, item) -> { + if (RepositoryPermissions.read().isPermitted(item)) { + collection.add(postProcess(item)); } }, start, limit); } @@ -363,7 +392,7 @@ public class DefaultRepositoryManager extends AbstractRepositoryManager { public Collection getAllNamespaces() { return getAll().stream() .map(Repository::getNamespace) - .collect(toSet()); + .collect(LINKED_HASH_SET_COLLECTOR); } @Override diff --git a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/CollectionResourceManagerAdapterTest.java b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/CollectionResourceManagerAdapterTest.java index c4cb6d2698..a766117c4f 100644 --- a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/CollectionResourceManagerAdapterTest.java +++ b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/CollectionResourceManagerAdapterTest.java @@ -21,7 +21,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE * SOFTWARE. */ - + package sonia.scm.api.v2.resources; import de.otto.edison.hal.HalRepresentation; @@ -38,6 +38,7 @@ import sonia.scm.api.rest.resources.Simple; import java.util.Comparator; import java.util.function.Predicate; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.when; @@ -61,11 +62,11 @@ public class CollectionResourceManagerAdapterTest { } @Test - public void shouldAcceptDefaultSortByParameter() { + public void shouldNotSortByDefault() { abstractManagerResource.getAll(0, 1, x -> true, null, true, r -> null); Comparator comparator = comparatorCaptor.getValue(); - assertTrue(comparator.compare(new Simple("1", null), new Simple("2", null)) > 0); + assertNull(comparator); } @Test