Create a more flexible and typesafe id for indexed objects (#1785)

Id's can now be combined with more than just a repository. It is now possible to build a more complex Id such as Comment -> Pull request -> Repository. The id's now bound to a specific type. This makes it harder to accidentally use a id within an index of the wrong type.

scm-webapp/src/test/java/sonia/scm/group/GroupIndexerTest.java

@@ -90,14 +90,14 @@ class GroupIndexerTest {
   void shouldCreateGroup() {
     indexer.createStoreTask(astronauts).update(index);
 
-    verify(index).store(Id.of(astronauts), GroupPermissions.read(astronauts).asShiroString(), astronauts);
+    verify(index).store(Id.of(Group.class, astronauts), GroupPermissions.read(astronauts).asShiroString(), astronauts);
   }
 
   @Test
   void shouldDeleteGroup() {
     indexer.createDeleteTask(astronauts).update(index);
 
-    verify(index.delete()).byId(Id.of(astronauts));
+    verify(index.delete()).byId(Id.of(Group.class, astronauts));
   }
 
   @Test
@@ -108,8 +108,8 @@ class GroupIndexerTest {
     reIndexAll.update(index);
 
     verify(index.delete()).all();
-    verify(index).store(Id.of(astronauts), GroupPermissions.read(astronauts).asShiroString(), astronauts);
-    verify(index).store(Id.of(planetCreators), GroupPermissions.read(planetCreators).asShiroString(), planetCreators);
+    verify(index).store(Id.of(Group.class, astronauts), GroupPermissions.read(astronauts).asShiroString(), astronauts);
+    verify(index).store(Id.of(Group.class, planetCreators), GroupPermissions.read(planetCreators).asShiroString(), planetCreators);
   }
 
   @Test
@@ -120,7 +120,7 @@ class GroupIndexerTest {
 
     verify(searchEngine.forType(Group.class)).update(captor.capture());
     captor.getValue().update(index);
-    verify(index.delete()).byId(Id.of(astronauts));
+    verify(index.delete()).byId(Id.of(Group.class, astronauts));
   }
 
 }

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

@@ -89,13 +89,24 @@ class RepositoryIndexerTest {
 
     indexer.createStoreTask(heartOfGold).update(index);
 
-    verify(index).store(Id.of(heartOfGold), RepositoryPermissions.read(heartOfGold).asShiroString(), heartOfGold);
+    verify(index).store(Id.of(Repository.class, heartOfGold), RepositoryPermissions.read(heartOfGold).asShiroString(), heartOfGold);
   }
 
   @Test
-  void shouldDeleteRepository() {
+  void shouldDeleteRepositoryById() {
     Repository heartOfGold = RepositoryTestData.createHeartOfGold();
 
+    when(index.getDetails().getType()).then(ic -> Repository.class);
+    indexer.createDeleteTask(heartOfGold).update(index);
+
+    verify(index.delete()).byId(Id.of(Repository.class, heartOfGold));
+  }
+
+  @Test
+  void shouldDeleteAllByRepository() {
+    Repository heartOfGold = RepositoryTestData.createHeartOfGold();
+
+    when(index.getDetails().getType()).then(ic -> String.class);
     indexer.createDeleteTask(heartOfGold).update(index);
 
     verify(index.delete()).byRepository(heartOfGold);
@@ -111,8 +122,8 @@ class RepositoryIndexerTest {
     reIndexAll.update(index);
 
     verify(index.delete()).all();
-    verify(index).store(Id.of(heartOfGold), RepositoryPermissions.read(heartOfGold).asShiroString(), heartOfGold);
-    verify(index).store(Id.of(puzzle), RepositoryPermissions.read(puzzle).asShiroString(), puzzle);
+    verify(index).store(Id.of(Repository.class, heartOfGold), RepositoryPermissions.read(heartOfGold).asShiroString(), heartOfGold);
+    verify(index).store(Id.of(Repository.class, puzzle), RepositoryPermissions.read(puzzle).asShiroString(), puzzle);
   }
 
   @Test
@@ -136,7 +147,7 @@ class RepositoryIndexerTest {
 
     verify(searchEngine.forType(Repository.class)).update(captor.capture());
     captor.getValue().update(index);
-    verify(index).store(Id.of(heartOfGold), RepositoryPermissions.read(heartOfGold).asShiroString(), heartOfGold);
+    verify(index).store(Id.of(Repository.class, heartOfGold), RepositoryPermissions.read(heartOfGold).asShiroString(), heartOfGold);
   }
 
 }

scm-webapp/src/test/java/sonia/scm/search/IdsTest.java

@@ -0,0 +1,66 @@
+/*
+ * MIT License
+ *
+ * Copyright (c) 2020-present Cloudogu GmbH and Contributors
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+package sonia.scm.search;
+
+import org.junit.jupiter.api.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static sonia.scm.search.Ids.asString;
+
+class IdsTest {
+
+  @Test
+  void shouldUseNameFromAnnotation() {
+    Id<String> id = Id.of(String.class, "0")
+      .and(Z.class, "zzz");
+    assertThat(Ids.others(id)).containsEntry("c", "zzz");
+  }
+
+  @Test
+  void shouldEscapeSemicolon() {
+    Id<String> id = Id.of(String.class, "a;b;c");
+    assertThat(Ids.asString(id)).isEqualTo("a\\;b\\;c");
+  }
+
+  @Test
+  void shouldSortInAlphabeticalOrder() {
+    Id<String> id = Id.of(String.class, "0")
+      .and(D.class, "4")
+      .and(A.class, "1")
+      .and(B.class, "2")
+      .and(Z.class, "3");
+
+    assertThat(asString(id)).isEqualTo("0;1;2;3;4");
+  }
+
+  private static class A {}
+
+  private static class B {}
+
+  @IndexedType("c")
+  private static class Z {}
+
+  private static class D {}
+}

scm-webapp/src/test/java/sonia/scm/search/LuceneIndexTest.java

@@ -24,7 +24,6 @@
 
 package sonia.scm.search;
 
-import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import lombok.Value;
 import org.apache.lucene.analysis.standard.StandardAnalyzer;
 import org.apache.lucene.index.DirectoryReader;
@@ -32,7 +31,6 @@ import org.apache.lucene.index.IndexWriter;
 import org.apache.lucene.index.IndexWriterConfig;
 import org.apache.lucene.index.Term;
 import org.apache.lucene.search.IndexSearcher;
-import org.apache.lucene.search.ScoreDoc;
 import org.apache.lucene.search.TermQuery;
 import org.apache.lucene.search.TopDocs;
 import org.apache.lucene.store.ByteBuffersDirectory;
@@ -43,9 +41,10 @@ import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
+import sonia.scm.group.Group;
 import sonia.scm.repository.Repository;
 import sonia.scm.repository.RepositoryTestData;
-import sonia.scm.repository.RepositoryType;
+import sonia.scm.user.User;
 
 import java.io.IOException;
 import java.util.function.Supplier;
@@ -61,8 +60,8 @@ import static sonia.scm.search.FieldNames.REPOSITORY;
 
 class LuceneIndexTest {
 
-  private static final Id ONE = Id.of("one");
-  private static final Id TWO = Id.of("two");
+  private static final Id<Storable> ONE = Id.of(Storable.class, "one");
+  private static final Id<Storable> TWO = Id.of(Storable.class, "two");
 
   private Directory directory;
 
@@ -102,7 +101,7 @@ class LuceneIndexTest {
   @Test
   void shouldStoreRepositoryOfId() throws IOException {
     try (LuceneIndex<Storable> index = createIndex(Storable.class)) {
-      index.store(ONE.withRepository("4211"), null, new Storable("Some text"));
+      index.store(ONE.and(Repository.class, "4211"), null, new Storable("Some text"));
     }
 
     assertHits(REPOSITORY, "4211", 1);
@@ -126,12 +125,12 @@ class LuceneIndexTest {
     Repository heartOfGold = RepositoryTestData.createHeartOfGold();
     Repository puzzle42 = RepositoryTestData.createHeartOfGold();
     try (LuceneIndex<Storable> index = createIndex(Storable.class)) {
-      index.store(ONE.withRepository(heartOfGold), null, new Storable("content"));
-      index.store(ONE.withRepository(puzzle42), null, new Storable("content"));
+      index.store(ONE.and(Repository.class, heartOfGold), null, new Storable("content"));
+      index.store(ONE.and(Repository.class, puzzle42), null, new Storable("content"));
     }
 
     try (LuceneIndex<Storable> index = createIndex(Storable.class)) {
-      index.delete().byId(ONE.withRepository(heartOfGold));
+      index.delete().byId(ONE.and(Repository.class, heartOfGold));
     }
 
     assertHits("value", "content", 1);
@@ -154,8 +153,8 @@ class LuceneIndexTest {
   @Test
   void shouldDeleteByRepository() throws IOException {
     try (LuceneIndex<Storable> index = createIndex(Storable.class)) {
-      index.store(ONE.withRepository("4211"), null, new Storable("content"));
-      index.store(TWO.withRepository("4212"), null, new Storable("content"));
+      index.store(ONE.and(Repository.class, "4211"), null, new Storable("content"));
+      index.store(TWO.and(Repository.class, "4212"), null, new Storable("content"));
     }
 
     try (LuceneIndex<Storable> index = createIndex(Storable.class)) {
@@ -168,7 +167,7 @@ class LuceneIndexTest {
   @Test
   void shouldStorePermission() throws IOException {
     try (LuceneIndex<Storable> index = createIndex(Storable.class)) {
-      index.store(ONE.withRepository("4211"), "repo:4211:read", new Storable("Some other text"));
+      index.store(ONE.and(Repository.class, "4211"), "repo:4211:read", new Storable("Some other text"));
     }
 
     assertHits(PERMISSION, "repo:4211:read", 1);
@@ -183,6 +182,18 @@ class LuceneIndexTest {
     }
   }
 
+  @Test
+  void shouldStoreIdFields() throws IOException {
+    Id<Storable> id = ONE.and(User.class, "trillian").and(Group.class, "heart-of-gold");
+    try (LuceneIndex<Storable> index = createIndex(Storable.class)) {
+      index.store(id, "repo:4211:read", new Storable("Some other text"));
+    }
+
+    assertHits("_id", "one;heart-of-gold;trillian", 1);
+    assertHits("_user", "trillian", 1);
+    assertHits("_group", "heart-of-gold", 1);
+  }
+
   @Nested
   @ExtendWith(MockitoExtension.class)
   class ExceptionTests {
@@ -209,7 +220,7 @@ class LuceneIndexTest {
     void shouldThrowSearchEngineExceptionOnDeleteById() throws IOException {
       when(writer.deleteDocuments(any(Term.class))).thenThrow(new IOException("failed to delete"));
 
-      Index.Deleter deleter = index.delete();
+      Index.Deleter<Storable> deleter = index.delete();
       assertThrows(SearchEngineException.class, () -> deleter.byId(ONE));
     }
 
@@ -217,7 +228,7 @@ class LuceneIndexTest {
     void shouldThrowSearchEngineExceptionOnDeleteAll() throws IOException {
       when(writer.deleteAll()).thenThrow(new IOException("failed to delete"));
 
-      Index.Deleter deleter = index.delete();
+      Index.Deleter<Storable> deleter = index.delete();
       assertThrows(SearchEngineException.class, deleter::all);
     }
 
@@ -225,7 +236,7 @@ class LuceneIndexTest {
     void shouldThrowSearchEngineExceptionOnDeleteByRepository() throws IOException {
       when(writer.deleteDocuments(any(Term.class))).thenThrow(new IOException("failed to delete"));
 
-      Index.Deleter deleter = index.delete();
+      Index.Deleter<Storable> deleter = index.delete();
       assertThrows(SearchEngineException.class, () -> deleter.byRepository("42"));
     }
 
@@ -237,13 +248,11 @@ class LuceneIndexTest {
 
   }
 
-  @CanIgnoreReturnValue
-  private ScoreDoc[] assertHits(String field, String value, int expectedHits) throws IOException {
+  private void assertHits(String field, String value, int expectedHits) throws IOException {
     try (DirectoryReader reader = DirectoryReader.open(directory)) {
       IndexSearcher searcher = new IndexSearcher(reader);
       TopDocs docs = searcher.search(new TermQuery(new Term(field, value)), 10);
       assertThat(docs.totalHits.value).isEqualTo(expectedHits);
-      return docs.scoreDocs;
     }
   }
 

scm-webapp/src/test/java/sonia/scm/search/NamesTest.java

@@ -0,0 +1,48 @@
+/*
+ * MIT License
+ *
+ * Copyright (c) 2020-present Cloudogu GmbH and Contributors
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+package sonia.scm.search;
+
+import org.junit.jupiter.api.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+class NamesTest {
+
+  @Test
+  void shouldUseNameFromAnnotation() {
+    assertThat(Names.create(WithAlias.class)).isEqualTo("alias");
+  }
+
+  @Test
+  void shouldUseNameFromClass() {
+    assertThat(Names.create(Simple.class)).isEqualTo("simple");
+  }
+
+  private static class Simple {}
+
+  @IndexedType("alias")
+  private static class WithAlias {}
+
+}

scm-webapp/src/test/java/sonia/scm/user/UserIndexerTest.java

@@ -89,7 +89,7 @@ class UserIndexerTest {
 
     indexer.createStoreTask(trillian).update(index);
 
-    verify(index).store(Id.of(trillian), UserPermissions.read(trillian).asShiroString(), trillian);
+    verify(index).store(Id.of(User.class, trillian), UserPermissions.read(trillian).asShiroString(), trillian);
   }
 
   @Test
@@ -98,7 +98,7 @@ class UserIndexerTest {
 
     indexer.createDeleteTask(trillian).update(index);
 
-    verify(index.delete()).byId(Id.of(trillian));
+    verify(index.delete()).byId(Id.of(User.class, trillian));
   }
 
   @Test
@@ -111,8 +111,8 @@ class UserIndexerTest {
     reIndexAll.update(index);
 
     verify(index.delete()).all();
-    verify(index).store(Id.of(trillian), UserPermissions.read(trillian).asShiroString(), trillian);
-    verify(index).store(Id.of(slarti), UserPermissions.read(slarti).asShiroString(), slarti);
+    verify(index).store(Id.of(User.class, trillian), UserPermissions.read(trillian).asShiroString(), trillian);
+    verify(index).store(Id.of(User.class, slarti), UserPermissions.read(slarti).asShiroString(), slarti);
   }
 
   @Test
@@ -124,7 +124,7 @@ class UserIndexerTest {
 
     verify(searchEngine.forType(User.class)).update(captor.capture());
     captor.getValue().update(index);
-    verify(index.delete()).byId(Id.of(trillian));
+    verify(index.delete()).byId(Id.of(User.class, trillian));
   }
 
 }