More flexible delete and query api (#1790)

Replaces the filter and delete by repository api's with a more flexible api, which allows to filter and delete by any id part.
This commit is contained in:
Sebastian Sdorra
2021-09-01 16:19:19 +02:00
committed by GitHub
parent ea7964d224
commit 70fba6c990
14 changed files with 346 additions and 102 deletions

View File

@@ -33,6 +33,7 @@ import lombok.EqualsAndHashCode;
import lombok.Getter;
import lombok.ToString;
import sonia.scm.ModelObject;
import sonia.scm.repository.Repository;
import java.util.Collections;
import java.util.Map;
@@ -90,6 +91,17 @@ public final class Id<T> {
return and(type, idObject.getId());
}
/**
* Creates a new combined id by adding the given repository.
* @param repository repository to add
* @return new combined id
* @since 2.23.0
*/
public Id<T> and(Repository repository) {
Preconditions.checkArgument(repository != null, "repository is required");
return and(Repository.class, repository);
}
/**
* Creates a new id.
*

View File

@@ -25,6 +25,7 @@
package sonia.scm.search;
import com.google.common.annotations.Beta;
import sonia.scm.ModelObject;
import sonia.scm.repository.Repository;
/**
@@ -81,20 +82,74 @@ public interface Index<T> {
void all();
/**
* Delete all objects which are related the given type and repository from index.
*
* @param repositoryId id of repository
* Returns a builder api to delete objects with the given part of the id.
* @param type type of id part
* @param id value of id part
* @return builder delete api
* @see Id#and(Class, String)
*/
void byRepository(String repositoryId);
DeleteBy by(Class<?> type, String id);
/**
* Delete all objects which are related the given type and repository from index.
*
* @param repository repository
* Returns a builder api to delete objects with the given part of the id.
* @param type type of id part
* @param idObject object which holds the id value
* @return builder delete api
* @see Id#and(Class, ModelObject)
*/
default void byRepository(Repository repository) {
byRepository(repository.getId());
default DeleteBy by(Class<?> type, ModelObject idObject) {
return by(type, idObject.getId());
}
/**
* Returns a builder api to delete objects with the given repository part of the id.
* @param repository repository part of the id
* @return builder delete api
* @see Id#and(Class, ModelObject)
*/
default DeleteBy by(Repository repository) {
return by(Repository.class, repository);
}
}
/**
* Builder api to delete all objects which match parts of their id.
* @since 2.23.0
* @see Id#and(Class, String)
*/
interface DeleteBy {
/**
* Select only objects for deletion which are matching the previous id parts and the given one.
* @param type type of id part
* @param id value of id part
* @return {@code this}
*/
DeleteBy and(Class<?> type, String id);
/**
* Select only objects for deletion which are matching the previous id parts and the given one.
* @param type type of id part
* @param idObject object which holds the id value
* @return {@code this}
*/
default DeleteBy and(Class<?> type, ModelObject idObject) {
return and(type, idObject.getId());
}
/**
* Select only objects for deletion which are matching the previous id parts and the given repository part.
* @param repository repository part of the id
* @return {@code this}
*/
default DeleteBy and(Repository repository) {
return and(Repository.class, repository.getId());
}
/**
* Delete all matching objects from index.
*/
void execute();
}
}

View File

