From 02417753a93b5613317c92c2aa02ab7566a8766d Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 3 Feb 2016 16:54:07 +0100 Subject: [PATCH] ChangesCollection: Lookup change in index when legacy ID is given So far when a legacy change ID was specified we looked up the change in the database (via ChangeControl#controlFor(Change.Id,CurrentUser)). We cannot load the change via ChangeNotes.Factory since we do not know the project. Hence lookup the change in the secondary index like we already do it when a Change-Id is given. ConsistencyCheckerIT creates changes manually and must now take care to put them into the index, so that the query can find them. Change-Id: I4885855b406c0da854ce5490ffb91b0f93808c3e Signed-off-by: Edwin Kempin --- .../server/change/ConsistencyCheckerIT.java | 1 + .../com/google/gerrit/server/ChangeUtil.java | 34 +++++++++---------- .../server/change/ChangesCollection.java | 24 +++++++------ .../query/change/InternalChangeQuery.java | 4 +++ 4 files changed, 36 insertions(+), 27 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java index 34d42f172f..050230868d 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java @@ -607,6 +607,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest { private Change insertChange() throws Exception { Change c = newChange(project, adminId); db.changes().insert(singleton(c)); + indexer.index(db, c); ChangeUpdate u = changeUpdateFactory.create( changeControlFactory.controlFor(c, userFactory.create(adminId))); 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 f51eeb10c8..d256346a03 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 @@ -16,7 +16,6 @@ package com.google.gerrit.server; import com.google.common.base.Function; import com.google.common.base.Optional; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Ordering; import com.google.common.primitives.Ints; @@ -166,7 +165,6 @@ public class ChangeUtil { private final Sequences seq; private final Provider queryProvider; private final PatchSetUtil psUtil; - private final ChangeControl.GenericFactory changeControlFactory; private final RevertedSender.Factory revertedSenderFactory; private final ChangeInserter.Factory changeInserterFactory; private final GitRepositoryManager gitManager; @@ -180,7 +178,6 @@ public class ChangeUtil { Sequences seq, Provider queryProvider, PatchSetUtil psUtil, - ChangeControl.GenericFactory changeControlFactory, RevertedSender.Factory revertedSenderFactory, ChangeInserter.Factory changeInserterFactory, GitRepositoryManager gitManager, @@ -192,7 +189,6 @@ public class ChangeUtil { this.seq = seq; this.queryProvider = queryProvider; this.psUtil = psUtil; - this.changeControlFactory = changeControlFactory; this.revertedSenderFactory = revertedSenderFactory; this.changeInserterFactory = changeInserterFactory; this.gitManager = gitManager; @@ -331,24 +327,19 @@ public class ChangeUtil { */ public List findChanges(String id, CurrentUser user) throws OrmException { - // Try legacy id - if (!id.isEmpty() && id.charAt(0) != '0') { - Integer n = Ints.tryParse(id); - try { - if (n != null) { - return ImmutableList.of( - changeControlFactory.controlFor(new Change.Id(n), user)); - } - } catch (NoSuchChangeException e) { - return Collections.emptyList(); - } - } - // 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); @@ -366,6 +357,15 @@ public class ChangeUtil { 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()); 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 96f2e1379c..bb8830ef50 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 @@ -29,7 +29,6 @@ import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.index.ChangeIndexer; import com.google.gerrit.server.project.ChangeControl; -import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.query.change.QueryChanges; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -45,7 +44,6 @@ public class ChangesCollection implements AcceptsPost { private final Provider db; private final Provider user; - private final ChangeControl.GenericFactory changeControlFactory; private final Provider queryFactory; private final DynamicMap> views; private final ChangeUtil changeUtil; @@ -56,7 +54,6 @@ public class ChangesCollection implements ChangesCollection( Provider db, Provider user, - ChangeControl.GenericFactory changeControlFactory, Provider queryFactory, DynamicMap> views, ChangeUtil changeUtil, @@ -64,7 +61,6 @@ public class ChangesCollection implements ChangeIndexer changeIndexer) { this.db = db; this.user = user; - this.changeControlFactory = changeControlFactory; this.queryFactory = queryFactory; this.views = views; this.changeUtil = changeUtil; @@ -112,15 +108,23 @@ public class ChangesCollection implements public ChangeResource parse(Change.Id id) throws ResourceNotFoundException, OrmException { - try { - ChangeControl ctl = changeControlFactory.controlFor(id, user.get()); - if (!ctl.isVisible(db.get())) { - throw new ResourceNotFoundException(toIdString(id)); + List ctls = changeUtil.findChanges(id, user.get()); + if (ctls.isEmpty()) { + try { + changeIndexer.delete(id); + } catch (IOException e) { + throw new ResourceNotFoundException(toIdString(id).get(), e); } - return new ChangeResource(ctl); - } catch (NoSuchChangeException e) { throw new ResourceNotFoundException(toIdString(id)); } + if (ctls.size() != 1) { + throw new ResourceNotFoundException("Multiple changes found for " + id); + } + ChangeControl ctl = ctls.get(0); + if (!ctl.isVisible(db.get())) { + throw new ResourceNotFoundException(toIdString(id)); + } + return new ChangeResource(ctl); } private static IdString toIdString(Change.Id id) { 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 5ef41fd837..7bd4816368 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 @@ -118,6 +118,10 @@ public class InternalChangeQuery { return query(new ChangeIdPredicate(prefix)); } + public List byLegacyChangeId(Change.Id id) throws OrmException { + return query(new LegacyChangeIdPredicate(id)); + } + public List byBranchKey(Branch.NameKey branch, Change.Key key) throws OrmException { return query(and(