Merge changes I75b79dc0,I489703dc,I87c6f803,Ib07315e2,I48ea2d9b
* changes:
Add performance log for loading robot comments
ChangeIT.changeDetailsDoesNotRequireIndex: Add comment about error in log
GetChange: Record performance event for loading the meta revision
GetChange: Avoid double loading of change notes when meta rev ID is not given
GetChange: Return Optional from getMetaRevId rather than possibly null
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java
index a7da7ad..233998d 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -54,6 +54,9 @@
import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.ReviewerStatusUpdate;
import com.google.gerrit.server.git.RefCache;
+import com.google.gerrit.server.logging.Metadata;
+import com.google.gerrit.server.logging.TraceContext;
+import com.google.gerrit.server.logging.TraceContext.TraceTimer;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.query.change.ChangeData;
@@ -595,8 +598,12 @@
private void loadRobotComments() {
if (robotCommentNotes == null) {
- robotCommentNotes = new RobotCommentNotes(args, change);
- robotCommentNotes.load();
+ try (TraceTimer timer =
+ TraceContext.newTimer(
+ "Load Robot Comments", Metadata.builder().changeId(change.getId().get()).build())) {
+ robotCommentNotes = new RobotCommentNotes(args, change);
+ robotCommentNotes.load();
+ }
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/GetChange.java b/java/com/google/gerrit/server/restapi/change/GetChange.java
index d126d8a..c1a8f45 100644
--- a/java/com/google/gerrit/server/restapi/change/GetChange.java
+++ b/java/com/google/gerrit/server/restapi/change/GetChange.java
@@ -37,6 +37,9 @@
import com.google.gerrit.server.change.PluginDefinedAttributesFactories;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.logging.Metadata;
+import com.google.gerrit.server.logging.TraceContext;
+import com.google.gerrit.server.logging.TraceContext.TraceTimer;
import com.google.gerrit.server.notedb.MissingMetaObjectException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
@@ -45,6 +48,7 @@
import java.util.EnumSet;
import java.util.HashMap;
import java.util.Map;
+import java.util.Optional;
import org.eclipse.jgit.errors.InvalidObjectIdException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
@@ -104,9 +108,14 @@
@Override
public Response<ChangeInfo> apply(ChangeResource rsrc) throws RestApiException {
try {
- Change change = rsrc.getChange();
- ObjectId changeMetaRevId = getMetaRevId(change);
- return Response.withMustRevalidate(newChangeJson().format(change, changeMetaRevId));
+ Optional<ObjectId> changeMetaRevId = getMetaRevId(rsrc.getChange());
+ ChangeInfo changeInfo;
+ if (changeMetaRevId.isPresent()) {
+ changeInfo = newChangeJson().format(rsrc.getChange(), changeMetaRevId.get());
+ } else {
+ changeInfo = newChangeJson().format(rsrc.getChangeData());
+ }
+ return Response.withMustRevalidate(changeInfo);
} catch (MissingMetaObjectException e) {
throw new PreconditionFailedException(e.getMessage());
}
@@ -116,22 +125,25 @@
return Response.withMustRevalidate(newChangeJson().format(rsrc));
}
- @Nullable
- private ObjectId getMetaRevId(Change change) throws RestApiException {
+ private Optional<ObjectId> getMetaRevId(Change change) throws RestApiException {
if (metaRevId.isEmpty()) {
- return null;
+ return Optional.empty();
}
- // 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.
- ObjectId metaRevObjectId;
- try {
- metaRevObjectId = ObjectId.fromString(metaRevId);
- } catch (InvalidObjectIdException e) {
- throw new BadRequestException("invalid meta SHA1: " + metaRevId, e);
+ try (TraceTimer timer =
+ TraceContext.newTimer(
+ "Get meta rev ID", Metadata.builder().changeId(change.getId().get()).build())) {
+ // 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.
+ ObjectId metaRevObjectId;
+ try {
+ metaRevObjectId = ObjectId.fromString(metaRevId);
+ } catch (InvalidObjectIdException e) {
+ throw new BadRequestException("invalid meta SHA1: " + metaRevId, e);
+ }
+ return verifyMetaId(change, metaRevObjectId);
}
- return verifyMetaId(change, metaRevObjectId);
}
private ChangeJson newChangeJson() {
@@ -144,10 +156,10 @@
cds, this, Streams.stream(pdiFactories.entries()));
}
- @Nullable
- private ObjectId verifyMetaId(Change change, @Nullable ObjectId id) throws RestApiException {
+ private Optional<ObjectId> verifyMetaId(Change change, @Nullable ObjectId id)
+ throws RestApiException {
if (id == null) {
- return null;
+ return Optional.empty();
}
String changeMetaRefName = RefNames.changeMetaRef(change.getId());
@@ -159,7 +171,7 @@
rw.markStart(tip);
for (RevCommit rev : rw) {
if (id.equals(rev)) {
- return id;
+ return Optional.of(id);
}
}
} catch (IOException e) {
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 84a7618..6d38e69 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -4676,6 +4676,11 @@
gApi.changes().id(change.getChangeId()).addReviewer(user.email());
int number = gApi.changes().id(change.getChangeId()).get()._number;
+
+ // Note: Computing the description of some UI actions does access the index. If the index is
+ // disabled computing these descriptions fails. UiActions#describe catches and ignores these
+ // exceptions so that the request is still successful. In this case the description of the UI
+ // action is omitted in the response.
try (AutoCloseable ignored = changeIndexOperations.disableReadsAndWrites()) {
assertThat(gApi.changes().id(project.get(), number).get(options).changeId)
.isEqualTo(change.getChangeId());
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/GetMetaDiffIT.java b/javatests/com/google/gerrit/acceptance/rest/change/GetMetaDiffIT.java
index 26e37f4..ace7b52 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/GetMetaDiffIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/GetMetaDiffIT.java
@@ -42,15 +42,15 @@
@Test
public void metaDiff() throws Exception {
- PushOneCommit.Result ch = createChange();
- ChangeApi chApi = gApi.changes().id(ch.getChangeId());
- chApi.topic(TOPIC);
- ChangeInfo oldInfo = chApi.get();
- chApi.topic(TOPIC + "-2");
- chApi.setHashtags(new HashtagsInput(ImmutableSet.of(HASHTAG)));
- ChangeInfo newInfo = chApi.get();
+ String changeId = createChange().getChangeId();
+ gApi.changes().id(changeId).topic(TOPIC);
+ ChangeInfo oldInfo = gApi.changes().id(changeId).get();
+ gApi.changes().id(changeId).topic(TOPIC + "-2");
+ gApi.changes().id(changeId).setHashtags(new HashtagsInput(ImmutableSet.of(HASHTAG)));
+ ChangeInfo newInfo = gApi.changes().id(changeId).get();
- ChangeInfoDifference difference = chApi.metaDiff(oldInfo.metaRevId, newInfo.metaRevId);
+ ChangeInfoDifference difference =
+ gApi.changes().id(changeId).metaDiff(oldInfo.metaRevId, newInfo.metaRevId);
assertThat(difference.added().topic).isEqualTo(newInfo.topic);
assertThat(difference.added().hashtags).isNotNull();
@@ -161,13 +161,12 @@
@Test
public void metaDiffNoOldMetaGivenUsesPatchSetBeforeNew() throws Exception {
- PushOneCommit.Result ch = createChange();
- ChangeApi chApi = gApi.changes().id(ch.getChangeId());
- chApi.topic(TOPIC);
- ChangeInfo newInfo = chApi.get();
- chApi.topic(TOPIC + "2");
+ String changeId = createChange().getChangeId();
+ gApi.changes().id(changeId).topic(TOPIC);
+ ChangeInfo newInfo = gApi.changes().id(changeId).get();
+ gApi.changes().id(changeId).topic(TOPIC + "2");
- ChangeInfoDifference difference = chApi.metaDiff(null, newInfo.metaRevId);
+ ChangeInfoDifference difference = gApi.changes().id(changeId).metaDiff(null, newInfo.metaRevId);
assertThat(difference.added().topic).isEqualTo(TOPIC);
assertThat(difference.removed().topic).isNull();
@@ -202,17 +201,18 @@
@Test
public void metaDiffWithOptionIncludesExtraInformation() throws Exception {
- PushOneCommit.Result ch = createChange();
- ChangeApi chApi = gApi.changes().id(ch.getChangeId());
- ChangeInfo oldInfo = chApi.get(ListChangesOption.CURRENT_REVISION);
- amendChange(ch.getChangeId());
- ChangeInfo newInfo = chApi.get(ListChangesOption.CURRENT_REVISION);
+ String changeId = createChange().getChangeId();
+ ChangeInfo oldInfo = gApi.changes().id(changeId).get(ListChangesOption.CURRENT_REVISION);
+ amendChange(changeId);
+ ChangeInfo newInfo = gApi.changes().id(changeId).get(ListChangesOption.CURRENT_REVISION);
ChangeInfoDifference difference =
- chApi.metaDiff(
- oldInfo.metaRevId,
- newInfo.metaRevId,
- ImmutableSet.of(ListChangesOption.CURRENT_REVISION));
+ gApi.changes()
+ .id(changeId)
+ .metaDiff(
+ oldInfo.metaRevId,
+ newInfo.metaRevId,
+ ImmutableSet.of(ListChangesOption.CURRENT_REVISION));
assertThat(newInfo.currentRevision).isNotNull();
assertThat(oldInfo.currentRevision).isNotNull();