@@ -26,13 +26,11 @@ package sonia.scm.search;
import com.google.common.annotations.Beta;
import lombok.Value;
import sonia.scm.NotFoundException;
import sonia.scm.ModelObject;
import sonia.scm.repository.Repository;
import java.util.Optional;
import static sonia.scm.ContextEntry.ContextBuilder.entity;
import static sonia.scm.NotFoundException.notFound;
import java.util.HashMap;
import java.util.Map;
/**
* Build and execute queries against an index.
@@ -43,29 +41,49 @@ import static sonia.scm.NotFoundException.notFound;
@Beta
public abstract class QueryBuilder<T> {
private String repositoryId;
private final Map<Class<?>, String> filters = new HashMap<>();
private int start = 0;
private int limit = 10;
/**
* Return only results which are related to the given repository.
* @param repository repository
* Return only results which are related to the given part of the id.
* Note: this function can be called multiple times.
* @param type type of id part
* @param id value of id part
* @return {@code this}
* @since 2.23.0
* @see Id#and(Class, String)
*/
public QueryBuilder<T> repository(Repository repository) {
return repository(repository.getId());
public QueryBuilder<T> filter(Class<?> type, String id) {
filters.put(type, id);
return this;
}
/**
* Return only results which are related to the repository with the given id.
* @param repositoryId id of the repository
* Return only results which are related to the given repository.
* @param repository repository for filter
* @return {@code this}
* @since 2.23.0
* @see Id#and(Class, String)
*/
public QueryBuilder<T> repository(String repositoryId) {
this.repositoryId = repositoryId;
public QueryBuilder<T> filter(Repository repository) {
filters.put(Repository.class, repository.getId());
return this;
}
/**
* Return only results which are related to the given part of the id.
* Note: this function can be called multiple times.
* @param type type of id part
* @param idObject object which holds the id value
* @return {@code this}
* @since 2.23.0
* @see Id#and(Class, ModelObject)
*/
public QueryBuilder<T> filter(Class<?> type, ModelObject idObject) {
return filter(type, idObject.getId());
}
/**
* The result should start at the given number.
* All matching objects before the given start are skipped.
@@ -94,7 +112,7 @@ public abstract class QueryBuilder<T> {
* @return result of query
*/
public QueryResult execute(String queryString){
return execute(new QueryParams(repositoryId, queryString, start, limit));
return execute(new QueryParams(queryString, filters, start, limit));
}
@@ -107,7 +125,7 @@ public abstract class QueryBuilder<T> {
* @since 2.22.0
*/
public QueryCountResult count(String queryString) {
return count(new QueryParams(repositoryId, queryString, start, limit));
return count(new QueryParams(queryString, filters, start, limit));
}
@@ -133,13 +151,10 @@ public abstract class QueryBuilder<T> {
*/
@Value
static class QueryParams {
String repositoryId;
String queryString;
Map<Class<?>, String> filters;
int start;
int limit;
public Optional<String> getRepositoryId() {
return Optional.ofNullable(repositoryId);
}
}
}

View File

@@ -92,9 +92,12 @@ class IdTest {
@Test
void shouldCreateIdWithOthers() {
Repository repository = new Repository();
repository.setId("heart-of-gold");
Id<User> id = Id.of(User.class, "trillian")
.and(Group.class, "hog")
.and(Repository.class, "heart-of-gold");
.and(repository);
assertThat(id.getMainType()).isEqualTo(User.class);
assertThat(id.getMainId()).isEqualTo("trillian");
@@ -127,4 +130,10 @@ class IdTest {
assertThrows(IllegalArgumentException.class, () -> id.and(Group.class, (ModelObject) null));
}
@Test
void shouldFailIfRepositoryIsNull() {
Id<User> id = Id.of(User.class, "trillian");
assertThrows(IllegalArgumentException.class, () -> id.and(null));
}
}

View File

@@ -0,0 +1,88 @@
/*
* 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 sonia.scm.group.Group;
import sonia.scm.repository.Repository;
import sonia.scm.user.User;
import static org.assertj.core.api.Assertions.assertThat;
class QueryBuilderTest {
private final QueryBuilder<String> queryBuilder = new CapturingQueryBuilder<>();
private QueryBuilder.QueryParams params;
@Test
void shouldCreateFilterForRepository() {
Repository repository = new Repository();
repository.setId("hog");
queryBuilder.filter(repository).execute("awesome");
assertThat(params.getFilters()).containsEntry(Repository.class, "hog");
}
@Test
void shouldCreateFilterMap() {
queryBuilder.filter(User.class, "one")
.filter(Group.class, new Group("xml", "crew"))
.count("awesome");
assertThat(params.getFilters())
.containsEntry(User.class, "one")
.containsEntry(Group.class, "crew");
}
@Test
void shouldPassPagingParameters() {
queryBuilder.start(10).limit(25).execute("...");
assertThat(params.getStart()).isEqualTo(10);
assertThat(params.getLimit()).isEqualTo(25);
}
@Test
void shouldCreateParamsWithDefaults() {
queryBuilder.execute("hello");
assertThat(params.getQueryString()).isEqualTo("hello");
assertThat(params.getFilters()).isEmpty();
assertThat(params.getStart()).isZero();
assertThat(params.getLimit()).isEqualTo(10);
}
private class CapturingQueryBuilder<T> extends QueryBuilder<T> {
@Override
protected QueryResult execute(QueryParams queryParams) {
QueryBuilderTest.this.params = queryParams;
return null;
}
}
}

View File

@@ -94,7 +94,7 @@ public class RepositoryIndexer implements Indexer<Repository> {
if (Repository.class.equals(index.getDetails().getType())) {
index.delete().byId(Id.of(Repository.class, repository.getId()));
} else {
index.delete().byRepository(repository);
index.delete().by(Repository.class, repository).execute();
}
};
}

View File

@@ -24,10 +24,12 @@
package sonia.scm.search;
import sonia.scm.repository.Repository;
final class FieldNames {
private FieldNames(){}
static final String ID = "_id";
static final String REPOSITORY = "_repository";
static final String REPOSITORY = Names.field(Repository.class);
static final String PERMISSION = "_permission";
}

View File

@@ -31,17 +31,18 @@ import org.apache.lucene.document.Field;
import org.apache.lucene.document.StringField;
import org.apache.lucene.index.IndexWriter;
import org.apache.lucene.index.Term;
import org.apache.lucene.search.Query;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import javax.annotation.Nonnull;
import java.io.IOException;
import java.util.HashMap;
import java.util.Map;
import java.util.function.Supplier;
import static sonia.scm.search.FieldNames.ID;
import static sonia.scm.search.FieldNames.PERMISSION;
import static sonia.scm.search.FieldNames.REPOSITORY;
class LuceneIndex<T> implements Index<T>, AutoCloseable {
@@ -127,8 +128,8 @@ class LuceneIndex<T> implements Index<T>, AutoCloseable {
@Override
public void byId(Id<T> id) {
try {
long count = writer.deleteDocuments(idTerm(id));
LOG.debug("delete {} document(s) by id {} from index {}", count, id, details);
LOG.debug("delete document(s) by id {} from index {}", id, details);
writer.deleteDocuments(idTerm(id));
} catch (IOException e) {
throw new SearchEngineException("failed to delete document from index", e);
}
@@ -137,26 +138,43 @@ class LuceneIndex<T> implements Index<T>, AutoCloseable {
@Override
public void all() {
try {
long count = writer.deleteAll();
LOG.debug("deleted all {} documents from index {}", count, details);
LOG.debug("deleted all documents from index {}", details);
writer.deleteAll();
} catch (IOException ex) {
throw new SearchEngineException("failed to delete documents by type " + searchableType.getName() + " from index", ex);
}
}
@Override
public void byRepository(String repositoryId) {
public DeleteBy by(Class<?> type, String id) {
return new LuceneDeleteBy(type, id);
}
}
private class LuceneDeleteBy implements DeleteBy {
private final Map<Class<?>, String> map = new HashMap<>();
private LuceneDeleteBy(Class<?> type, String id) {
map.put(type, id);
}
@Override
public DeleteBy and(Class<?> type, String id) {
map.put(type, id);
return this;
}
@Override
public void execute() {
Query query = Queries.filterQuery(map);
try {
long count = writer.deleteDocuments(repositoryTerm(repositoryId));
LOG.debug("deleted {} documents by repository {} from index {}", count, repositoryId, details);
} catch (IOException ex) {
throw new SearchEngineException("failed to delete documents by repository " + repositoryId + " from index", ex);
LOG.debug("delete document(s) by query {} from index {}", query, details);
writer.deleteDocuments(query);
} catch (IOException e) {
throw new SearchEngineException("failed to delete document from index", e);
}
}
@Nonnull
private Term repositoryTerm(String repositoryId) {
return new Term(REPOSITORY, repositoryId);
}
}
}

View File

@@ -48,4 +48,8 @@ final class Names {
}
return nameFromAnnotation;
}
public static String field(Class<?> type) {
return "_" + create(type);
}
}

View File

@@ -29,7 +29,7 @@ import org.apache.lucene.search.BooleanQuery;
import org.apache.lucene.search.Query;
import org.apache.lucene.search.TermQuery;
import java.util.Optional;
import java.util.Map;
import static org.apache.lucene.search.BooleanClause.Occur.MUST;
@@ -38,18 +38,30 @@ final class Queries {
private Queries() {
}
private static Query repositoryQuery(String repositoryId) {
return new TermQuery(new Term(FieldNames.REPOSITORY, repositoryId));
}
static Query filter(Query query, QueryBuilder.QueryParams params) {
Optional<String> repositoryId = params.getRepositoryId();
if (repositoryId.isPresent()) {
return new BooleanQuery.Builder()
.add(query, MUST)
.add(repositoryQuery(repositoryId.get()), MUST)
.build();
Map<Class<?>, String> filters = params.getFilters();
if (!filters.isEmpty()) {
BooleanQuery.Builder builder = builder(filters);
builder.add(query, MUST);
return builder.build();
}
return query;
}
static Query filterQuery(Map<Class<?>, String> filters) {
return builder(filters).build();
}
private static BooleanQuery.Builder builder(Map<Class<?>, String> filters) {
BooleanQuery.Builder builder = new BooleanQuery.Builder();
for (Map.Entry<Class<?>, String> e : filters.entrySet()) {
Term term = createTerm(e.getKey(), e.getValue());
builder.add(new TermQuery(term), MUST);
}
return builder;
}
private static Term createTerm(Class<?> type, String id) {
return new Term(Names.field(type), id);
}
}

View File

@@ -28,6 +28,7 @@ import com.google.common.annotations.VisibleForTesting;
import org.apache.lucene.document.Document;
import org.apache.lucene.index.IndexWriter;
import org.apache.lucene.index.Term;
import org.apache.lucene.search.Query;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -70,6 +71,10 @@ class SharableIndexWriter {
return writer.deleteDocuments(term);
}
long deleteDocuments(Query query) throws IOException {
return writer.deleteDocuments(query);
}
long deleteAll() throws IOException {
return writer.deleteAll();
}

View File

@@ -26,8 +26,6 @@ package sonia.scm.repository;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.EnumSource;
import org.mockito.Answers;
import org.mockito.ArgumentCaptor;
import org.mockito.Captor;
@@ -109,7 +107,7 @@ class RepositoryIndexerTest {
when(index.getDetails().getType()).then(ic -> String.class);
indexer.createDeleteTask(heartOfGold).update(index);
verify(index.delete()).byRepository(heartOfGold);
verify(index.delete().by(Repository.class, heartOfGold)).execute();
}
@Test
@@ -135,7 +133,8 @@ class RepositoryIndexerTest {
verify(searchEngine.forIndices().forResource(heartOfGold)).batch(captor.capture());
captor.getValue().update(index);
verify(index.delete()).byRepository(heartOfGold);
verify(index.delete().by(Repository.class, heartOfGold)).execute();
}
@Test

View File

@@ -31,6 +31,7 @@ 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.Query;
import org.apache.lucene.search.TermQuery;
import org.apache.lucene.search.TopDocs;
import org.apache.lucene.store.ByteBuffersDirectory;
@@ -158,7 +159,24 @@ class LuceneIndexTest {
}
try (LuceneIndex<Storable> index = createIndex(Storable.class)) {
index.delete().byRepository("4212");
index.delete().by(Repository.class, "4212").execute();
}
assertHits("value", "content", 1);
}
@Test
void shouldDeleteByMultipleFields() throws IOException {
Id<Storable> base = ONE.and(Repository.class, "4211");
try (LuceneIndex<Storable> index = createIndex(Storable.class)) {
index.store(base.and(String.class, "1"), null, new Storable("content"));
index.store(base.and(String.class, "2"), null, new Storable("content"));
index.store(base.and(String.class, "2"), null, new Storable("content"));
}
try (LuceneIndex<Storable> index = createIndex(Storable.class)) {
index.delete().by(Repository.class, "4211").and(String.class, "2").execute();
}
assertHits("value", "content", 1);
@@ -233,11 +251,11 @@ class LuceneIndexTest {
}
@Test
void shouldThrowSearchEngineExceptionOnDeleteByRepository() throws IOException {
when(writer.deleteDocuments(any(Term.class))).thenThrow(new IOException("failed to delete"));
void shouldThrowSearchEngineExceptionOnDeleteBy() throws IOException {
when(writer.deleteDocuments(any(Query.class))).thenThrow(new IOException("failed to delete"));
Index.Deleter<Storable> deleter = index.delete();
assertThrows(SearchEngineException.class, () -> deleter.byRepository("42"));
Index.DeleteBy deleter = index.delete().by(Repository.class, "42");
assertThrows(SearchEngineException.class, deleter::execute);
}
@Test

View File

@@ -49,11 +49,13 @@ 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.repository.Repository;
import java.io.IOException;
import java.time.Instant;
import java.time.temporal.ChronoUnit;
import java.util.List;
import java.util.function.Consumer;
import java.util.stream.Collectors;
import static org.assertj.core.api.Assertions.assertThat;
@@ -277,16 +279,9 @@ class LuceneQueryBuilderTest {
writer.addDocument(repositoryDoc("Awesome content three", "fgh"));
}
QueryResult result;
try (DirectoryReader reader = DirectoryReader.open(directory)) {
SearchableTypeResolver resolver = new SearchableTypeResolver(Simple.class);
LuceneSearchableType searchableType = resolver.resolve(Simple.class);
when(opener.openForRead(searchableType, "default")).thenReturn(reader);
LuceneQueryBuilder<Simple> builder = new LuceneQueryBuilder<>(
opener, "default", searchableType, new StandardAnalyzer()
);
result = builder.repository("cde").execute("content:awesome");
}
QueryResult result = query(
Simple.class, "content:awesome", builder -> builder.filter(Repository.class, "cde")
);
assertThat(result.getTotalHits()).isOne();
@@ -297,6 +292,24 @@ class LuceneQueryBuilderTest {
});
}
@Test
void shouldFilterByIdParts() throws IOException {
try (IndexWriter writer = writer()) {
writer.addDocument(idPartsDoc("Awesome content one", "abc", "one"));
writer.addDocument(idPartsDoc("Awesome content two", "abc", "two"));
writer.addDocument(idPartsDoc("Awesome content three", "cde", "two"));
}
QueryResult result = query(
Simple.class, "content:awesome",
builder -> builder.filter(Repository.class, "abc").filter(String.class, "two")
);
List<Hit> hits = result.getHits();
assertThat(hits).hasSize(1)
.allSatisfy(hit -> assertValueField(hit, "content", "Awesome content two"));
}
@Test
void shouldReturnStringFields() throws IOException {
try (IndexWriter writer = writer()) {
@@ -549,6 +562,17 @@ class LuceneQueryBuilderTest {
}
private <T> QueryResult query(Class<?> type, String queryString, Integer start, Integer limit) throws IOException {
return query(type, queryString, builder -> {
if (start != null) {
builder.start(start);
}
if (limit != null) {
builder.limit(limit);
}
});
}
private <T> QueryResult query(Class<T> type, String queryString, Consumer<LuceneQueryBuilder<T>> consumer) throws IOException {
try (DirectoryReader reader = DirectoryReader.open(directory)) {
SearchableTypeResolver resolver = new SearchableTypeResolver(type);
LuceneSearchableType searchableType = resolver.resolve(type);
@@ -557,12 +581,7 @@ class LuceneQueryBuilderTest {
LuceneQueryBuilder<T> builder = new LuceneQueryBuilder<>(
opener, "default", searchableType, new StandardAnalyzer()
);
if (start != null) {
builder.start(start);
}
if (limit != null) {
builder.limit(limit);
}
consumer.accept(builder);
return builder.execute(queryString);
}
}
@@ -576,14 +595,12 @@ class LuceneQueryBuilderTest {
private Document simpleDoc(String content) {
Document document = new Document();
document.add(new TextField("content", content, Field.Store.YES));
// document.add(new StringField(FieldNames.TYPE, "simple", Field.Store.YES));
return document;
}
private Document permissionDoc(String content, String permission) {
Document document = new Document();
document.add(new TextField("content", content, Field.Store.YES));
// document.add(new StringField(FieldNames.TYPE, "simple", Field.Store.YES));
document.add(new StringField(FieldNames.PERMISSION, permission, Field.Store.YES));
return document;
}
@@ -591,11 +608,18 @@ class LuceneQueryBuilderTest {
private Document repositoryDoc(String content, String repository) {
Document document = new Document();
document.add(new TextField("content", content, Field.Store.YES));
// document.add(new StringField(FieldNames.TYPE, "simple", Field.Store.YES));
document.add(new StringField(FieldNames.REPOSITORY, repository, Field.Store.YES));
return document;
}
private Document idPartsDoc(String content, String repository, String other) {
Document document = new Document();
document.add(new TextField("content", content, Field.Store.YES));
document.add(new StringField(FieldNames.REPOSITORY, repository, Field.Store.YES));
document.add(new StringField("_string", other, Field.Store.YES));
return document;
}
private Document inetOrgPersonDoc(String firstName, String lastName, String displayName, String carLicense) {
Document document = new Document();
document.add(new TextField("firstName", firstName, Field.Store.YES));
@@ -603,14 +627,12 @@ class LuceneQueryBuilderTest {
document.add(new TextField("displayName", displayName, Field.Store.YES));
document.add(new TextField("carLicense", carLicense, Field.Store.YES));
document.add(new StringField(FieldNames.ID, lastName, Field.Store.YES));
// document.add(new StringField(FieldNames.TYPE, "inetOrgPerson", Field.Store.YES));
return document;
}
private Document personDoc(String lastName) {
Document document = new Document();
document.add(new TextField("lastName", lastName, Field.Store.YES));
// document.add(new StringField(FieldNames.TYPE, "person", Field.Store.YES));
return document;
}
@@ -623,14 +645,6 @@ class LuceneQueryBuilderTest {
document.add(new StringField("boolValue", String.valueOf(boolValue), Field.Store.YES));
document.add(new LongPoint("instantValue", instantValue.toEpochMilli()));
document.add(new StoredField("instantValue", instantValue.toEpochMilli()));
// document.add(new StringField(FieldNames.TYPE, "types", Field.Store.YES));
return document;
}
private Document denyDoc(String value) {
Document document = new Document();
document.add(new TextField("value", value, Field.Store.YES));
// document.add(new StringField(FieldNames.TYPE, "deny", Field.Store.YES));
return document;
}
@@ -679,11 +693,4 @@ class LuceneQueryBuilderTest {
private String content;
}
@Getter
@IndexedType(permission = "deny:4711")
static class Deny {
@Indexed(defaultQuery = true)
private String value;
}
}