From 8be51b8a57550deebaad72b8f240dc1462d5cb8f Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 13 Apr 2016 14:33:29 +0200 Subject: [PATCH] Store users that starred a change in change index Within notedb each change that is starred by a user is tracked by a refs/starred-changes/XX/YYYY/ZZZZ ref in the All-Users meta repository where XX/YYYY is the sharded change ID and ZZZZ is the account ID. With this storage format finding all users that have starred a change is cheap because we just need to list all refs that have refs/starred-changes/XX/YYYY/ as prefix and looking up refs with a prefix that ends on '/' is optimized in jgit. However to find all changes that were starred by a user we must scan the complete refs/starred-changes/ namespace and check which of the refs end with the account ID. This results in bad performance when there are many starred changes. Having the users that starred a change stored in the change index makes this lookup cheap. Looking up all changes that were starred by a user is needed for supporting the "is:starred" query operator and for populating the "starred" field in ChangeInfo. Change-Id: Iecc9ca8ef133b0d0f86f1471b9ed31be78a43f6c Signed-off-by: Edwin Kempin --- .../gerrit/lucene/LuceneChangeIndex.java | 14 ++++ .../google/gerrit/server/IdentifiedUser.java | 4 +- .../gerrit/server/StarredChangesUtil.java | 74 +++++++++++++------ .../gerrit/server/account/StarredChanges.java | 11 ++- .../server/api/accounts/AccountApiImpl.java | 4 +- .../server/change/DeleteDraftChangeOp.java | 7 +- .../server/change/DeleteDraftPatchSet.java | 8 +- .../gerrit/server/index/SchemaUtil.java | 10 +++ .../server/index/change/ChangeField.java | 17 +++++ .../server/index/change/ChangeIndexer.java | 14 +++- .../index/change/ChangeSchemaDefinitions.java | 3 + .../gerrit/server/mail/ChangeEmail.java | 5 +- .../server/query/change/ChangeData.java | 26 ++++++- .../query/change/ChangeQueryBuilder.java | 26 ++++--- .../query/change/InternalChangeQuery.java | 5 ++ .../change/IsStarredByLegacyPredicate.java | 71 ++++++++++++++++++ .../query/change/IsStarredByPredicate.java | 52 ++++--------- .../change/AbstractQueryChangesTest.java | 17 +++++ 18 files changed, 280 insertions(+), 88 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/query/change/IsStarredByLegacyPredicate.java diff --git a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java index 8f7e89907f..840f8b4690 100644 --- a/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java +++ b/gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java @@ -114,6 +114,7 @@ public class LuceneChangeIndex implements ChangeIndex { private static final String PATCH_SET_FIELD = ChangeField.PATCH_SET.getName(); private static final String REVIEWEDBY_FIELD = ChangeField.REVIEWEDBY.getName(); + private static final String STARREDBY_FIELD = ChangeField.STARREDBY.getName(); static Term idTerm(ChangeData cd) { return QueryBuilder.intTerm(LEGACY_ID.getName(), cd.getId().get()); @@ -413,6 +414,9 @@ public class LuceneChangeIndex implements ChangeIndex { if (fields.contains(REVIEWEDBY_FIELD)) { decodeReviewedBy(doc, cd); } + if (fields.contains(STARREDBY_FIELD)) { + decodeStarredBy(doc, cd); + } return cd; } @@ -466,6 +470,16 @@ public class LuceneChangeIndex implements ChangeIndex { } } + private void decodeStarredBy(Document doc, ChangeData cd) { + IndexableField[] starredBy = doc.getFields(STARREDBY_FIELD); + Set accounts = + Sets.newHashSetWithExpectedSize(starredBy.length); + for (IndexableField r : starredBy) { + accounts.add(new Account.Id(r.numericValue().intValue())); + } + cd.setStarredBy(accounts); + } + private static List decodeProtos(Document doc, String fieldName, ProtobufCodec codec) { BytesRef[] bytesRefs = doc.getBinaryValues(fieldName); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java b/gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java index 50b20f0a86..fb75091bac 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java @@ -338,7 +338,7 @@ public class IdentifiedUser extends CurrentUser { FluentIterable.from( starredQuery != null ? starredQuery - : starredChangesUtil.query(accountId)) + : starredChangesUtil.queryFromIndex(accountId)) .toSet(); } finally { starredQuery = null; @@ -356,7 +356,7 @@ public class IdentifiedUser extends CurrentUser { public void asyncStarredChanges() { if (starredChanges == null && starredChangesUtil != null) { - starredQuery = starredChangesUtil.query(accountId); + starredQuery = starredChangesUtil.queryFromIndex(accountId); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/StarredChangesUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/StarredChangesUtil.java index 64134146b8..49d36896bb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/StarredChangesUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/StarredChangesUtil.java @@ -15,18 +15,24 @@ package com.google.gerrit.server; import com.google.common.base.Function; -import com.google.common.base.Predicate; import com.google.common.collect.FluentIterable; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterators; import com.google.common.collect.Lists; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.client.StarredChange; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.index.change.ChangeField; +import com.google.gerrit.server.index.change.ChangeIndexer; import com.google.gerrit.server.notedb.NotesMigration; +import com.google.gerrit.server.project.NoSuchChangeException; +import com.google.gerrit.server.query.change.ChangeData; +import com.google.gerrit.server.query.change.InternalChangeQuery; import com.google.gwtorm.server.ListResultSet; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.ResultSet; @@ -65,26 +71,33 @@ public class StarredChangesUtil { private final NotesMigration migration; private final Provider dbProvider; private final PersonIdent serverIdent; + private final ChangeIndexer indexer; + private final Provider queryProvider; @Inject StarredChangesUtil(GitRepositoryManager repoManager, AllUsersName allUsers, NotesMigration migration, Provider dbProvider, - @GerritPersonIdent PersonIdent serverIdent) { + @GerritPersonIdent PersonIdent serverIdent, + ChangeIndexer indexer, + Provider queryProvider) { this.repoManager = repoManager; this.allUsers = allUsers; this.migration = migration; this.dbProvider = dbProvider; this.serverIdent = serverIdent; + this.indexer = indexer; + this.queryProvider = queryProvider; } - public void star(Account.Id accountId, Change.Id changeId) - throws OrmException { + public void star(Account.Id accountId, Project.NameKey project, + Change.Id changeId) throws OrmException, IOException { dbProvider.get().starredChanges() .insert(Collections.singleton(new StarredChange( new StarredChange.Key(accountId, changeId)))); if (!migration.writeAccounts()) { + indexer.index(dbProvider.get(), project, changeId); return; } try (Repository repo = repoManager.openRepository(allUsers); @@ -98,6 +111,7 @@ public class StarredChangesUtil { RefUpdate.Result result = u.update(rw); switch (result) { case NEW: + indexer.index(dbProvider.get(), project, changeId); return; case FAST_FORWARD: case FORCED: @@ -128,12 +142,13 @@ public class StarredChangesUtil { } } - public void unstar(Account.Id accountId, Change.Id changeId) - throws OrmException { + public void unstar(Account.Id accountId, Project.NameKey project, + Change.Id changeId) throws OrmException, IOException { dbProvider.get().starredChanges() .delete(Collections.singleton(new StarredChange( new StarredChange.Key(accountId, changeId)))); if (!migration.writeAccounts()) { + indexer.index(dbProvider.get(), project, changeId); return; } try (Repository repo = repoManager.openRepository(allUsers); @@ -146,6 +161,7 @@ public class StarredChangesUtil { RefUpdate.Result result = u.delete(); switch (result) { case FORCED: + indexer.index(dbProvider.get(), project, changeId); return; case FAST_FORWARD: case IO_FAILURE: @@ -168,10 +184,12 @@ public class StarredChangesUtil { } } - public void unstarAll(Change.Id changeId) throws OrmException { + public void unstarAll(Project.NameKey project, Change.Id changeId) + throws OrmException, IOException, NoSuchChangeException { dbProvider.get().starredChanges().delete( dbProvider.get().starredChanges().byChange(changeId)); if (!migration.writeAccounts()) { + indexer.index(dbProvider.get(), project, changeId); return; } try (Repository repo = repoManager.openRepository(allUsers); @@ -180,7 +198,7 @@ public class StarredChangesUtil { batchUpdate.setAllowNonFastForwards(true); batchUpdate.setRefLogIdent(serverIdent); batchUpdate.setRefLogMessage("Unstar change " + changeId.get(), true); - for (Account.Id accountId : byChange(changeId)) { + for (Account.Id accountId : byChangeFromIndex(changeId)) { String refName = RefNames.refsStarredChanges(changeId, accountId); Ref ref = repo.getRefDatabase().getRef(refName); batchUpdate.addCommand(new ReceiveCommand(ref.getObjectId(), @@ -194,13 +212,14 @@ public class StarredChangesUtil { changeId.get(), command.getRefName(), command.getResult())); } } + indexer.index(dbProvider.get(), project, changeId); } catch (IOException e) { throw new OrmException( String.format("Unstar change %d failed", changeId.get()), e); } } - public Iterable byChange(Change.Id changeId) + public Set byChange(Change.Id changeId) throws OrmException { if (!migration.readAccounts()) { return FluentIterable @@ -210,7 +229,7 @@ public class StarredChangesUtil { public Account.Id apply(StarredChange in) { return in.getAccountId(); } - }); + }).toSet(); } return FluentIterable .from(getRefNames(RefNames.refsStarredChangesPrefix(changeId))) @@ -219,29 +238,40 @@ public class StarredChangesUtil { public Account.Id apply(String refPart) { return Account.Id.parse(refPart); } - }); + }).toSet(); + } + + public Set byChangeFromIndex(Change.Id changeId) + throws OrmException, NoSuchChangeException { + Set fields = ImmutableSet.of( + ChangeField.ID.getName(), + ChangeField.STARREDBY.getName()); + List changeData = queryProvider.get().setRequestedFields(fields) + .byLegacyChangeId(changeId); + if (changeData.size() != 1) { + throw new NoSuchChangeException(changeId); + } + return changeData.get(0).starredBy(); } @Deprecated - public ResultSet query(final Account.Id accountId) { + public ResultSet queryFromIndex(final Account.Id accountId) { try { if (!migration.readAccounts()) { return new ChangeIdResultSet( dbProvider.get().starredChanges().byAccount(accountId)); } + Set fields = ImmutableSet.of( + ChangeField.ID.getName()); + List changeData = + queryProvider.get().setRequestedFields(fields).byIsStarred(accountId); return new ListResultSet<>(FluentIterable - .from(getRefNames(RefNames.REFS_STARRED_CHANGES)) - .filter(new Predicate() { + .from(changeData) + .transform(new Function() { @Override - public boolean apply(String refPart) { - return refPart.endsWith("/" + accountId.get()); - } - }) - .transform(new Function() { - @Override - public Change.Id apply(String changeId) { - return Change.Id.parse(changeId); + public Change.Id apply(ChangeData cd) { + return cd.getId(); } }).toList()); } catch (OrmException | RuntimeException e) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/StarredChanges.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/StarredChanges.java index f8a23d8b17..1ed29a7015 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/StarredChanges.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/StarredChanges.java @@ -42,6 +42,8 @@ import com.google.inject.Singleton; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.io.IOException; + @Singleton public class StarredChanges implements ChildCollection, @@ -130,12 +132,13 @@ public class StarredChanges implements @Override public Response apply(AccountResource rsrc, EmptyInput in) - throws AuthException, OrmException { + throws AuthException, OrmException, IOException { if (self.get() != rsrc.getUser()) { throw new AuthException("not allowed to add starred change"); } try { - starredChangesUtil.star(self.get().getAccountId(), change.getId()); + starredChangesUtil.star(self.get().getAccountId(), change.getProject(), + change.getId()); } catch (OrmDuplicateKeyException e) { return Response.none(); } @@ -177,12 +180,12 @@ public class StarredChanges implements @Override public Response apply(AccountResource.StarredChange rsrc, - EmptyInput in) throws AuthException, OrmException { + EmptyInput in) throws AuthException, OrmException, IOException { if (self.get() != rsrc.getUser()) { throw new AuthException("not allowed remove starred change"); } starredChangesUtil.unstar(self.get().getAccountId(), - rsrc.getChange().getId()); + rsrc.getChange().getProject(), rsrc.getChange().getId()); return Response.none(); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java index 55d077f72b..b3126d32f6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/accounts/AccountApiImpl.java @@ -193,7 +193,7 @@ public class AccountApiImpl implements AccountApi { IdString.fromUrl(id)); starredChangesCreate.setChange(rsrc); starredChangesCreate.apply(account, new StarredChanges.EmptyInput()); - } catch (OrmException e) { + } catch (OrmException | IOException e) { throw new RestApiException("Cannot star change", e); } } @@ -207,7 +207,7 @@ public class AccountApiImpl implements AccountApi { new AccountResource.StarredChange(account.getUser(), rsrc); starredChangesDelete.apply(starredChange, new StarredChanges.EmptyInput()); - } catch (OrmException e) { + } catch (OrmException | IOException e) { throw new RestApiException("Cannot unstar change", e); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftChangeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftChangeOp.java index d90fc738a4..6ac17b2a19 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftChangeOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftChangeOp.java @@ -31,6 +31,7 @@ import com.google.gerrit.server.git.BatchUpdate; import com.google.gerrit.server.git.BatchUpdate.ChangeContext; import com.google.gerrit.server.git.BatchUpdate.RepoContext; import com.google.gerrit.server.git.BatchUpdateReviewDb; +import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.schema.DisabledChangesReviewDbWrapper; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -78,8 +79,8 @@ class DeleteDraftChangeOp extends BatchUpdate.Op { } @Override - public boolean updateChange(ChangeContext ctx) - throws RestApiException, OrmException { + public boolean updateChange(ChangeContext ctx) throws RestApiException, + OrmException, IOException, NoSuchChangeException { checkState(ctx.getOrder() == BatchUpdate.Order.DB_BEFORE_REPO, "must use DeleteDraftChangeOp with DB_BEFORE_REPO"); checkState(id == null, "cannot reuse DeleteDraftChangeOp"); @@ -117,7 +118,7 @@ class DeleteDraftChangeOp extends BatchUpdate.Op { // Non-atomic operation on Accounts table; not much we can do to make it // atomic. - starredChangesUtil.unstarAll(id); + starredChangesUtil.unstarAll(change.getProject(), id); ctx.deleteChange(); return true; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java index 59d429097f..2edff4bc3b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java @@ -35,6 +35,7 @@ import com.google.gerrit.server.git.BatchUpdate.RepoContext; import com.google.gerrit.server.git.UpdateException; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; +import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; @@ -99,8 +100,8 @@ public class DeleteDraftPatchSet implements RestModifyView(ImmutableList.copyOf(fields)); } + @SafeVarargs + public static Schema schema(Schema schema, + FieldDef... moreFields) { + return new Schema<>( + new ImmutableList.Builder>() + .addAll(schema.getFields().values()) + .addAll(ImmutableList.copyOf(moreFields)) + .build()); + } + @SafeVarargs public static Schema schema(FieldDef... fields) { return schema(ImmutableList.copyOf(fields)); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java index fdeb654766..67694ac63c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeField.java @@ -576,6 +576,23 @@ public class ChangeField { } }; + /** Users who have starred this change. */ + public static final FieldDef> STARREDBY = + new FieldDef.Repeatable( + ChangeQueryBuilder.FIELD_STARREDBY, FieldType.INTEGER, true) { + @Override + public Iterable get(ChangeData input, FillArgs args) + throws OrmException { + return Iterables.transform(input.starredBy(), + new Function() { + @Override + public Integer apply(Account.Id accountId) { + return accountId.get(); + } + }); + } + }; + /** Opaque group identifiers for this change's patch sets. */ public static final FieldDef> GROUP = new FieldDef.Repeatable( diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java index 48df562ac8..0ca992227d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java @@ -186,13 +186,25 @@ public class ChangeIndexer { /** * Synchronously index a change. * - * @param change change to index. * @param db review database. + * @param change change to index. */ public void index(ReviewDb db, Change change) throws IOException { index(changeDataFactory.create(db, change)); } + /** + * Synchronously index a change. + * + * @param db review database. + * @param project the project to which the change belongs. + * @param changeId ID of the change to index. + */ + public void index(ReviewDb db, Project.NameKey project, Change.Id changeId) + throws IOException { + index(changeDataFactory.create(db, project, changeId)); + } + /** * Start deleting a change. * diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java index b39a0b7a1f..9363f6271e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java @@ -96,8 +96,11 @@ public class ChangeSchemaDefinitions extends SchemaDefinitions { ChangeField.COMMITTER, ChangeField.DRAFTBY); + @Deprecated static final Schema V27 = schema(V26.getFields().values()); + static final Schema V28 = schema(V27, ChangeField.STARREDBY); + public static final ChangeSchemaDefinitions INSTANCE = new ChangeSchemaDefinitions(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java index b244ec46f7..4e1891aff1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java @@ -33,6 +33,7 @@ import com.google.gerrit.server.patch.PatchList; import com.google.gerrit.server.patch.PatchListEntry; import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; +import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.project.ProjectState; import com.google.gerrit.server.query.change.ChangeData; import com.google.gwtorm.server.OrmException; @@ -305,10 +306,10 @@ public abstract class ChangeEmail extends NotificationEmail { // BCC anyone who has starred this change. // for (Account.Id accountId : args.starredChangesUtil - .byChange(change.getId())) { + .byChangeFromIndex(change.getId())) { super.add(RecipientType.BCC, accountId); } - } catch (OrmException err) { + } catch (OrmException | NoSuchChangeException err) { // Just don't BCC everyone. Better to send a partial message to those // we already have queued up then to fail deliver entirely to people // who have a lower interest in the change. diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java index 40fadb4fca..452d51fa69 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java @@ -44,6 +44,7 @@ import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.PatchLineCommentsUtil; import com.google.gerrit.server.PatchSetUtil; +import com.google.gerrit.server.StarredChangesUtil; import com.google.gerrit.server.change.MergeabilityCache; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.MergeUtil; @@ -296,7 +297,7 @@ public class ChangeData { public static ChangeData createForTest(Project.NameKey project, Change.Id id, int currentPatchSetId) { ChangeData cd = new ChangeData(null, null, null, null, null, null, null, - null, null, null, null, null, null, null, project, id); + null, null, null, null, null, null, null, null, project, id); cd.currentPatchSet = new PatchSet(new PatchSet.Id(id, currentPatchSetId)); return cd; } @@ -315,6 +316,7 @@ public class ChangeData { private final PatchListCache patchListCache; private final NotesMigration notesMigration; private final MergeabilityCache mergeabilityCache; + private final StarredChangesUtil starredChangesUtil; private final Change.Id legacyId; private DataSource returnedBySource; private Project.NameKey project; @@ -338,6 +340,7 @@ public class ChangeData { private Set editsByUser; private Set reviewedBy; private Set draftsByUser; + private Set starredByUser; private PersonIdent author; private PersonIdent committer; @@ -356,6 +359,7 @@ public class ChangeData { PatchListCache patchListCache, NotesMigration notesMigration, MergeabilityCache mergeabilityCache, + StarredChangesUtil starredChangesUtil, @Assisted ReviewDb db, @Assisted Project.NameKey project, @Assisted Change.Id id) { @@ -373,6 +377,7 @@ public class ChangeData { this.patchListCache = patchListCache; this.notesMigration = notesMigration; this.mergeabilityCache = mergeabilityCache; + this.starredChangesUtil = starredChangesUtil; this.project = project; this.legacyId = id; } @@ -392,6 +397,7 @@ public class ChangeData { PatchListCache patchListCache, NotesMigration notesMigration, MergeabilityCache mergeabilityCache, + StarredChangesUtil starredChangesUtil, @Assisted ReviewDb db, @Assisted Change c) { this.db = db; @@ -408,6 +414,7 @@ public class ChangeData { this.patchListCache = patchListCache; this.notesMigration = notesMigration; this.mergeabilityCache = mergeabilityCache; + this.starredChangesUtil = starredChangesUtil; legacyId = c.getId(); change = c; project = c.getProject(); @@ -428,6 +435,7 @@ public class ChangeData { PatchListCache patchListCache, NotesMigration notesMigration, MergeabilityCache mergeabilityCache, + StarredChangesUtil starredChangesUtil, @Assisted ReviewDb db, @Assisted ChangeNotes cn) { this.db = db; @@ -444,6 +452,7 @@ public class ChangeData { this.patchListCache = patchListCache; this.notesMigration = notesMigration; this.mergeabilityCache = mergeabilityCache; + this.starredChangesUtil = starredChangesUtil; legacyId = cn.getChangeId(); change = cn.getChange(); project = cn.getProjectName(); @@ -465,6 +474,7 @@ public class ChangeData { PatchListCache patchListCache, NotesMigration notesMigration, MergeabilityCache mergeabilityCache, + StarredChangesUtil starredChangesUtil, @Assisted ReviewDb db, @Assisted ChangeControl c) { this.db = db; @@ -481,6 +491,7 @@ public class ChangeData { this.patchListCache = patchListCache; this.notesMigration = notesMigration; this.mergeabilityCache = mergeabilityCache; + this.starredChangesUtil = starredChangesUtil; legacyId = c.getId(); change = c.getChange(); changeControl = c; @@ -503,6 +514,7 @@ public class ChangeData { PatchListCache patchListCache, NotesMigration notesMigration, MergeabilityCache mergeabilityCache, + StarredChangesUtil starredChangesUtil, @Assisted ReviewDb db, @Assisted Change.Id id) { checkState(!notesMigration.readChanges(), @@ -521,6 +533,7 @@ public class ChangeData { this.patchListCache = patchListCache; this.notesMigration = notesMigration; this.mergeabilityCache = mergeabilityCache; + this.starredChangesUtil = starredChangesUtil; this.legacyId = id; this.project = null; } @@ -1005,6 +1018,17 @@ public class ChangeData { this.reviewedBy = reviewedBy; } + public Set starredBy() throws OrmException { + if (starredByUser == null) { + starredByUser = starredChangesUtil.byChange(legacyId); + } + return starredByUser; + } + + public void setStarredBy(Set starredByUser) { + this.starredByUser = starredByUser; + } + @AutoValue abstract static class ReviewedByEvent { private static ReviewedByEvent create(ChangeMessage msg) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java index cb27de406c..2679f29106 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java @@ -419,7 +419,7 @@ public class ChangeQueryBuilder extends QueryBuilder { @Operator public Predicate has(String value) throws QueryParseException { if ("star".equalsIgnoreCase(value)) { - return new IsStarredByPredicate(args); + return starredby(self()); } if ("draft".equalsIgnoreCase(value)) { @@ -435,7 +435,7 @@ public class ChangeQueryBuilder extends QueryBuilder { @Operator public Predicate is(String value) throws QueryParseException { if ("starred".equalsIgnoreCase(value)) { - return new IsStarredByPredicate(args); + return starredby(self()); } if ("watched".equalsIgnoreCase(value)) { @@ -648,17 +648,25 @@ public class ChangeQueryBuilder extends QueryBuilder { @Operator public Predicate starredby(String who) throws QueryParseException, OrmException { - if ("self".equals(who)) { - return new IsStarredByPredicate(args); - } - Set m = parseAccount(who); - List p = Lists.newArrayListWithCapacity(m.size()); - for (Account.Id id : m) { - p.add(new IsStarredByPredicate(args.asUser(id))); + return starredby(parseAccount(who)); + } + + private Predicate starredby(Set who) + throws QueryParseException { + List> p = Lists.newArrayListWithCapacity(who.size()); + for (Account.Id id : who) { + p.add(starredby(id)); } return Predicate.or(p); } + private Predicate starredby(Account.Id who) + throws QueryParseException { + return args.getSchema().hasField(ChangeField.STARREDBY) + ? new IsStarredByPredicate(who) + : new IsStarredByLegacyPredicate(args.asUser(who)); + } + @Operator public Predicate watchedby(String who) throws QueryParseException, OrmException { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java index bb72a1b09f..37cdfafabe 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java @@ -28,6 +28,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Lists; import com.google.common.collect.Sets; import com.google.gerrit.common.Nullable; +import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; @@ -297,6 +298,10 @@ public class InternalChangeQuery { return query(and(project(project), or(groupPredicates))); } + public List byIsStarred(Account.Id id) throws OrmException { + return query(new IsStarredByPredicate(id)); + } + public List query(Predicate p) throws OrmException { try { return qp.queryChanges(p).changes(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/IsStarredByLegacyPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/IsStarredByLegacyPredicate.java new file mode 100644 index 0000000000..718c3f6480 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/IsStarredByLegacyPredicate.java @@ -0,0 +1,71 @@ +// Copyright (C) 2010 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.query.change; + +import com.google.common.collect.Lists; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.query.OrPredicate; +import com.google.gerrit.server.query.Predicate; +import com.google.gerrit.server.query.QueryParseException; +import com.google.gerrit.server.query.change.ChangeQueryBuilder.Arguments; + +import java.util.List; +import java.util.Set; + +@Deprecated +class IsStarredByLegacyPredicate extends OrPredicate { + private static String describe(CurrentUser user) { + if (user.isIdentifiedUser()) { + return user.getAccountId().toString(); + } + return user.toString(); + } + + private static List> predicates(Set ids) { + List> r = Lists.newArrayListWithCapacity(ids.size()); + for (Change.Id id : ids) { + r.add(new LegacyChangeIdPredicate(id)); + } + return r; + } + + private final CurrentUser user; + + IsStarredByLegacyPredicate(Arguments args) throws QueryParseException { + super(predicates(args.getIdentifiedUser().getStarredChanges())); + this.user = args.getIdentifiedUser(); + } + + @Override + public boolean match(final ChangeData object) { + return user.getStarredChanges().contains(object.getId()); + } + + @Override + public int getCost() { + return 0; + } + + @Override + public String toString() { + String val = describe(user); + if (val.indexOf(' ') < 0) { + return ChangeQueryBuilder.FIELD_STARREDBY + ":" + val; + } else { + return ChangeQueryBuilder.FIELD_STARREDBY + ":\"" + val + "\""; + } + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/IsStarredByPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/IsStarredByPredicate.java index 5ff859a27f..50c54f6cc6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/IsStarredByPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/IsStarredByPredicate.java @@ -14,57 +14,31 @@ package com.google.gerrit.server.query.change; -import com.google.common.collect.Lists; -import com.google.gerrit.reviewdb.client.Change; -import com.google.gerrit.server.CurrentUser; -import com.google.gerrit.server.query.OrPredicate; -import com.google.gerrit.server.query.Predicate; -import com.google.gerrit.server.query.QueryParseException; -import com.google.gerrit.server.query.change.ChangeQueryBuilder.Arguments; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.server.index.IndexPredicate; +import com.google.gerrit.server.index.change.ChangeField; +import com.google.gwtorm.server.OrmException; -import java.util.List; -import java.util.Set; +class IsStarredByPredicate extends IndexPredicate { + private final Account.Id accountId; -class IsStarredByPredicate extends OrPredicate { - private static String describe(CurrentUser user) { - if (user.isIdentifiedUser()) { - return user.getAccountId().toString(); - } - return user.toString(); - } - - private static List> predicates(Set ids) { - List> r = Lists.newArrayListWithCapacity(ids.size()); - for (Change.Id id : ids) { - r.add(new LegacyChangeIdPredicate(id)); - } - return r; - } - - private final CurrentUser user; - - IsStarredByPredicate(Arguments args) throws QueryParseException { - super(predicates(args.getIdentifiedUser().getStarredChanges())); - this.user = args.getIdentifiedUser(); + IsStarredByPredicate(Account.Id accountId) { + super(ChangeField.STARREDBY, accountId.toString()); + this.accountId = accountId; } @Override - public boolean match(final ChangeData object) { - return user.getStarredChanges().contains(object.getId()); + public boolean match(ChangeData cd) throws OrmException { + return cd.starredBy().contains(accountId); } @Override public int getCost() { - return 0; + return 1; } @Override public String toString() { - String val = describe(user); - if (val.indexOf(' ') < 0) { - return ChangeQueryBuilder.FIELD_STARREDBY + ":" + val; - } else { - return ChangeQueryBuilder.FIELD_STARREDBY + ":\"" + val + "\""; - } + return ChangeQueryBuilder.FIELD_STARREDBY + ":" + accountId; } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java index 129c155407..6e2f9c92c4 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java @@ -1172,6 +1172,23 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { assertQuery("draftby:" + user2); } + @Test + public void byStarredBy() throws Exception { + TestRepository repo = createProject("repo"); + Change change1 = insert(repo, newChange(repo)); + Change change2 = insert(repo, newChange(repo)); + insert(repo, newChange(repo)); + + gApi.accounts().self().starChange(change1.getId().toString()); + gApi.accounts().self().starChange(change2.getId().toString()); + + int user2 = accountManager.authenticate(AuthRequest.forUser("anotheruser")) + .getAccountId().get(); + + assertQuery("starredby:self", change2, change1); + assertQuery("starredby:" + user2); + } + @Test public void byFrom() throws Exception { TestRepository repo = createProject("repo");