From 571025032cd34ff4a694f2b58817e784b9003f2d Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Tue, 31 Aug 2021 11:27:49 +0200 Subject: [PATCH] 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. --- .../src/main/java/sonia/scm/search/Id.java | 143 +++++++----------- .../src/main/java/sonia/scm/search/Index.java | 8 +- .../test/java/sonia/scm/search/IdTest.java | 104 ++++++------- .../java/sonia/scm/group/GroupIndexer.java | 4 +- .../scm/repository/RepositoryIndexer.java | 10 +- .../src/main/java/sonia/scm/search/Ids.java | 66 ++++++++ .../java/sonia/scm/search/LuceneIndex.java | 29 +++- .../scm/search/LuceneSearchableType.java | 15 +- .../src/main/java/sonia/scm/search/Names.java | 51 +++++++ .../main/java/sonia/scm/user/UserIndexer.java | 4 +- .../sonia/scm/group/GroupIndexerTest.java | 10 +- .../scm/repository/RepositoryIndexerTest.java | 21 ++- .../test/java/sonia/scm/search/IdsTest.java | 66 ++++++++ .../sonia/scm/search/LuceneIndexTest.java | 45 +++--- .../test/java/sonia/scm/search/NamesTest.java | 48 ++++++ .../java/sonia/scm/user/UserIndexerTest.java | 10 +- 16 files changed, 430 insertions(+), 204 deletions(-) create mode 100644 scm-webapp/src/main/java/sonia/scm/search/Ids.java create mode 100644 scm-webapp/src/main/java/sonia/scm/search/Names.java create mode 100644 scm-webapp/src/test/java/sonia/scm/search/IdsTest.java create mode 100644 scm-webapp/src/test/java/sonia/scm/search/NamesTest.java diff --git a/scm-core/src/main/java/sonia/scm/search/Id.java b/scm-core/src/main/java/sonia/scm/search/Id.java index 5d47c19252..a51ae78282 100644 --- a/scm-core/src/main/java/sonia/scm/search/Id.java +++ b/scm-core/src/main/java/sonia/scm/search/Id.java @@ -25,17 +25,17 @@ package sonia.scm.search; import com.google.common.annotations.Beta; -import com.google.common.base.Joiner; import com.google.common.base.Preconditions; import com.google.common.base.Strings; +import com.google.common.collect.ImmutableMap; +import lombok.AccessLevel; import lombok.EqualsAndHashCode; +import lombok.Getter; import lombok.ToString; import sonia.scm.ModelObject; -import sonia.scm.repository.Repository; -import javax.annotation.Nonnull; -import javax.annotation.Nullable; -import java.util.Optional; +import java.util.Collections; +import java.util.Map; /** * Describes the id of an indexed object. @@ -45,114 +45,75 @@ import java.util.Optional; @Beta @ToString @EqualsAndHashCode -public final class Id { +@Getter(AccessLevel.PACKAGE) +public final class Id { - private final String value; - private final String repository; + private final Class mainType; + private final String mainId; + private final Map, String> others; - private Id(@Nonnull String value, @Nullable String repository) { - this.value = value; - this.repository = repository; + private Id(Class mainType, String mainId, Map, String> others) { + this.mainType = mainType; + this.mainId = mainId; + this.others = others; } /** - * Returns the string representation of the id without the repository part. - * - * @return string representation without repository. + * Creates a new combined id by adding a new type and value. + * @param type other type + * @param id other id + * @return new combined id + * @since 2.23.0 */ - public String getValue() { - return value; + public Id and(Class type, String id) { + Preconditions.checkArgument(type != null, "type is required"); + Preconditions.checkArgument(!Strings.isNullOrEmpty(id), "id is required"); + return new Id<>( + mainType, + mainId, + ImmutableMap., String>builder() + .putAll(others) + .put(type, id) + .build() + ); } /** - * Returns the repository id part of the id or an empty optional if the id does not belong to a repository. - * @return repository id or empty + * Creates a new combined id by adding a new type and value. + * @param type other type + * @param idObject object which holds id + * @return new combined id + * @since 2.23.0 */ - public Optional getRepository() { - return Optional.ofNullable(repository); - } - - /** - * Creates the id with the id of the given repository. - * @param repository repository - * @return id with repository id - */ - public Id withRepository(@Nonnull Repository repository) { - checkRepository(repository); - return withRepository(repository.getId()); - } - - /** - * Creates the id with the given repository id. - * @param repository repository id - * @return id with repository id - */ - public Id withRepository(@Nonnull String repository) { - checkRepository(repository); - return new Id(value, repository); - } - - /** - * Returns the string representation of the id including the repository. - * @return string representation - */ - public String asString() { - if (repository != null) { - return value + "/" + repository; - } - return value; + public Id and(Class type, ModelObject idObject) { + Preconditions.checkArgument(idObject != null, "id object is required"); + return and(type, idObject.getId()); } /** * Creates a new id. * - * @param value primary value of the id - * @param others additional values which should be part of the id + * @param mainType main type of the id + * @param mainId main id of the id * * @return new id */ - public static Id of(@Nonnull String value, String... others) { - Preconditions.checkArgument(!Strings.isNullOrEmpty(value), "primary value is required"); - String idValue = value; - if (others.length > 0) { - idValue += ":" + Joiner.on(':').join(others); - } - return new Id(idValue, null); + public static Id of(Class mainType, String mainId) { + Preconditions.checkArgument(mainType != null, "main type is required"); + Preconditions.checkArgument(!Strings.isNullOrEmpty(mainId), "main id is required"); + return new Id<>(mainType, mainId, Collections.emptyMap()); } /** - * Creates a new id for the given repository. + * Creates a new id. * - * @param repository repository - * @return id of repository + * @param mainType main type of the id + * @param mainIdObject object which holds the main id + * + * @return new id */ - public static Id of(@Nonnull Repository repository) { - checkRepository(repository); - String id = repository.getId(); - checkRepository(id); - return new Id(id, id); - } - - /** - * Creates a new id for the given model object. - * @param object model object - * @param others additional values which should be part of the id - * @return new id from model object - */ - public static Id of(@Nonnull ModelObject object, String... others) { - checkObject(object); - return of(object.getId(), others); - } - - private static void checkRepository(Repository repository) { - Preconditions.checkArgument(repository != null, "repository is required"); - } - - private static void checkRepository(String repository) { - Preconditions.checkArgument(!Strings.isNullOrEmpty(repository), "repository id is required"); - } - - private static void checkObject(@Nonnull ModelObject object) { - Preconditions.checkArgument(object != null, "object is required"); + public static Id of(Class mainType, ModelObject mainIdObject) { + Preconditions.checkArgument(mainIdObject != null, "main id object is required"); + return of(mainType, mainIdObject.getId()); } } diff --git a/scm-core/src/main/java/sonia/scm/search/Index.java b/scm-core/src/main/java/sonia/scm/search/Index.java index 3a00fc50b0..7532c70a92 100644 --- a/scm-core/src/main/java/sonia/scm/search/Index.java +++ b/scm-core/src/main/java/sonia/scm/search/Index.java @@ -53,27 +53,27 @@ public interface Index { * * @see Indexed */ - void store(Id id, String permission, T object); + void store(Id id, String permission, T object); /** * Delete provides an api to delete objects from the index * @return delete api * @since 2.23.0 */ - Deleter delete(); + Deleter delete(); /** * Deleter provides an api to delete object from index. * * @since 2.23.0 */ - interface Deleter { + interface Deleter { /** * Delete the object with the given id and type from index. * @param id id of object */ - void byId(Id id); + void byId(Id id); /** * Delete all objects of the given type from index. diff --git a/scm-core/src/test/java/sonia/scm/search/IdTest.java b/scm-core/src/test/java/sonia/scm/search/IdTest.java index ae38ec2682..0cc36b6852 100644 --- a/scm-core/src/test/java/sonia/scm/search/IdTest.java +++ b/scm-core/src/test/java/sonia/scm/search/IdTest.java @@ -26,6 +26,7 @@ package sonia.scm.search; import org.junit.jupiter.api.Test; import sonia.scm.ModelObject; +import sonia.scm.group.Group; import sonia.scm.repository.Repository; import sonia.scm.user.User; @@ -35,94 +36,95 @@ import static org.junit.jupiter.api.Assertions.assertThrows; class IdTest { @Test - void shouldCreateIdFromPrimary() { - Id id = Id.of("one"); - assertThat(id.getValue()).isEqualTo("one"); + void shouldCreateSimpleId() { + Id id = Id.of(Repository.class, "42"); + assertThat(id.getMainType()).isEqualTo(Repository.class); + assertThat(id.getMainId()).isEqualTo("42"); + assertThat(id.getOthers()).isEmpty(); } @Test - void shouldCreateIdWithoutRepository() { - Id id = Id.of("one"); - assertThat(id.getRepository()).isEmpty(); + void shouldFailWithNullType() { + assertThrows(IllegalArgumentException.class, () -> Id.of(null, "42")); } @Test - void shouldFailWithoutPrimaryValue() { - assertThrows(IllegalArgumentException.class, () -> Id.of((String) null)); + void shouldFailWithEmptyId() { + assertThrows(IllegalArgumentException.class, () -> Id.of(Repository.class, "")); } @Test - void shouldFailWithEmptyPrimaryValue() { - assertThrows(IllegalArgumentException.class, () -> Id.of("")); + void shouldFailWithNullId() { + assertThrows(IllegalArgumentException.class, () -> Id.of(Repository.class, (String) null)); } @Test - void shouldCreateCombinedValue() { - Id id = Id.of("one", "two", "three"); - assertThat(id.getValue()).isEqualTo("one:two:three"); + void shouldFailWithNullIdObject() { + assertThrows(IllegalArgumentException.class, () -> Id.of(Repository.class, (ModelObject) null)); } @Test - void shouldAddRepositoryId() { - Id id = Id.of("one").withRepository("4211"); - assertThat(id.getRepository()).contains("4211"); - } - - @Test - void shouldAddRepository() { + void shouldCreateSimpleFromModelObject() { Repository repository = new Repository(); - repository.setId("4211"); - Id id = Id.of("one").withRepository(repository); - assertThat(id.getRepository()).contains("4211"); + repository.setId("42"); + Id id = Id.of(Repository.class, repository); + assertThat(id.getMainType()).isEqualTo(Repository.class); + assertThat(id.getMainId()).isEqualTo("42"); + assertThat(id.getOthers()).isEmpty(); } @Test - void shouldCreateIdFromRepository() { - Repository repository = new Repository(); - repository.setId("4211"); - Id id = Id.of(repository); - assertThat(id.getRepository()).contains("4211"); + void shouldCreateIdWithOneOther() { + Id id = Id.of(User.class, "trillian").and(Group.class, "hog"); + assertThat(id.getMainType()).isEqualTo(User.class); + assertThat(id.getMainId()).isEqualTo("trillian"); + assertThat(id.getOthers()).containsEntry(Group.class, "hog"); } @Test - void shouldFailWithoutRepository() { - Id id = Id.of("one"); - assertThrows(IllegalArgumentException.class, () -> id.withRepository((Repository) null)); + void shouldCreateIdWithOtherFromModelObject() { + Group group = new Group("xml", "hog"); + Id id = Id.of(User.class, "trillian").and(Group.class, group); + assertThat(id.getMainType()).isEqualTo(User.class); + assertThat(id.getMainId()).isEqualTo("trillian"); + assertThat(id.getOthers()).containsEntry(Group.class, "hog"); } @Test - void shouldFailWithoutRepositoryId() { - Id id = Id.of("one"); - assertThrows(IllegalArgumentException.class, () -> id.withRepository((String) null)); + void shouldCreateIdWithOthers() { + Id id = Id.of(User.class, "trillian") + .and(Group.class, "hog") + .and(Repository.class, "heart-of-gold"); + + assertThat(id.getMainType()).isEqualTo(User.class); + assertThat(id.getMainId()).isEqualTo("trillian"); + assertThat(id.getOthers()) + .containsEntry(Group.class, "hog") + .containsEntry(Repository.class, "heart-of-gold"); } @Test - void shouldFailWithEmptyRepositoryId() { - Id id = Id.of("one"); - assertThrows(IllegalArgumentException.class, () -> id.withRepository((String) null)); + void shouldFailIfOtherTypeIsNull() { + Id id = Id.of(User.class, "trillian"); + assertThrows(IllegalArgumentException.class, () -> id.and(null, "hog")); } @Test - void shouldCreateIdFromModelObject() { - Id id = Id.of(new User("trillian")); - assertThat(id.getValue()).isEqualTo("trillian"); + void shouldFailIfOtherIdIsNull() { + Id id = Id.of(User.class, "trillian"); + assertThrows(IllegalArgumentException.class, () -> id.and(Group.class, (String) null)); } @Test - void shouldFailWithoutModelObject() { - assertThrows(IllegalArgumentException.class, () -> Id.of((ModelObject) null)); + void shouldFailIfOtherIdIsEmpty() { + Id id = Id.of(User.class, "trillian"); + assertThrows(IllegalArgumentException.class, () -> id.and(Group.class, "")); } @Test - void shouldReturnSimpleIdAsString() { - Id id = Id.of("one", "two"); - assertThat(id.asString()).isEqualTo("one:two"); - } - - @Test - void shouldReturnIdWithRepositoryAsString() { - Id id = Id.of("one", "two").withRepository("4211"); - assertThat(id.asString()).isEqualTo("one:two/4211"); + void shouldFailIfOtherIdObjectIsNull() { + Id id = Id.of(User.class, "trillian"); + assertThrows(IllegalArgumentException.class, () -> id.and(Group.class, (ModelObject) null)); } } diff --git a/scm-webapp/src/main/java/sonia/scm/group/GroupIndexer.java b/scm-webapp/src/main/java/sonia/scm/group/GroupIndexer.java index 103b1d5638..af6b3f59f6 100644 --- a/scm-webapp/src/main/java/sonia/scm/group/GroupIndexer.java +++ b/scm-webapp/src/main/java/sonia/scm/group/GroupIndexer.java @@ -74,7 +74,7 @@ public class GroupIndexer implements Indexer { @Override public SerializableIndexTask createDeleteTask(Group group) { - return index -> index.delete().byId(Id.of(group)); + return index -> index.delete().byId(Id.of(Group.class, group)); } @Subscribe(async = false) @@ -83,7 +83,7 @@ public class GroupIndexer implements Indexer { } public static void store(Index index, Group group) { - index.store(Id.of(group), GroupPermissions.read(group).asShiroString(), group); + index.store(Id.of(Group.class, group), GroupPermissions.read(group).asShiroString(), group); } public static class ReIndexAll extends ReIndexAllTask { diff --git a/scm-webapp/src/main/java/sonia/scm/repository/RepositoryIndexer.java b/scm-webapp/src/main/java/sonia/scm/repository/RepositoryIndexer.java index 9016d72b1d..2ddc0131b6 100644 --- a/scm-webapp/src/main/java/sonia/scm/repository/RepositoryIndexer.java +++ b/scm-webapp/src/main/java/sonia/scm/repository/RepositoryIndexer.java @@ -90,11 +90,17 @@ public class RepositoryIndexer implements Indexer { @Override public SerializableIndexTask createDeleteTask(Repository repository) { - return index -> index.delete().byRepository(repository); + return index -> { + if (Repository.class.equals(index.getDetails().getType())) { + index.delete().byId(Id.of(Repository.class, repository.getId())); + } else { + index.delete().byRepository(repository); + } + }; } private static void store(Index index, Repository repository) { - index.store(Id.of(repository), RepositoryPermissions.read(repository).asShiroString(), repository); + index.store(Id.of(Repository.class, repository), RepositoryPermissions.read(repository).asShiroString(), repository); } public static class ReIndexAll extends ReIndexAllTask { diff --git a/scm-webapp/src/main/java/sonia/scm/search/Ids.java b/scm-webapp/src/main/java/sonia/scm/search/Ids.java new file mode 100644 index 0000000000..ec7239e681 --- /dev/null +++ b/scm-webapp/src/main/java/sonia/scm/search/Ids.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 java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.TreeMap; +import java.util.stream.Collectors; + +final class Ids { + + private Ids() { + } + + static Map others(Id id) { + Map result = new TreeMap<>(); + + Map, String> others = id.getOthers(); + for (Map.Entry, String> e : others.entrySet()) { + result.put(name(e.getKey()), e.getValue()); + } + + return result; + } + + static String id(String mainId, Map others) { + List values = new ArrayList<>(); + values.add(mainId); + values.addAll(others.values()); + return values.stream() + .map(v -> v.replace(";", "\\;" )) + .collect(Collectors.joining(";")); + } + + static String asString(Id id) { + return id(id.getMainId(), others(id)); + } + + private static String name(Class key) { + return Names.create(key, key.getAnnotation(IndexedType.class)); + } + +} diff --git a/scm-webapp/src/main/java/sonia/scm/search/LuceneIndex.java b/scm-webapp/src/main/java/sonia/scm/search/LuceneIndex.java index 4d201b38af..62a156e4c2 100644 --- a/scm-webapp/src/main/java/sonia/scm/search/LuceneIndex.java +++ b/scm-webapp/src/main/java/sonia/scm/search/LuceneIndex.java @@ -36,6 +36,7 @@ import org.slf4j.LoggerFactory; import javax.annotation.Nonnull; import java.io.IOException; +import java.util.Map; import java.util.function.Supplier; import static sonia.scm.search.FieldNames.ID; @@ -72,14 +73,23 @@ class LuceneIndex implements Index, AutoCloseable { } @Override - public void store(Id id, String permission, Object object) { + public void store(Id id, String permission, T object) { Document document = searchableType.getTypeConverter().convert(object); + + String mainId = id.getMainId(); + Map others = Ids.others(id); + try { - field(document, ID, id.asString()); - id.getRepository().ifPresent(repository -> field(document, REPOSITORY, repository)); + field(document, ID, Ids.id(mainId, others)); + + for (Map.Entry e : others.entrySet()) { + field(document, "_" + e.getKey(), e.getValue()); + } + if (!Strings.isNullOrEmpty(permission)) { field(document, PERMISSION, permission); } + writer.updateDocument(idTerm(id), document); } catch (IOException e) { throw new SearchEngineException("failed to add document to index", e); @@ -87,16 +97,19 @@ class LuceneIndex implements Index, AutoCloseable { } @Nonnull - private Term idTerm(Id id) { - return new Term(ID, id.asString()); + private Term idTerm(Id id) { + String mainId = id.getMainId(); + Map others = Ids.others(id); + return new Term(ID, Ids.id(mainId, others)); } + private void field(Document document, String type, String name) { document.add(new StringField(type, name, Field.Store.YES)); } @Override - public Deleter delete() { + public Deleter delete() { return new LuceneDeleter(); } @@ -109,10 +122,10 @@ class LuceneIndex implements Index, AutoCloseable { } } - private class LuceneDeleter implements Deleter { + private class LuceneDeleter implements Deleter { @Override - public void byId(Id id) { + public void byId(Id id) { try { long count = writer.deleteDocuments(idTerm(id)); LOG.debug("delete {} document(s) by id {} from index {}", count, id, details); diff --git a/scm-webapp/src/main/java/sonia/scm/search/LuceneSearchableType.java b/scm-webapp/src/main/java/sonia/scm/search/LuceneSearchableType.java index 61e1b5e793..0a7c4711bd 100644 --- a/scm-webapp/src/main/java/sonia/scm/search/LuceneSearchableType.java +++ b/scm-webapp/src/main/java/sonia/scm/search/LuceneSearchableType.java @@ -28,6 +28,8 @@ import com.google.common.base.Strings; import lombok.Value; import org.apache.lucene.queryparser.flexible.standard.config.PointsConfig; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; import java.util.Collection; import java.util.Collections; import java.util.HashMap; @@ -50,9 +52,9 @@ public class LuceneSearchableType implements SearchableType { Map pointsConfig; TypeConverter typeConverter; - public LuceneSearchableType(Class type, IndexedType annotation, List fields) { + public LuceneSearchableType(Class type, @Nonnull IndexedType annotation, List fields) { this.type = type; - this.name = name(type, annotation); + this.name = Names.create(type, annotation); this.permission = Strings.emptyToNull(annotation.permission()); this.fields = fields; this.fieldNames = fieldNames(fields); @@ -65,15 +67,6 @@ public class LuceneSearchableType implements SearchableType { return Optional.ofNullable(permission); } - private String name(Class type, IndexedType annotation) { - String nameFromAnnotation = annotation.value(); - if (Strings.isNullOrEmpty(nameFromAnnotation)) { - String simpleName = type.getSimpleName(); - return Character.toLowerCase(simpleName.charAt(0)) + simpleName.substring(1); - } - return nameFromAnnotation; - } - private String[] fieldNames(List fields) { return fields.stream() .filter(LuceneSearchableField::isDefaultQuery) diff --git a/scm-webapp/src/main/java/sonia/scm/search/Names.java b/scm-webapp/src/main/java/sonia/scm/search/Names.java new file mode 100644 index 0000000000..29ec0b3fc2 --- /dev/null +++ b/scm-webapp/src/main/java/sonia/scm/search/Names.java @@ -0,0 +1,51 @@ +/* + * 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 com.google.common.base.Strings; + +import javax.annotation.Nullable; + +final class Names { + + private Names() { + } + + static String create(Class type) { + return create(type, type.getAnnotation(IndexedType.class)); + } + + static String create(Class type, @Nullable IndexedType annotation) { + String nameFromAnnotation = null; + if (annotation != null) { + nameFromAnnotation = annotation.value(); + } + if (Strings.isNullOrEmpty(nameFromAnnotation)) { + String simpleName = type.getSimpleName(); + return Character.toLowerCase(simpleName.charAt(0)) + simpleName.substring(1); + } + return nameFromAnnotation; + } +} diff --git a/scm-webapp/src/main/java/sonia/scm/user/UserIndexer.java b/scm-webapp/src/main/java/sonia/scm/user/UserIndexer.java index 383945e714..62fc1baf87 100644 --- a/scm-webapp/src/main/java/sonia/scm/user/UserIndexer.java +++ b/scm-webapp/src/main/java/sonia/scm/user/UserIndexer.java @@ -74,7 +74,7 @@ public class UserIndexer implements Indexer { @Override public SerializableIndexTask createDeleteTask(User item) { - return index -> index.delete().byId(Id.of(item)); + return index -> index.delete().byId(Id.of(User.class, item)); } @Subscribe(async = false) @@ -83,7 +83,7 @@ public class UserIndexer implements Indexer { } private static void store(Index index, User user) { - index.store(Id.of(user), UserPermissions.read(user).asShiroString(), user); + index.store(Id.of(User.class, user), UserPermissions.read(user).asShiroString(), user); } public static class ReIndexAll extends ReIndexAllTask { diff --git a/scm-webapp/src/test/java/sonia/scm/group/GroupIndexerTest.java b/scm-webapp/src/test/java/sonia/scm/group/GroupIndexerTest.java index 099a7966d1..32fbffac53 100644 --- a/scm-webapp/src/test/java/sonia/scm/group/GroupIndexerTest.java +++ b/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)); } } diff --git a/scm-webapp/src/test/java/sonia/scm/repository/RepositoryIndexerTest.java b/scm-webapp/src/test/java/sonia/scm/repository/RepositoryIndexerTest.java index 7114296c69..a6126697a1 100644 --- a/scm-webapp/src/test/java/sonia/scm/repository/RepositoryIndexerTest.java +++ b/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); } } diff --git a/scm-webapp/src/test/java/sonia/scm/search/IdsTest.java b/scm-webapp/src/test/java/sonia/scm/search/IdsTest.java new file mode 100644 index 0000000000..6d9de70abe --- /dev/null +++ b/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 id = Id.of(String.class, "0") + .and(Z.class, "zzz"); + assertThat(Ids.others(id)).containsEntry("c", "zzz"); + } + + @Test + void shouldEscapeSemicolon() { + Id id = Id.of(String.class, "a;b;c"); + assertThat(Ids.asString(id)).isEqualTo("a\\;b\\;c"); + } + + @Test + void shouldSortInAlphabeticalOrder() { + Id 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 {} +} diff --git a/scm-webapp/src/test/java/sonia/scm/search/LuceneIndexTest.java b/scm-webapp/src/test/java/sonia/scm/search/LuceneIndexTest.java index f39e997f72..e216bb6128 100644 --- a/scm-webapp/src/test/java/sonia/scm/search/LuceneIndexTest.java +++ b/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 ONE = Id.of(Storable.class, "one"); + private static final Id TWO = Id.of(Storable.class, "two"); private Directory directory; @@ -102,7 +101,7 @@ class LuceneIndexTest { @Test void shouldStoreRepositoryOfId() throws IOException { try (LuceneIndex 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 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 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 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 index = createIndex(Storable.class)) { @@ -168,7 +167,7 @@ class LuceneIndexTest { @Test void shouldStorePermission() throws IOException { try (LuceneIndex 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 id = ONE.and(User.class, "trillian").and(Group.class, "heart-of-gold"); + try (LuceneIndex 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 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 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 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; } } diff --git a/scm-webapp/src/test/java/sonia/scm/search/NamesTest.java b/scm-webapp/src/test/java/sonia/scm/search/NamesTest.java new file mode 100644 index 0000000000..240ea0b67e --- /dev/null +++ b/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 {} + +} diff --git a/scm-webapp/src/test/java/sonia/scm/user/UserIndexerTest.java b/scm-webapp/src/test/java/sonia/scm/user/UserIndexerTest.java index 7321bdef53..c3e2195cd3 100644 --- a/scm-webapp/src/test/java/sonia/scm/user/UserIndexerTest.java +++ b/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)); } }