opendev/gerrit
Code Issues Proposed changes

Remove FieldDef.FillArgs

Browse Source 
Supporting this in FieldDef required a lot of unnecessary boilerplate
that was (prior to earlier cleanups in this series) only used by the
change index, no other indexes. This also breaks a dependency edge
between the index package and the rest of gerrit-server, making it more
feasible to factor out a new build rule in the future.

Change-Id: I0785c7e2c93df7951818c3ebaa23e1e988686064
This commit is contained in:
Dave Borowitz
2017-08-09 09:43:25 -04:00
parent 6fe38f4323
commit 65dc8b6dd8
17 changed files with 29 additions and 79 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java
View File

@@ -25,7 +25,6 @@ import com.google.common.collect.Iterables;
import com.google.common.collect.Streams;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.SitePaths;
import com.google.gerrit.server.index.FieldDef.FillArgs;
import com.google.gerrit.server.index.Index;
import com.google.gerrit.server.index.IndexUtils;
import com.google.gerrit.server.index.Schema;
@@ -60,7 +59,6 @@ abstract class AbstractElasticIndex<K, V> implements Index<K, V> {
}
private final Schema<V> schema;
private final FillArgs fillArgs;
private final SitePaths sitePaths;
protected final String indexName;
@@ -70,12 +68,10 @@ abstract class AbstractElasticIndex<K, V> implements Index<K, V> {
AbstractElasticIndex(
@GerritServerConfig Config cfg,
FillArgs fillArgs,
SitePaths sitePaths,
Schema<V> schema,
JestClientBuilder clientBuilder,
String indexName) {
this.fillArgs = fillArgs;
this.sitePaths = sitePaths;
this.schema = schema;
this.gson = new GsonBuilder().setFieldNamingPolicy(LOWER_CASE_WITH_UNDERSCORES).create();
@@ -160,7 +156,7 @@ abstract class AbstractElasticIndex<K, V> implements Index<K, V> {
private String toDoc(V v) throws IOException {
XContentBuilder builder = jsonBuilder().startObject();
for (Values<V> values : schema.buildFields(v, fillArgs)) {
for (Values<V> values : schema.buildFields(v)) {
String name = values.getField().getName();
if (values.getField().isRepeatable()) {
builder.field(

3
gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticAccountIndex.java
View File

@@ -83,8 +83,7 @@ public class ElasticAccountIndex extends AbstractElasticIndex<Account.Id, Accoun
Provider<AccountCache> accountCache,
JestClientBuilder clientBuilder,
@Assisted Schema<AccountState> schema) {
// No parts of FillArgs are currently required, just use null.
super(cfg, null, sitePaths, schema, clientBuilder, ACCOUNTS_PREFIX);
super(cfg, sitePaths, schema, clientBuilder, ACCOUNTS_PREFIX);
this.accountCache = accountCache;
this.mapping = new AccountMapping(schema);
}

4
gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java
View File

@@ -39,7 +39,6 @@ import com.google.gerrit.server.ReviewerByEmailSet;
import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.SitePaths;
import com.google.gerrit.server.index.FieldDef.FillArgs;
import com.google.gerrit.server.index.IndexUtils;
import com.google.gerrit.server.index.QueryOptions;
import com.google.gerrit.server.index.Schema;
@@ -106,11 +105,10 @@ class ElasticChangeIndex extends AbstractElasticIndex<Change.Id, ChangeData>
@GerritServerConfig Config cfg,
Provider<ReviewDb> db,
ChangeData.Factory changeDataFactory,
FillArgs fillArgs,
SitePaths sitePaths,
JestClientBuilder clientBuilder,
@Assisted Schema<ChangeData> schema) {
super(cfg, fillArgs, sitePaths, schema, clientBuilder, CHANGES_PREFIX);
super(cfg, sitePaths, schema, clientBuilder, CHANGES_PREFIX);
this.db = db;
this.changeDataFactory = changeDataFactory;
mapping = new ChangeMapping(schema);

3
gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/ElasticGroupIndex.java
View File

@@ -80,8 +80,7 @@ public class ElasticGroupIndex extends AbstractElasticIndex<AccountGroup.UUID, A
Provider<GroupCache> groupCache,
JestClientBuilder clientBuilder,
@Assisted Schema<AccountGroup> schema) {
// No parts of FillArgs are currently required, just use null.
super(cfg, null, sitePaths, schema, clientBuilder, GROUPS_PREFIX);
super(cfg, sitePaths, schema, clientBuilder, GROUPS_PREFIX);
this.groupCache = groupCache;
this.mapping = new GroupMapping(schema);
}

5
gerrit-lucene/src/main/java/com/google/gerrit/lucene/AbstractLuceneIndex.java
View File

@@ -27,7 +27,6 @@ import com.google.common.util.concurrent.MoreExecutors;
import com.google.common.util.concurrent.ThreadFactoryBuilder;
import com.google.gerrit.server.config.SitePaths;
import com.google.gerrit.server.index.FieldDef;
import com.google.gerrit.server.index.FieldDef.FillArgs;
import com.google.gerrit.server.index.FieldType;
import com.google.gerrit.server.index.Index;
import com.google.gerrit.server.index.IndexUtils;
@@ -285,9 +284,9 @@ public abstract class AbstractLuceneIndex<K, V> implements Index<K, V> {
searcherManager.release(searcher);
}
Document toDocument(V obj, FillArgs fillArgs) {
Document toDocument(V obj) {
Document result = new Document();
for (Values<V> vs : schema.buildFields(obj, fillArgs)) {
for (Values<V> vs : schema.buildFields(obj)) {
if (vs.getValues() != null) {
add(result, vs);
}

3
gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneAccountIndex.java
View File

@@ -109,8 +109,7 @@ public class LuceneAccountIndex extends AbstractLuceneIndex<Account.Id, AccountS
@Override
public void replace(AccountState as) throws IOException {
try {
// No parts of FillArgs are currently required, just use null.
replace(idTerm(as), toDocument(as, null)).get();
replace(idTerm(as), toDocument(as)).get();
} catch (ExecutionException | InterruptedException e) {
throw new IOException(e);
}

6
gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java
View File

@@ -44,7 +44,6 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.StarredChangesUtil;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.SitePaths;
import com.google.gerrit.server.index.FieldDef.FillArgs;
import com.google.gerrit.server.index.IndexExecutor;
import com.google.gerrit.server.index.IndexUtils;
import com.google.gerrit.server.index.QueryOptions;
@@ -142,7 +141,6 @@ public class LuceneChangeIndex implements ChangeIndex {
return QueryBuilder.intTerm(LEGACY_ID.getName(), id.get());
}
private final FillArgs fillArgs;
private final ListeningExecutorService executor;
private final Provider<ReviewDb> db;
private final ChangeData.Factory changeDataFactory;
@@ -158,10 +156,8 @@ public class LuceneChangeIndex implements ChangeIndex {
@IndexExecutor(INTERACTIVE) ListeningExecutorService executor,
Provider<ReviewDb> db,
ChangeData.Factory changeDataFactory,
FillArgs fillArgs,
@Assisted Schema<ChangeData> schema)
throws IOException {
this.fillArgs = fillArgs;
this.executor = executor;
this.db = db;
this.changeDataFactory = changeDataFactory;
@@ -215,7 +211,7 @@ public class LuceneChangeIndex implements ChangeIndex {
Term id = LuceneChangeIndex.idTerm(cd);
// toDocument is essentially static and doesn't depend on the specific
// sub-index, so just pick one.
Document doc = openIndex.toDocument(cd, fillArgs);
Document doc = openIndex.toDocument(cd);
try {
if (cd.change().getStatus().isOpen()) {
Futures.allAsList(closedIndex.delete(id), openIndex.replace(id, doc)).get();

3
gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneGroupIndex.java
View File

@@ -108,8 +108,7 @@ public class LuceneGroupIndex extends AbstractLuceneIndex<AccountGroup.UUID, Acc
@Override
public void replace(AccountGroup group) throws IOException {
try {
// No parts of FillArgs are currently required, just use null.
replace(idTerm(group), toDocument(group, null)).get();
replace(idTerm(group), toDocument(group)).get();
} catch (ExecutionException | InterruptedException e) {
throw new IOException(e);
}

29
gerrit-server/src/main/java/com/google/gerrit/server/index/FieldDef.java
View File

@@ -63,14 +63,6 @@ public final class FieldDef<I, T> {
T get(I input) throws OrmException, IOException;
}
@FunctionalInterface
public interface GetterWithArgs<I, T> {
T get(I input, FillArgs args) throws OrmException, IOException;
}
/** Arguments needed to fill in missing data in the input object. */
public static class FillArgs {}
public static class Builder<T> {
private final FieldType<T> type;
private final String name;
@@ -87,18 +79,10 @@ public final class FieldDef<I, T> {
}
public <I> FieldDef<I, T> build(Getter<I, T> getter) {
return build((in, a) -> getter.get(in));
}
public <I> FieldDef<I, T> build(GetterWithArgs<I, T> getter) {
return new FieldDef<>(name, type, stored, false, getter);
}
public <I> FieldDef<I, Iterable<T>> buildRepeatable(Getter<I, Iterable<T>> getter) {
return buildRepeatable((in, a) -> getter.get(in));
}
public <I> FieldDef<I, Iterable<T>> buildRepeatable(GetterWithArgs<I, Iterable<T>> getter) {
return new FieldDef<>(name, type, stored, true, getter);
}
}
@@ -107,14 +91,10 @@ public final class FieldDef<I, T> {
private final FieldType<?> type;
private final boolean stored;
private final boolean repeatable;
private final GetterWithArgs<I, T> getter;
private final Getter<I, T> getter;
private FieldDef(
String name,
FieldType<?> type,
boolean stored,
boolean repeatable,
GetterWithArgs<I, T> getter) {
String name, FieldType<?> type, boolean stored, boolean repeatable, Getter<I, T> getter) {
checkArgument(
!(repeatable && type == FieldType.INTEGER_RANGE),
"Range queries against repeated fields are unsupported");
@@ -150,13 +130,12 @@ public final class FieldDef<I, T> {
* Get the field contents from the input object.
*
* @param input input object.
* @param args arbitrary arguments needed to fill in indexable fields of the input object.
* @return the field value(s) to index.
* @throws OrmException
*/
public T get(I input, FillArgs args) throws OrmException {
public T get(I input) throws OrmException {
try {
return getter.get(input, args);
return getter.get(input);
} catch (IOException e) {
throw new OrmException(e);
}

6
gerrit-server/src/main/java/com/google/gerrit/server/index/Schema.java
View File

@@ -22,7 +22,6 @@ import com.google.common.base.Predicates;
import com.google.common.collect.FluentIterable;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.gerrit.server.index.FieldDef.FillArgs;
import com.google.gwtorm.server.OrmException;
import java.util.ArrayList;
import java.util.Arrays;
@@ -173,10 +172,9 @@ public class Schema<T> {
* <p>Null values are omitted, as are fields which cause errors, which are logged.
*
* @param obj input object.
* @param fillArgs arguments for filling fields.
* @return all non-null field values from the object.
*/
public final Iterable<Values<T>> buildFields(T obj, FillArgs fillArgs) {
public final Iterable<Values<T>> buildFields(T obj) {
return FluentIterable.from(fields.values())
.transform(
new Function<FieldDef<T, ?>, Values<T>>() {
@@ -184,7 +182,7 @@ public class Schema<T> {
public Values<T> apply(FieldDef<T, ?> f) {
Object v;
try {
v = f.get(obj, fillArgs);
v = f.get(obj);
} catch (OrmException e) {
log.error(String.format("error getting field %s of %s", f.getName(), obj), e);
return null;

2
gerrit-server/src/main/java/com/google/gerrit/server/query/change/AddedPredicate.java
View File

@@ -25,6 +25,6 @@ public class AddedPredicate extends IntegerRangeChangePredicate {
@Override
protected Integer getValueInt(ChangeData changeData) throws OrmException {
return ChangeField.ADDED.get(changeData, null);
return ChangeField.ADDED.get(changeData);
}
}

8
gerrit-server/src/main/java/com/google/gerrit/server/query/change/BooleanPredicate.java
View File

@@ -15,20 +15,16 @@
package com.google.gerrit.server.query.change;
import com.google.gerrit.server.index.FieldDef;
import com.google.gerrit.server.index.FieldDef.FillArgs;
import com.google.gwtorm.server.OrmException;
public class BooleanPredicate extends ChangeIndexPredicate {
protected final FillArgs args;
public BooleanPredicate(FieldDef<ChangeData, String> field, FillArgs args) {
public BooleanPredicate(FieldDef<ChangeData, String> field) {
super(field, "1");
this.args = args;
}
@Override
public boolean match(ChangeData object) throws OrmException {
return getValue().equals(getField().get(object, args));
return getValue().equals(getField().get(object));
}
@Override

20
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
View File

@@ -52,7 +52,6 @@ import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.strategy.SubmitDryRun;
import com.google.gerrit.server.group.ListMembers;
import com.google.gerrit.server.index.FieldDef;
import com.google.gerrit.server.index.IndexConfig;
import com.google.gerrit.server.index.Schema;
import com.google.gerrit.server.index.SchemaUtil;
@@ -205,7 +204,6 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
final ConflictsCache conflictsCache;
final DynamicMap<ChangeHasOperandFactory> hasOperands;
final DynamicMap<ChangeOperatorFactory> opFactories;
final FieldDef.FillArgs fillArgs;
final GitRepositoryManager repoManager;
final GroupBackend groupBackend;
final IdentifiedUser.GenericFactory userFactory;
@@ -237,7 +235,6 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
ChangeControl.GenericFactory changeControlGenericFactory,
ChangeNotes.Factory notesFactory,
ChangeData.Factory changeDataFactory,
FieldDef.FillArgs fillArgs,
CommentsUtil commentsUtil,
AccountResolver accountResolver,
GroupBackend groupBackend,
@@ -268,7 +265,6 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
changeControlGenericFactory,
notesFactory,
changeDataFactory,
fillArgs,
commentsUtil,
accountResolver,
groupBackend,
@@ -301,7 +297,6 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
ChangeControl.GenericFactory changeControlGenericFactory,
ChangeNotes.Factory notesFactory,
ChangeData.Factory changeDataFactory,
FieldDef.FillArgs fillArgs,
CommentsUtil commentsUtil,
AccountResolver accountResolver,
GroupBackend groupBackend,
@@ -330,7 +325,6 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
this.notesFactory = notesFactory;
this.changeControlGenericFactory = changeControlGenericFactory;
this.changeDataFactory = changeDataFactory;
this.fillArgs = fillArgs;
this.commentsUtil = commentsUtil;
this.accountResolver = accountResolver;
this.groupBackend = groupBackend;
@@ -365,7 +359,6 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
changeControlGenericFactory,
notesFactory,
changeDataFactory,
fillArgs,
commentsUtil,
accountResolver,
groupBackend,
@@ -573,7 +566,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
if ("reviewer".equalsIgnoreCase(value)) {
if (args.getSchema().hasField(ChangeField.WIP)) {
return Predicate.and(
Predicate.not(new BooleanPredicate(ChangeField.WIP, args.fillArgs)),
Predicate.not(new BooleanPredicate(ChangeField.WIP)),
ReviewerPredicate.reviewer(args, self()));
}
return ReviewerPredicate.reviewer(args, self());
@@ -584,12 +577,12 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
}
if ("mergeable".equalsIgnoreCase(value)) {
return new BooleanPredicate(ChangeField.MERGEABLE, args.fillArgs);
return new BooleanPredicate(ChangeField.MERGEABLE);
}
if ("private".equalsIgnoreCase(value)) {
if (args.getSchema().hasField(ChangeField.PRIVATE)) {
return new BooleanPredicate(ChangeField.PRIVATE, args.fillArgs);
return new BooleanPredicate(ChangeField.PRIVATE);
}
throw new QueryParseException(
"'is:private' operator is not supported by change index version");
@@ -613,7 +606,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
if ("started".equalsIgnoreCase(value)) {
if (args.getSchema().hasField(ChangeField.STARTED)) {
return new BooleanPredicate(ChangeField.STARTED, args.fillArgs);
return new BooleanPredicate(ChangeField.STARTED);
}
throw new QueryParseException(
"'is:started' operator is not supported by change index version");
@@ -621,7 +614,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
if ("wip".equalsIgnoreCase(value)) {
if (args.getSchema().hasField(ChangeField.WIP)) {
return new BooleanPredicate(ChangeField.WIP, args.fillArgs);
return new BooleanPredicate(ChangeField.WIP);
}
throw new QueryParseException("'is:wip' operator is not supported by change index version");
}
@@ -1023,8 +1016,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
return Predicate.any();
}
if (args.getSchema().hasField(ChangeField.WIP)) {
return Predicate.and(
Predicate.not(new BooleanPredicate(ChangeField.WIP, args.fillArgs)), byState);
return Predicate.and(Predicate.not(new BooleanPredicate(ChangeField.WIP)), byState);
}
return byState;
}

2
gerrit-server/src/main/java/com/google/gerrit/server/query/change/DeletedPredicate.java
View File

@@ -25,6 +25,6 @@ public class DeletedPredicate extends IntegerRangeChangePredicate {
@Override
protected Integer getValueInt(ChangeData changeData) throws OrmException {
return ChangeField.DELETED.get(changeData, null);
return ChangeField.DELETED.get(changeData);
}
}

2
gerrit-server/src/main/java/com/google/gerrit/server/query/change/DeltaPredicate.java
View File

@@ -25,6 +25,6 @@ public class DeltaPredicate extends IntegerRangeChangePredicate {
@Override
protected Integer getValueInt(ChangeData changeData) throws OrmException {
return ChangeField.DELTA.get(changeData, null);
return ChangeField.DELTA.get(changeData);
}
}

2
gerrit-server/src/main/java/com/google/gerrit/server/query/change/IsUnresolvedPredicate.java
View File

@@ -29,6 +29,6 @@ public class IsUnresolvedPredicate extends IntegerRangeChangePredicate {
@Override
protected Integer getValueInt(ChangeData changeData) throws OrmException {
return ChangeField.UNRESOLVED_COMMENT_COUNT.get(changeData, null);
return ChangeField.UNRESOLVED_COMMENT_COUNT.get(changeData);
}
}

4
gerrit-server/src/test/java/com/google/gerrit/server/index/change/FakeQueryBuilder.java
View File

@@ -27,8 +27,8 @@ public class FakeQueryBuilder extends ChangeQueryBuilder {
new FakeQueryBuilder.Definition<>(FakeQueryBuilder.class),
new ChangeQueryBuilder.Arguments(
null, null, null, null, null, null, null, null, null, null, null, null, null, null,
null, null, null, null, null, null, null, indexes, null, null, null, null, null, null,
null, null));
null, null, null, null, null, null, indexes, null, null, null, null, null, null, null,
null));
}
@Operator