Ensure no ambiguous change IDs when deleting in the index.changes api When deleteMissing is true, changes are potentially deleted from the index, it's therefore crucial that we do not use an ambiguous change-id formats that could lead to Gerrit deleting the wrong change from the index. This api essentially used a side-effect of ChangeFinder to identify if a change was present or not: - ChangeFinder uses the index to find the document if the identifier isn't a `project~changeNumber` - If the entry is found on the index, it then tries to load the associated NoteDb data from the Git repository - If the data is present in the index but NOT in NoteDb, then it returns an empty result The above doesn't happen if the change is in `project~changeNumber` format. The code was relying on the above logic to infer that a change was in the index, but not in NoteDb, by checking if the result was empty, which is a side-effect rather than a direct contract behaviour. Use the queryProvider to identify if a change is present in the index or not, and, if it is, check on disk if it's actually present. To ensure we don't use ambiguous changeIDs, force using project~changeNum as its the true unique identifier of a change. Finally, fail the request with a 400 status code if not all change ids are in said format. Release-Notes: Breaking change: require tilde changeIds in config/server/index.changes when requesting deletions and reject legacy numeric-only ids. Bug: Issue 515272357 Change-Id: I8350971af868ee34b17fc8703aa9ef40c03f5ec5
diff --git a/Documentation/rest-api-config.txt b/Documentation/rest-api-config.txt index 8c3c972..b86d732 100644 --- a/Documentation/rest-api-config.txt +++ b/Documentation/rest-api-config.txt
@@ -1548,8 +1548,7 @@ { "changes": [ "foo~101", - "bar~202", - "303" + "bar~202" ], "delete_missing": "true" } @@ -1561,8 +1560,13 @@ Content-Disposition: attachment ---- -When `delete_missing` is set to `true` changes to be reindexed which are missing in NoteDb -will be deleted in the index. +[NOTE] +When `delete_missing` is set to `true`, only Change-IDs in the format +project~changeNumber are accepted. Changes to be reindexed which are +missing in NoteDb will be deleted in the index. If any of the Change-IDs +are in an incorrect format, the server will return a 400 - Bad Request, +and none of the operations will be executed. + [[list-indexes]] === List Indexes @@ -2573,11 +2577,11 @@ |================================ |Field Name ||Description |`changes` || -List of link:rest-api-changes.html#change-id[change-ids] +List of link:rest-api-changes.html#change-id[change-ids]. When `delete_missing` is `true`, each entry must be in `project~changeNumber` format. |`delete_missing` |optional| Delete changes which are missing in NoteDb from the index. This can be used to get rid of stale index entries. Possible values are `true` and `false`. -By default set to `false`. +By default set to `false`. When `true`, all entries in `changes` must be in `project~changeNumber` format; otherwise the request fails with `400 Bad Request` and no operations are executed. |================================ [[jvm-summary-info]]
diff --git a/java/com/google/gerrit/server/restapi/config/IndexChanges.java b/java/com/google/gerrit/server/restapi/config/IndexChanges.java index ea970d0..94f52a9 100644 --- a/java/com/google/gerrit/server/restapi/config/IndexChanges.java +++ b/java/com/google/gerrit/server/restapi/config/IndexChanges.java
@@ -14,29 +14,38 @@ package com.google.gerrit.server.restapi.config; +import com.google.common.base.Preconditions; import com.google.common.flogger.FluentLogger; import com.google.gerrit.common.data.GlobalCapability; import com.google.gerrit.entities.Change; +import com.google.gerrit.entities.Project; import com.google.gerrit.extensions.annotations.RequiresCapability; +import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.server.change.ChangeFinder; import com.google.gerrit.server.config.ConfigResource; import com.google.gerrit.server.index.change.ChangeIndexer; import com.google.gerrit.server.notedb.ChangeNotes; +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.restapi.config.IndexChanges.Input; 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; -import java.util.Optional; import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; @RequiresCapability(GlobalCapability.ADMINISTRATE_SERVER) @Singleton public class IndexChanges implements RestModifyView<ConfigResource, Input> { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + private static final Pattern PROJECT_WITH_CHANGE_NUM_REGEX = Pattern.compile("^([^~]+)~(\\d+)$"); public record Input(Set<String> changes, boolean deleteMissing) { @@ -47,45 +56,88 @@ private final ChangeFinder changeFinder; private final ChangeData.Factory changeDataFactory; + private final Provider<InternalChangeQuery> queryProvider; + private final ChangeNotes.Factory notesFactory; private final ChangeIndexer indexer; @Inject IndexChanges( - ChangeFinder changeFinder, ChangeData.Factory changeDataFactory, ChangeIndexer indexer) { + ChangeFinder changeFinder, + ChangeData.Factory changeDataFactory, + Provider<InternalChangeQuery> queryProvider, + ChangeNotes.Factory notesFactory, + ChangeIndexer indexer) { this.changeFinder = changeFinder; this.changeDataFactory = changeDataFactory; + this.queryProvider = queryProvider; + this.notesFactory = notesFactory; this.indexer = indexer; } @Override - public Response<String> apply(ConfigResource resource, Input input) { + public Response<String> apply(ConfigResource resource, Input input) throws Exception { if (input == null || input.changes == null) { return Response.ok("Nothing to index"); } - for (String id : input.changes) { - List<ChangeNotes> notes = changeFinder.find(id); + if (input.deleteMissing) { + List<ProjectWithChangeNumTuple> changeIds = new ArrayList<>(); + for (String id : input.changes) { + changeIds.add(parseIntoProjectWithChangeNumTuple(id)); + } - if (notes.isEmpty()) { - logger.atWarning().log("Change %s missing in NoteDb", id); - if (input.deleteMissing) { - int tilde = id.lastIndexOf('~'); - String numericPart = tilde >= 0 ? id.substring(tilde + 1) : id; - Optional<Change.Id> changeId = Change.Id.tryParse(numericPart); - if (changeId.isPresent()) { - logger.atWarning().log("Deleting change %s from index", changeId.get()); - indexer.delete(changeId.get()); + for (ProjectWithChangeNumTuple changeInfo : changeIds) { + List<ChangeData> changes = + queryProvider.get().byProjectChangeNumber(changeInfo.project(), changeInfo.changeId()); + Preconditions.checkState( + changes.size() <= 1, + "Ambiguous change ID %s in project %s", + changeInfo.changeId(), + changeInfo.project()); + + if (!changes.isEmpty()) { + try { + // Probe NoteDb: NoSuchChangeException means the change is in the index + // but absent from disk, so it should be deleted. + ChangeNotes unused = notesFactory.create(changeInfo.project(), changeInfo.changeId()); + } catch (NoSuchChangeException e) { + logger.atWarning().log( + "Change %s~%s missing in NoteDb", changeInfo.project(), changeInfo.changeId()); + ChangeData cd = changes.getFirst(); + logger.atWarning().log( + "Deleting change %s~%s from index", cd.project(), cd.change().getChangeId()); + indexer.delete(cd.virtualId()); + continue; } } - continue; - } - for (ChangeNotes n : notes) { - indexer.index(changeDataFactory.create(n)); - logger.atFine().log("Indexed change %s", id); + indexer.index(changeInfo.project, changeInfo.changeId); + logger.atFine().log("Indexed change %s:%s", changeInfo.project, changeInfo.changeId); } + } else { + input.changes.stream() + .flatMap(cid -> changeFinder.find(cid).stream()) + .map(changeDataFactory::create) + .forEach( + cd -> { + indexer.index(cd); + logger.atFine().log("Indexed change %s:%s", cd.project(), cd.getId()); + }); } return Response.ok("Indexed changes " + input.changes); } + + record ProjectWithChangeNumTuple(Project.NameKey project, Change.Id changeId) {} + + ProjectWithChangeNumTuple parseIntoProjectWithChangeNumTuple(String id) + throws BadRequestException { + Matcher projectWithChangeNumMatcher = PROJECT_WITH_CHANGE_NUM_REGEX.matcher(id); + if (projectWithChangeNumMatcher.matches()) { + return new ProjectWithChangeNumTuple( + Project.nameKey(projectWithChangeNumMatcher.group(1)), + Change.id(Integer.parseInt(projectWithChangeNumMatcher.group(2)))); + } + throw new BadRequestException("Change ID must be in project~changeNumber format: " + id); + } }
diff --git a/javatests/com/google/gerrit/acceptance/rest/config/IndexChangesIT.java b/javatests/com/google/gerrit/acceptance/rest/config/IndexChangesIT.java index 12fd227..a75b998 100644 --- a/javatests/com/google/gerrit/acceptance/rest/config/IndexChangesIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/config/IndexChangesIT.java
@@ -16,7 +16,9 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block; +import static com.google.gerrit.entities.RefNames.changeMetaRef; import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; +import static com.google.gerrit.testing.TestActionRefUpdateContext.openTestRefUpdateContext; import com.google.common.collect.ImmutableSet; import com.google.gerrit.acceptance.AbstractDaemonTest; @@ -24,9 +26,11 @@ import com.google.gerrit.acceptance.ExtensionRegistry; import com.google.gerrit.acceptance.ExtensionRegistry.Registration; import com.google.gerrit.acceptance.PushOneCommit; +import com.google.gerrit.acceptance.config.GerritConfig; import com.google.gerrit.acceptance.testsuite.project.ProjectOperations; import com.google.gerrit.entities.Change; import com.google.gerrit.entities.Permission; +import com.google.gerrit.entities.Project; import com.google.gerrit.entities.RefNames; import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.index.IndexConfig; @@ -35,30 +39,46 @@ import com.google.gerrit.server.index.change.ChangeIndexCollection; import com.google.gerrit.server.index.change.IndexedChangeQuery; import com.google.gerrit.server.query.change.ChangeData; +import com.google.gerrit.server.query.change.ChangeNumberVirtualIdAlgorithm; import com.google.gerrit.server.restapi.config.IndexChanges; import com.google.inject.Inject; import java.util.Optional; import org.eclipse.jgit.junit.TestRepository; +import org.eclipse.jgit.lib.CommitBuilder; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.ObjectInserter; +import org.eclipse.jgit.lib.ObjectReader; +import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.lib.Ref; +import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.revwalk.RevWalk; import org.junit.Test; public class IndexChangesIT extends AbstractDaemonTest { + private static final String TEST_CHANGE_NUM = "1"; + private static final String TEST_CHANGE_ID = "I8350971af868ee34b17fc8703aa9ef40c03f5ec5"; + private static final boolean PRESERVE_MISSING = false; + private static final boolean DELETE_MISSING = true; @Inject private ProjectOperations projectOperations; @Inject private ExtensionRegistry extensionRegistry; @Inject private ChangeIndexCollection changeIndexCollection; @Inject private IndexConfig indexConfig; + @Inject private ChangeNumberVirtualIdAlgorithm changeNumberVirtualIdAlgorithm; @Test public void indexRequestFromNonAdminRejected() throws Exception { ChangeIndexedCounter changeIndexedCounter = new ChangeIndexedCounter(); try (Registration registration = extensionRegistry.newRegistration().add(changeIndexedCounter)) { - String changeId = createChange().getChangeId(); - IndexChanges.Input in = new IndexChanges.Input(ImmutableSet.of(changeId), false); + PushOneCommit.Result change = createChange(); changeIndexedCounter.clear(); - userRestSession.post("/config/server/index.changes", in).assertForbidden(); - assertThat(changeIndexedCounter.getCount(info(changeId))).isEqualTo(0); + userRestSession + .post("/config/server/index.changes", indexChangesInput(change.getChange().getId())) + .assertForbidden(); + assertThat(changeIndexedCounter.getCount(info(change.getChangeId()))).isEqualTo(0); } } @@ -67,47 +87,76 @@ ChangeIndexedCounter changeIndexedCounter = new ChangeIndexedCounter(); try (Registration registration = extensionRegistry.newRegistration().add(changeIndexedCounter)) { - String changeId = createChange().getChangeId(); - IndexChanges.Input in = new IndexChanges.Input(ImmutableSet.of(changeId), false); + PushOneCommit.Result change = createChange(); changeIndexedCounter.clear(); - adminRestSession.post("/config/server/index.changes", in).assertOK(); - assertThat(changeIndexedCounter.getCount(info(changeId))).isEqualTo(1); + adminRestSession + .post("/config/server/index.changes", indexChangesInput(change.getChange().getId())) + .assertOK(); + assertThat(changeIndexedCounter.getCount(info(change.getChangeId()))).isEqualTo(1); } } @Test + public void indexChangeNotInIndex() throws Exception { + PushOneCommit.Result change = createChange(); + Change.Id changeId = change.getChange().getId(); + + assertThat(getChangeFromIndex(changeId)).isPresent(); + indexer.delete(changeId); + assertThat(getChangeFromIndex(changeId)).isEmpty(); + + adminRestSession.post("/config/server/index.changes", indexChangesInput(changeId)).assertOK(); + assertThat(getChangeFromIndex(changeId)).isPresent(); + } + + @Test public void indexNonVisibleChange() throws Exception { ChangeIndexedCounter changeIndexedCounter = new ChangeIndexedCounter(); try (Registration registration = extensionRegistry.newRegistration().add(changeIndexedCounter)) { - String changeId = createChange().getChangeId(); + String changeId = projectAndChangeNumId(project, createChange().getChange().getId()); ChangeInfo changeInfo = info(changeId); projectOperations .project(project) .forUpdate() .add(block(Permission.READ).ref("refs/heads/master").group(REGISTERED_USERS)) .update(); - IndexChanges.Input in = new IndexChanges.Input(ImmutableSet.of(changeId), false); changeIndexedCounter.clear(); - adminRestSession.post("/config/server/index.changes", in).assertOK(); + adminRestSession.post("/config/server/index.changes", indexChangesInput(changeId)).assertOK(); assertThat(changeIndexedCounter.getCount(changeInfo)).isEqualTo(1); } } @Test - public void deleteMissingChangeFromIndexByNumericId() throws Exception { - PushOneCommit.Result result = createChange(); - Change.Id changeId = result.getChange().getId(); + public void indexChangeWithPlainNumericIdAccepted() throws Exception { + Change.Id changeId = createChange().getChange().getId(); + adminRestSession + .post("/config/server/index.changes", indexChangesInput(String.valueOf(changeId.get()))) + .assertOK(); + } - assertThat(getChangeFromIndex(changeId)).isPresent(); - deleteChangeFromNoteDbWithoutUpdatingIndex(changeId); - assertThat(getChangeFromIndex(changeId)).isPresent(); + @Test + public void deleteMissingChangeFromIndexWithPlainNumericIdRejected() throws Exception { + adminRestSession + .post("/config/server/index.changes", indexChangesInput(TEST_CHANGE_NUM, DELETE_MISSING)) + .assertBadRequest(); + } - IndexChanges.Input in = - new IndexChanges.Input(ImmutableSet.of(String.valueOf(changeId.get())), true); - adminRestSession.post("/config/server/index.changes", in).assertOK(); + @Test + public void indexChangeWithTripletIdAccepted() throws Exception { + String changeId = createChange().getChangeId(); + adminRestSession + .post("/config/server/index.changes", indexChangesInput(project + "~master~" + changeId)) + .assertOK(); + } - assertThat(getChangeFromIndex(changeId)).isEmpty(); + @Test + public void deleteMissingChangeFromIndexWithTripletIdRejected() throws Exception { + adminRestSession + .post( + "/config/server/index.changes", + indexChangesInput(project + "~master~" + TEST_CHANGE_ID, DELETE_MISSING)) + .assertBadRequest(); } @Test @@ -119,9 +168,11 @@ deleteChangeFromNoteDbWithoutUpdatingIndex(changeId); assertThat(getChangeFromIndex(changeId)).isPresent(); - IndexChanges.Input in = - new IndexChanges.Input(ImmutableSet.of(project.get() + "~" + changeId.get()), true); - adminRestSession.post("/config/server/index.changes", in).assertOK(); + adminRestSession + .post( + "/config/server/index.changes", + indexChangesInput(projectAndChangeNumId(project, changeId), DELETE_MISSING)) + .assertOK(); assertThat(getChangeFromIndex(changeId)).isEmpty(); } @@ -133,9 +184,9 @@ extensionRegistry.newRegistration().add(changeIndexedCounter)) { ImmutableSet.Builder<String> changeIds = ImmutableSet.builder(); for (int i = 0; i < 10; i++) { - changeIds.add(createChange().getChangeId()); + changeIds.add(projectAndChangeNumId(project, createChange().getChange().getId())); } - IndexChanges.Input in = new IndexChanges.Input(changeIds.build(), false); + IndexChanges.Input in = new IndexChanges.Input(changeIds.build(), PRESERVE_MISSING); changeIndexedCounter.clear(); adminRestSession.post("/config/server/index.changes", in).assertOK(); for (String changeId : in.changes()) { @@ -144,6 +195,82 @@ } } + @Test + @GerritConfig(name = "gerrit.importedServerId", value = "imported-server-id") + public void deleteMissingImportedChangeFromIndex() throws Exception { + PushOneCommit.Result result = createImportedChange(); + Change.Id changeId = result.getChange().getId(); + Change.Id virtualId = + changeNumberVirtualIdAlgorithm.apply(() -> "imported-server-id", changeId); + + assertThat(getChangeFromIndex(virtualId)).isPresent(); + deleteChangeFromNoteDbWithoutUpdatingIndex(changeId); + assertThat(getChangeFromIndex(virtualId)).isPresent(); + + adminRestSession + .post("/config/server/index.changes", indexChangesInput(changeId, PRESERVE_MISSING)) + .assertOK(); + assertThat(getChangeFromIndex(virtualId)).isPresent(); + + adminRestSession + .post("/config/server/index.changes", indexChangesInput(changeId, DELETE_MISSING)) + .assertOK(); + assertThat(getChangeFromIndex(virtualId)).isEmpty(); + } + + private IndexChanges.Input indexChangesInput(String changeId) { + return new IndexChanges.Input(ImmutableSet.of(changeId), PRESERVE_MISSING); + } + + private IndexChanges.Input indexChangesInput(Change.Id changeId) { + return indexChangesInput(projectAndChangeNumId(project, changeId)); + } + + private IndexChanges.Input indexChangesInput(String changeId, boolean deleteMissing) { + return new IndexChanges.Input(ImmutableSet.of(changeId), deleteMissing); + } + + private IndexChanges.Input indexChangesInput(Change.Id changeId, boolean deleteMissing) { + return indexChangesInput(projectAndChangeNumId(project, changeId), deleteMissing); + } + + private PushOneCommit.Result createImportedChange() throws Exception { + PushOneCommit.Result change = createChange(); + Change.Id changeId = change.getChange().getId(); + String metaRef = changeMetaRef(changeId); + + try (Repository repo = repoManager.openRepository(project); + ObjectInserter inserter = repo.newObjectInserter(); + ObjectReader reader = repo.newObjectReader(); + RevWalk revWalk = new RevWalk(reader); + var ignored = openTestRefUpdateContext()) { + + Ref ref = repo.getRefDatabase().exactRef(metaRef); + RevCommit tip = revWalk.parseCommit(ref.getObjectId()); + + CommitBuilder commit = new CommitBuilder(); + commit.setTreeId(tip.getTree()); + commit.setAuthor( + new PersonIdent("Gerrit User " + admin.id(), admin.id() + "@imported-server-id")); + commit.setCommitter(new PersonIdent("Gerrit Code Review", admin.email())); + commit.setMessage(tip.getFullMessage()); + + ObjectId commitId = inserter.insert(commit); + inserter.flush(); + + RefUpdate refUpdate = repo.updateRef(metaRef); + refUpdate.setNewObjectId(commitId); + refUpdate.forceUpdate(); + } + + // Re-index after rewriting the meta-ref so the index reflects the imported serverId, + // ensuring the virtualId in the index matches what the API will compute at delete time. + indexer.delete(changeId); + indexer.index(project, changeId); + + return change; + } + private void deleteChangeFromNoteDbWithoutUpdatingIndex(Change.Id changeId) throws Exception { try (Repository repo = repoManager.openRepository(project); TestRepository<Repository> testRepo = new TestRepository<>(repo)) { @@ -156,4 +283,8 @@ QueryOptions opts = IndexedChangeQuery.createOptions(indexConfig, 0, 1, ImmutableSet.of()); return idx.get(changeId, opts); } + + String projectAndChangeNumId(Project.NameKey project, Change.Id changeNum) { + return project + "~" + changeNum; + } }