Merge "Add ChangeKind for trivial rebase with commit message update."
diff --git a/.mailmap b/.mailmap
new file mode 100644
index 0000000..721f3c0
--- /dev/null
+++ b/.mailmap
@@ -0,0 +1,97 @@
+Adrian Görler <adrian.goerler@sap.com> Adrian Goerler <adrian.goerler@sap.com>
+Ahaan Ugale <ahaanugale@gmail.com> <augale@codeaurora.org>
+Alex Blewitt <alex.blewitt@gmail.com> <alex.blewitt@gs.com>
+Alex Blewitt <alex.blewitt@gmail.com> <alex.blewitt@credit-suisse.com>
+Alex Ryazantsev <alex.ryazantsev@gmail.com> alex <alex.ryazantsev@gmail.com>
+Alex Ryazantsev <alex.ryazantsev@gmail.com> alex.ryazantsev <alex.ryazantsev@gmail.com>
+Alice Kober-Sotzek <aliceks@google.com> <aliceks@google.com>
+Alexandre Philbert <alexandre.philbert@ericsson.com> <alexandre.philbert@hotmail.com>
+Andrew Bonventre <andybons@chromium.org> <andybons@google.com>
+Becky Siegel <beckysiegel@google.com> beckysiegel <beckysiegel@google.com>
+Ben Rohlfs <brohlfs@google.com> brohlfs <brohlfs@google.com>
+Brad Larson <bklarson@gmail.com> <brad.larson@garmin.com>
+Bruce Zu <bruce.zu.run10@gmail.com> <bruce.zu@sonyericsson.com>
+Bruce Zu <bruce.zu.run10@gmail.com> <bruce.zu@sonymobile.com>
+Carlos Eduardo Baldacin <carloseduardo.baldacin@sonyericsson.com> carloseduardo.baldacin <carloseduardo.baldacin@sonyericsson.com>
+Chad Horohoe <chorohoe@wikimedia.org> <chadh@wikimedia.org>
+Changcheng Xiao <xchangcheng@google.com> xchangcheng
+Cheng Ke <chengke.info@gmail.com> <chengke.info@gmail.com>
+Dariusz Luksza <dluksza@collab.net> <dariusz@luksza.org>
+Darrien Glasser <darrien@arista.com> darrien <darrien@arista.com>
+Dave Borowitz <dborowitz@google.com> <dborowitz@google.com>
+David Ostrovsky <david@ostrovsky.org> <d.ostrovsky@gmx.de>
+David Ostrovsky <david@ostrovsky.org> <david.ostrovsky@gmail.com>
+David Pursehouse <dpursehouse@collab.net> <david.pursehouse@sonymobile.com>
+Deniz Türkoglu <deniz@spotify.com> Deniz Türkoglu <deniz@spotify.com>
+Deniz Türkoglu <deniz@spotify.com> Deniz Turkoglu <deniz@spotify.com>
+Doug Kelly <dougk.ff7@gmail.com> <doug.kelly@garmin.com>
+Edwin Kempin <ekempin@google.com> Edwin Kempin <edwin.kempin@gmail.com>
+Edwin Kempin <ekempin@google.com> Edwin Kempin <edwin.kempin@sap.com>
+Edwin Kempin <ekempin@google.com> ekempin <ekempin@google.com>
+Eryk Szymanski <eryksz@gmail.com> <eryksz@google.com>
+Fredrik Luthander <fredrik.luthander@sonymobile.com> <fredrik@gandaraj.com>
+Fredrik Luthander <fredrik.luthander@sonymobile.com> <fredrik.luthander@sonyericsson.com>
+Gerrit Code Review <no-reply@gerritcodereview.com> <noreply-gerritcodereview@google.com>
+Gustaf Lundh <gustaflh@axis.com> <gustaf.lundh@axis.com>
+Gustaf Lundh <gustaflh@axis.com> <gustaf.lundh@sonyericsson.com>
+Gustaf Lundh <gustaflh@axis.com> <gustaf.lundh@sonymobile.com>
+Han-Wen Nienhuys <hanwen@google.com> <hanwen@google.com>
+Hector Oswaldo Caballero <hector.caballero@ericsson.com> <hector.caballero@ericsson.com>
+Hugo Arès <hugo.ares@ericsson.com> Hugo Ares <hugo.ares@ericsson.com>
+Hugo Arès <hugo.ares@ericsson.com> <hugares@gmail.com>
+Jacek Centkowski <jcentkowski@collab.net> <gemincia.programs@gmail.com>
+Jacek Centkowski <jcentkowski@collab.net> <geminica.programs@gmail.com>
+James E. Blair <jeblair@redhat.com> <jeblair@hp.com>
+Jason Huntley <jhuntley@houghtonassociates.com> jhuntley <jhuntley@houghtonassociates.com>
+Jiří Engelthaler <EngyCZ@gmail.com> <engycz@gmail.com>
+Joe Onorato <onoratoj@gmail.com> <joeo@android.com>
+Joel Dodge <dodgejoel@gmail.com> dodgejoel <dodgejoel@gmail.com>
+Johan Björk <jbjoerk@gmail.com> Johan Bjork <phb@spotify.com>
+JT Olds <hello@jtolds.com> <jtolds@gmail.com>
+Kasper Nilsson <kaspern@google.com> <kaspern@google.com>
+Lawrence Dubé <ldube@audiokinetic.com> <ldube@audiokinetic.com>
+Lei Sun <lei.sun01@sap.com> LeiSun <lei.sun01@sap.com>
+Lincoln Oliveira Campos Do Nascimento <lincoln.oliveiracamposdonascimento@sonyericsson.com> lincoln <lincoln.oliveiracamposdonascimento@sonyericsson.com>
+Luca Milanesio <luca.milanesio@gmail.com> <luca@gitent-scm.com>
+Magnus Bäck <magnus.back@axis.com> <baeck@google.com>
+Magnus Bäck <magnus.back@axis.com> <magnus.back@sonyericsson.com>
+Marco Miller <marco.miller@ericsson.com> <marco.mmiller@gmail.com>
+Mark Derricutt <mark.derricutt@smxemail.com> <mark@talios.com>
+Martin Fick <mfick@codeaurora.org> <mogulguy10@gmail.com>
+Martin Fick <mfick@codeaurora.org> <mogulguy@yahoo.com>
+Martin Wallgren <martinwa@axis.com> <martin.wallgren@axis.com>
+Matthias Sohn <matthias.sohn@sap.com> <matthias.sohn@gmail.com>
+Maxime Guerreiro <maximeg@google.com> <maximeg@google.com>
+Michael Zhou <moz@google.com> <zhoumotongxue008@gmail.com>
+Monty Taylor <mordred@inaugust.com> <monty.taylor@gmail.com>
+Mônica Dionísio <monica.dionisio@sonyericsson.com> monica.dionisio <monica.dionisio@sonyericsson.com>
+Nasser Grainawi <nasser@grainawi.org> <nasser@codeaurora.org>
+Nasser Grainawi <nasser@grainawi.org> <nasserg@quicinc.com>
+Orgad Shaneh <orgads@gmail.com> <orgad.shaneh@audiocodes.com>
+Paladox <thomasmulhall410@yahoo.com> <thomasmulhall410@yahoo.com>
+Patrick Hiesel <hiesel@google.com> <hiesel@hiesel-macbookpro2.roam.corp.google.com>
+Peter Jönsson <peter.joensson@gmail.com> Peter Jönsson <peter.joensson@gmail.com>
+Rafael Rabelo Silva <rafael.rabelosilva@sonyericsson.com> rafael.rabelosilva <rafael.rabelosilva@sonyericsson.com>
+Réda Housni Alaoui <reda.housnialaoui@gmail.com> <alaoui.rda@gmail.com>
+Richard Möhn <richard.moehn@posteo.de> <richard.moehn@fu-berlin.de>
+Sam Saccone <samccone@google.com> <samccone@gmail.com>
+Sam Saccone <samccone@google.com> <samccone@google.com>
+Saša Živkov <sasa.zivkov@sap.com> Sasa Zivkov <sasa.zivkov@sap.com>
+Saša Živkov <sasa.zivkov@sap.com> Saša Živkov <zivkov@gmail.com>
+Saša Živkov <sasa.zivkov@sap.com> Sasa Zivkov <zivkov@gmail.com>
+Scott Dial <scott@scottdial.com> <geekmug@gmail.com>
+Shawn Pearce <sop@google.com> Shawn O. Pearce <sop@google.com>
+Sixin Li <sixin210@gmail.com> sixin li <sixin210@gmail.com>
+Sven Selberg <svense@axis.com> <sven.selberg@axis.com>
+Sven Selberg <svense@axis.com> <sven.selberg@sonymobile.com>
+Thomas Dräbing <thomas.draebing@sap.com> <thomas.draebing@sap.com>
+Tom Wang <twang10@gmail.com> Tom <twang10@gmail.com>
+Tomas Westling <thomas.westling@sonyericsson.com> thomas.westling <thomas.westling@sonyericsson.com>
+Ulrik Sjölin <ulrik.sjolin@sonyericsson.com> <ulrik.sjolin@gmail.com>
+Ulrik Sjölin <ulrik.sjolin@sonyericsson.com> Ulrik Sjolin <ulrik.sjolin@gmail.com>
+Ulrik Sjölin <ulrik.sjolin@sonyericsson.com> Ulrik Sjölin <ulrik.sjolin@sonyericsson.com>
+Ulrik Sjölin <ulrik.sjolin@sonyericsson.com> Ulrik Sjolin <ulrik.sjolin@sonyericsson.com>
+Viktar Donich <viktard@google.com> viktard
+Yuxuan 'fishy' Wang <fishywang@google.com> Yuxuan Wang <fishywang@google.com>
+Zalán Blénessy <zalanb@axis.com> Zalan Blenessy <zalanb@axis.com>
+飞 李 <lifei@7v1.net> lifei <lifei@7v1.net>
diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt
index 4fa5e68..8c1b1ce 100644
--- a/Documentation/dev-plugins.txt
+++ b/Documentation/dev-plugins.txt
@@ -144,6 +144,40 @@
This enables plugins to influence other plugins by customizing or extending the
their behaviour.
+When a plugin wants to expose an API that *must* not be further overridden by
+other plugins, it could use the additional annotation `@DynamicItem.Final` which
+also gives the option to further limit the name of the plugin that is designated
+to bind the only implementation available.
+
+For example, a `plugin B` may declare an API as `@DynamicItem.Final` which is then
+bound in its `ApiModule`.
+
+```
+ @DynamicItem.Final(implementedByPlugin = "plugin-b-impl")
+ public interface PluginAPI {}
+
+ public class PluginApiModule extends AbstractModule {
+ @Override
+ protected void configure() {
+ DynamicItem.itemOf(binder(), PluginAPI.class);
+ }
+ }
+```
+
+The above definition of the `PluginApi` would be allowed to bound only by
+the `plugin-b-impl` which would associate its implementation class.
+
+```
+public class PluginImpl implements PluginAPI {}
+
+ public class PluginImplModule extends AbstractModule {
+ @Override
+ protected void configure() {
+ DynamicItem.bind(binder(), PluginAPI.class).to(PluginImpl.class);
+ }
+ }
+```
+
*Gotchas and Limitations*:
- A `plugin A` depending on a `plugin B` (declaring a `Gerrit-ApiModule`),
diff --git a/java/com/google/gerrit/acceptance/ExtensionRegistry.java b/java/com/google/gerrit/acceptance/ExtensionRegistry.java
index d2051d5..a35e427 100644
--- a/java/com/google/gerrit/acceptance/ExtensionRegistry.java
+++ b/java/com/google/gerrit/acceptance/ExtensionRegistry.java
@@ -45,7 +45,6 @@
import com.google.gerrit.extensions.webui.ResolveConflictsWebLink;
import com.google.gerrit.server.ExceptionHook;
import com.google.gerrit.server.account.GroupBackend;
-import com.google.gerrit.server.change.ChangeETagComputation;
import com.google.gerrit.server.change.FilterIncludedIn;
import com.google.gerrit.server.change.ReviewerSuggestion;
import com.google.gerrit.server.config.ProjectConfigEntry;
@@ -81,7 +80,6 @@
private final DynamicSet<SubmitRule> submitRules;
private final DynamicSet<SubmitRequirement> submitRequirements;
private final DynamicSet<ChangeMessageModifier> changeMessageModifiers;
- private final DynamicSet<ChangeETagComputation> changeETagComputations;
private final DynamicSet<ActionVisitor> actionVisitors;
private final DynamicMap<DownloadScheme> downloadSchemes;
private final DynamicSet<RefOperationValidationListener> refOperationValidationListeners;
@@ -128,7 +126,6 @@
DynamicSet<SubmitRule> submitRules,
DynamicSet<SubmitRequirement> submitRequirements,
DynamicSet<ChangeMessageModifier> changeMessageModifiers,
- DynamicSet<ChangeETagComputation> changeETagComputations,
DynamicSet<ActionVisitor> actionVisitors,
DynamicMap<DownloadScheme> downloadSchemes,
DynamicSet<RefOperationValidationListener> refOperationValidationListeners,
@@ -170,7 +167,6 @@
this.submitRules = submitRules;
this.submitRequirements = submitRequirements;
this.changeMessageModifiers = changeMessageModifiers;
- this.changeETagComputations = changeETagComputations;
this.actionVisitors = actionVisitors;
this.downloadSchemes = downloadSchemes;
this.refOperationValidationListeners = refOperationValidationListeners;
@@ -286,11 +282,6 @@
}
@CanIgnoreReturnValue
- public Registration add(ChangeETagComputation changeETagComputation) {
- return add(changeETagComputations, changeETagComputation);
- }
-
- @CanIgnoreReturnValue
public Registration add(ActionVisitor actionVisitor) {
return add(actionVisitors, actionVisitor);
}
diff --git a/java/com/google/gerrit/extensions/registration/DynamicItem.java b/java/com/google/gerrit/extensions/registration/DynamicItem.java
index 4464af7..4cc22f9 100644
--- a/java/com/google/gerrit/extensions/registration/DynamicItem.java
+++ b/java/com/google/gerrit/extensions/registration/DynamicItem.java
@@ -14,9 +14,12 @@
package com.google.gerrit.extensions.registration;
+import static java.lang.annotation.RetentionPolicy.RUNTIME;
+
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.gerrit.common.Nullable;
import com.google.inject.Binder;
+import com.google.inject.BindingAnnotation;
import com.google.inject.Key;
import com.google.inject.Provider;
import com.google.inject.ProvisionException;
@@ -25,6 +28,9 @@
import com.google.inject.binder.LinkedBindingBuilder;
import com.google.inject.util.Providers;
import com.google.inject.util.Types;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.Target;
import java.util.concurrent.atomic.AtomicReference;
/**
@@ -36,6 +42,15 @@
* exception is thrown.
*/
public class DynamicItem<T> {
+
+ /** Annotate a DynamicItem to be final and being bound at most once. */
+ @Target({ElementType.TYPE})
+ @Retention(RUNTIME)
+ @BindingAnnotation
+ public @interface Final {
+ String implementedByPlugin() default "";
+ }
+
/**
* Declare a singleton {@code DynamicItem<T>} with a binder.
*
diff --git a/java/com/google/gerrit/extensions/registration/PrivateInternals_DynamicTypes.java b/java/com/google/gerrit/extensions/registration/PrivateInternals_DynamicTypes.java
index 5b528cb..982ff98 100644
--- a/java/com/google/gerrit/extensions/registration/PrivateInternals_DynamicTypes.java
+++ b/java/com/google/gerrit/extensions/registration/PrivateInternals_DynamicTypes.java
@@ -14,12 +14,14 @@
package com.google.gerrit.extensions.registration;
+import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.extensions.events.LifecycleListener;
import com.google.inject.Binding;
import com.google.inject.Inject;
import com.google.inject.Injector;
import com.google.inject.Key;
+import com.google.inject.ProvisionException;
import com.google.inject.TypeLiteral;
import java.lang.reflect.ParameterizedType;
import java.util.ArrayList;
@@ -97,6 +99,26 @@
DynamicItem<Object> item = (DynamicItem<Object>) e.getValue();
for (Binding<Object> b : bindings(src, type)) {
+ Class<? super Object> rawType = type.getRawType();
+ DynamicItem.Final annotation = rawType.getAnnotation(DynamicItem.Final.class);
+ if (annotation != null) {
+ Object existingBinding = item.get();
+ if (existingBinding != null) {
+ throw new ProvisionException(
+ String.format(
+ "Attempting to bind a @DynamicItem.Final %s twice: it was already bound to %s and tried to bind again to %s",
+ rawType.getName(), existingBinding, b));
+ }
+
+ String implementedByPlugin = annotation.implementedByPlugin();
+ if (!Strings.isNullOrEmpty(implementedByPlugin)
+ && !implementedByPlugin.equals(pluginName)) {
+ throw new ProvisionException(
+ String.format(
+ "Attempting to bind a @DynamicItem.Final %s to unexpected plugin: it was supposed to be bound to %s plugin but tried bind to %s plugin",
+ rawType.getName(), implementedByPlugin, pluginName));
+ }
+ }
handles.add(item.set(b.getKey(), b.getProvider(), pluginName));
}
}
diff --git a/java/com/google/gerrit/extensions/restapi/ETagView.java b/java/com/google/gerrit/extensions/restapi/ETagView.java
deleted file mode 100644
index 9ac1706..0000000
--- a/java/com/google/gerrit/extensions/restapi/ETagView.java
+++ /dev/null
@@ -1,20 +0,0 @@
-// Copyright (C) 2015 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.extensions.restapi;
-
-/** A view which may change, although the underlying resource did not change */
-public interface ETagView<R extends RestResource> extends RestReadView<R> {
- String getETag(R rsrc);
-}
diff --git a/java/com/google/gerrit/extensions/restapi/RestResource.java b/java/com/google/gerrit/extensions/restapi/RestResource.java
index 3c8144a..30916bc 100644
--- a/java/com/google/gerrit/extensions/restapi/RestResource.java
+++ b/java/com/google/gerrit/extensions/restapi/RestResource.java
@@ -29,9 +29,4 @@
/** Returns time for the Last-Modified header. HTTP truncates the header value to seconds. */
Timestamp getLastModified();
}
-
- /** A resource with an ETag. */
- public interface HasETag {
- String getETag();
- }
}
diff --git a/java/com/google/gerrit/httpd/NumericChangeIdRedirectServlet.java b/java/com/google/gerrit/httpd/NumericChangeIdRedirectServlet.java
index 5d15a56..c797c47 100644
--- a/java/com/google/gerrit/httpd/NumericChangeIdRedirectServlet.java
+++ b/java/com/google/gerrit/httpd/NumericChangeIdRedirectServlet.java
@@ -14,6 +14,9 @@
package com.google.gerrit.httpd;
+import static com.google.common.collect.ImmutableList.toImmutableList;
+
+import com.google.common.collect.ImmutableList;
import com.google.gerrit.common.PageLinks;
import com.google.gerrit.entities.Change;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
@@ -25,7 +28,6 @@
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.Arrays;
-import java.util.List;
import java.util.Optional;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
@@ -60,7 +62,8 @@
int commentIdx = uriPath.indexOf("/comment");
String idString = commentIdx == -1 ? uriPath : uriPath.substring(0, commentIdx);
- List<String> uriSegments = Arrays.stream(idString.split("/")).toList();
+ ImmutableList<String> uriSegments =
+ Arrays.stream(idString.split("/")).collect(toImmutableList());
idString = uriSegments.get(0);
String psString = (uriSegments.size() > 1) ? uriSegments.get(1) : null;
diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index 6951398..0844ca9 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -40,7 +40,6 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Splitter;
import com.google.common.base.Strings;
-import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.Iterables;
@@ -65,7 +64,6 @@
import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.extensions.restapi.CacheControl;
import com.google.gerrit.extensions.restapi.DefaultInput;
-import com.google.gerrit.extensions.restapi.ETagView;
import com.google.gerrit.extensions.restapi.IdString;
import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.extensions.restapi.NeedsParams;
@@ -110,12 +108,10 @@
import com.google.gerrit.server.change.ChangeFinder;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.group.GroupAuditService;
-import com.google.gerrit.server.logging.Metadata;
import com.google.gerrit.server.logging.PerformanceLogContext;
import com.google.gerrit.server.logging.PerformanceLogger;
import com.google.gerrit.server.logging.RequestId;
import com.google.gerrit.server.logging.TraceContext;
-import com.google.gerrit.server.logging.TraceContext.TraceTimer;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
@@ -497,7 +493,7 @@
checkRequiresCapability(viewData);
}
- if (notModified(req, traceContext, viewData, rsrc)) {
+ if (notModified(req, rsrc)) {
logger.atFinest().log("REST call succeeded: %d", SC_NOT_MODIFIED);
res.sendError(SC_NOT_MODIFIED);
return;
@@ -607,7 +603,7 @@
statusCode = response.statusCode();
response.headers().forEach((k, v) -> res.setHeader(k, v));
- configureCaching(req, res, traceContext, rsrc, viewData, response.caching());
+ configureCaching(req, res, rsrc, response.caching());
res.setStatus(statusCode);
logger.atFinest().log("REST call succeeded: %d", statusCode);
}
@@ -800,47 +796,6 @@
globals.webSession.get().resetRefUpdatedEvents();
}
- private String getEtagWithRetry(
- HttpServletRequest req,
- TraceContext traceContext,
- ViewData viewData,
- ETagView<RestResource> view,
- RestResource rsrc) {
- try (TraceTimer ignored =
- TraceContext.newTimer(
- "RestApiServlet#getEtagWithRetry:view",
- Metadata.builder().restViewName(getViewName(viewData)).build())) {
- return invokeRestEndpointWithRetry(
- req,
- traceContext,
- getViewName(viewData) + "#etag",
- ActionType.REST_READ_REQUEST,
- () -> view.getETag(rsrc));
- } catch (Exception e) {
- Throwables.throwIfUnchecked(e);
- throw new IllegalStateException("Failed to get ETag for view", e);
- }
- }
-
- @Nullable
- private String getEtagWithRetry(
- HttpServletRequest req, TraceContext traceContext, RestResource.HasETag rsrc) {
- try (TraceTimer ignored =
- TraceContext.newTimer(
- "RestApiServlet#getEtagWithRetry:resource",
- Metadata.builder().restViewName(rsrc.getClass().getSimpleName()).build())) {
- return invokeRestEndpointWithRetry(
- req,
- traceContext,
- rsrc.getClass().getSimpleName() + "#etag",
- ActionType.REST_READ_REQUEST,
- () -> rsrc.getETag());
- } catch (Exception e) {
- Throwables.throwIfUnchecked(e);
- throw new IllegalStateException("Failed to get ETag for resource", e);
- }
- }
-
private RestResource parseResourceWithRetry(
HttpServletRequest req,
TraceContext traceContext,
@@ -1012,30 +967,11 @@
return defaultMessage;
}
- private boolean notModified(
- HttpServletRequest req, TraceContext traceContext, ViewData viewData, RestResource rsrc) {
+ private boolean notModified(HttpServletRequest req, RestResource rsrc) {
if (!isRead(req)) {
return false;
}
- RestView<RestResource> view = viewData.view;
- if (view instanceof ETagView) {
- String have = req.getHeader(HttpHeaders.IF_NONE_MATCH);
- if (have != null) {
- String eTag =
- getEtagWithRetry(req, traceContext, viewData, (ETagView<RestResource>) view, rsrc);
- return have.equals(eTag);
- }
- }
-
- if (rsrc instanceof RestResource.HasETag) {
- String have = req.getHeader(HttpHeaders.IF_NONE_MATCH);
- if (!Strings.isNullOrEmpty(have)) {
- String eTag = getEtagWithRetry(req, traceContext, (RestResource.HasETag) rsrc);
- return have.equals(eTag);
- }
- }
-
if (rsrc instanceof RestResource.HasLastModified) {
Timestamp m = ((RestResource.HasLastModified) rsrc).getLastModified();
long d = req.getDateHeader(HttpHeaders.IF_MODIFIED_SINCE);
@@ -1047,12 +983,7 @@
}
private <R extends RestResource> void configureCaching(
- HttpServletRequest req,
- HttpServletResponse res,
- TraceContext traceContext,
- R rsrc,
- ViewData viewData,
- CacheControl cacheControl) {
+ HttpServletRequest req, HttpServletResponse res, R rsrc, CacheControl cacheControl) {
setCacheHeaders(req, res, cacheControl);
if (isRead(req)) {
switch (cacheControl.getType()) {
@@ -1060,10 +991,10 @@
default:
break;
case PRIVATE:
- addResourceStateHeaders(req, res, traceContext, viewData, rsrc);
+ addResourceStateHeaders(res, rsrc);
break;
case PUBLIC:
- addResourceStateHeaders(req, res, traceContext, viewData, rsrc);
+ addResourceStateHeaders(res, rsrc);
break;
}
}
@@ -1095,23 +1026,7 @@
}
}
- private void addResourceStateHeaders(
- HttpServletRequest req,
- HttpServletResponse res,
- TraceContext traceContext,
- ViewData viewData,
- RestResource rsrc) {
- RestView<RestResource> view = viewData.view;
- if (view instanceof ETagView) {
- String eTag =
- getEtagWithRetry(req, traceContext, viewData, (ETagView<RestResource>) view, rsrc);
- res.setHeader(HttpHeaders.ETAG, eTag);
- } else if (rsrc instanceof RestResource.HasETag) {
- String eTag = getEtagWithRetry(req, traceContext, (RestResource.HasETag) rsrc);
- if (!Strings.isNullOrEmpty(eTag)) {
- res.setHeader(HttpHeaders.ETAG, eTag);
- }
- }
+ private void addResourceStateHeaders(HttpServletResponse res, RestResource rsrc) {
if (rsrc instanceof RestResource.HasLastModified) {
res.setDateHeader(
HttpHeaders.LAST_MODIFIED,
diff --git a/java/com/google/gerrit/server/change/ChangeETagComputation.java b/java/com/google/gerrit/server/change/ChangeETagComputation.java
deleted file mode 100644
index 2fd5755..0000000
--- a/java/com/google/gerrit/server/change/ChangeETagComputation.java
+++ /dev/null
@@ -1,63 +0,0 @@
-// 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.server.change;
-
-import com.google.gerrit.entities.Change;
-import com.google.gerrit.entities.Project;
-import com.google.gerrit.extensions.annotations.ExtensionPoint;
-
-/**
- * Allows plugins to contribute a value to the change ETag computation.
- *
- * <p>Plugins can affect the result of the get change / get change details REST endpoints by:
- *
- * <ul>
- * <li>providing plugin defined attributes to {@link
- * com.google.gerrit.extensions.common.ChangeInfo#plugins} (see {@link
- * ChangePluginDefinedInfoFactory})
- * <li>implementing a {@link com.google.gerrit.server.rules.SubmitRule} which affects the
- * computation of {@link com.google.gerrit.extensions.common.ChangeInfo#submittable}
- * </ul>
- *
- * <p>If the plugin defined part of {@link com.google.gerrit.extensions.common.ChangeInfo} depends
- * on plugin specific data, callers that use the change ETags to avoid unneeded recomputations of
- * ChangeInfos may see outdated plugin attributes and/or outdated submittable information, because a
- * ChangeInfo is only reloaded if the change ETag changes.
- *
- * <p>By implementating this interface plugins can contribute to the change ETag computation and
- * thus ensure that the ETag changes when the plugin data was changed. This way it is ensured that
- * callers do not see outdated ChangeInfos.
- *
- * @see ChangeResource#getETag()
- */
-@ExtensionPoint
-public interface ChangeETagComputation {
- /**
- * Computes an ETag of plugin-specific data for the given change.
- *
- * <p><strong>Note:</strong> Change ETags are computed very frequently and the computation must be
- * cheap. Take good care to not perform any expensive computations when implementing this.
- *
- * <p>If an error is encountered during the ETag computation the plugin can indicate this by
- * throwing any RuntimeException. In this case no value will be included in the change ETag
- * computation. This means if the error is transient, the ETag will differ when the computation
- * succeeds on a follow-up run.
- *
- * @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 the ETag
- */
- String getETag(Project.NameKey projectName, Change.Id changeId);
-}
diff --git a/java/com/google/gerrit/server/change/ChangeResource.java b/java/com/google/gerrit/server/change/ChangeResource.java
index 99a0b50..a7fa6f4 100644
--- a/java/com/google/gerrit/server/change/ChangeResource.java
+++ b/java/com/google/gerrit/server/change/ChangeResource.java
@@ -14,56 +14,20 @@
package com.google.gerrit.server.change;
-import static com.google.gerrit.server.experiments.ExperimentFeaturesConstants.DISABLE_CHANGE_ETAGS;
-import static com.google.gerrit.server.project.ProjectCache.illegalState;
-import static java.nio.charset.StandardCharsets.UTF_8;
-
-import com.google.common.base.MoreObjects;
-import com.google.common.hash.Hasher;
-import com.google.common.hash.Hashing;
-import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
-import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.Change;
-import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
-import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.restapi.RestResource;
-import com.google.gerrit.extensions.restapi.RestResource.HasETag;
import com.google.gerrit.extensions.restapi.RestView;
import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.PatchSetUtil;
-import com.google.gerrit.server.StarredChangesReader;
-import com.google.gerrit.server.account.AccountCache;
-import com.google.gerrit.server.account.AccountState;
-import com.google.gerrit.server.approval.ApprovalsUtil;
-import com.google.gerrit.server.experiments.ExperimentFeatures;
-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.ChangeNotes;
import com.google.gerrit.server.permissions.PermissionBackend;
-import com.google.gerrit.server.plugincontext.PluginSetContext;
-import com.google.gerrit.server.project.ProjectCache;
-import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.TypeLiteral;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
-import java.util.HashSet;
-import java.util.Optional;
-import java.util.Set;
-import org.eclipse.jgit.lib.ObjectId;
-public class ChangeResource implements RestResource, HasETag {
- /**
- * JSON format version number for ETag computations.
- *
- * <p>Should be bumped on any JSON format change (new fields, etc.) so that otherwise unmodified
- * changes get new ETags.
- */
- public static final int JSON_FORMAT_VERSION = 1;
-
+public class ChangeResource implements RestResource {
public static final TypeLiteral<RestView<ChangeResource>> CHANGE_KIND = new TypeLiteral<>() {};
public interface Factory {
@@ -72,64 +36,27 @@
ChangeResource create(ChangeData changeData, CurrentUser user);
}
- private static final String ZERO_ID_STRING = ObjectId.zeroId().name();
-
- private final ExperimentFeatures experimentFeatures;
- private final AccountCache accountCache;
- private final ApprovalsUtil approvalUtil;
- private final PatchSetUtil patchSetUtil;
private final PermissionBackend permissionBackend;
- private final StarredChangesReader starredChangesReader;
- private final ProjectCache projectCache;
- private final PluginSetContext<ChangeETagComputation> changeETagComputation;
private final ChangeData changeData;
private final CurrentUser user;
@AssistedInject
ChangeResource(
- ExperimentFeatures experimentFeatures,
- AccountCache accountCache,
- ApprovalsUtil approvalUtil,
- PatchSetUtil patchSetUtil,
PermissionBackend permissionBackend,
- StarredChangesReader starredChangesReader,
- ProjectCache projectCache,
- PluginSetContext<ChangeETagComputation> changeETagComputation,
ChangeData.Factory changeDataFactory,
@Assisted ChangeNotes notes,
@Assisted CurrentUser user) {
- this.experimentFeatures = experimentFeatures;
- this.accountCache = accountCache;
- this.approvalUtil = approvalUtil;
- this.patchSetUtil = patchSetUtil;
this.permissionBackend = permissionBackend;
- this.starredChangesReader = starredChangesReader;
- this.projectCache = projectCache;
- this.changeETagComputation = changeETagComputation;
this.changeData = changeDataFactory.create(notes);
this.user = user;
}
@AssistedInject
ChangeResource(
- ExperimentFeatures experimentFeatures,
- AccountCache accountCache,
- ApprovalsUtil approvalUtil,
- PatchSetUtil patchSetUtil,
PermissionBackend permissionBackend,
- StarredChangesReader starredChangesReader,
- ProjectCache projectCache,
- PluginSetContext<ChangeETagComputation> changeETagComputation,
@Assisted ChangeData changeData,
@Assisted CurrentUser user) {
- this.experimentFeatures = experimentFeatures;
- this.accountCache = accountCache;
- this.approvalUtil = approvalUtil;
- this.patchSetUtil = patchSetUtil;
this.permissionBackend = permissionBackend;
- this.starredChangesReader = starredChangesReader;
- this.projectCache = projectCache;
- this.changeETagComputation = changeETagComputation;
this.changeData = changeData;
this.user = user;
}
@@ -171,111 +98,4 @@
public Change.Id getVirtualId() {
return getChangeData().virtualId();
}
-
- // This includes all information relevant for ETag computation
- // unrelated to the UI.
- public void prepareETag(Hasher h, CurrentUser user) {
- h.putInt(JSON_FORMAT_VERSION)
- .putLong(getChange().getLastUpdatedOn().toEpochMilli())
- .putInt(user.isIdentifiedUser() ? user.getAccountId().get() : 0);
-
- if (user.isIdentifiedUser()) {
- for (AccountGroup.UUID uuid : user.getEffectiveGroups().getKnownGroups()) {
- h.putBytes(uuid.get().getBytes(UTF_8));
- }
- }
-
- byte[] buf = new byte[20];
- Set<Account.Id> accounts = new HashSet<>();
- accounts.add(getChange().getOwner());
- try {
- patchSetUtil.byChange(getNotes()).stream().map(PatchSet::uploader).forEach(accounts::add);
-
- // It's intentional to include the states for *all* reviewers into the ETag computation.
- // We need the states of all current reviewers and CCs because they are part of ChangeInfo.
- // Including removed reviewers is a cheap way of making sure that the states of accounts that
- // posted a message on the change are included. Loading all change messages to find the exact
- // set of accounts that posted a message is too expensive. However everyone who posts a
- // message is automatically added as reviewer. Hence if we include removed reviewers we can
- // be sure that we have all accounts that posted messages on the change.
- accounts.addAll(approvalUtil.getReviewers(getNotes()).all());
- } catch (StorageException e) {
- // This ETag will be invalidated if it loads next time.
- }
-
- for (Account.Id accountId : accounts) {
- Optional<AccountState> accountState = accountCache.get(accountId);
- if (accountState.isPresent()) {
- hashAccount(h, accountState.get(), buf);
- } else {
- h.putInt(accountId.get());
- }
- }
-
- ObjectId noteId;
- try {
- noteId = getNotes().loadRevision();
- } catch (StorageException e) {
- noteId = null; // This ETag will be invalidated if it loads next time.
- }
- hashObjectId(h, noteId, buf);
- // TODO(dborowitz): Include more NoteDb and other related refs, e.g. drafts
- // and edits.
-
- Iterable<ProjectState> projectStateTree =
- projectCache.get(getProject()).orElseThrow(illegalState(getProject())).tree();
- for (ProjectState p : projectStateTree) {
- hashObjectId(h, p.getConfig().getRevision().orElse(null), buf);
- }
-
- changeETagComputation.runEach(
- c -> {
- String pluginETag = c.getETag(changeData.project(), changeData.getId());
- if (pluginETag != null) {
- h.putString(pluginETag, UTF_8);
- }
- });
- }
-
- @Override
- @Nullable
- public String getETag() {
- if (experimentFeatures.isFeatureEnabled(DISABLE_CHANGE_ETAGS)) {
- return null;
- }
-
- try (TraceTimer ignored =
- TraceContext.newTimer(
- "Compute change ETag",
- Metadata.builder()
- .changeId(changeData.getId().get())
- .projectName(changeData.project().get())
- .build())) {
- Hasher h = Hashing.murmur3_128().newHasher();
- if (user.isIdentifiedUser()) {
- h.putBoolean(starredChangesReader.isStarred(user.getAccountId(), getVirtualId()));
- }
- prepareETag(h, user);
- return h.hash().toString();
- }
- }
-
- private void hashObjectId(Hasher h, @Nullable ObjectId id, byte[] buf) {
- MoreObjects.firstNonNull(id, ObjectId.zeroId()).copyRawTo(buf, 0);
- h.putBytes(buf);
- }
-
- private void hashAccount(Hasher h, AccountState accountState, byte[] buf) {
- h.putInt(accountState.account().id().get());
- // Based on the code, it seems the metaId should never be null in this place and so the
- // uniqueTag.
- // However, the null-check for metaId has been existed here for some time already - for safety
- // the same check is applied to uniqueTag.
- h.putString(
- MoreObjects.firstNonNull(
- accountState.account().uniqueTag(),
- MoreObjects.firstNonNull(accountState.account().metaId(), ZERO_ID_STRING)),
- UTF_8);
- accountState.externalIds().stream().forEach(e -> hashObjectId(h, e.blobId(), buf));
- }
}
diff --git a/java/com/google/gerrit/server/change/RevisionResource.java b/java/com/google/gerrit/server/change/RevisionResource.java
index 4a10158..a09cb1f 100644
--- a/java/com/google/gerrit/server/change/RevisionResource.java
+++ b/java/com/google/gerrit/server/change/RevisionResource.java
@@ -14,26 +14,20 @@
package com.google.gerrit.server.change;
-import com.google.common.hash.Hasher;
-import com.google.common.hash.Hashing;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.restapi.RestResource;
-import com.google.gerrit.extensions.restapi.RestResource.HasETag;
import com.google.gerrit.extensions.restapi.RestView;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.edit.ChangeEdit;
-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.ChangeNotes;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.inject.TypeLiteral;
import java.util.Optional;
-public class RevisionResource implements RestResource, HasETag {
+public class RevisionResource implements RestResource {
public static final TypeLiteral<RestView<RevisionResource>> REVISION_KIND =
new TypeLiteral<>() {};
@@ -90,28 +84,6 @@
return ps;
}
- @Override
- public String getETag() {
- try (TraceTimer ignored =
- TraceContext.newTimer(
- "Compute revision ETag",
- Metadata.builder()
- .changeId(changeResource.getId().get())
- .patchSetId(ps.number())
- .projectName(changeResource.getProject().get())
- .build())) {
- Hasher h = Hashing.murmur3_128().newHasher();
- prepareETag(h, getUser());
- return h.hash().toString();
- }
- }
-
- public void prepareETag(Hasher h, CurrentUser user) {
- // Conservative estimate: refresh the revision if its parent change has changed, so we don't
- // have to check whether a given modification affected this revision specifically.
- changeResource.prepareETag(h, user);
- }
-
public Account.Id getAccountId() {
return getUser().getAccountId();
}
diff --git a/java/com/google/gerrit/server/change/testing/TestChangeETagComputation.java b/java/com/google/gerrit/server/change/testing/TestChangeETagComputation.java
deleted file mode 100644
index 344b9b3..0000000
--- a/java/com/google/gerrit/server/change/testing/TestChangeETagComputation.java
+++ /dev/null
@@ -1,30 +0,0 @@
-// 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.server.change.testing;
-
-import com.google.gerrit.server.change.ChangeETagComputation;
-
-public class TestChangeETagComputation {
-
- public static ChangeETagComputation withETag(String etag) {
- return (p, id) -> etag;
- }
-
- public static ChangeETagComputation withException(RuntimeException e) {
- return (p, id) -> {
- throw e;
- };
- }
-}
diff --git a/java/com/google/gerrit/server/change/testing/package-info.java b/java/com/google/gerrit/server/change/testing/package-info.java
deleted file mode 100644
index 3cd4da3..0000000
--- a/java/com/google/gerrit/server/change/testing/package-info.java
+++ /dev/null
@@ -1,18 +0,0 @@
-// Copyright (C) 2023 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.
-
-@CheckReturnValue
-package com.google.gerrit.server.change.testing;
-
-import com.google.errorprone.annotations.CheckReturnValue;
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index a38a3fc..2b26956 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -114,7 +114,6 @@
import com.google.gerrit.server.cache.CacheRemovalListener;
import com.google.gerrit.server.change.AbandonOp;
import com.google.gerrit.server.change.AccountPatchReviewStore;
-import com.google.gerrit.server.change.ChangeETagComputation;
import com.google.gerrit.server.change.ChangeFinder;
import com.google.gerrit.server.change.ChangeJson;
import com.google.gerrit.server.change.ChangeKindCacheImpl;
@@ -458,7 +457,6 @@
DynamicSet.setOf(binder(), PerformanceLogger.class);
DynamicSet.setOf(binder(), RequestListener.class);
DynamicSet.bind(binder(), RequestListener.class).to(TraceRequestListener.class);
- DynamicSet.setOf(binder(), ChangeETagComputation.class);
DynamicSet.setOf(binder(), ExceptionHook.class);
DynamicSet.bind(binder(), ExceptionHook.class).to(ExceptionHookImpl.class);
DynamicSet.setOf(binder(), MailSoyTemplateProvider.class);
diff --git a/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java b/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
index 49b5d2a6..cd91745 100644
--- a/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
+++ b/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
@@ -60,7 +60,4 @@
/** Whether we allow fix suggestions in HumanComments. */
public static final String ALLOW_FIX_SUGGESTIONS_IN_COMMENTS =
"GerritBackendFeature__allow_fix_suggestions_in_comments";
-
- /** Whether etags should be disabled for change resources. */
- public static final String DISABLE_CHANGE_ETAGS = "GerritBackendFeature__disable_change_etags";
}
diff --git a/java/com/google/gerrit/server/restapi/change/Files.java b/java/com/google/gerrit/server/restapi/change/Files.java
index 96e5645..07e4372 100644
--- a/java/com/google/gerrit/server/restapi/change/Files.java
+++ b/java/com/google/gerrit/server/restapi/change/Files.java
@@ -17,8 +17,6 @@
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.flogger.FluentLogger;
-import com.google.common.hash.Hasher;
-import com.google.common.hash.Hashing;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
@@ -31,10 +29,10 @@
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.CacheControl;
import com.google.gerrit.extensions.restapi.ChildCollection;
-import com.google.gerrit.extensions.restapi.ETagView;
import com.google.gerrit.extensions.restapi.IdString;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.extensions.restapi.RestView;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.PatchSetUtil;
@@ -47,7 +45,6 @@
import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.patch.DiffOperations;
import com.google.gerrit.server.patch.DiffOptions;
-import com.google.gerrit.server.patch.PatchListKey;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.patch.filediff.FileDiffOutput;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -100,7 +97,7 @@
return new FileResource(rev, id.get());
}
- public static final class ListFiles implements ETagView<RevisionResource> {
+ public static final class ListFiles implements RestReadView<RevisionResource> {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@Option(name = "--base", metaVar = "revision-id")
@@ -353,16 +350,6 @@
return this;
}
- @Override
- public String getETag(RevisionResource resource) {
- Hasher h = Hashing.murmur3_128().newHasher();
- resource.prepareETag(h, resource.getUser());
- // File list comes from the PatchListCache, so any change to the key or value should
- // invalidate ETag.
- h.putLong(PatchListKey.serialVersionUID);
- return h.hash().toString();
- }
-
@Nullable
private ObjectId getOldId(Map<String, FileDiffOutput> fileDiffList) {
return fileDiffList.isEmpty()
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 6e635c9..f92702e 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -111,7 +111,6 @@
import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
-import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.annotations.Exports;
import com.google.gerrit.extensions.api.accounts.DeleteDraftCommentsInput;
import com.google.gerrit.extensions.api.changes.AttentionSetInput;
@@ -170,7 +169,6 @@
import com.google.gerrit.server.change.ChangeJson;
import com.google.gerrit.server.change.ChangeMessages;
import com.google.gerrit.server.change.ChangeResource;
-import com.google.gerrit.server.change.testing.TestChangeETagComputation;
import com.google.gerrit.server.events.CommitReceivedEvent;
import com.google.gerrit.server.git.ChangeMessageModifier;
import com.google.gerrit.server.git.validators.CommitValidationException;
@@ -1543,7 +1541,6 @@
private void testAddReviewerViaPostReview(AddReviewerCaller addReviewer) throws Exception {
PushOneCommit.Result r = createChange();
ChangeResource rsrc = parseResource(r);
- String oldETag = rsrc.getETag();
Instant oldTs = rsrc.getChange().getLastUpdatedOn();
addReviewer.call(r.getChangeId(), user.email());
@@ -1567,16 +1564,9 @@
// Nobody was added as CC.
assertThat(c.reviewers.get(CC)).isNull();
- // Ensure ETag and lastUpdatedOn are updated.
+ // Ensure lastUpdatedOn is updated.
rsrc = parseResource(r);
- assertThat(rsrc.getETag()).isNotEqualTo(oldETag);
assertThat(rsrc.getChange().getLastUpdatedOn()).isNotEqualTo(oldTs);
-
- // Change status of reviewer and ensure ETag is updated.
- oldETag = rsrc.getETag();
- accountOperations.account(user.id()).forUpdate().status("new status").update();
- rsrc = parseResource(r);
- assertThat(rsrc.getETag()).isNotEqualTo(oldETag);
}
@Test
@@ -1659,7 +1649,6 @@
public void addReviewerThatIsNotPerfectMatch() throws Exception {
PushOneCommit.Result r = createChange();
ChangeResource rsrc = parseResource(r);
- String oldETag = rsrc.getETag();
Instant oldTs = rsrc.getChange().getLastUpdatedOn();
// create a group named "ab" with one user: testUser
@@ -1697,9 +1686,8 @@
assertThat(reviewers).hasSize(1);
assertThat(reviewers.iterator().next()._accountId).isEqualTo(accountIdOfTestUser.get());
- // Ensure ETag and lastUpdatedOn are updated.
+ // Ensure lastUpdatedOn is updated.
rsrc = parseResource(r);
- assertThat(rsrc.getETag()).isNotEqualTo(oldETag);
assertThat(rsrc.getChange().getLastUpdatedOn()).isNotEqualTo(oldTs);
}
@@ -1708,7 +1696,6 @@
public void addGroupAsReviewersWhenANotPerfectMatchedUserExists() throws Exception {
PushOneCommit.Result r = createChange();
ChangeResource rsrc = parseResource(r);
- String oldETag = rsrc.getETag();
Instant oldTs = rsrc.getChange().getLastUpdatedOn();
// create a group named "kobe" with one user: lee
@@ -1758,9 +1745,8 @@
assertThat(reviewers).hasSize(1);
assertThat(reviewers.iterator().next()._accountId).isEqualTo(accountIdOfGroupUser.get());
- // Ensure ETag and lastUpdatedOn are updated.
+ // Ensure lastUpdatedOn is updated.
rsrc = parseResource(r);
- assertThat(rsrc.getETag()).isNotEqualTo(oldETag);
assertThat(rsrc.getChange().getLastUpdatedOn()).isNotEqualTo(oldTs);
}
@@ -1809,7 +1795,6 @@
public void addSelfAsReviewer() throws Exception {
PushOneCommit.Result r = createChange();
ChangeResource rsrc = parseResource(r);
- String oldETag = rsrc.getETag();
Instant oldTs = rsrc.getChange().getLastUpdatedOn();
ReviewerInput in = new ReviewerInput();
@@ -1827,9 +1812,8 @@
assertThat(reviewers).hasSize(1);
assertThat(reviewers.iterator().next()._accountId).isEqualTo(user.id().get());
- // Ensure ETag and lastUpdatedOn are updated.
+ // Ensure lastUpdatedOn is updated.
rsrc = parseResource(r);
- assertThat(rsrc.getETag()).isNotEqualTo(oldETag);
assertThat(rsrc.getChange().getLastUpdatedOn()).isNotEqualTo(oldTs);
}
@@ -1920,56 +1904,6 @@
}
@Test
- public void eTagChangesWhenOwnerUpdatesAccountStatus() throws Exception {
- PushOneCommit.Result r = createChange();
- ChangeResource rsrc = parseResource(r);
- String oldETag = rsrc.getETag();
-
- accountOperations.account(admin.id()).forUpdate().status("new status").update();
- rsrc = parseResource(r);
- assertThat(rsrc.getETag()).isNotEqualTo(oldETag);
- }
-
- @Test
- public void pluginCanContributeToETagComputation() throws Exception {
- PushOneCommit.Result r = createChange();
- String oldETag = parseResource(r).getETag();
-
- try (Registration registration =
- extensionRegistry.newRegistration().add(TestChangeETagComputation.withETag("foo"))) {
- assertThat(parseResource(r).getETag()).isNotEqualTo(oldETag);
- }
-
- assertThat(parseResource(r).getETag()).isEqualTo(oldETag);
- }
-
- @Test
- public void returningNullFromETagComputationDoesNotBreakGerrit() throws Exception {
- PushOneCommit.Result r = createChange();
- String oldETag = parseResource(r).getETag();
-
- try (Registration registration =
- extensionRegistry.newRegistration().add(TestChangeETagComputation.withETag(null))) {
- assertThat(parseResource(r).getETag()).isEqualTo(oldETag);
- }
- }
-
- @Test
- public void throwingExceptionFromETagComputationDoesNotBreakGerrit() throws Exception {
- PushOneCommit.Result r = createChange();
- String oldETag = parseResource(r).getETag();
-
- try (Registration registration =
- extensionRegistry
- .newRegistration()
- .add(
- TestChangeETagComputation.withException(
- new StorageException("exception during test")))) {
- assertThat(parseResource(r).getETag()).isEqualTo(oldETag);
- }
- }
-
- @Test
public void emailNotificationForFileLevelComment() throws Exception {
String changeId = createChange().getChangeId();
diff --git a/javatests/com/google/gerrit/acceptance/api/config/ListExperimentsIT.java b/javatests/com/google/gerrit/acceptance/api/config/ListExperimentsIT.java
index cef9ddd..fe3cb00 100644
--- a/javatests/com/google/gerrit/acceptance/api/config/ListExperimentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/config/ListExperimentsIT.java
@@ -52,7 +52,6 @@
.GERRIT_BACKEND_FEATURE_ALWAYS_REJECT_IMPLICIT_MERGES_ON_MERGE,
ExperimentFeaturesConstants.GERRIT_BACKEND_FEATURE_ATTACH_NONCE_TO_DOCUMENTATION,
ExperimentFeaturesConstants.GERRIT_BACKEND_FEATURE_CHECK_IMPLICIT_MERGES_ON_MERGE,
- ExperimentFeaturesConstants.DISABLE_CHANGE_ETAGS,
ExperimentFeaturesConstants.GERRIT_BACKEND_FEATURE_REJECT_IMPLICIT_MERGES_ON_MERGE)
.inOrder();
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ChangeEtagIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ChangeEtagIT.java
deleted file mode 100644
index 6df8c86..0000000
--- a/javatests/com/google/gerrit/acceptance/rest/change/ChangeEtagIT.java
+++ /dev/null
@@ -1,56 +0,0 @@
-// Copyright (C) 2024 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.acceptance.rest.change;
-
-import static com.google.common.truth.Truth.assertThat;
-
-import com.google.common.net.HttpHeaders;
-import com.google.gerrit.acceptance.AbstractDaemonTest;
-import com.google.gerrit.acceptance.RestResponse;
-import com.google.gerrit.acceptance.config.GerritConfig;
-import com.google.gerrit.acceptance.testsuite.change.ChangeOperations;
-import com.google.gerrit.entities.Change;
-import com.google.gerrit.server.experiments.ExperimentFeaturesConstants;
-import com.google.inject.Inject;
-import org.junit.Test;
-
-public class ChangeEtagIT extends AbstractDaemonTest {
- @Inject private ChangeOperations changeOperations;
-
- @Test
- @GerritConfig(
- name = "experiments.enabled",
- value = ExperimentFeaturesConstants.DISABLE_CHANGE_ETAGS)
- public void changeEtagsDisabled() throws Exception {
- Change.Id changeId = changeOperations.newChange().create();
-
- RestResponse response = adminRestSession.get(String.format("/changes/%s", changeId));
- assertThat(response.getHeader(HttpHeaders.ETAG)).isNull();
-
- response = adminRestSession.get(String.format("/changes/%s/detail", changeId));
- assertThat(response.getHeader(HttpHeaders.ETAG)).isNull();
- }
-
- @Test
- public void changeEtagsEnabled() throws Exception {
- Change.Id changeId = changeOperations.newChange().create();
-
- RestResponse response = adminRestSession.get(String.format("/changes/%s", changeId));
- assertThat(response.getHeader(HttpHeaders.ETAG)).isNotNull();
-
- response = adminRestSession.get(String.format("/changes/%s/detail", changeId));
- assertThat(response.getHeader(HttpHeaders.ETAG)).isNotNull();
- }
-}
diff --git a/javatests/com/google/gerrit/acceptance/server/change/GetRelatedIT.java b/javatests/com/google/gerrit/acceptance/server/change/GetRelatedIT.java
index 7eec5ea..cd1ae5af 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/GetRelatedIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/GetRelatedIT.java
@@ -146,15 +146,11 @@
testRepo.reset(c1_1);
pushHead(testRepo, "refs/for/master", false);
PatchSet.Id ps1_1 = getPatchSetId(c1_1);
- String oldETag = changes.parse(ps1_1.changeId()).getETag();
testRepo.reset(c2_1);
pushHead(testRepo, "refs/for/master", false);
PatchSet.Id ps2_1 = getPatchSetId(c2_1);
- // Push of change 2 should not affect groups (or anything else) of change 1.
- assertThat(changes.parse(ps1_1.changeId()).getETag()).isEqualTo(oldETag);
-
for (PatchSet.Id ps : ImmutableList.of(ps2_1, ps1_1)) {
assertRelated(ps, changeAndCommit(ps2_1, c2_1, 1), changeAndCommit(ps1_1, c1_1, 1));
}
diff --git a/javatests/com/google/gerrit/extensions/BUILD b/javatests/com/google/gerrit/extensions/BUILD
index 1bb39c8..86fd295 100644
--- a/javatests/com/google/gerrit/extensions/BUILD
+++ b/javatests/com/google/gerrit/extensions/BUILD
@@ -8,6 +8,7 @@
"//java/com/google/gerrit/common:annotations",
"//java/com/google/gerrit/extensions:api",
"//java/com/google/gerrit/extensions/common/testing:common-test-util",
+ "//java/com/google/gerrit/testing:gerrit-test-util",
"//lib:guava",
"//lib/guice",
"//lib/truth",
diff --git a/javatests/com/google/gerrit/extensions/registration/DynamicItemTest.java b/javatests/com/google/gerrit/extensions/registration/DynamicItemTest.java
new file mode 100644
index 0000000..828f6c1
--- /dev/null
+++ b/javatests/com/google/gerrit/extensions/registration/DynamicItemTest.java
@@ -0,0 +1,175 @@
+// Copyright (C) 2024 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.extensions.registration;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.common.Nullable;
+import com.google.inject.AbstractModule;
+import com.google.inject.Binder;
+import com.google.inject.Guice;
+import com.google.inject.Injector;
+import com.google.inject.ProvisionException;
+import com.google.inject.TypeLiteral;
+import java.util.function.Consumer;
+import org.junit.Test;
+
+public class DynamicItemTest {
+ private static final String PLUGIN_NAME = "plugin-name";
+
+ private static final String UNEXPECTED_PLUGIN_NAME = "unexpected-plugin";
+ private static final String DYNAMIC_ITEM_1 = "item-1";
+ private static final String DYNAMIC_ITEM_2 = "item-2";
+ private static final TypeLiteral<String> STRING_TYPE_LITERAL = new TypeLiteral<>() {};
+ private static final TypeLiteral<FinalItemApi> FINAL_ITEM_API_TYPE_LITERAL =
+ new TypeLiteral<>() {};
+ private static final TypeLiteral<FinalItemApiForPlugin>
+ FINAL_TARGET_PLUGIN_ITEM_API_TYPE_LITERAL = new TypeLiteral<>() {};
+
+ @DynamicItem.Final
+ private interface FinalItemApi {}
+
+ private static class FinalItemImpl implements FinalItemApi {
+ private static final FinalItemApi INSTANCE = new FinalItemImpl();
+ }
+
+ @DynamicItem.Final(implementedByPlugin = PLUGIN_NAME)
+ private interface FinalItemApiForPlugin {}
+
+ private static class FinalItemImplByPlugin implements FinalItemApiForPlugin {
+ private static final FinalItemApiForPlugin INSTANCE = new FinalItemImplByPlugin();
+ }
+
+ @Test
+ public void shouldAssignDynamicItemTwice() {
+ ImmutableMap<TypeLiteral<?>, DynamicItem<?>> bindings =
+ ImmutableMap.of(STRING_TYPE_LITERAL, DynamicItem.itemOf(String.class, null));
+
+ ImmutableList<RegistrationHandle> gerritRegistrations =
+ PrivateInternals_DynamicTypes.attachItems(
+ newInjector(
+ (binder) -> {
+ DynamicItem.itemOf(binder, String.class);
+ DynamicItem.bind(binder, String.class).toInstance(DYNAMIC_ITEM_1);
+ }),
+ PluginName.GERRIT,
+ bindings);
+ assertThat(gerritRegistrations).hasSize(1);
+ assertDynamicItem(bindings.get(STRING_TYPE_LITERAL), DYNAMIC_ITEM_1, PluginName.GERRIT);
+
+ ImmutableList<RegistrationHandle> pluginRegistrations =
+ PrivateInternals_DynamicTypes.attachItems(
+ newInjector(
+ (binder) -> DynamicItem.bind(binder, String.class).toInstance(DYNAMIC_ITEM_2)),
+ PLUGIN_NAME,
+ bindings);
+ assertThat(pluginRegistrations).hasSize(1);
+ assertDynamicItem(bindings.get(STRING_TYPE_LITERAL), DYNAMIC_ITEM_2, PLUGIN_NAME);
+ }
+
+ @Test
+ public void shouldFailToAssignFinalDynamicItemTwice() {
+ ImmutableMap<TypeLiteral<?>, DynamicItem<?>> bindings =
+ ImmutableMap.of(FINAL_ITEM_API_TYPE_LITERAL, DynamicItem.itemOf(FinalItemApi.class, null));
+
+ ImmutableList<RegistrationHandle> baseInjectorRegistrations =
+ PrivateInternals_DynamicTypes.attachItems(
+ newInjector(
+ (binder) -> {
+ DynamicItem.itemOf(binder, FinalItemApi.class);
+ DynamicItem.bind(binder, FinalItemApi.class).toInstance(FinalItemImpl.INSTANCE);
+ }),
+ PluginName.GERRIT,
+ bindings);
+ assertThat(baseInjectorRegistrations).hasSize(1);
+
+ ProvisionException ignored =
+ assertThrows(
+ ProvisionException.class,
+ () -> {
+ ImmutableList<RegistrationHandle> unused =
+ PrivateInternals_DynamicTypes.attachItems(
+ newInjector(
+ (binder) ->
+ DynamicItem.bind(binder, FinalItemApi.class)
+ .toInstance(FinalItemImpl.INSTANCE)),
+ PluginName.GERRIT,
+ bindings);
+ });
+ }
+
+ @Test
+ public void shouldFailToAssignFinalDynamicItemToDifferentPlugin() {
+ ImmutableMap<TypeLiteral<?>, DynamicItem<?>> bindings =
+ ImmutableMap.of(
+ FINAL_TARGET_PLUGIN_ITEM_API_TYPE_LITERAL,
+ DynamicItem.itemOf(FinalItemApi.class, null));
+
+ assertThrows(
+ ProvisionException.class,
+ () -> {
+ ImmutableList<RegistrationHandle> unused =
+ PrivateInternals_DynamicTypes.attachItems(
+ newInjector(
+ (binder) ->
+ DynamicItem.bind(binder, FinalItemApiForPlugin.class)
+ .toInstance(FinalItemImplByPlugin.INSTANCE)),
+ UNEXPECTED_PLUGIN_NAME,
+ bindings);
+ });
+ }
+
+ @Test
+ public void shouldAssignFinalDynamicItemToExpectedPlugin() {
+ ImmutableMap<TypeLiteral<?>, DynamicItem<?>> bindings =
+ ImmutableMap.of(
+ FINAL_TARGET_PLUGIN_ITEM_API_TYPE_LITERAL,
+ DynamicItem.itemOf(FinalItemApi.class, null));
+
+ ImmutableList<RegistrationHandle> pluginRegistrations =
+ PrivateInternals_DynamicTypes.attachItems(
+ newInjector(
+ (binder) ->
+ DynamicItem.bind(binder, FinalItemApiForPlugin.class)
+ .toInstance(FinalItemImplByPlugin.INSTANCE)),
+ PLUGIN_NAME,
+ bindings);
+ assertThat(pluginRegistrations).hasSize(1);
+ assertDynamicItem(
+ bindings.get(FINAL_TARGET_PLUGIN_ITEM_API_TYPE_LITERAL),
+ FinalItemImplByPlugin.INSTANCE,
+ PLUGIN_NAME);
+ }
+
+ private static Injector newInjector(Consumer<Binder> binding) {
+ return Guice.createInjector(
+ new AbstractModule() {
+ @Override
+ protected void configure() {
+ binding.accept(binder());
+ }
+ });
+ }
+
+ private static <T> void assertDynamicItem(
+ @Nullable DynamicItem<?> item, T itemVal, String pluginName) {
+ assertThat(item).isNotNull();
+ assertThat(item.get()).isEqualTo(itemVal);
+ assertThat(item.getPluginName()).isEqualTo(pluginName);
+ }
+}
diff --git a/plugins/replication b/plugins/replication
index aacb8b2..1c16901 160000
--- a/plugins/replication
+++ b/plugins/replication
@@ -1 +1 @@
-Subproject commit aacb8b2a20267e88ccda811d27293bac66e2006b
+Subproject commit 1c16901e971c96c20705b272703ad59b9b46200a
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
index ed42b3b..c02d474 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -145,7 +145,6 @@
changeViewModelToken,
ChangeViewState,
createChangeUrl,
- createEditUrl,
} from '../../../models/views/change';
import {rootUrl} from '../../../utils/url-util';
import {userModelToken} from '../../../models/user/user-model';
@@ -2399,13 +2398,10 @@
controls.openDeleteDialog(path);
break;
case GrEditConstants.Actions.OPEN.id:
- assertIsDefined(this.patchNum, 'patchset number');
this.getNavigation().setUrl(
- createEditUrl({
- changeNum: this.change._number,
- repo: this.change.project,
- patchNum: this.patchNum,
+ this.getViewModel().editUrl({
editView: {path},
+ patchNum: this.patchNum,
})
);
break;
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
index 1fd4aa2..7b9c291 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
@@ -73,7 +73,10 @@
import {Modifier} from '../../../utils/dom-util';
import {GrButton} from '../../shared/gr-button/gr-button';
import {GrCopyLinks} from '../gr-copy-links/gr-copy-links';
-import {ChangeChildView} from '../../../models/views/change';
+import {
+ ChangeChildView,
+ changeViewModelToken,
+} from '../../../models/views/change';
import {rootUrl} from '../../../utils/url-util';
import {testResolver} from '../../../test/common-test-setup';
import {UserModel, userModelToken} from '../../../models/user/user-model';
@@ -308,6 +311,12 @@
setup(async () => {
setUrlStub = sinon.stub(testResolver(navigationToken), 'setUrl');
+ sinon
+ .stub(testResolver(changeViewModelToken), 'editUrl')
+ .returns('fakeEditUrl');
+ sinon
+ .stub(testResolver(changeViewModelToken), 'diffUrl')
+ .returns('fakeDiffUrl');
stubRestApi('getConfig').returns(
Promise.resolve({
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
index 7a09518..3ecbf74 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
@@ -80,9 +80,8 @@
import {incrementalRepeat} from '../../lit/incremental-repeat';
import {ifDefined} from 'lit/directives/if-defined.js';
import {
- createDiffUrl,
- createEditUrl,
createChangeUrl,
+ changeViewModelToken,
} from '../../../models/views/change';
import {userModelToken} from '../../../models/user/user-model';
import {pluginLoaderToken} from '../../shared/gr-js-api-interface/gr-plugin-loader';
@@ -212,7 +211,7 @@
diffViewMode?: DiffViewMode;
@property({type: Boolean})
- editMode?: boolean;
+ editMode = false;
private _filesExpanded = FilesExpandedState.NONE;
@@ -313,6 +312,8 @@
private readonly getNavigation = resolve(this, navigationToken);
+ private readonly getViewModel = resolve(this, changeViewModelToken);
+
// private but used in test
fileCursor = new GrCursorManager();
@@ -2167,15 +2168,13 @@
// Private but used in tests.
openCursorFile() {
const diff = this.diffCursor?.getTargetDiffElement();
- if (!this.change || !diff || !this.patchRange || !diff.path) {
- throw new Error('change, diff and patchRange must be all set and valid');
+ if (!this.change || !diff || !this.patchNum || !diff.path) {
+ throw new Error('change, diff and pacthNum must be all set and valid');
}
this.getNavigation().setUrl(
- createDiffUrl({
- change: this.change,
- patchNum: this.patchRange.patchNum,
- basePatchNum: this.patchRange.basePatchNum,
+ this.getViewModel().diffUrl({
diffView: {path: diff.path},
+ patchNum: this.patchNum,
})
);
}
@@ -2188,15 +2187,13 @@
if (!this.files[this.fileCursor.index]) {
return;
}
- if (!this.change || !this.patchRange) {
+ if (!this.change || !this.patchNum) {
throw new Error('change and patchRange must be set');
}
this.getNavigation().setUrl(
- createDiffUrl({
- change: this.change,
- patchNum: this.patchRange.patchNum,
- basePatchNum: this.patchRange.basePatchNum,
+ this.getViewModel().diffUrl({
diffView: {path: this.files[this.fileCursor.index].__path},
+ patchNum: this.patchNum,
})
);
}
@@ -2214,30 +2211,21 @@
);
}
+ /** Returns an edit or diff URL depending on `editMode`. */
// Private but used in tests
- computeDiffURL(path?: string) {
- if (
- this.change === undefined ||
- this.patchRange?.patchNum === undefined ||
- path === undefined ||
- this.editMode === undefined
- ) {
- return;
- }
+ computeDiffURL(path?: string): string | undefined {
+ if (path === undefined) return;
+ if (this.patchNum === undefined) return;
+
if (this.editMode && path !== SpecialFilePath.MERGE_LIST) {
- return createEditUrl({
- changeNum: this.change._number,
- repo: this.change.project,
- patchNum: this.patchRange.patchNum,
+ return this.getViewModel().editUrl({
+ patchNum: this.patchNum,
editView: {path},
});
}
- return createDiffUrl({
- changeNum: this.change._number,
- repo: this.change.project,
- patchNum: this.patchRange.patchNum,
- basePatchNum: this.patchRange.basePatchNum,
+ return this.getViewModel().diffUrl({
diffView: {path},
+ patchNum: this.patchNum,
});
}
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.ts b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.ts
index eec829a..ca7ab14 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.ts
@@ -60,6 +60,11 @@
import {FileMode} from '../../../utils/file-util';
import {SinonStubbedMember} from 'sinon';
import {GrDiffCursor} from '../../../embed/diff/gr-diff-cursor/gr-diff-cursor';
+import {
+ ChangeChildView,
+ changeViewModelToken,
+} from '../../../models/views/change';
+import {GerritView} from '../../../services/router/router-model';
suite('gr-diff a11y test', () => {
test('audit', async () => {
@@ -79,6 +84,15 @@
let element: GrFileList;
let saveStub: sinon.SinonStub;
+ setup(async () => {
+ testResolver(changeViewModelToken).setState({
+ view: GerritView.CHANGE,
+ childView: ChangeChildView.OVERVIEW,
+ changeNum: 42 as NumericChangeId,
+ repo: 'gerrit' as RepoName,
+ });
+ });
+
suite('basic tests', async () => {
setup(async () => {
stubRestApi('getDiffComments').returns(Promise.resolve({}));
@@ -179,7 +193,7 @@
<gr-file-status></gr-file-status>
</div>
<span class="path" role="gridcell">
- <a class="pathLink">
+ <a class="pathLink" href="/c/gerrit/+/42/2/path/file0">
<span class="fullFileName" title="path/file0">
<span class="newFilePath"> path/ </span>
<span class="fileName"> file0 </span>
@@ -299,7 +313,7 @@
fileRows[0].querySelector('.path'),
/* HTML */ `
<span class="path" role="gridcell">
- <a class="pathLink">
+ <a class="pathLink" href="/c/gerrit/+/42/2/path/file0">
<span class="fullFileName" title="path/file0">
<span class="newFilePath"> path/ </span>
<span class="fileName"> file0 </span>
@@ -317,7 +331,7 @@
fileRows[1].querySelector('.path'),
/* HTML */ `
<span class="path" role="gridcell">
- <a class="pathLink">
+ <a class="pathLink" href="/c/gerrit/+/42/2/path/file1">
<span class="fullFileName" title="path/file1">
<span class="matchingFilePath"> path/ </span>
<span class="fileName"> file1 </span>
@@ -947,10 +961,10 @@
];
element.changeNum = 42 as NumericChangeId;
element.basePatchNum = PARENT;
- element.patchNum = 2 as RevisionPatchSetNum;
+ element.patchNum = 1 as RevisionPatchSetNum;
element.change = {
_number: 42 as NumericChangeId,
- project: 'test-project',
+ project: 'gerrit',
} as ParsedChangeInfo;
element.fileCursor.setCursorAtIndex(0);
await element.updateComplete;
@@ -1009,7 +1023,7 @@
assert.equal(setUrlStub.callCount, 1);
assert.equal(
setUrlStub.lastCall.firstArg,
- '/c/test-project/+/42/2/file_added_in_rev2.txt'
+ '/c/gerrit/+/42/1/file_added_in_rev2.txt'
);
pressKey(element, 'k');
@@ -1705,35 +1719,25 @@
suite('diff url file list', () => {
test('diff url', () => {
- element.change = {
- ...createParsedChange(),
- _number: 1 as NumericChangeId,
- project: 'gerrit' as RepoName,
- };
- element.basePatchNum = PARENT;
- element.patchNum = 1 as RevisionPatchSetNum;
const path = 'index.php';
+ element.patchNum = 1 as RevisionPatchSetNum;
element.editMode = false;
- assert.equal(element.computeDiffURL(path), '/c/gerrit/+/1/1/index.php');
+ assert.equal(element.computeDiffURL(path), '/c/gerrit/+/42/1/index.php');
});
test('diff url commit msg', () => {
- element.change = {
- ...createParsedChange(),
- _number: 1 as NumericChangeId,
- project: 'gerrit' as RepoName,
- };
- element.basePatchNum = PARENT;
- element.patchNum = 1 as RevisionPatchSetNum;
- element.editMode = false;
const path = '/COMMIT_MSG';
- assert.equal(element.computeDiffURL(path), '/c/gerrit/+/1/1//COMMIT_MSG');
+ element.editMode = false;
+ assert.equal(
+ element.computeDiffURL(path),
+ '/c/gerrit/+/42/1//COMMIT_MSG'
+ );
});
test('edit url', () => {
element.change = {
...createParsedChange(),
- _number: 1 as NumericChangeId,
+ _number: 42 as NumericChangeId,
project: 'gerrit' as RepoName,
};
element.basePatchNum = PARENT;
@@ -1742,14 +1746,14 @@
const path = 'index.php';
assert.equal(
element.computeDiffURL(path),
- '/c/gerrit/+/1/1/index.php,edit'
+ '/c/gerrit/+/42/1/index.php,edit'
);
});
test('edit url commit msg', () => {
element.change = {
...createParsedChange(),
- _number: 1 as NumericChangeId,
+ _number: 42 as NumericChangeId,
project: 'gerrit' as RepoName,
};
element.basePatchNum = PARENT;
@@ -1758,7 +1762,7 @@
const path = '/COMMIT_MSG';
assert.equal(
element.computeDiffURL(path),
- '/c/gerrit/+/1/1//COMMIT_MSG,edit'
+ '/c/gerrit/+/42/1//COMMIT_MSG,edit'
);
});
});
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-results.ts b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
index 72d3268..c5194d5 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-results.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
@@ -58,6 +58,7 @@
import {
DropdownLink,
LabelNameToInfoMap,
+ PARENT,
PatchSetNumber,
} from '../../types/common';
import {spinnerStyles} from '../../styles/gr-spinner-styles';
@@ -78,7 +79,7 @@
import {when} from 'lit/directives/when.js';
import {DropdownItem} from '../shared/gr-dropdown-list/gr-dropdown-list';
import './gr-checks-attempt';
-import {createDiffUrl, changeViewModelToken} from '../../models/views/change';
+import {changeViewModelToken} from '../../models/views/change';
import {formStyles} from '../../styles/form-styles';
/**
@@ -635,6 +636,8 @@
private getChangeModel = resolve(this, changeModelToken);
+ private readonly getViewModel = resolve(this, changeViewModelToken);
+
static override get styles() {
return [
sharedStyles,
@@ -714,14 +717,13 @@
const change = this.getChangeModel().getChange();
assertIsDefined(change);
const path = pointer.path;
- const patchset = this.result?.patchset as PatchSetNumber | undefined;
+ const patchset = this.result?.patchset as PatchSetNumber;
const line = pointer?.range?.start_line;
return {
icon: LinkIcon.CODE,
tooltip: `${path}${rangeText}`,
- url: createDiffUrl({
- changeNum: change._number,
- repo: change.project,
+ url: this.getViewModel().diffUrl({
+ basePatchNum: PARENT,
patchNum: patchset,
checksPatchset: patchset,
diffView: {path, lineNum: line},
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
index ee2a806..a5c2a99 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
@@ -92,7 +92,6 @@
import {when} from 'lit/directives/when.js';
import {styleMap} from 'lit/directives/style-map.js';
import {
- createDiffUrl,
ChangeChildView,
changeViewModelToken,
} from '../../../models/views/change';
@@ -1457,7 +1456,11 @@
if (!newPath) return;
if (newPath.up) return this.getChangeModel().changeUrl();
if (!newPath.path) return;
- return this.getChangeModel().diffUrl({path: newPath.path});
+ if (!this.patchNum) return;
+ return this.getViewModel().diffUrl({
+ diffView: {path: newPath.path},
+ patchNum: this.patchNum,
+ });
}
private goToEditFile() {
@@ -1504,20 +1507,14 @@
}
private updateUrlToDiffUrl(lineNum?: number, leftSide?: boolean) {
- if (!this.change) return;
- if (!this.patchNum) return;
- if (!this.changeNum) return;
- if (!this.path) return;
- const url = createDiffUrl({
- changeNum: this.changeNum,
- repo: this.change.project,
- patchNum: this.patchNum,
- basePatchNum: this.basePatchNum,
+ if (!this.path || !this.patchNum) return;
+ const url = this.getViewModel().diffUrl({
diffView: {
path: this.path,
lineNum,
leftSide,
},
+ patchNum: this.patchNum,
});
history.replaceState(null, '', url);
}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts
index 93424b1..96467b4 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view_test.ts
@@ -137,9 +137,9 @@
stubRestApi('getDiffDrafts').returns(Promise.resolve({}));
stubRestApi('getPortedComments').returns(Promise.resolve({}));
- element = await fixture(html`<gr-diff-view></gr-diff-view>`);
viewModel = testResolver(changeViewModelToken);
viewModel.setState(createDiffViewState());
+ element = await fixture(html`<gr-diff-view></gr-diff-view>`);
await waitUntil(() => element.changeNum === TEST_NUMERIC_CHANGE_ID);
element.path = 'some/path.txt';
element.change = createParsedChange();
@@ -188,16 +188,18 @@
test('renders', async () => {
browserModel.setScreenWidth(0);
- element.patchNum = 10 as RevisionPatchSetNum;
+ const patchNum = 10 as RevisionPatchSetNum;
+ element.patchNum = patchNum;
element.basePatchNum = PARENT;
const change = {
...createParsedChange(),
_number: 42 as NumericChangeId,
revisions: {
- a: createRevision(10),
+ a: createRevision(patchNum),
},
};
changeModel.updateStateChange(change);
+ viewModel.updateState({patchNum});
element.files = getFilesFromFileList([
'chell.go',
'glados.txt',
@@ -1012,14 +1014,16 @@
});
test('prev/up/next links', async () => {
+ const patchNum = 10 as RevisionPatchSetNum;
viewModel.setState({
...createDiffViewState(),
+ patchNum,
});
const change = {
...createParsedChange(),
_number: 42 as NumericChangeId,
revisions: {
- a: createRevision(10),
+ a: createRevision(patchNum),
},
};
changeModel.updateStateChange(change);
diff --git a/polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls.ts b/polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls.ts
index 1e871ce..ec8faae 100644
--- a/polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls.ts
+++ b/polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls.ts
@@ -29,7 +29,7 @@
import {customElement, property, query, state} from 'lit/decorators.js';
import {BindValueChangeEvent} from '../../../types/events';
import {IronInputElement} from '@polymer/iron-input/iron-input';
-import {createEditUrl} from '../../../models/views/change';
+import {changeViewModelToken} from '../../../models/views/change';
import {resolve} from '../../../models/dependency';
import {modalStyles} from '../../../styles/gr-modal-styles';
import {whenVisible} from '../../../utils/dom-util';
@@ -81,6 +81,8 @@
private readonly getChangeModel = resolve(this, changeModelToken);
+ private readonly getViewModel = resolve(this, changeViewModelToken);
+
private readonly getNavigation = resolve(this, navigationToken);
static override get styles() {
@@ -431,13 +433,10 @@
return;
}
assertIsDefined(this.patchNum, 'patchset number');
- const url = createEditUrl({
- changeNum: this.change._number,
- repo: this.change.project,
- patchNum: this.patchNum,
+ const url = this.getViewModel().editUrl({
editView: {path: this.path},
+ patchNum: this.patchNum,
});
-
this.getNavigation().setUrl(url);
this.closeDialog(this.getDialogFromEvent(e));
};
diff --git a/polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls_test.ts b/polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls_test.ts
index 0e6778a..aacbb36 100644
--- a/polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls_test.ts
+++ b/polygerrit-ui/app/elements/edit/gr-edit-controls/gr-edit-controls_test.ts
@@ -32,6 +32,11 @@
changeModelToken,
} from '../../../models/change/change-model';
import {SinonStubbedMember} from 'sinon';
+import {
+ ChangeChildView,
+ changeViewModelToken,
+} from '../../../models/views/change';
+import {GerritView} from '../../../services/router/router-model';
suite('gr-edit-controls tests', () => {
let element: GrEditControls;
@@ -45,6 +50,13 @@
>;
setup(async () => {
+ testResolver(changeViewModelToken).setState({
+ view: GerritView.CHANGE,
+ childView: ChangeChildView.OVERVIEW,
+ changeNum: 42 as NumericChangeId,
+ repo: 'gerrit' as RepoName,
+ });
+
element = await fixture<GrEditControls>(html`
<gr-edit-controls></gr-edit-controls>
`);
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
index 0b03581..8787d3c 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
@@ -70,7 +70,11 @@
import {commentsModelToken} from '../../../models/comments/comments-model';
import {changeModelToken} from '../../../models/change/change-model';
import {whenRendered} from '../../../utils/dom-util';
-import {createChangeUrl, createDiffUrl} from '../../../models/views/change';
+import {
+ changeViewModelToken,
+ createChangeUrl,
+ createDiffUrl,
+} from '../../../models/views/change';
import {userModelToken} from '../../../models/user/user-model';
import {highlightServiceToken} from '../../../services/highlight/highlight-service';
import {noAwait, waitUntil} from '../../../utils/async-util';
@@ -248,6 +252,8 @@
private readonly getUserModel = resolve(this, userModelToken);
+ private readonly getViewModel = resolve(this, changeViewModelToken);
+
private readonly shortcuts = new ShortcutController(this);
private readonly syntaxLayer = new GrSyntaxLayerWorker(
@@ -706,12 +712,15 @@
}
private getDiffUrlForPath() {
- if (!this.changeNum || !this.repoName || !this.thread?.path) {
+ if (
+ !this.changeNum ||
+ !this.repoName ||
+ !this.thread?.path ||
+ !this.thread?.patchNum
+ ) {
return undefined;
}
- return createDiffUrl({
- changeNum: this.changeNum,
- repo: this.repoName,
+ return this.getViewModel().diffUrl({
patchNum: this.thread.patchNum,
diffView: {path: this.thread.path},
});
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
index 571a322..b56fcfd 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_test.ts
@@ -38,6 +38,11 @@
commentsModelToken,
} from '../../../models/comments/comments-model';
import {testResolver} from '../../../test/common-test-setup';
+import {
+ ChangeChildView,
+ changeViewModelToken,
+} from '../../../models/views/change';
+import {GerritView} from '../../../services/router/router-model';
const c1: CommentInfo = {
author: {name: 'Kermit'},
@@ -80,6 +85,12 @@
let element: GrCommentThread;
setup(async () => {
+ testResolver(changeViewModelToken).setState({
+ view: GerritView.CHANGE,
+ childView: ChangeChildView.OVERVIEW,
+ changeNum: 1 as NumericChangeId,
+ repo: 'test-repo-name' as RepoName,
+ });
stubRestApi('getLoggedIn').returns(Promise.resolve(false));
element = await fixture(html`<gr-comment-thread></gr-comment-thread>`);
element.changeNum = 1 as NumericChangeId;
diff --git a/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content.ts b/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content.ts
index 3964422..eeccda6 100644
--- a/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content.ts
+++ b/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content.ts
@@ -43,7 +43,7 @@
import {storageServiceToken} from '../../../services/storage/gr-storage_impl';
import {resolve} from '../../../models/dependency';
import {formStyles} from '../../../styles/form-styles';
-import {createEditUrl} from '../../../models/views/change';
+import {changeViewModelToken} from '../../../models/views/change';
import {SpecialFilePath} from '../../../constants/constants';
const RESTORED_MESSAGE = 'Content restored from a previous edit.';
@@ -128,6 +128,8 @@
private readonly getNavigation = resolve(this, navigationToken);
+ private readonly getViewModel = resolve(this, changeViewModelToken);
+
// Tests use this so needs to be non private
storeTask?: DelayedTask;
@@ -500,12 +502,11 @@
if (this.editMode) {
assertIsDefined(this.changeNum, 'changeNum');
assertIsDefined(this.repoName, 'repoName');
+ assertIsDefined(this.patchNum, 'patchNum');
this.getNavigation().setUrl(
- createEditUrl({
- changeNum: this.changeNum,
- repo: this.repoName,
- patchNum: this.patchNum,
+ this.getViewModel().editUrl({
editView: {path: SpecialFilePath.COMMIT_MESSAGE},
+ patchNum: this.patchNum,
})
);
diff --git a/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content_test.ts b/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content_test.ts
index e8ccdd1..dbb89db 100644
--- a/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-editable-content/gr-editable-content_test.ts
@@ -19,6 +19,7 @@
RepoName,
RevisionPatchSetNum,
} from '../../../api/rest-api';
+import {changeViewModelToken} from '../../../models/views/change';
const emails = [
{
@@ -189,6 +190,11 @@
suite('in editMode', () => {
test('click opens edit url', async () => {
+ const editUrlStub = sinon.stub(
+ testResolver(changeViewModelToken),
+ 'editUrl'
+ );
+ editUrlStub.returns('fakeUrl');
const setUrlStub = sinon.stub(testResolver(navigationToken), 'setUrl');
element.editMode = true;
element.changeNum = 42 as NumericChangeId;
@@ -201,10 +207,7 @@
);
editButton.click();
assert.isTrue(setUrlStub.called);
- assert.equal(
- setUrlStub.lastCall.args[0],
- '/c/Test+Repo/+/42/1//COMMIT_MSG,edit'
- );
+ assert.equal(setUrlStub.lastCall.args[0], 'fakeUrl');
});
});
});
diff --git a/polygerrit-ui/app/models/change/change-model.ts b/polygerrit-ui/app/models/change/change-model.ts
index 373f199..04fa0d6 100644
--- a/polygerrit-ui/app/models/change/change-model.ts
+++ b/polygerrit-ui/app/models/change/change-model.ts
@@ -46,8 +46,6 @@
ChangeChildView,
ChangeViewModel,
createChangeUrl,
- createDiffUrl,
- createEditUrl,
} from '../views/change';
import {NavigationService} from '../../elements/core/gr-navigation/gr-navigation';
import {getRevertCreatedChangeIds} from '../../utils/message-util';
@@ -657,27 +655,13 @@
return this.getState().change;
}
- diffUrl(
- diffView: {path: string; lineNum?: number},
- patchNum = this.patchNum,
- basePatchNum = this.basePatchNum
- ) {
- if (!this.change) return;
- if (!this.patchNum) return;
- return createDiffUrl({
- change: this.change,
- patchNum,
- basePatchNum,
- diffView,
- });
- }
-
navigateToDiff(
diffView: {path: string; lineNum?: number},
patchNum = this.patchNum,
basePatchNum = this.basePatchNum
) {
- const url = this.diffUrl(diffView, patchNum, basePatchNum);
+ if (!patchNum) return;
+ const url = this.viewModel.diffUrl({diffView, patchNum, basePatchNum});
if (!url) return;
this.navigation.setUrl(url);
}
@@ -715,18 +699,9 @@
this.navigation.setUrl(url);
}
- editUrl(editView: {path: string; lineNum?: number}) {
- if (!this.change) return;
- return createEditUrl({
- changeNum: this.change._number,
- repo: this.change.project,
- patchNum: this.patchNum,
- editView,
- });
- }
-
navigateToEdit(editView: {path: string; lineNum?: number}) {
- const url = this.editUrl(editView);
+ if (!this.patchNum) return;
+ const url = this.viewModel.editUrl({editView, patchNum: this.patchNum});
if (!url) return;
this.navigation.setUrl(url);
}
diff --git a/polygerrit-ui/app/models/views/change.ts b/polygerrit-ui/app/models/views/change.ts
index 06d981a..661c74f 100644
--- a/polygerrit-ui/app/models/views/change.ts
+++ b/polygerrit-ui/app/models/views/change.ts
@@ -11,11 +11,12 @@
ChangeInfo,
PatchSetNumber,
EDIT,
+ PARENT,
} from '../../api/rest-api';
import {Tab} from '../../constants/constants';
import {GerritView} from '../../services/router/router-model';
import {UrlEncodedCommentId} from '../../types/common';
-import {toggleSet} from '../../utils/common-util';
+import {assertIsDefined, toggleSet} from '../../utils/common-util';
import {select} from '../../utils/observable-util';
import {
encodeURL,
@@ -26,6 +27,7 @@
import {define} from '../dependency';
import {Model} from '../base/model';
import {ViewState} from './base';
+import {isNumber} from '../../utils/patch-set-util';
export enum ChangeChildView {
OVERVIEW = 'OVERVIEW',
@@ -85,7 +87,7 @@
/** These properties apply to the DIFF child view only. */
diffView?: {
- path?: string;
+ path: string;
// TODO: Use LineNumber as a type, i.e. accept FILE and LOST.
lineNum?: number;
leftSide?: boolean;
@@ -93,11 +95,28 @@
/** These properties apply to the EDIT child view only. */
editView?: {
- path?: string;
+ path: string;
lineNum?: number;
};
}
+export type DiffViewState = Partial<ChangeViewState> & {
+ patchNum: RevisionPatchSetNum;
+ diffView: {
+ path: string;
+ lineNum?: number;
+ leftSide?: boolean;
+ };
+};
+
+export type EditViewState = Partial<ChangeViewState> & {
+ patchNum: RevisionPatchSetNum;
+ editView: {
+ path: string;
+ lineNum?: number;
+ };
+};
+
/**
* This is a convenience type such that you can pass a `ChangeInfo` object
* as the `change` property instead of having to set both the `changeNum` and
@@ -145,7 +164,7 @@
export function createChangeUrl(
obj: CreateChangeUrlObject | Omit<ChangeViewState, 'view' | 'childView'>
-) {
+): string {
const state: ChangeViewState = objToState({
...obj,
childView: ChangeChildView.OVERVIEW,
@@ -198,7 +217,7 @@
export function createDiffUrl(
obj: CreateChangeUrlObject | Omit<ChangeViewState, 'view' | 'childView'>
-) {
+): string {
const state: ChangeViewState = objToState({
...obj,
childView: ChangeChildView.DIFF,
@@ -378,6 +397,49 @@
}
}
+ /**
+ * Wrapper around createDiffUrl() that falls back to the current state for all
+ * properties that are not explicitly provided as an override.
+ */
+ diffUrl(override: DiffViewState): string {
+ const current = this.getState();
+ assertIsDefined(current?.changeNum);
+ assertIsDefined(current?.repo);
+
+ const patchNum = override.patchNum ?? current.patchNum;
+ let basePatchNum = override.basePatchNum ?? current.basePatchNum;
+ if (isNumber(basePatchNum) && isNumber(patchNum)) {
+ if ((patchNum as number) <= (basePatchNum as number)) {
+ basePatchNum = PARENT;
+ }
+ }
+ return createDiffUrl({
+ changeNum: override.changeNum ?? current.changeNum,
+ repo: override.repo ?? current.repo,
+ patchNum,
+ basePatchNum,
+ checksPatchset: override.checksPatchset ?? current.checksPatchset,
+ diffView: override.diffView ?? current.diffView,
+ });
+ }
+
+ /**
+ * Wrapper around createEditUrl() that falls back to the current state for all
+ * properties that are not explicitly provided as an override.
+ */
+ editUrl(override: EditViewState): string {
+ const current = this.getState();
+ assertIsDefined(current?.changeNum);
+ assertIsDefined(current?.repo);
+
+ return createEditUrl({
+ changeNum: override.changeNum ?? current.changeNum,
+ repo: override.repo ?? current.repo,
+ patchNum: override.patchNum ?? current.patchNum,
+ editView: override.editView ?? current.editView,
+ });
+ }
+
toggleSelectedCheckRun(checkName: string) {
const current = this.getState()?.checksRunsSelected ?? new Set();
const next = new Set(current);
diff --git a/polygerrit-ui/app/utils/patch-set-util.ts b/polygerrit-ui/app/utils/patch-set-util.ts
index 7a5cd45..dc7adc1 100644
--- a/polygerrit-ui/app/utils/patch-set-util.ts
+++ b/polygerrit-ui/app/utils/patch-set-util.ts
@@ -70,7 +70,7 @@
return patchset as PatchSetNum;
}
-export function isNumber(psn: PatchSetNum): psn is PatchSetNumber {
+export function isNumber(psn?: PatchSetNum): psn is PatchSetNumber {
return typeof psn === 'number';
}
