GetChange: provide meta=SHA1 option
This change lets callers inspect a change at a given NoteDb commit.
$ curl -u admin:secret http://localhost:8080/a/changes/101/'?pp=1&O=22'
..
"current_revision": "f3cccef96155ed3f9de2aaed44d8cf00960a21ca",
"revisions": {
"f3cccef96155ed3f9de2aaed44d8cf00960a21ca": {
"kind": "REWORK",
"_number": 5,
$ curl -u admin:secret http://localhost:8080/a/changes/101/'?pp=1&O=22&meta=8ebdac0e8a2047ee2d2433a09f05de230d3050f2'
..
"current_revision": "e24af8d95295181064078a99a3bde31cf9b643a2",
"revisions": {
"e24af8d95295181064078a99a3bde31cf9b643a2": {
"kind": "NO_CODE_CHANGE",
"_number": 3,
This is part of an alternate approach to event notifications, where
notification is split into two parts:
1) notifying a client that *something* changed
2) letting the client discover precisely *what* changed.
This change provides a way for clients to get 2): A subscriber, when
receiving a notification, can use /changes/ID?meta=XXX to get the old
and new states of the ChangeInfo. By analyzing the difference between
both ChangeInfos, subscribers can tell what happened.
As a follow-up, we plan to provide a diffing functionality as a
built-in REST endpoint.
If the server hasn't seen the NoteDb meta SHA1 yet, it replies with
412. This is useful in a distributed setting: notifications can be
delivered ahead of the underlying Git data. If this happens, this also
results in a 412 error code. The client can respond by waiting and
retrying.
For 1), the minimal amount of information required is a list of opaque
identifiers (change number, old meta SHA1, new meta SHA1).
The opaque identifiers could be discovered through several means
* By finding recently modified changes, through a query such as
"age:1h status:open", and comparing the meta SHA1s with output of an
earlier query
* By executing ls-remote, and comparing its output with an earlier
run.
* By hooking into ref updates at the storage level, and exposing the
result over SSH. For vanilla JGit, this will need JGit changes,
because RefChangedEvent in JGit doesn't provide per-Ref data.
At Google, the plan is to generate these notifications at the storage
level in the global ref database.
This approach to notifications has several advantages over the
existing stream-events mechanism:
* The event notification only contains opaque data, and can be sent to
any subscriber without leaking data. The GetChange endpoint enforces
visibility.
* It uses the same endpoint as the UI, so there is no chance of new
features being forgotten for stream-events (for example, stream-events
does not support attention set updates).
* It doesn't rely on having a stream. Without streaming, the
change index (query "status:open age:INTERVAL") or the output of ls-remote
can still provide data on for polling clients.
* A subscriber can protect against missed notifications, by keeping
track of the meta SHA1s locally. If the connection breaks, they can
resynchronize using the mechanism for polling clients.
Change-Id: Ia54acb37e1187bc64e190d5694de0b17674595f8
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 3b9320c..3a5fb96 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -564,6 +564,12 @@
}
----
+Historical state of the change can be retrieved by specifying the
+`meta=SHA1` parameter. This will use a historical NoteDb snapshot to
+populate ChangeInfo. If the SHA1 is not reachable as a NoteDb state,
+status code 412 is returned.
+
+
[[get-change-detail]]
=== Get Change Detail
--
diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java
index a461b59..c7aedb6 100644
--- a/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/java/com/google/gerrit/server/change/ChangeJson.java
@@ -47,6 +47,7 @@
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Address;
import com.google.gerrit.entities.Change;
@@ -121,6 +122,7 @@
import java.util.Set;
import java.util.stream.Collectors;
import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.ObjectId;
/**
* Produces {@link ChangeInfo} (which is serialized to JSON afterwards) from {@link ChangeData}.
@@ -283,6 +285,11 @@
return format(changeDataFactory.create(change));
}
+ public ChangeInfo format(Change change, @Nullable ObjectId metaRevId) {
+ ChangeNotes notes = notesFactory.createChecked(change.getProject(), change.getId(), metaRevId);
+ return format(changeDataFactory.create(notes));
+ }
+
public ChangeInfo format(ChangeData cd) {
return format(cd, Optional.empty(), true, getPluginInfos(cd));
}
@@ -325,9 +332,13 @@
}
public ChangeInfo format(Project.NameKey project, Change.Id id) {
+ return format(project, id, null);
+ }
+
+ public ChangeInfo format(Project.NameKey project, Change.Id id, @Nullable ObjectId metaRevId) {
ChangeNotes notes;
try {
- notes = notesFactory.createChecked(project, id);
+ notes = notesFactory.createChecked(project, id, metaRevId);
} catch (StorageException e) {
if (!has(CHECK)) {
throw e;
diff --git a/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java b/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
index 9b5b4d4..a7c7757 100644
--- a/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
@@ -37,6 +37,8 @@
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
/** View of contents at a single ref related to some change. * */
public abstract class AbstractChangeNotes<T> {
@@ -118,9 +120,14 @@
private ObjectId revision;
private boolean loaded;
- protected AbstractChangeNotes(Args args, Change.Id changeId) {
+ protected AbstractChangeNotes(Args args, Change.Id changeId, @Nullable ObjectId metaSha1) {
this.args = requireNonNull(args);
this.changeId = requireNonNull(changeId);
+ this.revision = metaSha1;
+ }
+
+ protected AbstractChangeNotes(Args args, Change.Id changeId) {
+ this(args, changeId, null);
}
public Change.Id getChangeId() {
@@ -144,7 +151,7 @@
Repository repo = args.repoManager.openRepository(getProjectName());
// Call openHandle even if reading is disabled, to trigger
// auto-rebuilding before this object may get passed to a ChangeUpdate.
- LoadHandle handle = openHandle(repo)) {
+ LoadHandle handle = openHandle(repo, revision)) {
revision = handle.id();
onLoad(handle);
loaded = true;
@@ -168,13 +175,17 @@
* @param repo open repository.
* @return handle for reading the entity.
* @throws NoSuchChangeException change does not exist.
+ * @throws MissingMetaObjectException specified SHA1 isn't reachable from meta branch.
* @throws IOException a repo-level error occurred.
*/
- protected LoadHandle openHandle(Repository repo) throws NoSuchChangeException, IOException {
- return openHandle(repo, readRef(repo));
- }
+ protected LoadHandle openHandle(Repository repo, @Nullable ObjectId id)
+ throws NoSuchChangeException, IOException, MissingMetaObjectException {
+ if (id == null) {
+ id = readRef(repo);
+ } else {
+ verifyMetaId(repo, id);
+ }
- protected LoadHandle openHandle(Repository repo, ObjectId id) {
return new LoadHandle(repo, id);
}
@@ -215,4 +226,20 @@
protected final T self() {
return (T) this;
}
+
+ private void verifyMetaId(Repository repo, ObjectId id)
+ throws IOException, MissingMetaObjectException {
+ try (RevWalk rw = new RevWalk(repo)) {
+ Ref ref = repo.getRefDatabase().exactRef(getRefName());
+ RevCommit tip = rw.parseCommit(ref.getObjectId());
+ rw.markStart(tip);
+ for (RevCommit rev : rw) {
+ if (id.equals(rev)) {
+ return;
+ }
+ }
+ }
+
+ throw new MissingMetaObjectException(id.getName() + " not reachable from " + getRefName());
+ }
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java
index cf09ff3..77d7bc0 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -114,9 +114,14 @@
return createChecked(c.getProject(), c.getId());
}
- public ChangeNotes createChecked(Project.NameKey project, Change.Id changeId) {
+ public ChangeNotes createChecked(
+ Project.NameKey project, Change.Id changeId, @Nullable ObjectId metaRevId) {
Change change = newChange(project, changeId);
- return new ChangeNotes(args, change, true, null).load();
+ return new ChangeNotes(args, change, true, null, metaRevId).load();
+ }
+
+ public ChangeNotes createChecked(Project.NameKey project, Change.Id changeId) {
+ return createChecked(project, changeId, null);
}
public static Change newChange(Project.NameKey project, Change.Id changeId) {
@@ -333,14 +338,23 @@
private ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals;
private ImmutableSet<Comment.Key> commentKeys;
- @VisibleForTesting
- public ChangeNotes(Args args, Change change, boolean shouldExist, @Nullable RefCache refs) {
- super(args, change.getId());
+ public ChangeNotes(
+ Args args,
+ Change change,
+ boolean shouldExist,
+ @Nullable RefCache refs,
+ @Nullable ObjectId metaSha1) {
+ super(args, change.getId(), metaSha1);
this.change = new Change(change);
this.shouldExist = shouldExist;
this.refs = refs;
}
+ @VisibleForTesting
+ public ChangeNotes(Args args, Change change, boolean shouldExist, @Nullable RefCache refs) {
+ this(args, change, shouldExist, refs, null);
+ }
+
public Change getChange() {
return change;
}
diff --git a/java/com/google/gerrit/server/notedb/MissingMetaObjectException.java b/java/com/google/gerrit/server/notedb/MissingMetaObjectException.java
new file mode 100644
index 0000000..ffe9fa8
--- /dev/null
+++ b/java/com/google/gerrit/server/notedb/MissingMetaObjectException.java
@@ -0,0 +1,22 @@
+// Copyright (C) 2021 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.notedb;
+
+/** Separate exception type to throw if requested meta SHA1 is not available. */
+public class MissingMetaObjectException extends RuntimeException {
+ MissingMetaObjectException(String msg) {
+ super(msg);
+ }
+}
diff --git a/java/com/google/gerrit/server/restapi/change/GetChange.java b/java/com/google/gerrit/server/restapi/change/GetChange.java
index f28d2b3..2f1c61e 100644
--- a/java/com/google/gerrit/server/restapi/change/GetChange.java
+++ b/java/com/google/gerrit/server/restapi/change/GetChange.java
@@ -16,12 +16,15 @@
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.Streams;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Change;
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.client.ListOption;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.PluginDefinedInfo;
import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.PreconditionFailedException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.server.DynamicOptions;
@@ -31,12 +34,15 @@
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.PluginDefinedAttributesFactories;
import com.google.gerrit.server.change.RevisionResource;
+import com.google.gerrit.server.notedb.MissingMetaObjectException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
import java.util.Collection;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.Map;
+import org.eclipse.jgit.errors.InvalidObjectIdException;
+import org.eclipse.jgit.lib.ObjectId;
import org.kohsuke.args4j.Option;
public class GetChange
@@ -53,6 +59,9 @@
options.add(o);
}
+ @Option(name = "--meta", usage = "NoteDb meta SHA1")
+ String metaRevId = "";
+
@Option(name = "-O", usage = "Output option flags, in hex")
void setOptionFlagsHex(String hex) {
options.addAll(ListOption.fromBits(ListChangesOption.class, Integer.parseInt(hex, 16)));
@@ -75,14 +84,35 @@
}
@Override
- public Response<ChangeInfo> apply(ChangeResource rsrc) {
- return Response.withMustRevalidate(newChangeJson().format(rsrc));
+ public Response<ChangeInfo> apply(ChangeResource rsrc)
+ throws BadRequestException, PreconditionFailedException {
+ try {
+ return Response.withMustRevalidate(newChangeJson().format(rsrc.getChange(), getMetaRevId()));
+ } catch (MissingMetaObjectException e) {
+ throw new PreconditionFailedException(e.getMessage());
+ }
}
Response<ChangeInfo> apply(RevisionResource rsrc) {
return Response.withMustRevalidate(newChangeJson().format(rsrc));
}
+ @Nullable
+ private ObjectId getMetaRevId() throws BadRequestException {
+ if (metaRevId.isEmpty()) {
+ return null;
+ }
+
+ // It might be interesting to also allow {SHA1}^^, so callers can walk back into history
+ // without having to fetch the entire /meta ref. If we do so, we have to be careful that
+ // the error messages can't be abused to fetch hidden data.
+ try {
+ return ObjectId.fromString(metaRevId);
+ } catch (InvalidObjectIdException e) {
+ throw new BadRequestException("invalid meta SHA1: " + metaRevId, e);
+ }
+ }
+
private ChangeJson newChangeJson() {
return json.create(options, this::createPluginDefinedInfos);
}
diff --git a/java/com/google/gerrit/server/restapi/change/GetDetail.java b/java/com/google/gerrit/server/restapi/change/GetDetail.java
index e31d84b..15362d5 100644
--- a/java/com/google/gerrit/server/restapi/change/GetDetail.java
+++ b/java/com/google/gerrit/server/restapi/change/GetDetail.java
@@ -16,6 +16,8 @@
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.PreconditionFailedException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.server.DynamicOptions;
@@ -58,7 +60,8 @@
}
@Override
- public Response<ChangeInfo> apply(ChangeResource rsrc) {
+ public Response<ChangeInfo> apply(ChangeResource rsrc)
+ throws BadRequestException, PreconditionFailedException {
return delegate.apply(rsrc);
}
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ChangeMetaIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ChangeMetaIT.java
index 3787a35..e025c52 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/ChangeMetaIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/ChangeMetaIT.java
@@ -20,8 +20,19 @@
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.entities.Change;
import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.PluginDefinedInfo;
+import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.server.change.ChangePluginDefinedInfoFactory;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gson.reflect.TypeToken;
+import com.google.gson.stream.JsonReader;
+import com.google.inject.AbstractModule;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
import org.eclipse.jgit.lib.Repository;
import org.junit.Test;
@@ -55,4 +66,85 @@
repo.exactRef(changeMetaRef(Change.id(before._number))).getObjectId().getName());
}
}
+
+ @Test
+ public void ChangeInfo_metaSha1_parameter() throws Exception {
+ PushOneCommit.Result result = createChange();
+ String changeId = result.getChangeId();
+ gApi.changes().id(changeId).setMessage("before\n\n" + "Change-Id: " + result.getChangeId());
+ ChangeInfo before = gApi.changes().id(changeId).get();
+ gApi.changes().id(changeId).setMessage("after\n\n" + "Change-Id: " + result.getChangeId());
+ ChangeInfo after = gApi.changes().id(changeId).get();
+ assertThat(after.metaRevId).isNotEqualTo(before.metaRevId);
+
+ RestResponse resp = adminRestSession.get("/changes/" + changeId + "/?meta=" + before.metaRevId);
+ resp.assertOK();
+
+ ChangeInfo got;
+ try (JsonReader jsonReader = new JsonReader(resp.getReader())) {
+ jsonReader.setLenient(true);
+ got = newGson().fromJson(jsonReader, ChangeInfo.class);
+ }
+ assertThat(got.subject).isEqualTo(before.subject);
+ }
+
+ @Test
+ public void metaUnreachableSha1() throws Exception {
+ PushOneCommit.Result ch1 = createChange();
+ PushOneCommit.Result ch2 = createChange();
+
+ ChangeInfo info2 = gApi.changes().id(ch2.getChangeId()).get();
+
+ RestResponse resp =
+ adminRestSession.get("/changes/" + ch1.getChangeId() + "/?meta=" + info2.metaRevId);
+
+ resp.assertStatus(412);
+ }
+
+ protected static class PluginDefinedSimpleAttributeModule extends AbstractModule {
+ static class MyMetaHash extends PluginDefinedInfo {
+ String myMetaRef;
+ };
+
+ static PluginDefinedInfo newMyMetaHash(ChangeData cd) {
+ MyMetaHash mmh = new MyMetaHash();
+ mmh.myMetaRef = cd.notes().getMetaId().name();
+ return mmh;
+ }
+
+ @Override
+ public void configure() {
+ DynamicSet.bind(binder(), ChangePluginDefinedInfoFactory.class)
+ .toInstance(
+ (cds, bp, p) -> {
+ Map<Change.Id, PluginDefinedInfo> out = new HashMap<>();
+ cds.forEach(cd -> out.put(cd.getId(), newMyMetaHash(cd)));
+ return out;
+ });
+ }
+ }
+
+ @Test
+ public void pluginDefinedAttribute() throws Exception {
+ try (AutoCloseable ignored =
+ installPlugin("my-plugin", PluginDefinedSimpleAttributeModule.class)) {
+ PushOneCommit.Result result = createChange();
+ String changeId = result.getChangeId();
+ gApi.changes().id(changeId).setMessage("before\n\n" + "Change-Id: " + result.getChangeId());
+ ChangeInfo before = gApi.changes().id(changeId).get();
+ gApi.changes().id(changeId).setMessage("after\n\n" + "Change-Id: " + result.getChangeId());
+ ChangeInfo after = gApi.changes().id(changeId).get();
+
+ RestResponse resp =
+ adminRestSession.get("/changes/" + changeId + "/?meta=" + before.metaRevId);
+ resp.assertOK();
+
+ Map<String, Object> changeInfo =
+ newGson().fromJson(resp.getReader(), new TypeToken<Map<String, Object>>() {}.getType());
+ List<Object> plugins = (List<Object>) changeInfo.get("plugins");
+ Map<String, Object> myplugin = (Map<String, Object>) plugins.get(0);
+
+ assertThat(myplugin.get("my_meta_ref")).isEqualTo(before.metaRevId);
+ }
+ }
}
