Return X-Gerrit-UpdatedRef in the response headers of WRITE requests
For each REST-API request, we can track which refs were changed via
GitReferenceUpdatedListener which is already used and tested by the
plugins, caches, and indices.
Using the information of which refs were changed, for each request that
changed any of the refs, we return the changed refs in the response
header of `X-Gerrit-UpdatedRef` in the format:
REPONAME~REFNAME~OLD_SHA-1~NEW_SHA-1.
This is useful because:
1. Successful operations on changes are signaled by an update on the
meta ref (and similarly for other operations such as updating accounts
signaled by update to the account ref).
2. Returning the SHA-1s generically in the response headers can reduce
load on Gerrit: users will not have to call Gerrit to get this
information since Gerrit already always returns it. For example, some
users need the new SHA-1 of the destination branch, which we don't
return on submission, and right now they are forced to call Gerrit again
after submission to retrieve the SHA-1.
3. Because of (2), for multi-site setups, we are not impacted by
replication lags that could be caused by the users calling Gerrit more
times than they really need.
While at it, we change WebSession to be an abstract class instead of an
interface to have ref updates as part of the class.
Change-Id: Iaac3bf1036408c7f0b2d48494b544032c76c5462
diff --git a/Documentation/rest-api.txt b/Documentation/rest-api.txt
index eabcaa9..8709843 100644
--- a/Documentation/rest-api.txt
+++ b/Documentation/rest-api.txt
@@ -244,6 +244,38 @@
Given the trace ID an administrator can find the corresponding logs and
investigate issues more easily.
+[[updated-refs]]
+=== X-Gerrit-UpdatedRef
+For each write REST request, we return X-Gerrit-UpdatedRef headers as the refs
+that were updated in the current request (involved in a ref transaction in the
+current request).
+
+The format of those headers is `PROJECT_NAME~REF_NAME~OLD_SHA-1~NEW_SHA-1`. The
+project and ref names are URL-encoded, and must use %7E for '~'.
+
+A new SHA-1 of `0000000000000000000000000000000000000000` is treated as a
+deleted ref.
+If the new SHA-1 is not `0000000000000000000000000000000000000000`, the ref was
+either updated or created.
+If the old SHA-1 is `0000000000000000000000000000000000000000`, the ref was
+created.
+
+.Example Request
+----
+ DELETE /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940
+----
+
+.Example Response
+----
+HTTP/1.1 204 NO CONTENT
+ Content-Disposition: attachment
+ Content-Type: application/json; charset=UTF-8
+ X-Gerrit-UpdatedRef: myProject~refs%2Fchanges%2F01%2F1%2F1~deadbeefdeadbeefdeadbeefdeadbeefdeadbeef~0000000000000000000000000000000000000000
+ X-Gerrit-UpdatedRef: myProject~refs%2Fchanges%2F01%2F1%2Fmeta~deadbeefdeadbeefdeadbeefdeadbeefdeadbeef~0000000000000000000000000000000000000000
+
+ )]}'
+----
+
GERRIT
------
Part of link:index.html[Gerrit Code Review]
diff --git a/java/com/google/gerrit/httpd/CacheBasedWebSession.java b/java/com/google/gerrit/httpd/CacheBasedWebSession.java
index 7212e3e..0a54448 100644
--- a/java/com/google/gerrit/httpd/CacheBasedWebSession.java
+++ b/java/com/google/gerrit/httpd/CacheBasedWebSession.java
@@ -42,7 +42,7 @@
import org.eclipse.jgit.http.server.GitSmartHttpTools;
@RequestScoped
-public abstract class CacheBasedWebSession implements WebSession {
+public abstract class CacheBasedWebSession extends WebSession {
@VisibleForTesting public static final String ACCOUNT_COOKIE = "GerritAccount";
protected static final long MAX_AGE_MINUTES = HOURS.toMinutes(12);
diff --git a/java/com/google/gerrit/httpd/GitReferenceUpdatedTracker.java b/java/com/google/gerrit/httpd/GitReferenceUpdatedTracker.java
new file mode 100644
index 0000000..c37d30b
--- /dev/null
+++ b/java/com/google/gerrit/httpd/GitReferenceUpdatedTracker.java
@@ -0,0 +1,43 @@
+// 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.httpd;
+
+import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
+import com.google.gerrit.extensions.registration.DynamicItem;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+
+/**
+ * Stores the updated refs whenever they are updated, so that we can export this information in the
+ * response headers.
+ */
+@Singleton
+public class GitReferenceUpdatedTracker implements GitReferenceUpdatedListener {
+
+ private final DynamicItem<WebSession> webSession;
+
+ @Inject
+ GitReferenceUpdatedTracker(DynamicItem<WebSession> webSession) {
+ this.webSession = webSession;
+ }
+
+ @Override
+ public void onGitReferenceUpdated(GitReferenceUpdatedListener.Event event) {
+ WebSession currentSession = webSession.get();
+ if (currentSession != null) {
+ currentSession.addRefUpdatedEvents(event);
+ }
+ }
+}
diff --git a/java/com/google/gerrit/httpd/HttpdModule.java b/java/com/google/gerrit/httpd/HttpdModule.java
new file mode 100644
index 0000000..1f1ec2f
--- /dev/null
+++ b/java/com/google/gerrit/httpd/HttpdModule.java
@@ -0,0 +1,14 @@
+package com.google.gerrit.httpd;
+
+import com.google.gerrit.extensions.annotations.Exports;
+import com.google.gerrit.extensions.config.FactoryModule;
+import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
+
+public class HttpdModule extends FactoryModule {
+ @Override
+ protected void configure() {
+ bind(GitReferenceUpdatedListener.class)
+ .annotatedWith(Exports.named(GitReferenceUpdatedTracker.class.getSimpleName()))
+ .to(GitReferenceUpdatedTracker.class);
+ }
+}
diff --git a/java/com/google/gerrit/httpd/WebSession.java b/java/com/google/gerrit/httpd/WebSession.java
index daf30ff..df8402e 100644
--- a/java/com/google/gerrit/httpd/WebSession.java
+++ b/java/com/google/gerrit/httpd/WebSession.java
@@ -16,30 +16,61 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
+import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
import com.google.gerrit.server.AccessPath;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.AuthResult;
+import com.google.inject.servlet.RequestScoped;
+import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
-public interface WebSession {
- boolean isSignedIn();
+/**
+ * A thread safe class that contains details about a specific user web session.
+ *
+ * <p>WARNING: All implementors must have {@link RequestScoped} annotation to maintain thread
+ * safety.
+ */
+public abstract class WebSession {
+ public abstract boolean isSignedIn();
@Nullable
- String getXGerritAuth();
+ public abstract String getXGerritAuth();
- boolean isValidXGerritAuth(String keyIn);
+ public abstract boolean isValidXGerritAuth(String keyIn);
- CurrentUser getUser();
+ public abstract CurrentUser getUser();
- void login(AuthResult res, boolean rememberMe);
+ public abstract void login(AuthResult res, boolean rememberMe);
/** Set the user account for this current request only. */
- void setUserAccountId(Account.Id id);
+ public abstract void setUserAccountId(Account.Id id);
- boolean isAccessPathOk(AccessPath path);
+ public abstract boolean isAccessPathOk(AccessPath path);
- void setAccessPathOk(AccessPath path, boolean ok);
+ public abstract void setAccessPathOk(AccessPath path, boolean ok);
- void logout();
+ public abstract void logout();
- String getSessionId();
+ public abstract String getSessionId();
+
+ /**
+ * Store and return the ref updates in this session. This class is {@link RequestScoped}, hence
+ * this is thread safe.
+ *
+ * <p>The same session could perform separate requests one after another, so resetting the ref
+ * updates is necessary between requests.
+ */
+ private List<GitReferenceUpdatedListener.Event> refUpdatedEvents = new CopyOnWriteArrayList<>();
+
+ public List<GitReferenceUpdatedListener.Event> getRefUpdatedEvents() {
+ return refUpdatedEvents;
+ }
+
+ public void addRefUpdatedEvents(GitReferenceUpdatedListener.Event event) {
+ refUpdatedEvents.add(event);
+ }
+
+ public void resetRefUpdatedEvents() {
+ refUpdatedEvents.clear();
+ }
}
diff --git a/java/com/google/gerrit/httpd/init/WebAppInitializer.java b/java/com/google/gerrit/httpd/init/WebAppInitializer.java
index 3a77a8a..079f306 100644
--- a/java/com/google/gerrit/httpd/init/WebAppInitializer.java
+++ b/java/com/google/gerrit/httpd/init/WebAppInitializer.java
@@ -28,6 +28,7 @@
import com.google.gerrit.httpd.GitOverHttpModule;
import com.google.gerrit.httpd.H2CacheBasedWebSession;
import com.google.gerrit.httpd.HttpCanonicalWebUrlProvider;
+import com.google.gerrit.httpd.HttpdModule;
import com.google.gerrit.httpd.RequestCleanupFilter;
import com.google.gerrit.httpd.RequestContextFilter;
import com.google.gerrit.httpd.RequestMetricsFilter;
@@ -393,6 +394,7 @@
modules.add(RequestMetricsFilter.module());
modules.add(sysInjector.getInstance(GerritAuthModule.class));
modules.add(sysInjector.getInstance(GitOverHttpModule.class));
+ modules.add(sysInjector.getInstance(HttpdModule.class));
modules.add(RequestCleanupFilter.module());
modules.add(SetThreadNameFilter.module());
modules.add(AllRequestFilter.module());
diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index 269d1c4..b50707b 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -66,6 +66,7 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.RawInputUtil;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.extensions.events.GitReferenceUpdatedListener;
import com.google.gerrit.extensions.registration.DynamicItem;
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.registration.DynamicSet;
@@ -97,6 +98,7 @@
import com.google.gerrit.extensions.restapi.RestView;
import com.google.gerrit.extensions.restapi.TopLevelResource;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
+import com.google.gerrit.extensions.restapi.Url;
import com.google.gerrit.httpd.WebSession;
import com.google.gerrit.httpd.restapi.ParameterParser.QueryParams;
import com.google.gerrit.json.OutputFormat;
@@ -204,6 +206,7 @@
private static final String FORM_TYPE = "application/x-www-form-urlencoded";
@VisibleForTesting public static final String X_GERRIT_TRACE = "X-Gerrit-Trace";
+ @VisibleForTesting public static final String X_GERRIT_UPDATED_REF = "X-Gerrit-UpdatedRef";
private static final String X_REQUESTED_WITH = "X-Requested-With";
private static final String X_GERRIT_AUTH = "X-Gerrit-Auth";
@@ -593,6 +596,8 @@
throw new ResourceNotFoundException();
}
+ setXGerritUpdatedRefResponseHeaders(req, res);
+
if (response instanceof Response.Redirect) {
CacheHeaders.setNotCacheable(res);
String location = ((Response.Redirect) response).location();
@@ -753,6 +758,33 @@
}
}
+ /**
+ * Fill in the refs that were updated during this request in the response header. The updated refs
+ * will be in the form of "project~ref~updated_SHA-1".
+ */
+ private void setXGerritUpdatedRefResponseHeaders(
+ HttpServletRequest request, HttpServletResponse response) {
+ for (GitReferenceUpdatedListener.Event refUpdate :
+ globals.webSession.get().getRefUpdatedEvents()) {
+ String refUpdateFormat =
+ String.format(
+ "%s~%s~%s~%s",
+ // encode the project and ref names since they may contain `~`
+ Url.encode(refUpdate.getProjectName()),
+ Url.encode(refUpdate.getRefName()),
+ refUpdate.getOldObjectId(),
+ refUpdate.getNewObjectId());
+
+ if (isRead(request)) {
+ logger.atWarning().log(
+ "request %s performed a ref update %s although the request is a READ request",
+ request.getRequestURL().toString(), refUpdateFormat);
+ }
+ response.addHeader(X_GERRIT_UPDATED_REF, refUpdateFormat);
+ }
+ globals.webSession.get().resetRefUpdatedEvents();
+ }
+
private String getEtagWithRetry(
HttpServletRequest req,
TraceContext traceContext,
diff --git a/java/com/google/gerrit/pgm/Daemon.java b/java/com/google/gerrit/pgm/Daemon.java
index 07bab24..29c5788 100644
--- a/java/com/google/gerrit/pgm/Daemon.java
+++ b/java/com/google/gerrit/pgm/Daemon.java
@@ -32,6 +32,7 @@
import com.google.gerrit.httpd.GitOverHttpModule;
import com.google.gerrit.httpd.H2CacheBasedWebSession;
import com.google.gerrit.httpd.HttpCanonicalWebUrlProvider;
+import com.google.gerrit.httpd.HttpdModule;
import com.google.gerrit.httpd.RequestCleanupFilter;
import com.google.gerrit.httpd.RequestContextFilter;
import com.google.gerrit.httpd.RequestMetricsFilter;
@@ -580,6 +581,7 @@
modules.add(H2CacheBasedWebSession.module());
modules.add(sysInjector.getInstance(GerritAuthModule.class));
modules.add(sysInjector.getInstance(GitOverHttpModule.class));
+ modules.add(sysInjector.getInstance(HttpdModule.class));
if (sshd) {
modules.add(new ProjectQoSFilter.Module());
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/RestApiServletIT.java b/javatests/com/google/gerrit/acceptance/rest/RestApiServletIT.java
index bc45460..4d90492 100644
--- a/javatests/com/google/gerrit/acceptance/rest/RestApiServletIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/RestApiServletIT.java
@@ -14,15 +14,25 @@
package com.google.gerrit.acceptance.rest;
+import static com.google.common.net.HttpHeaders.ORIGIN;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.httpd.restapi.RestApiServlet.X_GERRIT_UPDATED_REF;
import static org.apache.http.HttpStatus.SC_OK;
import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.PushOneCommit.Result;
import com.google.gerrit.acceptance.RestResponse;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.restapi.Url;
import com.google.gerrit.httpd.restapi.RestApiServlet;
import java.io.IOException;
+import java.util.List;
import java.util.regex.Pattern;
import org.apache.http.message.BasicHeader;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Repository;
import org.junit.Test;
public class RestApiServletIT extends AbstractDaemonTest {
@@ -61,6 +71,200 @@
.containsMatch(ANY_SPACE);
}
+ @Test
+ public void xGerritUpdatedRefNotSetForReadRequests() throws Exception {
+ RestResponse response = adminRestSession.getWithHeader(ANY_REST_API, ACCEPT_STAR_HEADER);
+ assertThat(response.getStatusCode()).isEqualTo(SC_OK);
+ assertThat(response.getHeader(X_GERRIT_UPDATED_REF)).isNull();
+ }
+
+ @Test
+ public void xGerritUpdatedRefSetForDifferentWriteRequests() throws Exception {
+ Result change = createChange();
+ String origin = adminRestSession.url();
+ String project = change.getChange().project().get();
+ String metaRef = RefNames.changeMetaRef(change.getChange().getId());
+
+ ObjectId originalMetaRefSha1 = getMetaRefSha1(change);
+
+ RestResponse response =
+ adminRestSession.putWithHeader(
+ "/changes/" + change.getChangeId() + "/topic", new BasicHeader(ORIGIN, origin), "A");
+ response.assertOK();
+ assertThat(gApi.changes().id(change.getChangeId()).topic()).isEqualTo("A");
+ ObjectId firstMetaRefSha1 = getMetaRefSha1(change);
+
+ // Meta ref updated because of topic update.
+ assertThat(response.getHeader(X_GERRIT_UPDATED_REF))
+ .isEqualTo(
+ String.format(
+ "%s~%s~%s~%s",
+ Url.encode(project),
+ Url.encode(metaRef),
+ originalMetaRefSha1.getName(),
+ firstMetaRefSha1.getName()));
+
+ response =
+ adminRestSession.putWithHeader(
+ "/changes/" + change.getChangeId() + "/topic", new BasicHeader(ORIGIN, origin), "B");
+ response.assertOK();
+ assertThat(gApi.changes().id(change.getChangeId()).topic()).isEqualTo("B");
+
+ ObjectId secondMetaRefSha1 = getMetaRefSha1(change);
+
+ // Meta ref updated again because of another topic update.
+ assertThat(response.getHeader(X_GERRIT_UPDATED_REF))
+ .isEqualTo(
+ String.format(
+ "%s~%s~%s~%s",
+ Url.encode(project),
+ Url.encode(metaRef),
+ firstMetaRefSha1.getName(),
+ secondMetaRefSha1.getName()));
+
+ // Ensure the meta ref SHA-1 changed for the project~metaRef which means we return different
+ // X-Gerrit-UpdatedRef headers.
+ assertThat(secondMetaRefSha1).isNotEqualTo(firstMetaRefSha1);
+ }
+
+ @Test
+ public void xGerritUpdatedRefDeleted() throws Exception {
+ Result change = createChange();
+ String project = change.getChange().project().get();
+ String metaRef = RefNames.changeMetaRef(change.getChange().getId());
+ String patchSetRef = RefNames.patchSetRef(change.getPatchSetId());
+
+ ObjectId originalMetaRefSha1 = getMetaRefSha1(change);
+ ObjectId originalchangeRefSha1 = change.getCommit().getId();
+
+ RestResponse response = adminRestSession.delete("/changes/" + change.getChangeId());
+ response.assertNoContent();
+
+ List<String> headers = response.getHeaders(X_GERRIT_UPDATED_REF);
+
+ // The change was deleted, so the refs were deleted which means they are ObjectId.zeroId().
+ assertThat(headers)
+ .containsExactly(
+ String.format(
+ "%s~%s~%s~%s",
+ Url.encode(project),
+ Url.encode(metaRef),
+ originalMetaRefSha1.getName(),
+ ObjectId.zeroId().getName()),
+ String.format(
+ "%s~%s~%s~%s",
+ Url.encode(project),
+ Url.encode(patchSetRef),
+ originalchangeRefSha1.getName(),
+ ObjectId.zeroId().getName()));
+ }
+
+ @Test
+ public void xGerritUpdatedRefWithProjectNameContainingTilde() throws Exception {
+ Project.NameKey project = createProjectOverAPI("~~pr~oje~ct~~~~", null, true, null);
+ Result change = createChange(cloneProject(project, admin));
+ String metaRef = RefNames.changeMetaRef(change.getChange().getId());
+ String patchSetRef = RefNames.patchSetRef(change.getPatchSetId());
+
+ ObjectId originalMetaRefSha1 = getMetaRefSha1(change);
+ ObjectId originalchangeRefSha1 = change.getCommit().getId();
+
+ RestResponse response = adminRestSession.delete("/changes/" + change.getChangeId());
+ response.assertNoContent();
+
+ List<String> headers = response.getHeaders(X_GERRIT_UPDATED_REF);
+
+ // The change was deleted, so the refs were deleted which means they are ObjectId.zeroId().
+ assertThat(headers)
+ .containsExactly(
+ String.format(
+ "%s~%s~%s~%s",
+ Url.encode(project.get()),
+ Url.encode(metaRef),
+ originalMetaRefSha1.getName(),
+ ObjectId.zeroId().getName()),
+ String.format(
+ "%s~%s~%s~%s",
+ Url.encode(project.get()),
+ Url.encode(patchSetRef),
+ originalchangeRefSha1.getName(),
+ ObjectId.zeroId().getName()));
+
+ // Ensures ~ gets encoded to %7E.
+ assertThat(Url.encode(project.get())).endsWith("%7E%7Epr%7Eoje%7Ect%7E%7E%7E%7E");
+ }
+
+ @Test
+ public void xGerritUpdatedRefSetMultipleHeadersForSubmit() throws Exception {
+ Result change1 = createChange();
+ Result change2 = createChange();
+ String metaRef1 = RefNames.changeMetaRef(change1.getChange().getId());
+ String metaRef2 = RefNames.changeMetaRef(change2.getChange().getId());
+
+ gApi.changes().id(change1.getChangeId()).current().review(ReviewInput.approve());
+ gApi.changes().id(change2.getChangeId()).current().review(ReviewInput.approve());
+
+ Project.NameKey project = change1.getChange().project();
+
+ try (Repository repository = repoManager.openRepository(project)) {
+ ObjectId originalFirstMetaRefSha1 = getMetaRefSha1(change1);
+ ObjectId originalSecondMetaRefSha1 = getMetaRefSha1(change2);
+ ObjectId originalDestinationBranchSha1 =
+ repository.resolve(change1.getChange().change().getDest().branch());
+
+ RestResponse response =
+ adminRestSession.post("/changes/" + change2.getChangeId() + "/submit");
+ response.assertOK();
+
+ ObjectId firstMetaRefSha1 = getMetaRefSha1(change1);
+ ObjectId secondMetaRefSha1 = getMetaRefSha1(change2);
+
+ List<String> headers = response.getHeaders(X_GERRIT_UPDATED_REF);
+
+ String branch = change1.getChange().change().getDest().branch();
+ String branchSha1 =
+ repository
+ .getRefDatabase()
+ .exactRef(change1.getChange().change().getDest().branch())
+ .getObjectId()
+ .name();
+
+ // During submit, all relevant meta refs of the latest patchset are updated + the destination
+ // branch/es.
+ // TODO(paiking): This doesn't work well for torn submissions: If the changes were in
+ // different projects in the same topic, and we tried to submit those changes together, it's
+ // possible that the first submission only submitted one of the changes, and then the retry
+ // submitted the other change. If that happens, when the user retries, they will not get the
+ // meta ref updates for the change that got submitted on the previous submission attempt.
+ // Ideally, submit should be idempotent and always return all meta refs on all submission
+ // attempts.
+ assertThat(headers)
+ .containsExactly(
+ String.format(
+ "%s~%s~%s~%s",
+ Url.encode(project.get()),
+ Url.encode(metaRef1),
+ originalFirstMetaRefSha1.getName(),
+ firstMetaRefSha1.getName()),
+ String.format(
+ "%s~%s~%s~%s",
+ Url.encode(project.get()),
+ Url.encode(metaRef2),
+ originalSecondMetaRefSha1.getName(),
+ secondMetaRefSha1.getName()),
+ String.format(
+ "%s~%s~%s~%s",
+ Url.encode(project.get()),
+ Url.encode(branch),
+ originalDestinationBranchSha1.getName(),
+ branchSha1));
+ }
+ }
+
+ private ObjectId getMetaRefSha1(Result change) {
+ return change.getChange().notes().getRevision();
+ }
+
private RestResponse prettyJsonRestResponse(String ppArgument, int ppValue) throws Exception {
RestResponse response =
adminRestSession.getWithHeader(
