diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java index 5b891107c1..ebca92bbe4 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java @@ -52,7 +52,7 @@ import com.google.gerrit.extensions.restapi.IdString; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.reviewdb.client.Patch; import com.google.gerrit.reviewdb.client.PatchSet; -import com.google.gerrit.server.ChangeUtil; +import com.google.gerrit.server.ChangeFinder; import com.google.gerrit.server.change.ChangeResource; import com.google.gerrit.server.change.GetRevisionActions; import com.google.gerrit.server.change.RevisionResource; @@ -80,7 +80,7 @@ import java.util.Map; public class RevisionIT extends AbstractDaemonTest { @Inject - private ChangeUtil changeUtil; + private ChangeFinder changeFinder; @Inject private GetRevisionActions getRevisionActions; @@ -693,7 +693,7 @@ public class RevisionIT extends AbstractDaemonTest { private RevisionResource parseRevisionResource(PushOneCommit.Result r) throws Exception { PatchSet.Id psId = r.getPatchSetId(); - List ctls = changeUtil.findChanges( + List ctls = changeFinder.find( Integer.toString(psId.getParentKey().get()), atrScope.get().getUser()); assertThat(ctls).hasSize(1); return revisions.parse( diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeFinder.java b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeFinder.java new file mode 100644 index 0000000000..0665bc7105 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeFinder.java @@ -0,0 +1,102 @@ +// Copyright (C) 2016 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; + +import com.google.common.base.Optional; +import com.google.common.collect.ImmutableSet; +import com.google.common.primitives.Ints; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.server.change.ChangeTriplet; +import com.google.gerrit.server.project.ChangeControl; +import com.google.gerrit.server.query.change.ChangeData; +import com.google.gerrit.server.query.change.InternalChangeQuery; +import com.google.gwtorm.server.OrmException; +import com.google.inject.Inject; +import com.google.inject.Provider; +import com.google.inject.Singleton; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +@Singleton +public class ChangeFinder { + private final Provider queryProvider; + + @Inject + ChangeFinder(Provider queryProvider) { + this.queryProvider = queryProvider; + } + + /** + * Find changes matching the given identifier. + * + * @param id change identifier, either a numeric ID, a Change-Id, or + * project~branch~id triplet. + * @param user user to wrap in controls. + * @return possibly-empty list of controls for all matching changes, + * corresponding to the given user; may or may not be visible. + * @throws OrmException if an error occurred querying the database. + */ + public List find(String id, CurrentUser user) + throws OrmException { + // Use the index to search for changes, but don't return any stored fields, + // to force rereading in case the index is stale. + InternalChangeQuery query = queryProvider.get() + .setRequestedFields(ImmutableSet. of()); + + // Try legacy id + if (!id.isEmpty() && id.charAt(0) != '0') { + Integer n = Ints.tryParse(id); + if (n != null) { + return asChangeControls(query.byLegacyChangeId(new Change.Id(n)), user); + } + } + + // Try isolated changeId + if (!id.contains("~")) { + return asChangeControls(query.byKeyPrefix(id), user); + } + + // Try change triplet + Optional triplet = ChangeTriplet.parse(id); + if (triplet.isPresent()) { + return asChangeControls(query.byBranchKey( + triplet.get().branch(), + triplet.get().id()), + user); + } + + return Collections.emptyList(); + } + + public List find(Change.Id id, CurrentUser user) + throws OrmException { + // Use the index to search for changes, but don't return any stored fields, + // to force rereading in case the index is stale. + InternalChangeQuery query = queryProvider.get() + .setRequestedFields(ImmutableSet. of()); + return asChangeControls(query.byLegacyChangeId(id), user); + } + + private List asChangeControls(List cds, + CurrentUser user) throws OrmException { + List ctls = new ArrayList<>(cds.size()); + for (ChangeData cd : cds) { + ctls.add(cd.changeControl(user)); + } + return ctls; + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java index 7c76644547..c49498de80 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java @@ -15,10 +15,7 @@ package com.google.gerrit.server; import com.google.common.base.Function; -import com.google.common.base.Optional; -import com.google.common.collect.ImmutableSet; import com.google.common.collect.Ordering; -import com.google.common.primitives.Ints; import com.google.gerrit.common.TimeUtil; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.RestApiException; @@ -29,7 +26,6 @@ import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.change.ChangeInserter; import com.google.gerrit.server.change.ChangeMessages; -import com.google.gerrit.server.change.ChangeTriplet; import com.google.gerrit.server.git.BatchUpdate; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.UpdateException; @@ -39,8 +35,6 @@ import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.project.ChangeControl; 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.gerrit.server.util.IdGenerator; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -65,9 +59,7 @@ import org.slf4j.LoggerFactory; import java.io.IOException; import java.text.MessageFormat; -import java.util.ArrayList; import java.util.Collections; -import java.util.List; import java.util.Map; @Singleton @@ -163,7 +155,6 @@ public class ChangeUtil { private final Provider user; private final Provider db; private final Sequences seq; - private final Provider queryProvider; private final PatchSetUtil psUtil; private final RevertedSender.Factory revertedSenderFactory; private final ChangeInserter.Factory changeInserterFactory; @@ -176,7 +167,6 @@ public class ChangeUtil { ChangeUtil(Provider user, Provider db, Sequences seq, - Provider queryProvider, PatchSetUtil psUtil, RevertedSender.Factory revertedSenderFactory, ChangeInserter.Factory changeInserterFactory, @@ -187,7 +177,6 @@ public class ChangeUtil { this.user = user; this.db = db; this.seq = seq; - this.queryProvider = queryProvider; this.psUtil = psUtil; this.revertedSenderFactory = revertedSenderFactory; this.changeInserterFactory = changeInserterFactory; @@ -315,66 +304,6 @@ public class ChangeUtil { } } - /** - * Find changes matching the given identifier. - * - * @param id change identifier, either a numeric ID, a Change-Id, or - * project~branch~id triplet. - * @param user user to wrap in controls. - * @return possibly-empty list of controls for all matching changes, - * corresponding to the given user; may or may not be visible. - * @throws OrmException if an error occurred querying the database. - */ - public List findChanges(String id, CurrentUser user) - throws OrmException { - // Use the index to search for changes, but don't return any stored fields, - // to force rereading in case the index is stale. - InternalChangeQuery query = queryProvider.get() - .setRequestedFields(ImmutableSet. of()); - - // Try legacy id - if (!id.isEmpty() && id.charAt(0) != '0') { - Integer n = Ints.tryParse(id); - if (n != null) { - return asChangeControls(query.byLegacyChangeId(new Change.Id(n)), user); - } - } - - // Try isolated changeId - if (!id.contains("~")) { - return asChangeControls(query.byKeyPrefix(id), user); - } - - // Try change triplet - Optional triplet = ChangeTriplet.parse(id); - if (triplet.isPresent()) { - return asChangeControls(query.byBranchKey( - triplet.get().branch(), - triplet.get().id()), - user); - } - - return Collections.emptyList(); - } - - public List findChanges(Change.Id id, CurrentUser user) - throws OrmException { - // Use the index to search for changes, but don't return any stored fields, - // to force rereading in case the index is stale. - InternalChangeQuery query = queryProvider.get() - .setRequestedFields(ImmutableSet. of()); - return asChangeControls(query.byLegacyChangeId(id), user); - } - - private List asChangeControls(List cds, - CurrentUser user) throws OrmException { - List ctls = new ArrayList<>(cds.size()); - for (ChangeData cd : cds) { - ctls.add(cd.changeControl(user)); - } - return ctls; - } - public static PatchSet.Id nextPatchSetId(PatchSet.Id id) { return new PatchSet.Id(id.getParentKey(), id.get() + 1); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangesCollection.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangesCollection.java index bb8830ef50..af3c576e2b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangesCollection.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangesCollection.java @@ -25,7 +25,7 @@ import com.google.gerrit.extensions.restapi.RestView; import com.google.gerrit.extensions.restapi.TopLevelResource; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.server.ReviewDb; -import com.google.gerrit.server.ChangeUtil; +import com.google.gerrit.server.ChangeFinder; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.index.ChangeIndexer; import com.google.gerrit.server.project.ChangeControl; @@ -46,7 +46,7 @@ public class ChangesCollection implements private final Provider user; private final Provider queryFactory; private final DynamicMap> views; - private final ChangeUtil changeUtil; + private final ChangeFinder changeFinder; private final CreateChange createChange; private final ChangeIndexer changeIndexer; @@ -56,14 +56,14 @@ public class ChangesCollection implements Provider user, Provider queryFactory, DynamicMap> views, - ChangeUtil changeUtil, + ChangeFinder changeFinder, CreateChange createChange, ChangeIndexer changeIndexer) { this.db = db; this.user = user; this.queryFactory = queryFactory; this.views = views; - this.changeUtil = changeUtil; + this.changeFinder = changeFinder; this.createChange = createChange; this.changeIndexer = changeIndexer; } @@ -81,7 +81,8 @@ public class ChangesCollection implements @Override public ChangeResource parse(TopLevelResource root, IdString id) throws ResourceNotFoundException, OrmException { - List ctls = changeUtil.findChanges(id.encoded(), user.get()); + List ctls = + changeFinder.find(id.encoded(), user.get()); if (ctls.isEmpty()) { Integer changeId = Ints.tryParse(id.get()); if (changeId != null) { @@ -108,7 +109,7 @@ public class ChangesCollection implements public ChangeResource parse(Change.Id id) throws ResourceNotFoundException, OrmException { - List ctls = changeUtil.findChanges(id, user.get()); + List ctls = changeFinder.find(id, user.get()); if (ctls.isEmpty()) { try { changeIndexer.delete(id); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java index 5a1e6ffa90..d47eb8b7db 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java @@ -33,7 +33,7 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.server.ReviewDb; -import com.google.gerrit.server.ChangeUtil; +import com.google.gerrit.server.ChangeFinder; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; @@ -84,7 +84,7 @@ public class CreateChange implements private final ProjectsCollection projectsCollection; private final ChangeInserter.Factory changeInserterFactory; private final ChangeJson.Factory jsonFactory; - private final ChangeUtil changeUtil; + private final ChangeFinder changeFinder; private final BatchUpdate.Factory updateFactory; private final PatchSetUtil psUtil; private final boolean allowDrafts; @@ -98,7 +98,7 @@ public class CreateChange implements ProjectsCollection projectsCollection, ChangeInserter.Factory changeInserterFactory, ChangeJson.Factory json, - ChangeUtil changeUtil, + ChangeFinder changeFinder, BatchUpdate.Factory updateFactory, PatchSetUtil psUtil, @GerritServerConfig Config config) { @@ -110,7 +110,7 @@ public class CreateChange implements this.projectsCollection = projectsCollection; this.changeInserterFactory = changeInserterFactory; this.jsonFactory = json; - this.changeUtil = changeUtil; + this.changeFinder = changeFinder; this.updateFactory = updateFactory; this.psUtil = psUtil; this.allowDrafts = config.getBoolean("change", "allowDrafts", true); @@ -162,7 +162,7 @@ public class CreateChange implements ObjectId parentCommit; List groups; if (input.baseChange != null) { - List ctls = changeUtil.findChanges( + List ctls = changeFinder.find( input.baseChange, rsrc.getControl().getUser()); if (ctls.size() != 1) { throw new InvalidChangeOperationException( diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java index 49cdc4fdb7..ca9357eb10 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java @@ -29,6 +29,7 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.ApprovalsUtil; +import com.google.gerrit.server.ChangeFinder; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.query.change.ChangeData; @@ -47,15 +48,18 @@ public class ChangeControl { private final ProjectControl.GenericFactory projectControl; private final Provider db; private final ChangeNotes.Factory notesFactory; + private final ChangeFinder changeFinder; @Inject GenericFactory( ProjectControl.GenericFactory p, Provider d, - ChangeNotes.Factory n) { + ChangeNotes.Factory n, + ChangeFinder f) { projectControl = p; db = d; notesFactory = n; + changeFinder = f; } public ChangeControl controlFor(Project.NameKey project, Change.Id changeId, @@ -88,11 +92,11 @@ public class ChangeControl { public ChangeControl validateFor(Change.Id changeId, CurrentUser user) throws NoSuchChangeException, OrmException { - Change change = db.get().changes().get(changeId); - if (change == null) { + List ctls = changeFinder.find(changeId, user); + if (ctls.size() != 1) { throw new NoSuchChangeException(changeId); } - return validateFor(change, user); + return validateFor(ctls.get(0).getChange(), user); } public ChangeControl validateFor(Change change, CurrentUser user) diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/SetReviewersCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/SetReviewersCommand.java index 076942e618..0718dc6179 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/SetReviewersCommand.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/SetReviewersCommand.java @@ -20,7 +20,7 @@ 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.server.ReviewDb; -import com.google.gerrit.server.ChangeUtil; +import com.google.gerrit.server.ChangeFinder; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.change.ChangeResource; import com.google.gerrit.server.change.ChangesCollection; @@ -94,7 +94,7 @@ public class SetReviewersCommand extends SshCommand { private ChangesCollection changesCollection; @Inject - private ChangeUtil changeUtil; + private ChangeFinder changeFinder; private Set toRemove = new HashSet<>(); @@ -165,7 +165,7 @@ public class SetReviewersCommand extends SshCommand { private void addChangeImpl(String id) throws UnloggedFailure, OrmException { List matched = - changeUtil.findChanges(id, userProvider.get()); + changeFinder.find(id, userProvider.get()); List toAdd = new ArrayList<>(changes.size()); for (ChangeControl ctl : matched) { if (!changes.containsKey(ctl.getId()) && inProject(ctl.getProject())