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.
scm-manager · Jan 18, 2022 · f2a1eff · f2a1eff
1 parent 6ca88e6
commit f2a1eff
Show file tree

Hide file tree

Showing 16 changed files with 204 additions and 162 deletions.
diff --git a/gradle/changelog/autocomplete_sorted.yaml 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
@@ -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 void setLastModified(long lastModified)
   /** Field description */
   @XmlJavaTypeAdapter(XmlGroupMapAdapter.class)
   @XmlElement(name = "groups")
-  private Map<String, Group> groupMap = new LinkedHashMap<>();
+  private Map<String, Group> 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
@@ -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 XmlGroupList marshal(Map<String, Group> groupMap) throws Exception
   @Override
   public Map<String, Group> unmarshal(XmlGroupList groups) throws Exception
   {
-    Map<String, Group> groupMap = new LinkedHashMap<>();
+    Map<String, Group> 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
@@ -41,16 +41,20 @@
 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
  */
 @Singleton
 public class XmlRepositoryDAO implements RepositoryDAO {
 
-
   private final MetadataStore metadataStore = new MetadataStore();
 
   private final PathBasedRepositoryLocationResolver repositoryLocationResolver;
@@ -59,25 +63,28 @@ public class XmlRepositoryDAO implements RepositoryDAO {
 
   private final Map<String, Repository> byId;
   private final Map<NamespaceAndName, Repository> byNamespaceAndName;
+  private final ReadWriteLock byNamespaceLock = new ReentrantReadWriteLock();
 
   @Inject
   public XmlRepositoryDAO(PathBasedRepositoryLocationResolver repositoryLocationResolver, FileSystem fileSystem, RepositoryExportingCheck repositoryExportingCheck) {
     this.repositoryLocationResolver = repositoryLocationResolver;
     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<Path> 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<Path> 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 synchronized void add(Repository repository, Object location) {
       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<Repository> getAll() {
-    return ImmutableList.copyOf(byNamespaceAndName.values());
+    return withReadLockedMaps(() -> ImmutableList.copyOf(byNamespaceAndName.values()));
   }
 
   @Override
@@ -147,14 +156,14 @@ public void modify(Repository repository) {
       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,23 +173,24 @@ public void modify(Repository repository) {
   }
 
   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
   public void delete(Repository repository) {
     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 Long getLastModified() {
 
   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> T withWriteLockedMaps(Supplier<T> runnable) {
+    Lock lock = byNamespaceLock.writeLock();
+    lock.lock();
+    try {
+      return runnable.get();
+    } finally {
+      lock.unlock();
+    }
+  }
+
+  private <T> T withReadLockedMaps(Supplier<T> 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
@@ -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.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<RepositoryRole> {
 
   @XmlJavaTypeAdapter(XmlRepositoryRoleMapAdapter.class)
   @XmlElement(name = "roles")
-  private Map<String, RepositoryRole> roleMap = new LinkedHashMap<>();
+  private Map<String, RepositoryRole> 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
@@ -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<XmlRepositoryRoleList, Map<String, RepositoryRole>> {
@@ -40,7 +40,7 @@ public XmlRepositoryRoleList marshal(Map<String, RepositoryRole> roleMap) {
 
   @Override
   public Map<String, RepositoryRole> unmarshal(XmlRepositoryRoleList roles) {
-    Map<String, RepositoryRole> roleMap = new LinkedHashMap<>();
+    Map<String, RepositoryRole> 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
@@ -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 void setLastModified(long lastModified)
   /** Field description */
   @XmlJavaTypeAdapter(XmlUserMapAdapter.class)
   @XmlElement(name = "users")
-  private Map<String, User> userMap = new LinkedHashMap<>();
+  private Map<String, User> 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
@@ -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 XmlUserList marshal(Map<String, User> userMap) throws Exception
   @Override
   public Map<String, User> unmarshal(XmlUserList users) throws Exception
   {
-    Map<String, User> userMap = new LinkedHashMap<>();
+    Map<String, User> userMap = new TreeMap<>();
 
     for (User user : users)
     {