Ensure that submittability info of change is refreshed when check is created/updated
If a user loaded a change screen in the browser and then a check was
created/updated that changed the submittability of the change, the new
submittability info was not reflected on the change screen if the user
reloaded the screen. This was because the browser remembers the ETag for
the change details and the ETag for the revision actions, and the ETag
computation didn't consider the checks of the change.
Fix this by implementing the ChangeETagComputation extension point that
allows to provide a value for the change ETag computation. Since all
checks of a change are stored in a single ref, we can just return the
SHA1 of this ref to ensure that the change ETag changes each time when a
check is created or updated. Strictly speaking this changes the change
ETag a little too often, since we need a new change ETag only if the
submittability of the change changes, which only happens when the
combined check state changes (and not all check updates affect the
combined check state). This is fine for now and may be optimized later.
The implementation of ChangeETagComputation delegates to Checks to do
the actual computation of the ETag because this computation is specific
to NoteDb and all NoteDb-specifics are hidden behind the
Checks and ChecksUpdate interfaces.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I98f3198986c0a543321d56ae15d4332691248b56
diff --git a/java/com/google/gerrit/plugins/checks/Checks.java b/java/com/google/gerrit/plugins/checks/Checks.java
index c4527e5..fa15c0e 100644
--- a/java/com/google/gerrit/plugins/checks/Checks.java
+++ b/java/com/google/gerrit/plugins/checks/Checks.java
@@ -18,6 +18,7 @@
import com.google.common.collect.ImmutableList;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.plugins.checks.api.CombinedCheckState;
+import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import java.io.IOException;
@@ -90,6 +91,16 @@
boolean areAllRequiredCheckersPassing(Project.NameKey projectName, PatchSet.Id patchSetId)
throws IOException, StorageException;
+ /**
+ * Computes an ETag for the checks of the given change.
+ *
+ * @param projectName the name of the project that contains the change
+ * @param changeId ID of the change for which the ETag should be computed
+ * @return ETag for the checks of the given change
+ * @throws IOException if failed to access the checks data
+ */
+ String getETag(Project.NameKey projectName, Change.Id changeId) throws IOException;
+
@AutoValue
abstract class GetCheckOptions {
public static GetCheckOptions defaults() {
diff --git a/java/com/google/gerrit/plugins/checks/ChecksETagComputation.java b/java/com/google/gerrit/plugins/checks/ChecksETagComputation.java
new file mode 100644
index 0000000..7dc1501
--- /dev/null
+++ b/java/com/google/gerrit/plugins/checks/ChecksETagComputation.java
@@ -0,0 +1,53 @@
+// Copyright (C) 2019 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.plugins.checks;
+
+import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.change.ChangeETagComputation;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+import java.io.IOException;
+
+/**
+ * Ensures that the ETag of a change changes when there was any check update for the change.
+ *
+ * <p>This prevents callers using ETags from potentially seeing outdated submittability and combined
+ * check state information in {@link com.google.gerrit.extensions.common.ChangeInfo} when a check
+ * for the change was created or updated.
+ */
+@Singleton
+public class ChecksETagComputation implements ChangeETagComputation {
+ private final Checks checks;
+
+ @Inject
+ ChecksETagComputation(Checks checks) {
+ this.checks = checks;
+ }
+
+ @Override
+ public String getETag(Project.NameKey projectName, Change.Id changeId) {
+ try {
+ return checks.getETag(projectName, changeId);
+ } catch (IOException e) {
+ // The change ETag will be invalidated if the checks can be accessed the next time.
+ throw new StorageException(
+ String.format(
+ "Failed to compute ETag for checks of change %s in project %s",
+ changeId, projectName));
+ }
+ }
+}
diff --git a/java/com/google/gerrit/plugins/checks/Module.java b/java/com/google/gerrit/plugins/checks/Module.java
index 8440501..140497b 100644
--- a/java/com/google/gerrit/plugins/checks/Module.java
+++ b/java/com/google/gerrit/plugins/checks/Module.java
@@ -28,6 +28,7 @@
import com.google.gerrit.plugins.checks.rules.ChecksSubmitRule;
import com.google.gerrit.server.DynamicOptions;
import com.google.gerrit.server.change.ChangeAttributeFactory;
+import com.google.gerrit.server.change.ChangeETagComputation;
import com.google.gerrit.server.git.validators.CommitValidationListener;
import com.google.gerrit.server.git.validators.MergeValidationListener;
import com.google.gerrit.server.git.validators.RefOperationValidationListener;
@@ -56,6 +57,10 @@
.to(CheckerRefOperationValidator.class)
.in(SINGLETON);
+ DynamicSet.bind(binder(), ChangeETagComputation.class)
+ .to(ChecksETagComputation.class)
+ .in(SINGLETON);
+
DynamicSet.bind(binder(), ChangeAttributeFactory.class).to(ChangeCheckAttributeFactory.class);
bind(DynamicOptions.DynamicBean.class)
.annotatedWith(Exports.named(GetChange.class))
diff --git a/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java b/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
index 0d97576..1e245fd 100644
--- a/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
+++ b/java/com/google/gerrit/plugins/checks/db/NoteDbChecks.java
@@ -26,6 +26,7 @@
import com.google.gerrit.plugins.checks.CheckKey;
import com.google.gerrit.plugins.checks.Checker;
import com.google.gerrit.plugins.checks.CheckerQuery;
+import com.google.gerrit.plugins.checks.CheckerRef;
import com.google.gerrit.plugins.checks.CheckerUuid;
import com.google.gerrit.plugins.checks.Checkers;
import com.google.gerrit.plugins.checks.Checks;
@@ -33,8 +34,10 @@
import com.google.gerrit.plugins.checks.api.CheckerStatus;
import com.google.gerrit.plugins.checks.api.CombinedCheckState;
import com.google.gerrit.plugins.checks.api.CombinedCheckState.CheckStateCount;
+import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -44,6 +47,10 @@
import java.util.Map;
import java.util.Optional;
import java.util.stream.Stream;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevWalk;
/** Class to read checks from NoteDb. */
@Singleton
@@ -53,6 +60,7 @@
private final Checkers checkers;
private final CheckBackfiller checkBackfiller;
private final Provider<CheckerQuery> checkerQueryProvider;
+ private final GitRepositoryManager repoManager;
@Inject
NoteDbChecks(
@@ -60,12 +68,14 @@
CheckNotes.Factory checkNotesFactory,
Checkers checkers,
CheckBackfiller checkBackfiller,
- Provider<CheckerQuery> checkerQueryProvider) {
+ Provider<CheckerQuery> checkerQueryProvider,
+ GitRepositoryManager repoManager) {
this.changeDataFactory = changeDataFactory;
this.checkNotesFactory = checkNotesFactory;
this.checkers = checkers;
this.checkBackfiller = checkBackfiller;
this.checkerQueryProvider = checkerQueryProvider;
+ this.repoManager = repoManager;
}
@Override
@@ -146,6 +156,15 @@
&& checkStateCount.inProgressRequiredCount() == 0;
}
+ @Override
+ public String getETag(Project.NameKey projectName, Change.Id changeId) throws IOException {
+ try (Repository repo = repoManager.openRepository(projectName);
+ RevWalk rw = new RevWalk(repo)) {
+ Ref checkRef = repo.getRefDatabase().exactRef(CheckerRef.checksRef(changeId));
+ return checkRef != null ? checkRef.getObjectId().getName() : ObjectId.zeroId().getName();
+ }
+ }
+
private ImmutableListMultimap<CheckState, Boolean> getStatesAndRequiredMap(
Project.NameKey projectName, PatchSet.Id patchSetId) throws IOException, StorageException {
ChangeData changeData = changeDataFactory.create(projectName, patchSetId.changeId());
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckIT.java
index a8a8d19..d16df11 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/CreateCheckIT.java
@@ -372,6 +372,36 @@
assertThat(response.getEntityContent()).isEqualTo("checks are not supported on a change edit");
}
+ @Test
+ public void creationOfCheckChangesETagOfChange() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+
+ String oldETag = parseChangeResource(patchSetId.changeId().toString()).getETag();
+
+ CheckInput input = new CheckInput();
+ input.checkerUuid = checkerUuid.get();
+ input.state = CheckState.RUNNING;
+ checksApiFactory.revision(patchSetId).create(input).get();
+
+ String newETag = parseChangeResource(patchSetId.changeId().toString()).getETag();
+ assertThat(newETag).isNotEqualTo(oldETag);
+ }
+
+ @Test
+ public void creationOfCheckChangesETagOfRevisionActions() throws Exception {
+ CheckerUuid checkerUuid = checkerOperations.newChecker().repository(project).create();
+
+ String oldETag = gApi.changes().id(patchSetId.changeId().toString()).current().etag();
+
+ CheckInput input = new CheckInput();
+ input.checkerUuid = checkerUuid.get();
+ input.state = CheckState.RUNNING;
+ checksApiFactory.revision(patchSetId).create(input).get();
+
+ String newETag = gApi.changes().id(patchSetId.changeId().toString()).current().etag();
+ assertThat(newETag).isNotEqualTo(oldETag);
+ }
+
// TODO(gerrit-team) More tests, especially for multiple checkers and PS and how commits behave
private Check getCheck(Project.NameKey project, PatchSet.Id patchSetId, CheckerUuid checkerUuid)
diff --git a/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckIT.java b/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckIT.java
index 6d15caa..6781a6f 100644
--- a/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckIT.java
+++ b/javatests/com/google/gerrit/plugins/checks/acceptance/api/UpdateCheckIT.java
@@ -360,4 +360,56 @@
assertThat(info.message).isEqualTo("some message");
assertThat(info.url).isEqualTo("https://www.example.com");
}
+
+ @Test
+ public void updateOfCheckChangesETagOfChange() throws Exception {
+ String oldETag = parseChangeResource(patchSetId.changeId().toString()).getETag();
+
+ CheckInput input = new CheckInput();
+ input.state = CheckState.FAILED;
+ checksApiFactory.revision(patchSetId).id(checkKey.checkerUuid()).update(input);
+
+ String newETag = parseChangeResource(patchSetId.changeId().toString()).getETag();
+ assertThat(newETag).isNotEqualTo(oldETag);
+ }
+
+ @Test
+ public void noOpUpdateOfCheckDoesNotChangeETagOfChange() throws Exception {
+ CheckInput input = new CheckInput();
+ input.state = CheckState.FAILED;
+ checksApiFactory.revision(patchSetId).id(checkKey.checkerUuid()).update(input);
+
+ String oldETag = parseChangeResource(patchSetId.changeId().toString()).getETag();
+
+ checksApiFactory.revision(patchSetId).id(checkKey.checkerUuid()).update(input);
+
+ String newETag = parseChangeResource(patchSetId.changeId().toString()).getETag();
+ assertThat(newETag).isEqualTo(oldETag);
+ }
+
+ @Test
+ public void updateOfCheckChangesETagOfRevisionActions() throws Exception {
+ String oldETag = gApi.changes().id(patchSetId.changeId().toString()).current().etag();
+
+ CheckInput input = new CheckInput();
+ input.state = CheckState.FAILED;
+ checksApiFactory.revision(patchSetId).id(checkKey.checkerUuid()).update(input);
+
+ String newETag = gApi.changes().id(patchSetId.changeId().toString()).current().etag();
+ assertThat(newETag).isNotEqualTo(oldETag);
+ }
+
+ @Test
+ public void noOpUpdateOfCheckDoesNotChangeETagOfRevisionActions() throws Exception {
+ CheckInput input = new CheckInput();
+ input.state = CheckState.FAILED;
+ checksApiFactory.revision(patchSetId).id(checkKey.checkerUuid()).update(input);
+
+ String oldETag = gApi.changes().id(patchSetId.changeId().toString()).current().etag();
+
+ checksApiFactory.revision(patchSetId).id(checkKey.checkerUuid()).update(input);
+
+ String newETag = gApi.changes().id(patchSetId.changeId().toString()).current().etag();
+ assertThat(newETag).isEqualTo(oldETag);
+ }
}
