Merge changes I87b6035a,I897cb105
* changes:
Convert gr-textarea_test.js to typescript
Fix template problems with gr-textarea
diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 7ddf2ba..f476566 100644
--- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -369,11 +369,11 @@
if (commonServer != null) {
try {
commonServer.close();
- } catch (Throwable t) {
+ } catch (Exception e) {
throw new AssertionError(
"Error stopping common server in "
+ (firstTest != null ? firstTest.getTestClass().getName() : "unknown test class"),
- t);
+ e);
} finally {
commonServer = null;
}
diff --git a/java/com/google/gerrit/entities/Change.java b/java/com/google/gerrit/entities/Change.java
index ca13db9..d1826bc 100644
--- a/java/com/google/gerrit/entities/Change.java
+++ b/java/com/google/gerrit/entities/Change.java
@@ -443,9 +443,6 @@
/** Globally assigned unique identifier of the change */
protected Key changeKey;
- /** optimistic locking */
- protected int rowVersion;
-
/** When this change was first introduced into the database. */
protected Timestamp createdOn;
@@ -526,7 +523,6 @@
assignee = other.assignee;
changeId = other.changeId;
changeKey = other.changeKey;
- rowVersion = other.rowVersion;
createdOn = other.createdOn;
lastUpdatedOn = other.lastUpdatedOn;
owner = other.owner;
@@ -587,10 +583,6 @@
lastUpdatedOn = now;
}
- public int getRowVersion() {
- return rowVersion;
- }
-
public Account.Id getOwner() {
return owner;
}
diff --git a/java/com/google/gerrit/entities/converter/ChangeProtoConverter.java b/java/com/google/gerrit/entities/converter/ChangeProtoConverter.java
index 25e68f9..689b4aa 100644
--- a/java/com/google/gerrit/entities/converter/ChangeProtoConverter.java
+++ b/java/com/google/gerrit/entities/converter/ChangeProtoConverter.java
@@ -43,7 +43,6 @@
Entities.Change.Builder builder =
Entities.Change.newBuilder()
.setChangeId(changeIdConverter.toProto(change.getId()))
- .setRowVersion(change.getRowVersion())
.setChangeKey(changeKeyConverter.toProto(change.getKey()))
.setCreatedOn(change.getCreatedOn().getTime())
.setLastUpdatedOn(change.getLastUpdatedOn().getTime())
diff --git a/java/com/google/gerrit/httpd/BUILD b/java/com/google/gerrit/httpd/BUILD
index cd3ebb9..ea7c609 100644
--- a/java/com/google/gerrit/httpd/BUILD
+++ b/java/com/google/gerrit/httpd/BUILD
@@ -20,6 +20,7 @@
"//java/com/google/gerrit/metrics",
"//java/com/google/gerrit/server",
"//java/com/google/gerrit/server/audit",
+ "//java/com/google/gerrit/server/cancellation",
"//java/com/google/gerrit/server/git/receive",
"//java/com/google/gerrit/server/ioutil",
"//java/com/google/gerrit/server/logging",
diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index 7e6ab58..375aa54 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -114,6 +114,7 @@
import com.google.gerrit.server.audit.ExtendedHttpAuditEvent;
import com.google.gerrit.server.cache.PerThreadCache;
import com.google.gerrit.server.cancellation.RequestCancelledException;
+import com.google.gerrit.server.cancellation.RequestStateProvider;
import com.google.gerrit.server.change.ChangeFinder;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.config.GerritServerConfig;
@@ -712,50 +713,45 @@
messageOr(e, "Quota limit reached"),
e.caching(),
e);
- } catch (RequestCancelledException e) {
- cause = Optional.of(e);
- switch (e.getCancellationReason()) {
- case CLIENT_CLOSED_REQUEST:
- statusCode = SC_CLIENT_CLOSED_REQUEST;
- break;
- case CLIENT_PROVIDED_DEADLINE_EXCEEDED:
- case SERVER_DEADLINE_EXCEEDED:
- statusCode = SC_REQUEST_TIMEOUT;
- break;
- }
-
- StringBuilder msg = new StringBuilder(e.formatCancellationReason());
- if (e.getCancellationMessage().isPresent()) {
- msg.append("\n\n");
- msg.append(e.getCancellationMessage().get());
- }
-
- responseBytes = replyError(req, res, statusCode, msg.toString(), e);
} catch (Exception e) {
cause = Optional.of(e);
- statusCode = SC_INTERNAL_SERVER_ERROR;
- Optional<ExceptionHook.Status> status = getStatus(e);
- statusCode = status.map(ExceptionHook.Status::statusCode).orElse(SC_INTERNAL_SERVER_ERROR);
-
- if (res.isCommitted()) {
- responseBytes = 0;
- if (statusCode == SC_INTERNAL_SERVER_ERROR) {
- logger.atSevere().withCause(e).log(
- "Error in %s %s, response already committed", req.getMethod(), uriForLogging(req));
- } else {
- logger.atWarning().log(
- "Response for %s %s already committed, wanted to set status %d",
- req.getMethod(), uriForLogging(req), statusCode);
- }
+ Optional<RequestCancelledException> requestCancelledException =
+ RequestCancelledException.getFromCausalChain(e);
+ if (requestCancelledException.isPresent()) {
+ statusCode =
+ getCancellationStatusCode(requestCancelledException.get().getCancellationReason());
+ responseBytes =
+ replyError(
+ req, res, statusCode, getCancellationMessage(requestCancelledException.get()), e);
} else {
- res.reset();
- traceContext.getTraceId().ifPresent(traceId -> res.addHeader(X_GERRIT_TRACE, traceId));
+ statusCode = SC_INTERNAL_SERVER_ERROR;
- if (status.isPresent()) {
- responseBytes = reply(req, res, e, status.get(), getUserMessages(traceContext, e));
+ Optional<ExceptionHook.Status> status = getStatus(e);
+ statusCode =
+ status.map(ExceptionHook.Status::statusCode).orElse(SC_INTERNAL_SERVER_ERROR);
+
+ if (res.isCommitted()) {
+ responseBytes = 0;
+ if (statusCode == SC_INTERNAL_SERVER_ERROR) {
+ logger.atSevere().withCause(e).log(
+ "Error in %s %s, response already committed",
+ req.getMethod(), uriForLogging(req));
+ } else {
+ logger.atWarning().log(
+ "Response for %s %s already committed, wanted to set status %d",
+ req.getMethod(), uriForLogging(req), statusCode);
+ }
} else {
- responseBytes = replyInternalServerError(req, res, e, getUserMessages(traceContext, e));
+ res.reset();
+ traceContext.getTraceId().ifPresent(traceId -> res.addHeader(X_GERRIT_TRACE, traceId));
+
+ if (status.isPresent()) {
+ responseBytes = reply(req, res, e, status.get(), getUserMessages(traceContext, e));
+ } else {
+ responseBytes =
+ replyInternalServerError(req, res, e, getUserMessages(traceContext, e));
+ }
}
}
} finally {
@@ -1961,6 +1957,28 @@
return replyBinaryResult(req, res, BinaryResult.create(text).setContentType(PLAIN_TEXT));
}
+ private static int getCancellationStatusCode(RequestStateProvider.Reason cancellationReason) {
+ switch (cancellationReason) {
+ case CLIENT_CLOSED_REQUEST:
+ return SC_CLIENT_CLOSED_REQUEST;
+ case CLIENT_PROVIDED_DEADLINE_EXCEEDED:
+ case SERVER_DEADLINE_EXCEEDED:
+ return SC_REQUEST_TIMEOUT;
+ }
+ logger.atSevere().log("Unexpected cancellation reason: %s", cancellationReason);
+ return SC_INTERNAL_SERVER_ERROR;
+ }
+
+ private static String getCancellationMessage(
+ RequestCancelledException requestCancelledException) {
+ StringBuilder msg = new StringBuilder(requestCancelledException.formatCancellationReason());
+ if (requestCancelledException.getCancellationMessage().isPresent()) {
+ msg.append("\n\n");
+ msg.append(requestCancelledException.getCancellationMessage().get());
+ }
+ return msg.toString();
+ }
+
private static boolean acceptsGzip(HttpServletRequest req) {
if (req != null) {
String accepts = req.getHeader(HttpHeaders.ACCEPT_ENCODING);
diff --git a/java/com/google/gerrit/index/testing/AbstractFakeIndex.java b/java/com/google/gerrit/index/testing/AbstractFakeIndex.java
index 5cc8e3c..b727e96 100644
--- a/java/com/google/gerrit/index/testing/AbstractFakeIndex.java
+++ b/java/com/google/gerrit/index/testing/AbstractFakeIndex.java
@@ -36,6 +36,8 @@
import com.google.gerrit.index.query.Predicate;
import com.google.gerrit.index.query.ResultSet;
import com.google.gerrit.server.account.AccountState;
+import com.google.gerrit.server.change.MergeabilityComputationBehavior;
+import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.SitePaths;
import com.google.gerrit.server.index.IndexUtils;
import com.google.gerrit.server.index.account.AccountIndex;
@@ -50,6 +52,7 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import org.eclipse.jgit.lib.Config;
/**
* Fake secondary index implementation for usage in tests. All values are kept in-memory.
@@ -179,14 +182,17 @@
public static class FakeChangeIndex
extends AbstractFakeIndex<Change.Id, ChangeData, Map<String, Object>> implements ChangeIndex {
private final ChangeData.Factory changeDataFactory;
+ private final boolean skipMergable;
@Inject
FakeChangeIndex(
SitePaths sitePaths,
ChangeData.Factory changeDataFactory,
- @Assisted Schema<ChangeData> schema) {
+ @Assisted Schema<ChangeData> schema,
+ @GerritServerConfig Config cfg) {
super(schema, sitePaths, "changes");
this.changeDataFactory = changeDataFactory;
+ this.skipMergable = !MergeabilityComputationBehavior.fromConfig(cfg).includeInIndex();
}
@Override
@@ -208,6 +214,9 @@
protected Map<String, Object> docFor(ChangeData value) {
ImmutableMap.Builder<String, Object> doc = ImmutableMap.builder();
for (FieldDef<ChangeData, ?> field : getSchema().getFields().values()) {
+ if (ChangeField.MERGEABLE.getName().equals(field.getName()) && skipMergable) {
+ continue;
+ }
Object docifiedValue = field.get(value);
if (docifiedValue != null) {
doc.put(field.getName(), field.get(value));
diff --git a/java/com/google/gerrit/lifecycle/LifecycleManager.java b/java/com/google/gerrit/lifecycle/LifecycleManager.java
index 4f09a09..42123d7 100644
--- a/java/com/google/gerrit/lifecycle/LifecycleManager.java
+++ b/java/com/google/gerrit/lifecycle/LifecycleManager.java
@@ -107,7 +107,7 @@
LifecycleListener obj = listeners.get(i).get();
try {
obj.stop();
- } catch (Throwable err) {
+ } catch (RuntimeException err) {
logger.atWarning().withCause(err).log("Failed to stop %s", obj.getClass());
}
startedIndex = i - 1;
diff --git a/java/com/google/gerrit/pgm/Daemon.java b/java/com/google/gerrit/pgm/Daemon.java
index 2b4cfef..a3605f7 100644
--- a/java/com/google/gerrit/pgm/Daemon.java
+++ b/java/com/google/gerrit/pgm/Daemon.java
@@ -310,7 +310,7 @@
RuntimeShutdown.waitFor();
}
return 0;
- } catch (Throwable err) {
+ } catch (RuntimeException err) {
logger.atSevere().withCause(err).log("Unable to start daemon");
return 1;
}
diff --git a/java/com/google/gerrit/pgm/http/jetty/JettyServer.java b/java/com/google/gerrit/pgm/http/jetty/JettyServer.java
index 89b4228..6f3514f 100644
--- a/java/com/google/gerrit/pgm/http/jetty/JettyServer.java
+++ b/java/com/google/gerrit/pgm/http/jetty/JettyServer.java
@@ -547,7 +547,7 @@
filterHolder.setInitParameters(initParams);
}
app.addFilter(filterHolder, "/*", EnumSet.of(DispatcherType.REQUEST, DispatcherType.ASYNC));
- } catch (Throwable e) {
+ } catch (Exception e) {
throw new IllegalArgumentException(
"Unable to instantiate front-end HTTP Filter " + filterClassName, e);
}
diff --git a/java/com/google/gerrit/server/RequestCleanup.java b/java/com/google/gerrit/server/RequestCleanup.java
index f405c57..e07d148 100644
--- a/java/com/google/gerrit/server/RequestCleanup.java
+++ b/java/com/google/gerrit/server/RequestCleanup.java
@@ -44,7 +44,7 @@
for (Iterator<Runnable> i = cleanup.iterator(); i.hasNext(); ) {
try {
i.next().run();
- } catch (Throwable err) {
+ } catch (Exception err) {
logger.atSevere().withCause(err).log("Failed to execute per-request cleanup");
}
i.remove();
diff --git a/java/com/google/gerrit/server/account/externalids/ExternalIdNotes.java b/java/com/google/gerrit/server/account/externalids/ExternalIdNotes.java
index 50a2f69..435a524 100644
--- a/java/com/google/gerrit/server/account/externalids/ExternalIdNotes.java
+++ b/java/com/google/gerrit/server/account/externalids/ExternalIdNotes.java
@@ -29,6 +29,7 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.git.ObjectIds;
import com.google.gerrit.metrics.Counter0;
import com.google.gerrit.metrics.Description;
@@ -97,12 +98,17 @@
protected final ExternalIdCache externalIdCache;
protected final MetricMaker metricMaker;
protected final AllUsersName allUsersName;
+ protected final DynamicMap<ExternalIdUpsertPreprocessor> upsertPreprocessors;
protected ExternalIdNotesLoader(
- ExternalIdCache externalIdCache, MetricMaker metricMaker, AllUsersName allUsersName) {
+ ExternalIdCache externalIdCache,
+ MetricMaker metricMaker,
+ AllUsersName allUsersName,
+ DynamicMap<ExternalIdUpsertPreprocessor> upsertPreprocessors) {
this.externalIdCache = externalIdCache;
this.metricMaker = metricMaker;
this.allUsersName = allUsersName;
+ this.upsertPreprocessors = upsertPreprocessors;
}
/**
@@ -192,21 +198,24 @@
ExternalIdCache externalIdCache,
Provider<AccountIndexer> accountIndexer,
MetricMaker metricMaker,
- AllUsersName allUsersName) {
- super(externalIdCache, metricMaker, allUsersName);
+ AllUsersName allUsersName,
+ DynamicMap<ExternalIdUpsertPreprocessor> upsertPreprocessors) {
+ super(externalIdCache, metricMaker, allUsersName, upsertPreprocessors);
this.accountIndexer = accountIndexer;
}
@Override
public ExternalIdNotes load(Repository allUsersRepo)
throws IOException, ConfigInvalidException {
- return new ExternalIdNotes(metricMaker, allUsersName, allUsersRepo).load();
+ return new ExternalIdNotes(metricMaker, allUsersName, allUsersRepo, upsertPreprocessors)
+ .load();
}
@Override
public ExternalIdNotes load(Repository allUsersRepo, @Nullable ObjectId rev)
throws IOException, ConfigInvalidException {
- return new ExternalIdNotes(metricMaker, allUsersName, allUsersRepo).load(rev);
+ return new ExternalIdNotes(metricMaker, allUsersName, allUsersRepo, upsertPreprocessors)
+ .load(rev);
}
@Override
@@ -220,20 +229,27 @@
@Inject
FactoryNoReindex(
- ExternalIdCache externalIdCache, MetricMaker metricMaker, AllUsersName allUsersName) {
- super(externalIdCache, metricMaker, allUsersName);
+ ExternalIdCache externalIdCache,
+ MetricMaker metricMaker,
+ AllUsersName allUsersName,
+ DynamicMap<ExternalIdUpsertPreprocessor> upsertPreprocessors) {
+ super(externalIdCache, metricMaker, allUsersName, upsertPreprocessors);
}
@Override
public ExternalIdNotes load(Repository allUsersRepo)
throws IOException, ConfigInvalidException {
- return new ExternalIdNotes(metricMaker, allUsersName, allUsersRepo).setNoReindex().load();
+ return new ExternalIdNotes(metricMaker, allUsersName, allUsersRepo, upsertPreprocessors)
+ .setNoReindex()
+ .load();
}
@Override
public ExternalIdNotes load(Repository allUsersRepo, @Nullable ObjectId rev)
throws IOException, ConfigInvalidException {
- return new ExternalIdNotes(metricMaker, allUsersName, allUsersRepo).setNoReindex().load(rev);
+ return new ExternalIdNotes(metricMaker, allUsersName, allUsersRepo, upsertPreprocessors)
+ .setNoReindex()
+ .load(rev);
}
@Override
@@ -255,7 +271,8 @@
public static ExternalIdNotes loadReadOnly(
AllUsersName allUsersName, Repository allUsersRepo, @Nullable ObjectId rev)
throws IOException, ConfigInvalidException {
- return new ExternalIdNotes(new DisabledMetricMaker(), allUsersName, allUsersRepo)
+ return new ExternalIdNotes(
+ new DisabledMetricMaker(), allUsersName, allUsersRepo, DynamicMap.emptyMap())
.setReadOnly()
.setNoCacheUpdate()
.setNoReindex()
@@ -275,7 +292,8 @@
public static ExternalIdNotes loadNoCacheUpdate(
AllUsersName allUsersName, Repository allUsersRepo)
throws IOException, ConfigInvalidException {
- return new ExternalIdNotes(new DisabledMetricMaker(), allUsersName, allUsersRepo)
+ return new ExternalIdNotes(
+ new DisabledMetricMaker(), allUsersName, allUsersRepo, DynamicMap.emptyMap())
.setNoCacheUpdate()
.setNoReindex()
.load();
@@ -284,6 +302,7 @@
private final AllUsersName allUsersName;
private final Counter0 updateCount;
private final Repository repo;
+ private final DynamicMap<ExternalIdUpsertPreprocessor> upsertPreprocessors;
private final CallerFinder callerFinder;
private NoteMap noteMap;
@@ -312,13 +331,17 @@
private boolean noReindex = false;
private ExternalIdNotes(
- MetricMaker metricMaker, AllUsersName allUsersName, Repository allUsersRepo) {
+ MetricMaker metricMaker,
+ AllUsersName allUsersName,
+ Repository allUsersRepo,
+ DynamicMap<ExternalIdUpsertPreprocessor> upsertPreprocessors) {
this.updateCount =
metricMaker.newCounter(
"notedb/external_id_update_count",
new Description("Total number of external ID updates.").setRate().setUnit("updates"));
this.allUsersName = requireNonNull(allUsersName, "allUsersRepo");
this.repo = requireNonNull(allUsersRepo, "allUsersRepo");
+ this.upsertPreprocessors = upsertPreprocessors;
this.callerFinder =
CallerFinder.builder()
// 1. callers that come through ExternalIds
@@ -490,6 +513,7 @@
(rw, n) -> {
for (ExternalId extId : extIds) {
ExternalId insertedExtId = upsert(rw, inserter, noteMap, extId);
+ preprocessUpsert(insertedExtId);
newExtIds.add(insertedExtId);
}
});
@@ -519,6 +543,7 @@
(rw, n) -> {
for (ExternalId extId : extIds) {
ExternalId updatedExtId = upsert(rw, inserter, noteMap, extId);
+ preprocessUpsert(updatedExtId);
updatedExtIds.add(updatedExtId);
}
});
@@ -634,6 +659,7 @@
for (ExternalId extId : toAdd) {
ExternalId insertedExtId = upsert(rw, inserter, noteMap, extId);
+ preprocessUpsert(insertedExtId);
updatedExtIds.add(insertedExtId);
}
});
@@ -667,6 +693,7 @@
for (ExternalId extId : toAdd) {
ExternalId insertedExtId = upsert(rw, inserter, noteMap, extId);
+ preprocessUpsert(insertedExtId);
updatedExtIds.add(insertedExtId);
}
});
@@ -809,9 +836,9 @@
}
/**
- * Insert or updates an new external ID and sets it in the note map.
+ * Inserts or updates a new external ID and sets it in the note map.
*
- * <p>If the external ID already exists it is overwritten.
+ * <p>If the external ID already exists, it is overwritten.
*/
private static ExternalId upsert(
RevWalk rw, ObjectInserter ins, NoteMap noteMap, ExternalId extId)
@@ -867,6 +894,7 @@
* @return the external ID that was removed, {@code null} if no external ID with the specified key
* exists
*/
+ @Nullable
private static ExternalId remove(
RevWalk rw, NoteMap noteMap, ExternalId.Key extIdKey, Account.Id expectedAccountId)
throws IOException, ConfigInvalidException {
@@ -917,6 +945,10 @@
checkState(noteMap != null, "External IDs not loaded yet");
}
+ private void preprocessUpsert(ExternalId extId) {
+ upsertPreprocessors.forEach(p -> p.get().upsert(extId));
+ }
+
@FunctionalInterface
private interface NoteMapUpdate {
void execute(RevWalk rw, NoteMap noteMap)
diff --git a/java/com/google/gerrit/server/account/externalids/ExternalIdUpsertPreprocessor.java b/java/com/google/gerrit/server/account/externalids/ExternalIdUpsertPreprocessor.java
new file mode 100644
index 0000000..c0697db
--- /dev/null
+++ b/java/com/google/gerrit/server/account/externalids/ExternalIdUpsertPreprocessor.java
@@ -0,0 +1,30 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.account.externalids;
+
+import com.google.gerrit.extensions.annotations.ExtensionPoint;
+
+/**
+ * This optional preprocessor is called in {@link ExternalIdNotes} before an update is committed.
+ */
+@ExtensionPoint
+public interface ExternalIdUpsertPreprocessor {
+ /**
+ * Called when inserting or updating an external ID. {@link ExternalId#blobId()} is set. The
+ * upsert can be blocked by throwing {@link com.google.gerrit.exceptions.StorageException}, e.g.
+ * when a precondition or preparatory work fails.
+ */
+ void upsert(ExternalId extId);
+}
diff --git a/java/com/google/gerrit/server/cancellation/BUILD b/java/com/google/gerrit/server/cancellation/BUILD
new file mode 100644
index 0000000..05530a5
--- /dev/null
+++ b/java/com/google/gerrit/server/cancellation/BUILD
@@ -0,0 +1,14 @@
+load("@rules_java//java:defs.bzl", "java_library")
+
+java_library(
+ name = "cancellation",
+ srcs = glob(
+ ["*.java"],
+ ),
+ visibility = ["//visibility:public"],
+ deps = [
+ "//java/com/google/gerrit/common:annotations",
+ "//lib:guava",
+ "//lib/commons:lang",
+ ],
+)
diff --git a/java/com/google/gerrit/server/cancellation/RequestCancelledException.java b/java/com/google/gerrit/server/cancellation/RequestCancelledException.java
index 3c668fb..d89701f 100644
--- a/java/com/google/gerrit/server/cancellation/RequestCancelledException.java
+++ b/java/com/google/gerrit/server/cancellation/RequestCancelledException.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.cancellation;
+import com.google.common.base.Throwables;
import com.google.gerrit.common.Nullable;
import java.util.Optional;
import org.apache.commons.lang.WordUtils;
@@ -22,6 +23,17 @@
public class RequestCancelledException extends RuntimeException {
private static final long serialVersionUID = 1L;
+ /**
+ * Checks whether the given exception was caused by {@link RequestCancelledException}. If yes, the
+ * {@link RequestCancelledException} is returned. If not, {@link Optional#empty()} is returned.
+ */
+ public static Optional<RequestCancelledException> getFromCausalChain(Throwable e) {
+ return Throwables.getCausalChain(e).stream()
+ .filter(RequestCancelledException.class::isInstance)
+ .map(RequestCancelledException.class::cast)
+ .findFirst();
+ }
+
private final RequestStateProvider.Reason cancellationReason;
private final Optional<String> cancellationMessage;
diff --git a/java/com/google/gerrit/server/cancellation/RequestStateContext.java b/java/com/google/gerrit/server/cancellation/RequestStateContext.java
new file mode 100644
index 0000000..183d779
--- /dev/null
+++ b/java/com/google/gerrit/server/cancellation/RequestStateContext.java
@@ -0,0 +1,136 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.cancellation;
+
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableSet;
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * Context that allows to register {@link RequestStateProvider}s.
+ *
+ * <p>The registered {@link RequestStateProvider}s are stored in {@link ThreadLocal} so that they
+ * can be accessed during the request execution (via {@link #getRequestStateProviders()}.
+ *
+ * <p>On {@link #close()} the {@link RequestStateProvider}s that have been registered by this {@code
+ * RequestStateContext} instance are removed from {@link ThreadLocal}.
+ *
+ * <p>Nesting {@code RequestStateContext}s is possible.
+ *
+ * <p>Currently there is no logic to automatically copy the {@link RequestStateContext} to
+ * background threads, but implementing this may be considered in the future. This means that by
+ * default we only support cancellation of the main thread, but not of background threads. That's
+ * fine as all significant work is being done in the main thread.
+ *
+ * <p>{@link com.google.gerrit.server.util.RequestContext} is also a context that is available for
+ * the time of the request, but it is not suitable to manage registrations of {@link
+ * RequestStateProvider}s. Hence {@link RequestStateProvider} registrations are managed by a
+ * separate context, which is this class, {@link RequestStateContext}:
+ *
+ * <ul>
+ * <li>{@link com.google.gerrit.server.util.RequestContext} is an interface that has many
+ * implementations and hence cannot manage a {@link ThreadLocal} state.
+ * <li>{@link com.google.gerrit.server.util.RequestContext} is not an {@link AutoCloseable} and
+ * hence cannot cleanup any {@link ThreadLocal} state on close (turning it into an {@link
+ * AutoCloseable} would require a large refactoring).
+ * <li>Despite the name {@link com.google.gerrit.server.util.RequestContext} is not only used for
+ * requests scopes but also for other scopes that are not a request (e.g. plugin invocations,
+ * email sending, manual scopes).
+ * <li>{@link com.google.gerrit.server.util.RequestContext} is not copied to background and should
+ * not be, but for {@link RequestStateContext} we may consider doing this in the future.
+ * </ul>
+ */
+public class RequestStateContext implements AutoCloseable {
+ /** The {@link RequestStateProvider}s that have been registered for the thread. */
+ private static final ThreadLocal<Set<RequestStateProvider>> threadLocalRequestStateProviders =
+ new ThreadLocal<>();
+
+ /**
+ * Aborts the current request by throwing a {@link RequestCancelledException} if any of the
+ * registered {@link RequestStateProvider}s reports the request as cancelled.
+ *
+ * @throws RequestCancelledException thrown if the current request is cancelled and should be
+ * aborted
+ */
+ public static void abortIfCancelled() throws RequestCancelledException {
+ getRequestStateProviders()
+ .forEach(
+ requestStateProvider ->
+ requestStateProvider.checkIfCancelled(
+ (reason, message) -> {
+ throw new RequestCancelledException(reason, message);
+ }));
+ }
+
+ /** Returns the {@link RequestStateProvider}s that have been registered for the thread. */
+ @VisibleForTesting
+ static ImmutableSet<RequestStateProvider> getRequestStateProviders() {
+ if (threadLocalRequestStateProviders.get() == null) {
+ return ImmutableSet.of();
+ }
+ return ImmutableSet.copyOf(threadLocalRequestStateProviders.get());
+ }
+
+ /** Opens a {@code RequestStateContext}. */
+ public static RequestStateContext open() {
+ return new RequestStateContext();
+ }
+
+ /**
+ * The {@link RequestStateProvider}s that have been registered by this {@code
+ * RequestStateContext}.
+ */
+ private Set<RequestStateProvider> requestStateProviders = new HashSet<>();
+
+ private RequestStateContext() {}
+
+ /**
+ * Registers a {@link RequestStateProvider}.
+ *
+ * @param requestStateProvider the {@link RequestStateProvider} that should be registered
+ * @return the {@code RequestStateContext} instance for chaining calls
+ */
+ public RequestStateContext addRequestStateProvider(RequestStateProvider requestStateProvider) {
+ if (threadLocalRequestStateProviders.get() == null) {
+ threadLocalRequestStateProviders.set(new HashSet<>());
+ }
+ if (threadLocalRequestStateProviders.get().add(requestStateProvider)) {
+ requestStateProviders.add(requestStateProvider);
+ }
+ return this;
+ }
+
+ /**
+ * Closes this {@code RequestStateContext}.
+ *
+ * <p>Ensures that all {@link RequestStateProvider}s that have been registered by this {@code
+ * RequestStateContext} instance are removed from {@link #threadLocalRequestStateProviders}.
+ *
+ * <p>If no {@link RequestStateProvider}s remain in {@link #threadLocalRequestStateProviders},
+ * {@link #threadLocalRequestStateProviders} is unset.
+ */
+ @Override
+ public void close() {
+ if (threadLocalRequestStateProviders.get() != null) {
+ requestStateProviders.forEach(
+ requestStateProvider ->
+ threadLocalRequestStateProviders.get().remove(requestStateProvider));
+ if (threadLocalRequestStateProviders.get().isEmpty()) {
+ threadLocalRequestStateProviders.remove();
+ }
+ }
+ }
+}
diff --git a/java/com/google/gerrit/server/cancellation/RequestStateProvider.java b/java/com/google/gerrit/server/cancellation/RequestStateProvider.java
index e1716eb..683ca1d 100644
--- a/java/com/google/gerrit/server/cancellation/RequestStateProvider.java
+++ b/java/com/google/gerrit/server/cancellation/RequestStateProvider.java
@@ -31,6 +31,7 @@
void checkIfCancelled(OnCancelled onCancelled);
/** Callback interface to be invoked if a request is cancelled. */
+ @FunctionalInterface
interface OnCancelled {
/**
* Callback that is invoked if the request is cancelled.
diff --git a/java/com/google/gerrit/server/change/AbandonUtil.java b/java/com/google/gerrit/server/change/AbandonUtil.java
index 1bc1fad..d030ec1 100644
--- a/java/com/google/gerrit/server/change/AbandonUtil.java
+++ b/java/com/google/gerrit/server/change/AbandonUtil.java
@@ -93,7 +93,7 @@
try {
batchAbandon.batchAbandon(updateFactory, project, internalUser, changes, message);
count += changes.size();
- } catch (Throwable e) {
+ } catch (Exception e) {
StringBuilder msg = new StringBuilder("Failed to auto-abandon inactive change(s):");
for (ChangeData change : changes) {
msg.append(" ").append(change.getId().get());
diff --git a/java/com/google/gerrit/server/change/ChangeResource.java b/java/com/google/gerrit/server/change/ChangeResource.java
index 27b71d6..0d0df0d 100644
--- a/java/com/google/gerrit/server/change/ChangeResource.java
+++ b/java/com/google/gerrit/server/change/ChangeResource.java
@@ -167,7 +167,6 @@
public void prepareETag(Hasher h, CurrentUser user) {
h.putInt(JSON_FORMAT_VERSION)
.putLong(getChange().getLastUpdatedOn().getTime())
- .putInt(getChange().getRowVersion())
.putInt(user.isIdentifiedUser() ? user.getAccountId().get() : 0);
if (user.isIdentifiedUser()) {
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index 076ba46..201f122 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -100,6 +100,7 @@
import com.google.gerrit.server.account.ServiceUserClassifierImpl;
import com.google.gerrit.server.account.VersionedAuthorizedKeys;
import com.google.gerrit.server.account.externalids.ExternalIdModule;
+import com.google.gerrit.server.account.externalids.ExternalIdUpsertPreprocessor;
import com.google.gerrit.server.approval.ApprovalCacheImpl;
import com.google.gerrit.server.approval.ApprovalsUtil;
import com.google.gerrit.server.auth.AuthBackend;
@@ -166,6 +167,7 @@
import com.google.gerrit.server.mime.FileTypeRegistry;
import com.google.gerrit.server.mime.MimeUtilFileTypeRegistry;
import com.google.gerrit.server.notedb.NoteDbModule;
+import com.google.gerrit.server.notedb.StoreSubmitRequirementsOp;
import com.google.gerrit.server.patch.DiffOperationsImpl;
import com.google.gerrit.server.patch.PatchListCacheImpl;
import com.google.gerrit.server.patch.PatchScriptFactory;
@@ -466,6 +468,7 @@
factory(MergedByPushOp.Factory.class);
factory(GitModules.Factory.class);
factory(VersionedAuthorizedKeys.Factory.class);
+ factory(StoreSubmitRequirementsOp.Factory.class);
bind(AccountManager.class);
bind(SubscriptionGraph.Factory.class).to(ConfiguredSubscriptionGraphFactory.class);
@@ -476,5 +479,6 @@
bind(ReloadPluginListener.class)
.annotatedWith(UniqueAnnotations.create())
.to(PluginConfigFactory.class);
+ DynamicMap.mapOf(binder(), ExternalIdUpsertPreprocessor.class);
}
}
diff --git a/java/com/google/gerrit/server/git/MultiProgressMonitor.java b/java/com/google/gerrit/server/git/MultiProgressMonitor.java
index 2d854a5..1122551 100644
--- a/java/com/google/gerrit/server/git/MultiProgressMonitor.java
+++ b/java/com/google/gerrit/server/git/MultiProgressMonitor.java
@@ -131,7 +131,7 @@
private int spinnerIndex;
private char spinnerState = NO_SPINNER;
private boolean done;
- private boolean write = true;
+ private boolean clientDisconnected;
private final long maxIntervalNanos;
@@ -343,14 +343,14 @@
}
private void send(StringBuilder s) {
- if (write) {
+ if (!clientDisconnected) {
try {
out.write(Constants.encode(s.toString()));
out.flush();
} catch (IOException e) {
logger.atWarning().withCause(e).log(
"Sending progress to client failed. Stop sending updates for task %s", taskName);
- write = false;
+ clientDisconnected = true;
}
}
}
diff --git a/java/com/google/gerrit/server/git/receive/BUILD b/java/com/google/gerrit/server/git/receive/BUILD
index b59d431..5c1cf52 100644
--- a/java/com/google/gerrit/server/git/receive/BUILD
+++ b/java/com/google/gerrit/server/git/receive/BUILD
@@ -18,6 +18,7 @@
"//java/com/google/gerrit/index",
"//java/com/google/gerrit/metrics",
"//java/com/google/gerrit/server",
+ "//java/com/google/gerrit/server/cancellation",
"//java/com/google/gerrit/server/logging",
"//java/com/google/gerrit/server/restapi",
"//java/com/google/gerrit/server/util/time",
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index d074f1e..29c2698 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -46,6 +46,7 @@
import com.google.common.base.Joiner;
import com.google.common.base.Splitter;
import com.google.common.base.Strings;
+import com.google.common.base.Throwables;
import com.google.common.collect.BiMap;
import com.google.common.collect.HashBiMap;
import com.google.common.collect.ImmutableList;
@@ -645,10 +646,18 @@
try {
processCommandsUnsafe(commands, progress);
rejectRemaining(commands, INTERNAL_SERVER_ERROR);
- } catch (RequestCancelledException e) {
- StringBuilder msg = new StringBuilder(e.formatCancellationReason());
- if (e.getCancellationMessage().isPresent()) {
- msg.append(String.format(" (%s)", e.getCancellationMessage().get()));
+ } catch (RuntimeException e) {
+ Optional<RequestCancelledException> requestCancelledException =
+ RequestCancelledException.getFromCausalChain(e);
+ if (!requestCancelledException.isPresent()) {
+ Throwables.throwIfUnchecked(e);
+ }
+ StringBuilder msg =
+ new StringBuilder(requestCancelledException.get().formatCancellationReason());
+ if (requestCancelledException.get().getCancellationMessage().isPresent()) {
+ msg.append(
+ String.format(
+ " (%s)", requestCancelledException.get().getCancellationMessage().get()));
}
rejectRemaining(commands, msg.toString());
}
diff --git a/java/com/google/gerrit/server/group/PeriodicGroupIndexer.java b/java/com/google/gerrit/server/group/PeriodicGroupIndexer.java
index cae213f..2823548 100644
--- a/java/com/google/gerrit/server/group/PeriodicGroupIndexer.java
+++ b/java/com/google/gerrit/server/group/PeriodicGroupIndexer.java
@@ -145,7 +145,7 @@
}
groupUuids = newGroupUuids;
logger.atInfo().log("Run group indexer, %s groups reindexed", reindexCounter);
- } catch (Throwable t) {
+ } catch (Exception t) {
logger.atSevere().withCause(t).log("Failed to reindex groups");
}
}
diff --git a/java/com/google/gerrit/server/index/change/ChangeField.java b/java/com/google/gerrit/server/index/change/ChangeField.java
index 1ee12fe..5caceef 100644
--- a/java/com/google/gerrit/server/index/change/ChangeField.java
+++ b/java/com/google/gerrit/server/index/change/ChangeField.java
@@ -70,6 +70,7 @@
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.index.change.StalenessChecker.RefStatePattern;
import com.google.gerrit.server.notedb.ReviewerStateInternal;
+import com.google.gerrit.server.notedb.SubmitRequirementProtoConverter;
import com.google.gerrit.server.project.SubmitRuleOptions;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.ChangeQueryBuilder;
@@ -1078,6 +1079,27 @@
return result;
}
+ /** Serialized submit requirements, used for pre-populating results. */
+ public static final FieldDef<ChangeData, Iterable<byte[]>> STORED_SUBMIT_REQUIREMENTS =
+ storedOnly("full_submit_requirements")
+ .buildRepeatable(
+ cd ->
+ toProtos(
+ SubmitRequirementProtoConverter.INSTANCE, cd.submitRequirements().values()),
+ (cd, field) -> parseSubmitRequirements(field, cd));
+
+ private static void parseSubmitRequirements(Iterable<byte[]> values, ChangeData out) {
+ out.setSubmitRequirements(
+ StreamSupport.stream(values.spliterator(), false)
+ .map(
+ f ->
+ SubmitRequirementProtoConverter.INSTANCE.fromProto(
+ Protos.parseUnchecked(
+ SubmitRequirementProtoConverter.INSTANCE.getParser(), f)))
+ .collect(
+ ImmutableMap.toImmutableMap(sr -> sr.submitRequirement(), Function.identity())));
+ }
+
/**
* All values of all refs that were used in the course of indexing this document.
*
diff --git a/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java b/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
index 879da4f..bee0c35 100644
--- a/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
+++ b/java/com/google/gerrit/server/index/change/ChangeSchemaDefinitions.java
@@ -148,10 +148,14 @@
* The computation of the {@link ChangeField#DIRECTORY} field is changed, hence reindexing is
* required.
*/
- static final Schema<ChangeData> V63 = schema(V62, false);
+ @Deprecated static final Schema<ChangeData> V63 = schema(V62, false);
/** Added support for MIN/MAX/ANY for {@link ChangeField#LABEL} */
- static final Schema<ChangeData> V64 = schema(V63, false);
+ @Deprecated static final Schema<ChangeData> V64 = schema(V63, false);
+
+ /** Added new field for submit requirements. */
+ static final Schema<ChangeData> V65 =
+ new Schema.Builder<ChangeData>().add(V64).add(ChangeField.STORED_SUBMIT_REQUIREMENTS).build();
/**
* Name of the change index to be used when contacting index backends or loading configurations.
diff --git a/java/com/google/gerrit/server/logging/BUILD b/java/com/google/gerrit/server/logging/BUILD
index c60af0d..ee0168c 100644
--- a/java/com/google/gerrit/server/logging/BUILD
+++ b/java/com/google/gerrit/server/logging/BUILD
@@ -9,6 +9,7 @@
deps = [
"//java/com/google/gerrit/common:annotations",
"//java/com/google/gerrit/extensions:api",
+ "//java/com/google/gerrit/server/cancellation",
"//java/com/google/gerrit/server/util/time",
"//lib:gson",
"//lib:guava",
diff --git a/java/com/google/gerrit/server/logging/PerformanceLogContext.java b/java/com/google/gerrit/server/logging/PerformanceLogContext.java
index b6dafdc..65e033b15 100644
--- a/java/com/google/gerrit/server/logging/PerformanceLogContext.java
+++ b/java/com/google/gerrit/server/logging/PerformanceLogContext.java
@@ -92,7 +92,7 @@
p -> {
try (TraceContext traceContext = newPluginTrace(p)) {
performanceLogRecords.forEach(r -> r.writeTo(p.get()));
- } catch (Throwable e) {
+ } catch (RuntimeException e) {
logger.atWarning().withCause(e).log(
"Failure in %s of plugin %s", p.get().getClass(), p.getPluginName());
}
diff --git a/java/com/google/gerrit/server/logging/TraceContext.java b/java/com/google/gerrit/server/logging/TraceContext.java
index 2fc19b5..681dfbc 100644
--- a/java/com/google/gerrit/server/logging/TraceContext.java
+++ b/java/com/google/gerrit/server/logging/TraceContext.java
@@ -24,6 +24,7 @@
import com.google.common.collect.Table;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
+import com.google.gerrit.server.cancellation.RequestStateContext;
import java.util.Optional;
import java.util.concurrent.TimeUnit;
import java.util.function.Consumer;
@@ -208,6 +209,7 @@
}
private TraceTimer(Runnable startLogFn, Consumer<Long> doneLogFn) {
+ RequestStateContext.abortIfCancelled();
startLogFn.run();
this.doneLogFn = doneLogFn;
this.stopwatch = Stopwatch.createStarted();
@@ -217,6 +219,7 @@
public void close() {
stopwatch.stop();
doneLogFn.accept(stopwatch.elapsed(TimeUnit.MILLISECONDS));
+ RequestStateContext.abortIfCancelled();
}
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesState.java b/java/com/google/gerrit/server/notedb/ChangeNotesState.java
index e7da025..4d6b9cf 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesState.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesState.java
@@ -61,14 +61,10 @@
import com.google.gerrit.server.cache.proto.Cache.ChangeNotesStateProto.ReviewerByEmailSetEntryProto;
import com.google.gerrit.server.cache.proto.Cache.ChangeNotesStateProto.ReviewerSetEntryProto;
import com.google.gerrit.server.cache.proto.Cache.ChangeNotesStateProto.ReviewerStatusUpdateProto;
-import com.google.gerrit.server.cache.proto.Cache.SubmitRequirementResultProto;
import com.google.gerrit.server.cache.serialize.CacheSerializer;
import com.google.gerrit.server.cache.serialize.ObjectIdConverter;
-import com.google.gerrit.server.cache.serialize.entities.SubmitRequirementExpressionResultSerializer;
-import com.google.gerrit.server.cache.serialize.entities.SubmitRequirementSerializer;
import com.google.gerrit.server.index.change.ChangeField.StoredSubmitRecord;
import com.google.gson.Gson;
-import com.google.protobuf.Descriptors.FieldDescriptor;
import java.sql.Timestamp;
import java.time.Instant;
import java.util.List;
@@ -478,11 +474,6 @@
private static final Converter<String, ReviewerStateInternal> REVIEWER_STATE_CONVERTER =
Enums.stringConverter(ReviewerStateInternal.class);
- private static final FieldDescriptor SR_APPLICABILITY_EXPR_RESULT_FIELD =
- SubmitRequirementResultProto.getDescriptor().findFieldByNumber(2);
- private static final FieldDescriptor SR_OVERRIDE_EXPR_RESULT_FIELD =
- SubmitRequirementResultProto.getDescriptor().findFieldByNumber(4);
-
@Override
public byte[] serialize(ChangeNotesState object) {
checkArgument(object.metaId() != null, "meta ID is required in: %s", object);
@@ -539,7 +530,10 @@
object.publishedComments().values().forEach(c -> b.addPublishedComment(GSON.toJson(c)));
object
.submitRequirementsResult()
- .forEach(sr -> b.addSubmitRequirementResult(toSubmitRequirementResultProto(sr)));
+ .forEach(
+ sr ->
+ b.addSubmitRequirementResult(
+ SubmitRequirementProtoConverter.INSTANCE.toProto(sr)));
b.setUpdateCount(object.updateCount());
if (object.mergedOn() != null) {
b.setMergedOnMillis(object.mergedOn().getTime());
@@ -634,53 +628,6 @@
return builder.build();
}
- private static SubmitRequirementResultProto toSubmitRequirementResultProto(
- SubmitRequirementResult r) {
- SubmitRequirementResultProto.Builder builder = SubmitRequirementResultProto.newBuilder();
- builder
- .setSubmitRequirement(SubmitRequirementSerializer.serialize(r.submitRequirement()))
- .setCommit(ObjectIdConverter.create().toByteString(r.patchSetCommitId()));
- if (r.applicabilityExpressionResult().isPresent()) {
- builder.setApplicabilityExpressionResult(
- SubmitRequirementExpressionResultSerializer.serialize(
- r.applicabilityExpressionResult().get()));
- }
- builder.setSubmittabilityExpressionResult(
- SubmitRequirementExpressionResultSerializer.serialize(
- r.submittabilityExpressionResult()));
- if (r.overrideExpressionResult().isPresent()) {
- builder.setOverrideExpressionResult(
- SubmitRequirementExpressionResultSerializer.serialize(
- r.overrideExpressionResult().get()));
- }
- return builder.build();
- }
-
- private static SubmitRequirementResult toSubmitRequirementResult(
- SubmitRequirementResultProto proto) {
- SubmitRequirementResult.Builder builder =
- SubmitRequirementResult.builder()
- .patchSetCommitId(ObjectIdConverter.create().fromByteString(proto.getCommit()))
- .submitRequirement(
- SubmitRequirementSerializer.deserialize(proto.getSubmitRequirement()));
- if (proto.hasField(SR_APPLICABILITY_EXPR_RESULT_FIELD)) {
- builder.applicabilityExpressionResult(
- Optional.of(
- SubmitRequirementExpressionResultSerializer.deserialize(
- proto.getApplicabilityExpressionResult())));
- }
- builder.submittabilityExpressionResult(
- SubmitRequirementExpressionResultSerializer.deserialize(
- proto.getSubmittabilityExpressionResult()));
- if (proto.hasField(SR_OVERRIDE_EXPR_RESULT_FIELD)) {
- builder.overrideExpressionResult(
- Optional.of(
- SubmitRequirementExpressionResultSerializer.deserialize(
- proto.getOverrideExpressionResult())));
- }
- return builder.build();
- }
-
@Override
public ChangeNotesState deserialize(byte[] in) {
ChangeNotesStateProto proto = Protos.parseUnchecked(ChangeNotesStateProto.parser(), in);
@@ -728,7 +675,7 @@
.collect(toImmutableListMultimap(HumanComment::getCommitId, c -> c)))
.submitRequirementsResult(
proto.getSubmitRequirementResultList().stream()
- .map(sr -> toSubmitRequirementResult(sr))
+ .map(sr -> SubmitRequirementProtoConverter.INSTANCE.fromProto(sr))
.collect(toImmutableList()))
.updateCount(proto.getUpdateCount())
.mergedOn(proto.getHasMergedOn() ? new Timestamp(proto.getMergedOnMillis()) : null);
diff --git a/java/com/google/gerrit/server/notedb/StoreSubmitRequirementsOp.java b/java/com/google/gerrit/server/notedb/StoreSubmitRequirementsOp.java
index 47948d7..4d99f93 100644
--- a/java/com/google/gerrit/server/notedb/StoreSubmitRequirementsOp.java
+++ b/java/com/google/gerrit/server/notedb/StoreSubmitRequirementsOp.java
@@ -15,16 +15,26 @@
package com.google.gerrit.server.notedb;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.server.project.SubmitRequirementsEvaluator;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
+import com.google.inject.Inject;
/** A {@link BatchUpdateOp} that stores the evaluated submit requirements of a change in NoteDb. */
public class StoreSubmitRequirementsOp implements BatchUpdateOp {
private final ChangeData.Factory changeDataFactory;
+ private final SubmitRequirementsEvaluator evaluator;
- public StoreSubmitRequirementsOp(ChangeData.Factory changeDataFactory) {
+ public interface Factory {
+ StoreSubmitRequirementsOp create();
+ }
+
+ @Inject
+ public StoreSubmitRequirementsOp(
+ ChangeData.Factory changeDataFactory, SubmitRequirementsEvaluator evaluator) {
this.changeDataFactory = changeDataFactory;
+ this.evaluator = evaluator;
}
@Override
@@ -32,7 +42,7 @@
Change change = ctx.getChange();
ChangeData changeData = changeDataFactory.create(change);
ChangeUpdate update = ctx.getUpdate(change.currentPatchSetId());
- update.putSubmitRequirementResults(changeData.submitRequirements().values());
+ update.putSubmitRequirementResults(evaluator.getResults(changeData).values());
return !changeData.submitRequirements().isEmpty();
}
}
diff --git a/java/com/google/gerrit/server/notedb/SubmitRequirementProtoConverter.java b/java/com/google/gerrit/server/notedb/SubmitRequirementProtoConverter.java
new file mode 100644
index 0000000..416b850
--- /dev/null
+++ b/java/com/google/gerrit/server/notedb/SubmitRequirementProtoConverter.java
@@ -0,0 +1,88 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.notedb;
+
+import com.google.errorprone.annotations.Immutable;
+import com.google.gerrit.entities.SubmitRequirementResult;
+import com.google.gerrit.entities.converter.ProtoConverter;
+import com.google.gerrit.server.cache.proto.Cache.SubmitRequirementResultProto;
+import com.google.gerrit.server.cache.serialize.ObjectIdConverter;
+import com.google.gerrit.server.cache.serialize.entities.SubmitRequirementExpressionResultSerializer;
+import com.google.gerrit.server.cache.serialize.entities.SubmitRequirementSerializer;
+import com.google.protobuf.Descriptors.FieldDescriptor;
+import com.google.protobuf.Parser;
+import java.util.Optional;
+
+@Immutable
+public enum SubmitRequirementProtoConverter
+ implements ProtoConverter<SubmitRequirementResultProto, SubmitRequirementResult> {
+ INSTANCE;
+
+ private static final FieldDescriptor SR_APPLICABILITY_EXPR_RESULT_FIELD =
+ SubmitRequirementResultProto.getDescriptor().findFieldByNumber(2);
+ private static final FieldDescriptor SR_OVERRIDE_EXPR_RESULT_FIELD =
+ SubmitRequirementResultProto.getDescriptor().findFieldByNumber(4);
+
+ @Override
+ public SubmitRequirementResultProto toProto(SubmitRequirementResult r) {
+ SubmitRequirementResultProto.Builder builder = SubmitRequirementResultProto.newBuilder();
+ builder
+ .setSubmitRequirement(SubmitRequirementSerializer.serialize(r.submitRequirement()))
+ .setCommit(ObjectIdConverter.create().toByteString(r.patchSetCommitId()));
+ if (r.applicabilityExpressionResult().isPresent()) {
+ builder.setApplicabilityExpressionResult(
+ SubmitRequirementExpressionResultSerializer.serialize(
+ r.applicabilityExpressionResult().get()));
+ }
+ builder.setSubmittabilityExpressionResult(
+ SubmitRequirementExpressionResultSerializer.serialize(r.submittabilityExpressionResult()));
+ if (r.overrideExpressionResult().isPresent()) {
+ builder.setOverrideExpressionResult(
+ SubmitRequirementExpressionResultSerializer.serialize(
+ r.overrideExpressionResult().get()));
+ }
+ return builder.build();
+ }
+
+ @Override
+ public SubmitRequirementResult fromProto(SubmitRequirementResultProto proto) {
+ SubmitRequirementResult.Builder builder =
+ SubmitRequirementResult.builder()
+ .patchSetCommitId(ObjectIdConverter.create().fromByteString(proto.getCommit()))
+ .submitRequirement(
+ SubmitRequirementSerializer.deserialize(proto.getSubmitRequirement()));
+ if (proto.hasField(SR_APPLICABILITY_EXPR_RESULT_FIELD)) {
+ builder.applicabilityExpressionResult(
+ Optional.of(
+ SubmitRequirementExpressionResultSerializer.deserialize(
+ proto.getApplicabilityExpressionResult())));
+ }
+ builder.submittabilityExpressionResult(
+ SubmitRequirementExpressionResultSerializer.deserialize(
+ proto.getSubmittabilityExpressionResult()));
+ if (proto.hasField(SR_OVERRIDE_EXPR_RESULT_FIELD)) {
+ builder.overrideExpressionResult(
+ Optional.of(
+ SubmitRequirementExpressionResultSerializer.deserialize(
+ proto.getOverrideExpressionResult())));
+ }
+ return builder.build();
+ }
+
+ @Override
+ public Parser<SubmitRequirementResultProto> getParser() {
+ return SubmitRequirementResultProto.parser();
+ }
+}
diff --git a/java/com/google/gerrit/server/plugincontext/PluginContext.java b/java/com/google/gerrit/server/plugincontext/PluginContext.java
index 90d56c8..1cfee65 100644
--- a/java/com/google/gerrit/server/plugincontext/PluginContext.java
+++ b/java/com/google/gerrit/server/plugincontext/PluginContext.java
@@ -204,7 +204,7 @@
try (TraceContext traceContext = newTrace(extension);
Timer3.Context<String, String, String> ctx = pluginMetrics.startLatency(extension)) {
extensionImplConsumer.run(extensionImpl);
- } catch (Throwable e) {
+ } catch (Exception e) {
pluginMetrics.incrementErrorCount(extension);
logger.atWarning().withCause(e).log(
"Failure in %s of plugin %s", extensionImpl.getClass(), extension.getPluginName());
@@ -233,7 +233,7 @@
try (TraceContext traceContext = newTrace(extension);
Timer3.Context<String, String, String> ctx = pluginMetrics.startLatency(extension)) {
extensionConsumer.run(extension);
- } catch (Throwable e) {
+ } catch (Exception e) {
pluginMetrics.incrementErrorCount(extension);
logger.atWarning().withCause(e).log(
"Failure in %s of plugin %s", extensionImpl.getClass(), extension.getPluginName());
@@ -267,7 +267,7 @@
try (TraceContext traceContext = newTrace(extension);
Timer3.Context<String, String, String> ctx = pluginMetrics.startLatency(extension)) {
extensionImplConsumer.run(extensionImpl);
- } catch (Throwable e) {
+ } catch (Exception e) {
Throwables.throwIfInstanceOf(e, exceptionClass);
Throwables.throwIfUnchecked(e);
pluginMetrics.incrementErrorCount(extension);
@@ -304,7 +304,7 @@
try (TraceContext traceContext = newTrace(extension);
Timer3.Context<String, String, String> ctx = pluginMetrics.startLatency(extension)) {
extensionConsumer.run(extension);
- } catch (Throwable e) {
+ } catch (Exception e) {
Throwables.throwIfInstanceOf(e, exceptionClass);
Throwables.throwIfUnchecked(e);
pluginMetrics.incrementErrorCount(extension);
diff --git a/java/com/google/gerrit/server/plugins/PluginLoader.java b/java/com/google/gerrit/server/plugins/PluginLoader.java
index 0a06081..8d17d85 100644
--- a/java/com/google/gerrit/server/plugins/PluginLoader.java
+++ b/java/com/google/gerrit/server/plugins/PluginLoader.java
@@ -253,7 +253,7 @@
FileSnapshot snapshot = FileSnapshot.save(off.toFile());
Plugin offPlugin = loadPlugin(name, off, snapshot);
disabled.put(name, offPlugin);
- } catch (Throwable e) {
+ } catch (Exception e) {
// This shouldn't happen, as the plugin was loaded earlier.
logger.atWarning().withCause(e.getCause()).log(
"Cannot load disabled plugin %s", active.getName());
@@ -510,7 +510,7 @@
if (!newPlugin.isDisabled()) {
try {
newPlugin.start(env);
- } catch (Throwable e) {
+ } catch (Exception e) {
newPlugin.stop(env);
throw e;
}
@@ -528,7 +528,7 @@
}
broken.remove(name);
return newPlugin;
- } catch (Throwable err) {
+ } catch (Exception err) {
broken.put(name, snapshot);
throw new PluginInstallException(err);
}
diff --git a/java/com/google/gerrit/server/project/SubmitRequirementsEvaluator.java b/java/com/google/gerrit/server/project/SubmitRequirementsEvaluator.java
index 65d9d9e..e66b974 100644
--- a/java/com/google/gerrit/server/project/SubmitRequirementsEvaluator.java
+++ b/java/com/google/gerrit/server/project/SubmitRequirementsEvaluator.java
@@ -14,6 +14,9 @@
package com.google.gerrit.server.project;
+import static com.google.gerrit.server.project.ProjectCache.illegalState;
+
+import com.google.common.collect.ImmutableMap;
import com.google.gerrit.entities.SubmitRequirement;
import com.google.gerrit.entities.SubmitRequirementExpression;
import com.google.gerrit.entities.SubmitRequirementExpressionResult;
@@ -26,16 +29,20 @@
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
+import java.util.Map;
import java.util.Optional;
/** Evaluates submit requirements for different change data. */
@Singleton
public class SubmitRequirementsEvaluator {
private final Provider<ChangeQueryBuilder> changeQueryBuilderProvider;
+ private final ProjectCache projectCache;
@Inject
- private SubmitRequirementsEvaluator(Provider<ChangeQueryBuilder> changeQueryBuilderProvider) {
+ private SubmitRequirementsEvaluator(
+ Provider<ChangeQueryBuilder> changeQueryBuilderProvider, ProjectCache projectCache) {
this.changeQueryBuilderProvider = changeQueryBuilderProvider;
+ this.projectCache = projectCache;
}
/**
@@ -50,6 +57,21 @@
changeQueryBuilderProvider.get().parse(expression.expressionString());
}
+ /**
+ * Evaluate and return all submit requirements for a change. Submit requirements are retrieved for
+ * the project containing the change and parent projects as well.
+ */
+ public Map<SubmitRequirement, SubmitRequirementResult> getResults(ChangeData cd) {
+ ProjectState state = projectCache.get(cd.project()).orElseThrow(illegalState(cd.project()));
+ Map<String, SubmitRequirement> requirements = state.getSubmitRequirements();
+ ImmutableMap.Builder<SubmitRequirement, SubmitRequirementResult> result =
+ ImmutableMap.builderWithExpectedSize(requirements.size());
+ for (SubmitRequirement requirement : requirements.values()) {
+ result.put(requirement, evaluate(requirement, cd));
+ }
+ return result.build();
+ }
+
/** Evaluate a {@link SubmitRequirement} on a given {@link ChangeData}. */
public SubmitRequirementResult evaluate(SubmitRequirement sr, ChangeData cd) {
SubmitRequirementExpressionResult blockingResult =
diff --git a/java/com/google/gerrit/server/query/change/ChangeData.java b/java/com/google/gerrit/server/query/change/ChangeData.java
index f912250..8b4e4c7 100644
--- a/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -38,7 +38,6 @@
import com.google.common.flogger.FluentLogger;
import com.google.common.primitives.Ints;
import com.google.gerrit.common.Nullable;
-import com.google.gerrit.common.UsedAt;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AttentionSetUpdate;
import com.google.gerrit.entities.Change;
@@ -106,6 +105,7 @@
import java.util.Optional;
import java.util.Set;
import java.util.function.Function;
+import java.util.stream.Collectors;
import java.util.stream.Stream;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
@@ -932,21 +932,36 @@
return messages;
}
- /** Get all submit requirements for this change, including those from parent projects. */
+ /**
+ * Get all evaluated submit requirements for this change, including those from parent projects.
+ * For closed changes, submit requirements are read from the change notes. For active changes,
+ * submit requirements are evaluated online.
+ *
+ * <p>For changes loaded from the index, the value will be set from index field {@link
+ * com.google.gerrit.server.index.change.ChangeField#STORED_SUBMIT_REQUIREMENTS}.
+ */
public Map<SubmitRequirement, SubmitRequirementResult> submitRequirements() {
if (submitRequirements == null) {
- ProjectState state = projectCache.get(project()).orElseThrow(illegalState(project()));
- Map<String, SubmitRequirement> requirements = state.getSubmitRequirements();
- ImmutableMap.Builder<SubmitRequirement, SubmitRequirementResult> result =
- ImmutableMap.builderWithExpectedSize(requirements.size());
- for (SubmitRequirement requirement : requirements.values()) {
- result.put(requirement, submitRequirementsEvaluator.evaluate(requirement, this));
+ if (!lazyload()) {
+ return Collections.emptyMap();
}
- submitRequirements = result.build();
+ Change c = change();
+ if (c != null && c.isClosed()) {
+ submitRequirements =
+ notes().getSubmitRequirementsResult().stream()
+ .collect(Collectors.toMap(r -> r.submitRequirement(), Function.identity()));
+ } else {
+ submitRequirements = submitRequirementsEvaluator.getResults(this);
+ }
}
return submitRequirements;
}
+ public void setSubmitRequirements(
+ Map<SubmitRequirement, SubmitRequirementResult> submitRequirements) {
+ this.submitRequirements = submitRequirements;
+ }
+
public List<SubmitRecord> submitRecords(SubmitRuleOptions options) {
// If the change is not submitted yet, 'strict' and 'lenient' both have the same result. If the
// change is submitted, SubmitRecord requested with 'strict' will contain just a single entry
@@ -1271,12 +1286,6 @@
return refStates;
}
- @UsedAt(UsedAt.Project.GOOGLE)
- public void setRefStates(Iterable<byte[]> refStates) {
- // TODO(hanwen): remove Google use, and drop this method.
- setRefStates(RefState.parseStates(refStates));
- }
-
public void setRefStates(ImmutableSetMultimap<Project.NameKey, RefState> refStates) {
this.refStates = refStates;
if (draftsByUser == null) {
diff --git a/java/com/google/gerrit/server/rules/PrologEnvironment.java b/java/com/google/gerrit/server/rules/PrologEnvironment.java
index 1a563ad..7d626da 100644
--- a/java/com/google/gerrit/server/rules/PrologEnvironment.java
+++ b/java/com/google/gerrit/server/rules/PrologEnvironment.java
@@ -143,7 +143,7 @@
for (Iterator<Runnable> i = cleanup.iterator(); i.hasNext(); ) {
try {
i.next().run();
- } catch (Throwable err) {
+ } catch (Exception err) {
logger.atSevere().withCause(err).log("Failed to execute cleanup for PrologEnvironment");
}
i.remove();
diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java
index 363cdca..942f024 100644
--- a/java/com/google/gerrit/server/submit/MergeOp.java
+++ b/java/com/google/gerrit/server/submit/MergeOp.java
@@ -241,6 +241,7 @@
private final NotifyResolver notifyResolver;
private final RetryHelper retryHelper;
private final ChangeData.Factory changeDataFactory;
+ private final StoreSubmitRequirementsOp.Factory storeSubmitRequirementsOpFactory;
// Changes that were updated by this MergeOp.
private final Map<Change.Id, Change> updatedChanges;
@@ -274,7 +275,8 @@
NotifyResolver notifyResolver,
TopicMetrics topicMetrics,
RetryHelper retryHelper,
- ChangeData.Factory changeDataFactory) {
+ ChangeData.Factory changeDataFactory,
+ StoreSubmitRequirementsOp.Factory storeSubmitRequirementsOpFactory) {
this.cmUtil = cmUtil;
this.batchUpdateFactory = batchUpdateFactory;
this.internalUserFactory = internalUserFactory;
@@ -291,6 +293,7 @@
this.topicMetrics = topicMetrics;
this.changeDataFactory = changeDataFactory;
this.updatedChanges = new HashMap<>();
+ this.storeSubmitRequirementsOpFactory = storeSubmitRequirementsOpFactory;
}
@Override
@@ -665,7 +668,7 @@
Change.Id changeId = entry.getKey();
batchUpdatesByProject
.get(project)
- .addOp(changeId, new StoreSubmitRequirementsOp(changeDataFactory));
+ .addOp(changeId, storeSubmitRequirementsOpFactory.create());
}
try {
submissionExecutor.setAdditionalBatchUpdateListeners(
diff --git a/java/com/google/gerrit/server/update/RetryableChangeAction.java b/java/com/google/gerrit/server/update/RetryableChangeAction.java
index 152db2c..84ec2bb 100644
--- a/java/com/google/gerrit/server/update/RetryableChangeAction.java
+++ b/java/com/google/gerrit/server/update/RetryableChangeAction.java
@@ -82,11 +82,11 @@
public T call() throws UpdateException, RestApiException {
try {
return super.call();
- } catch (Throwable t) {
- Throwables.throwIfUnchecked(t);
- Throwables.throwIfInstanceOf(t, UpdateException.class);
- Throwables.throwIfInstanceOf(t, RestApiException.class);
- throw new UpdateException(t);
+ } catch (Exception e) {
+ Throwables.throwIfUnchecked(e);
+ Throwables.throwIfInstanceOf(e, UpdateException.class);
+ Throwables.throwIfInstanceOf(e, RestApiException.class);
+ throw new UpdateException(e);
}
}
}
diff --git a/java/com/google/gerrit/server/update/RetryableIndexQueryAction.java b/java/com/google/gerrit/server/update/RetryableIndexQueryAction.java
index cf733a6..d66edcf 100644
--- a/java/com/google/gerrit/server/update/RetryableIndexQueryAction.java
+++ b/java/com/google/gerrit/server/update/RetryableIndexQueryAction.java
@@ -87,9 +87,9 @@
public T call() {
try {
return super.call();
- } catch (Throwable t) {
- Throwables.throwIfUnchecked(t);
- throw new StorageException(t);
+ } catch (Exception e) {
+ Throwables.throwIfUnchecked(e);
+ throw new StorageException(e);
}
}
}
diff --git a/java/com/google/gerrit/sshd/BUILD b/java/com/google/gerrit/sshd/BUILD
index 0668c1e..f3bd5e1 100644
--- a/java/com/google/gerrit/sshd/BUILD
+++ b/java/com/google/gerrit/sshd/BUILD
@@ -17,6 +17,7 @@
"//java/com/google/gerrit/metrics",
"//java/com/google/gerrit/server",
"//java/com/google/gerrit/server/audit",
+ "//java/com/google/gerrit/server/cancellation",
"//java/com/google/gerrit/server/git/receive",
"//java/com/google/gerrit/server/ioutil",
"//java/com/google/gerrit/server/logging",
diff --git a/java/com/google/gerrit/sshd/BaseCommand.java b/java/com/google/gerrit/sshd/BaseCommand.java
index 48a5512..42aabfb 100644
--- a/java/com/google/gerrit/sshd/BaseCommand.java
+++ b/java/com/google/gerrit/sshd/BaseCommand.java
@@ -370,7 +370,7 @@
err.flush();
} catch (IOException e2) {
// Ignored
- } catch (Throwable e2) {
+ } catch (RuntimeException e2) {
logger.atWarning().withCause(e2).log("Cannot send failure message to client");
}
return f.exitCode;
@@ -381,7 +381,7 @@
err.flush();
} catch (IOException e2) {
// Ignored
- } catch (Throwable e2) {
+ } catch (RuntimeException e2) {
logger.atWarning().withCause(e2).log("Cannot send internal server error message to client");
}
return 128;
@@ -500,15 +500,15 @@
out.flush();
err.flush();
- } catch (Throwable e) {
+ } catch (Exception e) {
try {
out.flush();
- } catch (Throwable e2) {
+ } catch (Exception e2) {
// Ignored
}
try {
err.flush();
- } catch (Throwable e2) {
+ } catch (Exception e2) {
// Ignored
}
rc = handleError(e);
diff --git a/java/com/google/gerrit/sshd/SshCommand.java b/java/com/google/gerrit/sshd/SshCommand.java
index 93c6c2c..e58040f 100644
--- a/java/com/google/gerrit/sshd/SshCommand.java
+++ b/java/com/google/gerrit/sshd/SshCommand.java
@@ -14,6 +14,7 @@
package com.google.gerrit.sshd;
+import com.google.common.base.Throwables;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.server.AccessPath;
import com.google.gerrit.server.DynamicOptions;
@@ -28,6 +29,7 @@
import com.google.inject.Inject;
import java.io.IOException;
import java.io.PrintWriter;
+import java.util.Optional;
import org.apache.sshd.server.Environment;
import org.apache.sshd.server.channel.ChannelSession;
import org.eclipse.jgit.lib.Config;
@@ -62,10 +64,18 @@
RequestInfo.builder(RequestInfo.RequestType.SSH, user, traceContext).build();
requestListeners.runEach(l -> l.onRequest(requestInfo));
SshCommand.this.run();
- } catch (RequestCancelledException e) {
- StringBuilder msg = new StringBuilder(e.formatCancellationReason());
- if (e.getCancellationMessage().isPresent()) {
- msg.append(String.format(" (%s)", e.getCancellationMessage().get()));
+ } catch (RuntimeException e) {
+ Optional<RequestCancelledException> requestCancelledException =
+ RequestCancelledException.getFromCausalChain(e);
+ if (!requestCancelledException.isPresent()) {
+ Throwables.throwIfUnchecked(e);
+ }
+ StringBuilder msg =
+ new StringBuilder(requestCancelledException.get().formatCancellationReason());
+ if (requestCancelledException.get().getCancellationMessage().isPresent()) {
+ msg.append(
+ String.format(
+ " (%s)", requestCancelledException.get().getCancellationMessage().get()));
}
stderr.println(msg.toString());
} finally {
diff --git a/java/com/google/gerrit/sshd/SshKeyCacheImpl.java b/java/com/google/gerrit/sshd/SshKeyCacheImpl.java
index 773c25b..d5f0ee8 100644
--- a/java/com/google/gerrit/sshd/SshKeyCacheImpl.java
+++ b/java/com/google/gerrit/sshd/SshKeyCacheImpl.java
@@ -138,7 +138,7 @@
// to do with the key object, and instead we must abort this load.
//
throw e;
- } catch (Throwable e) {
+ } catch (Exception e) {
markInvalid(k);
}
}
diff --git a/java/com/google/gerrit/sshd/commands/UploadArchive.java b/java/com/google/gerrit/sshd/commands/UploadArchive.java
index 0eda433..c1f4a7b 100644
--- a/java/com/google/gerrit/sshd/commands/UploadArchive.java
+++ b/java/com/google/gerrit/sshd/commands/UploadArchive.java
@@ -214,16 +214,16 @@
} catch (GitAPIException e) {
throw new Failure(7, "fatal: git api exception, " + e);
}
- } catch (Throwable t) {
+ } catch (Exception e) {
// Report the error in ERROR sideband channel. Catch Throwable too so we can also catch
// NoClassDefFound.
try (SideBandOutputStream sidebandError =
new SideBandOutputStream(
SideBandOutputStream.CH_ERROR, SideBandOutputStream.MAX_BUF, out)) {
- sidebandError.write(t.getMessage().getBytes(UTF_8));
+ sidebandError.write(e.getMessage().getBytes(UTF_8));
sidebandError.flush();
}
- throw t;
+ throw e;
} finally {
// In any case, cleanly close the packetOut channel
packetOut.end();
diff --git a/java/com/google/gerrit/testing/InMemoryModule.java b/java/com/google/gerrit/testing/InMemoryModule.java
index 06d9453..12da2c1 100644
--- a/java/com/google/gerrit/testing/InMemoryModule.java
+++ b/java/com/google/gerrit/testing/InMemoryModule.java
@@ -134,7 +134,6 @@
cfg.setString("user", null, "name", "Gerrit Code Review");
cfg.setString("user", null, "email", "gerrit@localhost");
cfg.unset("cache", null, "directory");
- cfg.setString("index", null, "type", "lucene");
cfg.setBoolean("index", "lucene", "testInmemory", true);
cfg.setInt("sendemail", null, "threadPoolSize", 0);
cfg.setBoolean("receive", null, "enableSignedPush", false);
@@ -240,7 +239,8 @@
bind(AllChangesIndexer.class).toProvider(Providers.of(null));
bind(AllGroupsIndexer.class).toProvider(Providers.of(null));
- IndexType indexType = new IndexType(cfg.getString("index", null, "type"));
+ String indexTypeCfg = cfg.getString("index", null, "type");
+ IndexType indexType = new IndexType(indexTypeCfg != null ? indexTypeCfg : "fake");
// For custom index types, callers must provide their own module.
if (indexType.isLucene()) {
install(luceneIndexModule());
diff --git a/java/com/google/gerrit/testing/IndexConfig.java b/java/com/google/gerrit/testing/IndexConfig.java
index daef2b3..d68dcad 100644
--- a/java/com/google/gerrit/testing/IndexConfig.java
+++ b/java/com/google/gerrit/testing/IndexConfig.java
@@ -38,7 +38,9 @@
}
public static Config createForLucene() {
- return create();
+ Config cfg = create();
+ cfg.setString("index", null, "type", "lucene");
+ return cfg;
}
public static Config createForElasticsearch() {
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index c08aa7f..7785776 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -4325,6 +4325,101 @@
}
@Test
+ public void submitRequirement_retrievedFromNoteDbForClosedChanges() throws Exception {
+ configSubmitRequirement(
+ project,
+ SubmitRequirement.builder()
+ .setName("code-review")
+ .setSubmittabilityExpression(SubmitRequirementExpression.create("label:code-review=+2"))
+ .setAllowOverrideInChildProjects(false)
+ .build());
+
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+
+ ChangeInfo change = gApi.changes().id(changeId).get();
+ assertThat(change.submitRequirements).hasSize(1);
+ assertSubmitRequirementStatus(change.submitRequirements, "code-review", Status.UNSATISFIED);
+
+ voteLabel(changeId, "code-review", 2);
+
+ change = gApi.changes().id(changeId).get();
+ assertThat(change.submitRequirements).hasSize(1);
+ assertSubmitRequirementStatus(change.submitRequirements, "code-review", Status.SATISFIED);
+
+ gApi.changes().id(changeId).current().submit();
+
+ // Add new submit requirement
+ configSubmitRequirement(
+ project,
+ SubmitRequirement.builder()
+ .setName("verified")
+ .setSubmittabilityExpression(SubmitRequirementExpression.create("label:verified=+1"))
+ .setAllowOverrideInChildProjects(false)
+ .build());
+
+ // The new "verified" submit requirement is not returned, since this change is closed
+ change = gApi.changes().id(changeId).get();
+ assertThat(change.submitRequirements).hasSize(1);
+ assertSubmitRequirementStatus(change.submitRequirements, "code-review", Status.SATISFIED);
+ }
+
+ @Test
+ public void submitRequirement_backFilledFromIndexForActiveChanges() throws Exception {
+ configSubmitRequirement(
+ project,
+ SubmitRequirement.builder()
+ .setName("code-review")
+ .setSubmittabilityExpression(SubmitRequirementExpression.create("label:code-review=+2"))
+ .setAllowOverrideInChildProjects(false)
+ .build());
+
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+
+ voteLabel(changeId, "code-review", 2);
+
+ // Query the change. ChangeInfo is back-filled from the change index.
+ List<ChangeInfo> changeInfos =
+ gApi.changes()
+ .query()
+ .withQuery("project:{" + project.get() + "} (status:open OR status:closed)")
+ .withOptions(ImmutableSet.of(ListChangesOption.SUBMIT_REQUIREMENTS))
+ .get();
+ assertThat(changeInfos).hasSize(1);
+ assertSubmitRequirementStatus(
+ changeInfos.get(0).submitRequirements, "code-review", Status.SATISFIED);
+ }
+
+ @Test
+ public void submitRequirement_backFilledFromIndexForClosedChanges() throws Exception {
+ configSubmitRequirement(
+ project,
+ SubmitRequirement.builder()
+ .setName("code-review")
+ .setSubmittabilityExpression(SubmitRequirementExpression.create("label:code-review=+2"))
+ .setAllowOverrideInChildProjects(false)
+ .build());
+
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+
+ voteLabel(changeId, "code-review", 2);
+ gApi.changes().id(changeId).current().submit();
+
+ // Query the change. ChangeInfo is back-filled from the change index.
+ List<ChangeInfo> changeInfos =
+ gApi.changes()
+ .query()
+ .withQuery("project:{" + project.get() + "} (status:open OR status:closed)")
+ .withOptions(ImmutableSet.of(ListChangesOption.SUBMIT_REQUIREMENTS))
+ .get();
+ assertThat(changeInfos).hasSize(1);
+ assertSubmitRequirementStatus(
+ changeInfos.get(0).submitRequirements, "code-review", Status.SATISFIED);
+ }
+
+ @Test
public void fourByteEmoji() throws Exception {
// U+1F601 GRINNING FACE WITH SMILING EYES
String smile = new String(Character.toChars(0x1f601));
diff --git a/javatests/com/google/gerrit/acceptance/rest/BUILD b/javatests/com/google/gerrit/acceptance/rest/BUILD
index 84887da..a62d551 100644
--- a/javatests/com/google/gerrit/acceptance/rest/BUILD
+++ b/javatests/com/google/gerrit/acceptance/rest/BUILD
@@ -5,6 +5,7 @@
group = "rest_bindings_collection",
labels = ["rest"],
deps = [
+ "//java/com/google/gerrit/server/cancellation",
"//java/com/google/gerrit/server/logging",
],
)
diff --git a/javatests/com/google/gerrit/acceptance/rest/CancellationIT.java b/javatests/com/google/gerrit/acceptance/rest/CancellationIT.java
index 29d54cc..0713fe6 100644
--- a/javatests/com/google/gerrit/acceptance/rest/CancellationIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/CancellationIT.java
@@ -121,6 +121,27 @@
}
@Test
+ public void handleWrappedRequestCancelledException() throws Exception {
+ ProjectCreationValidationListener projectCreationListener =
+ new ProjectCreationValidationListener() {
+ @Override
+ public void validateNewProject(CreateProjectArgs args) throws ValidationException {
+ // Simulate an exceeded deadline by throwing RequestCancelledException.
+ throw new RuntimeException(
+ new RequestCancelledException(
+ RequestStateProvider.Reason.SERVER_DEADLINE_EXCEEDED, "deadline = 10m"));
+ }
+ };
+ try (Registration registration =
+ extensionRegistry.newRegistration().add(projectCreationListener)) {
+ RestResponse response = adminRestSession.put("/projects/" + name("new"));
+ assertThat(response.getStatusCode()).isEqualTo(SC_REQUEST_TIMEOUT);
+ assertThat(response.getEntityContent())
+ .isEqualTo("Server Deadline Exceeded\n\ndeadline = 10m");
+ }
+ }
+
+ @Test
public void handleClientDisconnectedForPush() throws Exception {
CommitValidationListener commitValidationListener =
new CommitValidationListener() {
@@ -185,6 +206,27 @@
}
@Test
+ public void handleWrappedRequestCancelledExceptionForPush() throws Exception {
+ CommitValidationListener commitValidationListener =
+ new CommitValidationListener() {
+ @Override
+ public List<CommitValidationMessage> onCommitReceived(CommitReceivedEvent receiveEvent)
+ throws CommitValidationException {
+ // Simulate an exceeded deadline by throwing RequestCancelledException.
+ throw new RuntimeException(
+ new RequestCancelledException(
+ RequestStateProvider.Reason.SERVER_DEADLINE_EXCEEDED, "deadline = 10m"));
+ }
+ };
+ try (Registration registration =
+ extensionRegistry.newRegistration().add(commitValidationListener)) {
+ PushOneCommit push = pushFactory.create(admin.newIdent(), testRepo);
+ PushOneCommit.Result r = push.to("refs/heads/master");
+ r.assertErrorStatus("Server Deadline Exceeded (deadline = 10m)");
+ }
+ }
+
+ @Test
public void handleRequestCancellationWithMessageForPush() throws Exception {
CommitValidationListener commitValidationListener =
new CommitValidationListener() {
diff --git a/javatests/com/google/gerrit/acceptance/server/notedb/ExternalIdNotesUpsertPreprocessorIT.java b/javatests/com/google/gerrit/acceptance/server/notedb/ExternalIdNotesUpsertPreprocessorIT.java
new file mode 100644
index 0000000..64ad900
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/server/notedb/ExternalIdNotesUpsertPreprocessorIT.java
@@ -0,0 +1,209 @@
+// 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.acceptance.server.notedb;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
+import com.google.gerrit.acceptance.TestPlugin;
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.extensions.annotations.Exports;
+import com.google.gerrit.server.ServerInitiated;
+import com.google.gerrit.server.account.Accounts;
+import com.google.gerrit.server.account.AccountsUpdate;
+import com.google.gerrit.server.account.externalids.ExternalId;
+import com.google.gerrit.server.account.externalids.ExternalIdNotes;
+import com.google.gerrit.server.account.externalids.ExternalIdUpsertPreprocessor;
+import com.google.gerrit.server.git.meta.MetaDataUpdate;
+import com.google.gerrit.server.notedb.Sequences;
+import com.google.inject.AbstractModule;
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.Singleton;
+import java.util.ArrayList;
+import java.util.List;
+import org.eclipse.jgit.lib.Repository;
+import org.junit.Before;
+import org.junit.Test;
+
+/** Tests {@link ExternalIdUpsertPreprocessor}. */
+@TestPlugin(
+ name = "external-id-update-preprocessor",
+ sysModule =
+ "com.google.gerrit.acceptance.server.notedb.ExternalIdNotesUpsertPreprocessorIT$Module")
+public class ExternalIdNotesUpsertPreprocessorIT extends LightweightPluginDaemonTest {
+ @Inject private Sequences sequences;
+ @Inject private @ServerInitiated Provider<AccountsUpdate> accountsUpdateProvider;
+ @Inject private ExternalIdNotes.Factory extIdNotesFactory;
+ @Inject private Accounts accounts;
+
+ public static class Module extends AbstractModule {
+ @Override
+ protected void configure() {
+ bind(ExternalIdUpsertPreprocessor.class)
+ .annotatedWith(Exports.named("TestPreprocessor"))
+ .to(TestPreprocessor.class);
+ }
+ }
+
+ private TestPreprocessor testPreprocessor;
+
+ @Before
+ public void setUp() {
+ testPreprocessor = plugin.getSysInjector().getInstance(TestPreprocessor.class);
+ testPreprocessor.reset();
+ }
+
+ @Test
+ public void insertAccount() throws Exception {
+ Account.Id id = Account.id(sequences.nextAccountId());
+ ExternalId extId = ExternalId.create("foo", "bar", id);
+ accountsUpdateProvider.get().insert("test", id, u -> u.addExternalId(extId));
+ assertThat(testPreprocessor.upserted).containsExactly(extId);
+ }
+
+ @Test
+ public void replaceByKeys() throws Exception {
+ Account.Id id = Account.id(sequences.nextAccountId());
+ ExternalId extId1 = ExternalId.create("foo", "bar1", id);
+ ExternalId extId2 = ExternalId.create("foo", "bar2", id);
+ accountsUpdateProvider.get().insert("test", id, u -> u.addExternalId(extId1));
+
+ testPreprocessor.reset();
+ try (Repository allUsersRepo = repoManager.openRepository(allUsers);
+ MetaDataUpdate md = metaDataUpdateFactory.create(allUsers)) {
+ ExternalIdNotes extIdNotes = extIdNotesFactory.load(allUsersRepo);
+ extIdNotes.replaceByKeys(ImmutableSet.of(extId1.key()), ImmutableSet.of(extId2));
+ extIdNotes.commit(md);
+ }
+ assertThat(testPreprocessor.upserted).containsExactly(extId2);
+ }
+
+ @Test
+ public void insert() throws Exception {
+ Account.Id id = Account.id(sequences.nextAccountId());
+ ExternalId extId = ExternalId.create("foo", "bar", id);
+
+ try (Repository allUsersRepo = repoManager.openRepository(allUsers);
+ MetaDataUpdate md = metaDataUpdateFactory.create(allUsers)) {
+ ExternalIdNotes extIdNotes = extIdNotesFactory.load(allUsersRepo);
+ extIdNotes.insert(extId);
+ extIdNotes.commit(md);
+ }
+ assertThat(testPreprocessor.upserted).containsExactly(extId);
+ }
+
+ @Test
+ public void upsert() throws Exception {
+ Account.Id id = Account.id(sequences.nextAccountId());
+ ExternalId extId = ExternalId.create("foo", "bar", id);
+
+ try (Repository allUsersRepo = repoManager.openRepository(allUsers);
+ MetaDataUpdate md = metaDataUpdateFactory.create(allUsers)) {
+ ExternalIdNotes extIdNotes = extIdNotesFactory.load(allUsersRepo);
+ extIdNotes.upsert(extId);
+ extIdNotes.commit(md);
+ }
+ assertThat(testPreprocessor.upserted).containsExactly(extId);
+ }
+
+ @Test
+ public void replace() throws Exception {
+ Account.Id id = Account.id(sequences.nextAccountId());
+ ExternalId extId1 = ExternalId.create("foo", "bar1", id);
+ ExternalId extId2 = ExternalId.create("foo", "bar2", id);
+ accountsUpdateProvider.get().insert("test", id, u -> u.addExternalId(extId1));
+
+ testPreprocessor.reset();
+ try (Repository allUsersRepo = repoManager.openRepository(allUsers);
+ MetaDataUpdate md = metaDataUpdateFactory.create(allUsers)) {
+ ExternalIdNotes extIdNotes = extIdNotesFactory.load(allUsersRepo);
+ extIdNotes.replace(ImmutableSet.of(extId1), ImmutableSet.of(extId2));
+ extIdNotes.commit(md);
+ }
+ assertThat(testPreprocessor.upserted).containsExactly(extId2);
+ }
+
+ @Test
+ public void replace_viaAccountsUpdate() throws Exception {
+ Account.Id id = Account.id(sequences.nextAccountId());
+ ExternalId extId1 = ExternalId.create("foo", "bar", id, "email1@foo", "hash");
+ ExternalId extId2 = ExternalId.create("foo", "bar", id, "email2@foo", "hash");
+ accountsUpdateProvider.get().insert("test", id, u -> u.addExternalId(extId1));
+
+ testPreprocessor.reset();
+ accountsUpdateProvider.get().update("test", id, u -> u.updateExternalId(extId2));
+ assertThat(testPreprocessor.upserted).containsExactly(extId2);
+ }
+
+ @Test
+ public void blockUpsert() throws Exception {
+ Account.Id id = Account.id(sequences.nextAccountId());
+ ExternalId extId = ExternalId.create("foo", "bar", id);
+ testPreprocessor.throwException = true;
+ StorageException e =
+ assertThrows(
+ StorageException.class,
+ () -> accountsUpdateProvider.get().insert("test", id, u -> u.addExternalId(extId)));
+ assertThat(e).hasMessageThat().contains("upsert not good");
+ assertThat(testPreprocessor.upserted).isEmpty();
+ }
+
+ @Test
+ public void blockUpsert_replace() throws Exception {
+ Account.Id id = Account.id(sequences.nextAccountId());
+ ExternalId extId1 = ExternalId.create("foo", "bar", id, "email1@foo", "hash");
+ ExternalId extId2 = ExternalId.create("foo", "bar", id, "email2@foo", "hash");
+ accountsUpdateProvider.get().insert("test", id, u -> u.addExternalId(extId1));
+
+ assertThat(accounts.get(id).get().externalIds()).containsExactly(extId1);
+
+ testPreprocessor.reset();
+ testPreprocessor.throwException = true;
+ try (Repository allUsersRepo = repoManager.openRepository(allUsers);
+ MetaDataUpdate md = metaDataUpdateFactory.create(allUsers)) {
+ ExternalIdNotes extIdNotes = extIdNotesFactory.load(allUsersRepo);
+ extIdNotes.replace(ImmutableSet.of(extId1), ImmutableSet.of(extId2));
+ StorageException e = assertThrows(StorageException.class, () -> extIdNotes.commit(md));
+ assertThat(e).hasMessageThat().contains("upsert not good");
+ }
+ assertThat(testPreprocessor.upserted).isEmpty();
+ assertThat(accounts.get(id).get().externalIds()).containsExactly(extId1);
+ }
+
+ @Singleton
+ public static class TestPreprocessor implements ExternalIdUpsertPreprocessor {
+ List<ExternalId> upserted = new ArrayList<>();
+
+ boolean throwException = false;
+
+ @Override
+ public void upsert(ExternalId extId) {
+ assertThat(extId.blobId()).isNotNull();
+ if (throwException) {
+ throw new StorageException("upsert not good");
+ }
+ upserted.add(extId);
+ }
+
+ void reset() {
+ upserted.clear();
+ throwException = false;
+ }
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/ssh/BUILD b/javatests/com/google/gerrit/acceptance/ssh/BUILD
index 5634322..ee1b221 100644
--- a/javatests/com/google/gerrit/acceptance/ssh/BUILD
+++ b/javatests/com/google/gerrit/acceptance/ssh/BUILD
@@ -18,6 +18,7 @@
vm_args = ["-Xmx512m"],
deps = [
":util",
+ "//java/com/google/gerrit/server/cancellation",
"//java/com/google/gerrit/server/logging",
"//lib/commons:compress",
],
diff --git a/javatests/com/google/gerrit/acceptance/ssh/SshCancellationIT.java b/javatests/com/google/gerrit/acceptance/ssh/SshCancellationIT.java
index 2cb9637..3e31c16 100644
--- a/javatests/com/google/gerrit/acceptance/ssh/SshCancellationIT.java
+++ b/javatests/com/google/gerrit/acceptance/ssh/SshCancellationIT.java
@@ -99,4 +99,22 @@
adminSshSession.assertFailure("Server Deadline Exceeded (deadline = 10m)");
}
}
+
+ @Test
+ public void handleWrappedRequestCancelledException() throws Exception {
+ ProjectCreationValidationListener projectCreationListener =
+ new ProjectCreationValidationListener() {
+ @Override
+ public void validateNewProject(CreateProjectArgs args) throws ValidationException {
+ throw new RuntimeException(
+ new RequestCancelledException(
+ RequestStateProvider.Reason.SERVER_DEADLINE_EXCEEDED, "deadline = 10m"));
+ }
+ };
+ try (Registration registration =
+ extensionRegistry.newRegistration().add(projectCreationListener)) {
+ adminSshSession.exec("gerrit create-project " + name("new"));
+ adminSshSession.assertFailure("Server Deadline Exceeded (deadline = 10m)");
+ }
+ }
}
diff --git a/javatests/com/google/gerrit/entities/converter/ChangeProtoConverterTest.java b/javatests/com/google/gerrit/entities/converter/ChangeProtoConverterTest.java
index ae8e06d..8c5e449 100644
--- a/javatests/com/google/gerrit/entities/converter/ChangeProtoConverterTest.java
+++ b/javatests/com/google/gerrit/entities/converter/ChangeProtoConverterTest.java
@@ -60,7 +60,6 @@
Entities.Change.newBuilder()
.setChangeId(Entities.Change_Id.newBuilder().setId(14))
.setChangeKey(Entities.Change_Key.newBuilder().setId("change 1"))
- .setRowVersion(0)
.setCreatedOn(987654L)
.setLastUpdatedOn(1234567L)
.setOwnerAccountId(Entities.Account_Id.newBuilder().setId(35))
@@ -109,7 +108,6 @@
.setBranch("refs/heads/branch-74"))
// Default values which can't be unset.
.setCurrentPatchSetId(0)
- .setRowVersion(0)
.setStatus(Change.STATUS_NEW)
.setIsPrivate(false)
.setWorkInProgress(false)
@@ -147,7 +145,6 @@
.setBranch("refs/heads/branch-74"))
.setCurrentPatchSetId(0)
// Default values which can't be unset.
- .setRowVersion(0)
.setStatus(Change.STATUS_NEW)
.setIsPrivate(false)
.setWorkInProgress(false)
@@ -185,7 +182,6 @@
.setCurrentPatchSetId(23)
.setSubject("subject ABC")
// Default values which can't be unset.
- .setRowVersion(0)
.setStatus(Change.STATUS_NEW)
.setIsPrivate(false)
.setWorkInProgress(false)
@@ -251,7 +247,6 @@
assertThat(change.getSubject()).isNull();
assertThat(change.currentPatchSetId()).isNull();
// Default values for unset protobuf fields which can't be unset in the entity object.
- assertThat(change.getRowVersion()).isEqualTo(0);
assertThat(change.isNew()).isTrue();
assertThat(change.isPrivate()).isFalse();
assertThat(change.isWorkInProgress()).isFalse();
@@ -284,7 +279,6 @@
ImmutableMap.<String, Type>builder()
.put("changeId", Change.Id.class)
.put("changeKey", Change.Key.class)
- .put("rowVersion", int.class)
.put("createdOn", Timestamp.class)
.put("lastUpdatedOn", Timestamp.class)
.put("owner", Account.Id.class)
@@ -309,7 +303,6 @@
private static void assertEqualChange(Change change, Change expectedChange) {
assertThat(change.getChangeId()).isEqualTo(expectedChange.getChangeId());
assertThat(change.getKey()).isEqualTo(expectedChange.getKey());
- assertThat(change.getRowVersion()).isEqualTo(expectedChange.getRowVersion());
assertThat(change.getCreatedOn()).isEqualTo(expectedChange.getCreatedOn());
assertThat(change.getLastUpdatedOn()).isEqualTo(expectedChange.getLastUpdatedOn());
assertThat(change.getOwner()).isEqualTo(expectedChange.getOwner());
diff --git a/javatests/com/google/gerrit/server/BUILD b/javatests/com/google/gerrit/server/BUILD
index 7ab7ae9..a2825322 100644
--- a/javatests/com/google/gerrit/server/BUILD
+++ b/javatests/com/google/gerrit/server/BUILD
@@ -55,6 +55,7 @@
"//java/com/google/gerrit/server/account/externalids/testing",
"//java/com/google/gerrit/server/cache/serialize",
"//java/com/google/gerrit/server/cache/testing",
+ "//java/com/google/gerrit/server/cancellation",
"//java/com/google/gerrit/server/fixes/testing",
"//java/com/google/gerrit/server/git/receive:ref_cache",
"//java/com/google/gerrit/server/ioutil",
diff --git a/javatests/com/google/gerrit/server/cancellation/RequestStateContextTest.java b/javatests/com/google/gerrit/server/cancellation/RequestStateContextTest.java
new file mode 100644
index 0000000..e2c43eb
--- /dev/null
+++ b/javatests/com/google/gerrit/server/cancellation/RequestStateContextTest.java
@@ -0,0 +1,178 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.cancellation;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+
+import com.google.common.collect.ImmutableSet;
+import org.junit.Test;
+
+public class RequestStateContextTest {
+ @Test
+ public void openContext() {
+ assertNoRequestStateProviders();
+
+ RequestStateProvider requestStateProvider1 = new TestRequestStateProvider();
+ try (RequestStateContext requestStateContext =
+ RequestStateContext.open().addRequestStateProvider(requestStateProvider1)) {
+ RequestStateProvider requestStateProvider2 = new TestRequestStateProvider();
+ requestStateContext.addRequestStateProvider(requestStateProvider2);
+ assertRequestStateProviders(ImmutableSet.of(requestStateProvider1, requestStateProvider2));
+ }
+
+ assertNoRequestStateProviders();
+ }
+
+ @Test
+ public void openNestedContexts() {
+ assertNoRequestStateProviders();
+
+ RequestStateProvider requestStateProvider1 = new TestRequestStateProvider();
+ try (RequestStateContext requestStateContext =
+ RequestStateContext.open().addRequestStateProvider(requestStateProvider1)) {
+ RequestStateProvider requestStateProvider2 = new TestRequestStateProvider();
+ requestStateContext.addRequestStateProvider(requestStateProvider2);
+ assertRequestStateProviders(ImmutableSet.of(requestStateProvider1, requestStateProvider2));
+
+ RequestStateProvider requestStateProvider3 = new TestRequestStateProvider();
+ try (RequestStateContext requestStateContext2 =
+ RequestStateContext.open().addRequestStateProvider(requestStateProvider3)) {
+ RequestStateProvider requestStateProvider4 = new TestRequestStateProvider();
+ requestStateContext2.addRequestStateProvider(requestStateProvider4);
+ assertRequestStateProviders(
+ ImmutableSet.of(
+ requestStateProvider1,
+ requestStateProvider2,
+ requestStateProvider3,
+ requestStateProvider4));
+ }
+
+ assertRequestStateProviders(ImmutableSet.of(requestStateProvider1, requestStateProvider2));
+ }
+
+ assertNoRequestStateProviders();
+ }
+
+ @Test
+ public void openNestedContextsWithSameRequestStateProviders() {
+ assertNoRequestStateProviders();
+
+ RequestStateProvider requestStateProvider1 = new TestRequestStateProvider();
+ try (RequestStateContext requestStateContext =
+ RequestStateContext.open().addRequestStateProvider(requestStateProvider1)) {
+ RequestStateProvider requestStateProvider2 = new TestRequestStateProvider();
+ requestStateContext.addRequestStateProvider(requestStateProvider2);
+ assertRequestStateProviders(ImmutableSet.of(requestStateProvider1, requestStateProvider2));
+
+ try (RequestStateContext requestStateContext2 =
+ RequestStateContext.open().addRequestStateProvider(requestStateProvider1)) {
+ requestStateContext2.addRequestStateProvider(requestStateProvider2);
+
+ assertRequestStateProviders(ImmutableSet.of(requestStateProvider1, requestStateProvider2));
+ }
+
+ assertRequestStateProviders(ImmutableSet.of(requestStateProvider1, requestStateProvider2));
+ }
+
+ assertNoRequestStateProviders();
+ }
+
+ @Test
+ public void abortIfCancelled_noRequestStateProvider() {
+ assertNoRequestStateProviders();
+
+ // Calling abortIfCancelled() shouldn't throw an exception.
+ RequestStateContext.abortIfCancelled();
+ }
+
+ @Test
+ public void abortIfCancelled_requestNotCancelled() {
+ try (RequestStateContext requestStateContext =
+ RequestStateContext.open()
+ .addRequestStateProvider(
+ new RequestStateProvider() {
+ @Override
+ public void checkIfCancelled(OnCancelled onCancelled) {}
+ })) {
+ // Calling abortIfCancelled() shouldn't throw an exception.
+ RequestStateContext.abortIfCancelled();
+ }
+ }
+
+ @Test
+ public void abortIfCancelled_requestCancelled() {
+ try (RequestStateContext requestStateContext =
+ RequestStateContext.open()
+ .addRequestStateProvider(
+ new RequestStateProvider() {
+ @Override
+ public void checkIfCancelled(OnCancelled onCancelled) {
+ onCancelled.onCancel(
+ RequestStateProvider.Reason.CLIENT_CLOSED_REQUEST, /* message= */ null);
+ }
+ })) {
+ RequestCancelledException requestCancelledException =
+ assertThrows(
+ RequestCancelledException.class, () -> RequestStateContext.abortIfCancelled());
+ assertThat(requestCancelledException)
+ .hasMessageThat()
+ .isEqualTo("Request cancelled: CLIENT_CLOSED_REQUEST");
+ assertThat(requestCancelledException.getCancellationReason())
+ .isEqualTo(RequestStateProvider.Reason.CLIENT_CLOSED_REQUEST);
+ assertThat(requestCancelledException.getCancellationMessage()).isEmpty();
+ }
+ }
+
+ @Test
+ public void abortIfCancelled_requestCancelled_withMessage() {
+ try (RequestStateContext requestStateContext =
+ RequestStateContext.open()
+ .addRequestStateProvider(
+ new RequestStateProvider() {
+ @Override
+ public void checkIfCancelled(OnCancelled onCancelled) {
+ onCancelled.onCancel(
+ RequestStateProvider.Reason.SERVER_DEADLINE_EXCEEDED, "deadline = 10m");
+ }
+ })) {
+ RequestCancelledException requestCancelledException =
+ assertThrows(
+ RequestCancelledException.class, () -> RequestStateContext.abortIfCancelled());
+ assertThat(requestCancelledException)
+ .hasMessageThat()
+ .isEqualTo("Request cancelled: SERVER_DEADLINE_EXCEEDED (deadline = 10m)");
+ assertThat(requestCancelledException.getCancellationReason())
+ .isEqualTo(RequestStateProvider.Reason.SERVER_DEADLINE_EXCEEDED);
+ assertThat(requestCancelledException.getCancellationMessage()).hasValue("deadline = 10m");
+ }
+ }
+
+ private void assertNoRequestStateProviders() {
+ assertRequestStateProviders(ImmutableSet.of());
+ }
+
+ private void assertRequestStateProviders(
+ ImmutableSet<RequestStateProvider> expectedRequestStateProviders) {
+ assertThat(RequestStateContext.getRequestStateProviders())
+ .containsExactlyElementsIn(expectedRequestStateProviders);
+ }
+
+ private static class TestRequestStateProvider implements RequestStateProvider {
+ @Override
+ public void checkIfCancelled(OnCancelled onCancelled) {}
+ }
+}
diff --git a/javatests/com/google/gerrit/server/update/BatchUpdateTest.java b/javatests/com/google/gerrit/server/update/BatchUpdateTest.java
index 287a7fe..10599c6 100644
--- a/javatests/com/google/gerrit/server/update/BatchUpdateTest.java
+++ b/javatests/com/google/gerrit/server/update/BatchUpdateTest.java
@@ -64,6 +64,7 @@
cfg.setInt("change", null, "maxFiles", 2);
cfg.setInt("change", null, "maxPatchSets", MAX_PATCH_SETS);
cfg.setInt("change", null, "maxUpdates", MAX_UPDATES);
+ cfg.setString("index", null, "type", "fake");
return cfg;
});
diff --git a/polygerrit-ui/app/BUILD b/polygerrit-ui/app/BUILD
index 3c9faf8..464e5ba 100644
--- a/polygerrit-ui/app/BUILD
+++ b/polygerrit-ui/app/BUILD
@@ -123,7 +123,6 @@
"elements/shared/gr-comment/gr-comment_html.ts",
"elements/shared/gr-confirm-delete-comment-dialog/gr-confirm-delete-comment-dialog_html.ts",
"elements/shared/gr-dialog/gr-dialog_html.ts",
- "elements/shared/gr-diff-preferences/gr-diff-preferences_html.ts",
"elements/shared/gr-download-commands/gr-download-commands_html.ts",
"elements/shared/gr-dropdown-list/gr-dropdown-list_html.ts",
"elements/shared/gr-dropdown/gr-dropdown_html.ts",
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts
index af565cb..a090aee 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog.ts
@@ -150,18 +150,25 @@
this._query = (text: string) => this._getProjectBranchesSuggestions(text);
}
+ containsDuplicateProject(changes: ChangeInfo[]) {
+ const projects: {[projectName: string]: boolean} = {};
+ for (let i = 0; i < changes.length; i++) {
+ const change = changes[i];
+ if (projects[change.project]) {
+ return true;
+ }
+ projects[change.project] = true;
+ }
+ return false;
+ }
+
updateChanges(changes: ChangeInfo[]) {
this.changes = changes;
this._statuses = {};
- const projects: {[projectName: string]: boolean} = {};
- this._duplicateProjectChanges = false;
changes.forEach(change => {
this.selectedChangeIds.add(change.id);
- if (projects[change.project]) {
- this._duplicateProjectChanges = true;
- }
- projects[change.project] = true;
});
+ this._duplicateProjectChanges = this.containsDuplicateProject(changes);
this._changesCount = changes.length;
this._showCherryPickTopic = changes.length > 1;
}
@@ -185,6 +192,10 @@
if (this.selectedChangeIds.has(changeId))
this.selectedChangeIds.delete(changeId);
else this.selectedChangeIds.add(changeId);
+ const changes = this.changes.filter(change =>
+ this.selectedChangeIds.has(change.id)
+ );
+ this._duplicateProjectChanges = this.containsDuplicateProject(changes);
}
_computeTopicErrorMessage(duplicateProjectChanges: boolean) {
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_test.js b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_test.js
index 536a4ab..1034674 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_test.js
+++ b/polygerrit-ui/app/elements/change/gr-confirm-cherrypick-dialog/gr-confirm-cherrypick-dialog_test.js
@@ -130,6 +130,8 @@
});
test('deselecting a change removes it from being cherry picked', () => {
+ const duplicateChangesStub = sinon.stub(element,
+ 'containsDuplicateProject');
element.branch = 'master';
const executeChangeActionStub = stubRestApi(
'executeChangeAction').returns(Promise.resolve([]));
@@ -142,6 +144,7 @@
querySelector('gr-dialog').$.confirm);
flush();
assert.equal(executeChangeActionStub.callCount, 1);
+ assert.isTrue(duplicateChangesStub.called);
});
test('deselecting all change shows error message', () => {
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 5206fdb..f4f37c1 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
@@ -82,13 +82,9 @@
import {Timing} from '../../../constants/reporting';
import {RevisionInfo} from '../../shared/revision-info/revision-info';
import {preferences$} from '../../../services/user/user-model';
-import {
- changeComments$,
- drafts$,
-} from '../../../services/comments/comments-model';
+import {changeComments$} from '../../../services/comments/comments-model';
import {Subject} from 'rxjs';
import {takeUntil} from 'rxjs/operators';
-import {UIDraft} from '../../../utils/comment-util';
export const DEFAULT_NUM_FILES_SHOWN = 200;
@@ -316,9 +312,6 @@
@property({type: Array})
_dynamicPrependedContentEndpoints?: string[];
- @property({type: Object})
- diffDrafts?: {[path: string]: UIDraft[]} = {};
-
private readonly reporting = appContext.reportingService;
private readonly restApiService = appContext.restApiService;
@@ -373,9 +366,6 @@
/** @override */
connectedCallback() {
super.connectedCallback();
- drafts$.pipe(takeUntil(this.disconnected$)).subscribe(drafts => {
- this.diffDrafts = drafts;
- });
changeComments$
.pipe(takeUntil(this.disconnected$))
.subscribe(changeComments => {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_html.ts b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_html.ts
index 5b248da..98956c9 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_html.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_html.ts
@@ -63,6 +63,7 @@
table {
border-collapse: collapse;
table-layout: fixed;
+ background-color: var(--diff-blank-background-color);
}
/*
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 2ad3be6..afff6f3 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
@@ -225,18 +225,6 @@
this.addEventListener('comment-update', e =>
this._handleCommentUpdate(e as CustomEvent)
);
- // Wait for comment to be rendered before scrolling to it
- if (this.shouldScrollIntoView) {
- const resizeObserver = new ResizeObserver(
- (_entries: ResizeObserverEntry[], observer: ResizeObserver) => {
- if (this.offsetHeight > 0) {
- this.scrollIntoView();
- observer.unobserve(this);
- }
- }
- );
- resizeObserver.observe(this);
- }
}
/** @override */
diff --git a/polygerrit-ui/app/elements/shared/gr-diff-preferences/gr-diff-preferences.ts b/polygerrit-ui/app/elements/shared/gr-diff-preferences/gr-diff-preferences.ts
index 70a7bf3..e560773 100644
--- a/polygerrit-ui/app/elements/shared/gr-diff-preferences/gr-diff-preferences.ts
+++ b/polygerrit-ui/app/elements/shared/gr-diff-preferences/gr-diff-preferences.ts
@@ -21,18 +21,23 @@
import {PolymerElement} from '@polymer/polymer/polymer-element';
import {htmlTemplate} from './gr-diff-preferences_html';
import {customElement, property} from '@polymer/decorators';
-import {DiffPreferencesInfo} from '../../../types/diff';
+import {DiffPreferencesInfo, IgnoreWhitespaceType} from '../../../types/diff';
import {GrSelect} from '../gr-select/gr-select';
import {appContext} from '../../../services/app-context';
export interface GrDiffPreferences {
$: {
+ contextLineSelect: HTMLInputElement;
+ columnsInput: HTMLInputElement;
+ tabSizeInput: HTMLInputElement;
+ fontSizeInput: HTMLInputElement;
lineWrappingInput: HTMLInputElement;
showTabsInput: HTMLInputElement;
showTrailingWhitespaceInput: HTMLInputElement;
automaticReviewInput: HTMLInputElement;
syntaxHighlightInput: HTMLInputElement;
contextSelect: GrSelect;
+ ignoreWhiteSpace: HTMLInputElement;
};
save(): Promise<void>;
}
@@ -61,11 +66,31 @@
this.hasUnsavedChanges = true;
}
+ _handleDiffContextChanged() {
+ this.set('diffPrefs.context', Number(this.$.contextLineSelect.value));
+ this._handleDiffPrefsChanged();
+ }
+
_handleLineWrappingTap() {
this.set('diffPrefs.line_wrapping', this.$.lineWrappingInput.checked);
this._handleDiffPrefsChanged();
}
+ _handleDiffLineLengthChanged() {
+ this.set('diffPrefs.line_length', Number(this.$.columnsInput.value));
+ this._handleDiffPrefsChanged();
+ }
+
+ _handleDiffTabSizeChanged() {
+ this.set('diffPrefs.tab_size', Number(this.$.tabSizeInput.value));
+ this._handleDiffPrefsChanged();
+ }
+
+ _handleDiffFontSizeChanged() {
+ this.set('diffPrefs.font_size', Number(this.$.fontSizeInput.value));
+ this._handleDiffPrefsChanged();
+ }
+
_handleShowTabsTap() {
this.set('diffPrefs.show_tabs', this.$.showTabsInput.checked);
this._handleDiffPrefsChanged();
@@ -92,6 +117,14 @@
this._handleDiffPrefsChanged();
}
+ _handleDiffIgnoreWhitespaceChanged() {
+ this.set(
+ 'diffPrefs.ignore_whitespace',
+ this.$.ignoreWhiteSpace.value as IgnoreWhitespaceType
+ );
+ this._handleDiffPrefsChanged();
+ }
+
save() {
if (!this.diffPrefs)
return Promise.reject(new Error('Missing diff preferences'));
@@ -99,6 +132,27 @@
this.hasUnsavedChanges = false;
});
}
+
+ /**
+ * bind-value has type string so we have to convert
+ * anything inputed to string.
+ *
+ * This is so typescript checker doesn't fail.
+ */
+ _convertToString(key?: number | IgnoreWhitespaceType) {
+ return key !== undefined ? String(key) : '';
+ }
+
+ /**
+ * input 'checked' does not allow undefined,
+ * so we make sure the value is boolean
+ * by returning false if undefined.
+ *
+ * This is so typescript checker doesn't fail.
+ */
+ _convertToBoolean(key?: boolean) {
+ return key !== undefined ? key : false;
+ }
}
declare global {
diff --git a/polygerrit-ui/app/elements/shared/gr-diff-preferences/gr-diff-preferences_html.ts b/polygerrit-ui/app/elements/shared/gr-diff-preferences/gr-diff-preferences_html.ts
index ed3d695..51867c8 100644
--- a/polygerrit-ui/app/elements/shared/gr-diff-preferences/gr-diff-preferences_html.ts
+++ b/polygerrit-ui/app/elements/shared/gr-diff-preferences/gr-diff-preferences_html.ts
@@ -27,12 +27,12 @@
<section>
<label for="contextLineSelect" class="title">Context</label>
<span class="value">
- <gr-select id="contextSelect" bind-value="{{diffPrefs.context}}">
- <select
- id="contextLineSelect"
- on-keypress="_handleDiffPrefsChanged"
- on-change="_handleDiffPrefsChanged"
- >
+ <gr-select
+ id="contextSelect"
+ bind-value="[[_convertToString(diffPrefs.context)]]"
+ on-change="_handleDiffContextChanged"
+ >
+ <select id="contextLineSelect">
<option value="3">3 lines</option>
<option value="10">10 lines</option>
<option value="25">25 lines</option>
@@ -50,7 +50,7 @@
<input
id="lineWrappingInput"
type="checkbox"
- checked="[[diffPrefs.line_wrapping]]"
+ checked="[[_convertToBoolean(diffPrefs.line_wrapping)]]"
on-change="_handleLineWrappingTap"
/>
</span>
@@ -59,23 +59,11 @@
<label for="columnsInput" class="title">Diff width</label>
<span class="value">
<iron-input
- type="number"
- prevent-invalid-input=""
allowed-pattern="[0-9]"
- bind-value="{{diffPrefs.line_length}}"
- on-keypress="_handleDiffPrefsChanged"
- on-change="_handleDiffPrefsChanged"
+ bind-value="[[_convertToString(diffPrefs.line_length)]]"
+ on-change="_handleDiffLineLengthChanged"
>
- <input
- is="iron-input"
- type="number"
- id="columnsInput"
- prevent-invalid-input=""
- allowed-pattern="[0-9]"
- bind-value="{{diffPrefs.line_length}}"
- on-keypress="_handleDiffPrefsChanged"
- on-change="_handleDiffPrefsChanged"
- />
+ <input id="columnsInput" type="number" />
</iron-input>
</span>
</section>
@@ -83,23 +71,11 @@
<label for="tabSizeInput" class="title">Tab width</label>
<span class="value">
<iron-input
- type="number"
- prevent-invalid-input=""
allowed-pattern="[0-9]"
- bind-value="{{diffPrefs.tab_size}}"
- on-keypress="_handleDiffPrefsChanged"
- on-change="_handleDiffPrefsChanged"
+ bind-value="[[_convertToString(diffPrefs.tab_size)]]"
+ on-change="_handleDiffTabSizeChanged"
>
- <input
- is="iron-input"
- type="number"
- id="tabSizeInput"
- prevent-invalid-input=""
- allowed-pattern="[0-9]"
- bind-value="{{diffPrefs.tab_size}}"
- on-keypress="_handleDiffPrefsChanged"
- on-change="_handleDiffPrefsChanged"
- />
+ <input id="tabSizeInput" type="number" />
</iron-input>
</span>
</section>
@@ -107,23 +83,11 @@
<label for="fontSizeInput" class="title">Font size</label>
<span class="value">
<iron-input
- type="number"
- prevent-invalid-input=""
allowed-pattern="[0-9]"
- bind-value="{{diffPrefs.font_size}}"
- on-keypress="_handleDiffPrefsChanged"
- on-change="_handleDiffPrefsChanged"
+ bind-value="[[_convertToString(diffPrefs.font_size)]]"
+ on-change="_handleDiffFontSizeChanged"
>
- <input
- is="iron-input"
- type="number"
- id="fontSizeInput"
- prevent-invalid-input=""
- allowed-pattern="[0-9]"
- bind-value="{{diffPrefs.font_size}}"
- on-keypress="_handleDiffPrefsChanged"
- on-change="_handleDiffPrefsChanged"
- />
+ <input id="fontSizeInput" type="number" />
</iron-input>
</span>
</section>
@@ -133,7 +97,7 @@
<input
id="showTabsInput"
type="checkbox"
- checked="[[diffPrefs.show_tabs]]"
+ checked="[[_convertToBoolean(diffPrefs.show_tabs)]]"
on-change="_handleShowTabsTap"
/>
</span>
@@ -146,7 +110,7 @@
<input
id="showTrailingWhitespaceInput"
type="checkbox"
- checked="[[diffPrefs.show_whitespace_errors]]"
+ checked="[[_convertToBoolean(diffPrefs.show_whitespace_errors)]]"
on-change="_handleShowTrailingWhitespaceTap"
/>
</span>
@@ -159,7 +123,7 @@
<input
id="syntaxHighlightInput"
type="checkbox"
- checked="[[diffPrefs.syntax_highlighting]]"
+ checked="[[_convertToBoolean(diffPrefs.syntax_highlighting)]]"
on-change="_handleSyntaxHighlightTap"
/>
</span>
@@ -172,7 +136,7 @@
<input
id="automaticReviewInput"
type="checkbox"
- checked="[[!diffPrefs.manual_review]]"
+ checked="[[!_convertToBoolean(diffPrefs.manual_review)]]"
on-change="_handleAutomaticReviewTap"
/>
</span>
@@ -181,12 +145,11 @@
<div class="pref">
<label for="ignoreWhiteSpace" class="title">Ignore Whitespace</label>
<span class="value">
- <gr-select bind-value="{{diffPrefs.ignore_whitespace}}">
- <select
- id="ignoreWhiteSpace"
- on-keypress="_handleDiffPrefsChanged"
- on-change="_handleDiffPrefsChanged"
- >
+ <gr-select
+ bind-value="[[_convertToString(diffPrefs.ignore_whitespace)]]"
+ on-change="_handleDiffIgnoreWhitespaceChanged"
+ >
+ <select id="ignoreWhiteSpace">
<option value="IGNORE_NONE">None</option>
<option value="IGNORE_TRAILING">Trailing</option>
<option value="IGNORE_LEADING_AND_TRAILING">
diff --git a/polygerrit-ui/app/elements/shared/gr-diff-preferences/gr-diff-preferences_test.js b/polygerrit-ui/app/elements/shared/gr-diff-preferences/gr-diff-preferences_test.js
deleted file mode 100644
index 716ef2f..0000000
--- a/polygerrit-ui/app/elements/shared/gr-diff-preferences/gr-diff-preferences_test.js
+++ /dev/null
@@ -1,105 +0,0 @@
-/**
- * @license
- * Copyright (C) 2016 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.
- */
-
-import '../../../test/common-test-setup-karma.js';
-import './gr-diff-preferences.js';
-import {stubRestApi} from '../../../test/test-utils.js';
-
-const basicFixture = fixtureFromElement('gr-diff-preferences');
-
-suite('gr-diff-preferences tests', () => {
- let element;
-
- let diffPreferences;
-
- function valueOf(title, fieldsetid) {
- const sections = element.$[fieldsetid].querySelectorAll('section');
- let titleEl;
- for (let i = 0; i < sections.length; i++) {
- titleEl = sections[i].querySelector('.title');
- if (titleEl.textContent.trim() === title) {
- return sections[i].querySelector('.value');
- }
- }
- }
-
- setup(() => {
- diffPreferences = {
- context: 10,
- line_wrapping: false,
- line_length: 100,
- tab_size: 8,
- font_size: 12,
- show_tabs: true,
- show_whitespace_errors: true,
- syntax_highlighting: true,
- manual_review: false,
- ignore_whitespace: 'IGNORE_NONE',
- };
-
- stubRestApi('getDiffPreferences').returns(Promise.resolve(diffPreferences));
-
- element = basicFixture.instantiate();
-
- return element.loadData();
- });
-
- test('renders', () => {
- // Rendered with the expected preferences selected.
- assert.equal(valueOf('Context', 'diffPreferences')
- .firstElementChild.bindValue, diffPreferences.context);
- assert.equal(valueOf('Fit to screen', 'diffPreferences')
- .firstElementChild.checked, diffPreferences.line_wrapping);
- assert.equal(valueOf('Diff width', 'diffPreferences')
- .firstElementChild.bindValue, diffPreferences.line_length);
- assert.equal(valueOf('Tab width', 'diffPreferences')
- .firstElementChild.bindValue, diffPreferences.tab_size);
- assert.equal(valueOf('Font size', 'diffPreferences')
- .firstElementChild.bindValue, diffPreferences.font_size);
- assert.equal(valueOf('Show tabs', 'diffPreferences')
- .firstElementChild.checked, diffPreferences.show_tabs);
- assert.equal(valueOf('Show trailing whitespace', 'diffPreferences')
- .firstElementChild.checked, diffPreferences.show_whitespace_errors);
- assert.equal(valueOf('Syntax highlighting', 'diffPreferences')
- .firstElementChild.checked, diffPreferences.syntax_highlighting);
- assert.equal(
- valueOf('Automatically mark viewed files reviewed', 'diffPreferences')
- .firstElementChild.checked, !diffPreferences.manual_review);
- assert.equal(valueOf('Ignore Whitespace', 'diffPreferences')
- .firstElementChild.bindValue, diffPreferences.ignore_whitespace);
-
- assert.isFalse(element.hasUnsavedChanges);
- });
-
- test('save changes', () => {
- stubRestApi('saveDiffPreferences')
- .returns(Promise.resolve());
- const showTrailingWhitespaceCheckbox =
- valueOf('Show trailing whitespace', 'diffPreferences')
- .firstElementChild;
- showTrailingWhitespaceCheckbox.checked = false;
- element._handleShowTrailingWhitespaceTap();
-
- assert.isTrue(element.hasUnsavedChanges);
-
- // Save the change.
- return element.save().then(() => {
- assert.isFalse(element.hasUnsavedChanges);
- });
- });
-});
-
diff --git a/polygerrit-ui/app/elements/shared/gr-diff-preferences/gr-diff-preferences_test.ts b/polygerrit-ui/app/elements/shared/gr-diff-preferences/gr-diff-preferences_test.ts
new file mode 100644
index 0000000..6c1404e
--- /dev/null
+++ b/polygerrit-ui/app/elements/shared/gr-diff-preferences/gr-diff-preferences_test.ts
@@ -0,0 +1,134 @@
+/**
+ * @license
+ * Copyright (C) 2016 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.
+ */
+
+import '../../../test/common-test-setup-karma';
+import './gr-diff-preferences';
+import {GrDiffPreferences} from './gr-diff-preferences';
+import {stubRestApi} from '../../../test/test-utils';
+import {DiffPreferencesInfo} from '../../../types/diff';
+import {createDefaultDiffPrefs} from '../../../constants/constants';
+import {IronInputElement} from '@polymer/iron-input';
+import {GrSelect} from '../gr-select/gr-select';
+
+const basicFixture = fixtureFromElement('gr-diff-preferences');
+
+suite('gr-diff-preferences tests', () => {
+ let element: GrDiffPreferences;
+
+ let diffPreferences: DiffPreferencesInfo;
+
+ function valueOf(title: string, id: string) {
+ const sections = element.root?.querySelectorAll(`#${id} section`) ?? [];
+ let titleEl;
+ for (let i = 0; i < sections.length; i++) {
+ titleEl = sections[i].querySelector('.title');
+ if (titleEl?.textContent?.trim() === title) {
+ const el = sections[i].querySelector('.value');
+ if (el) return el;
+ }
+ }
+ assert.fail(`element with title ${title} not found`);
+ }
+
+ setup(async () => {
+ diffPreferences = createDefaultDiffPrefs();
+
+ stubRestApi('getDiffPreferences').returns(Promise.resolve(diffPreferences));
+
+ element = basicFixture.instantiate();
+
+ await element.loadData();
+ await flush();
+ });
+
+ test('renders', () => {
+ // Rendered with the expected preferences selected.
+ const contextInput = valueOf('Context', 'diffPreferences')
+ .firstElementChild as IronInputElement;
+ assert.equal(contextInput.bindValue, `${diffPreferences.context}`);
+
+ const lineWrappingInput = valueOf('Fit to screen', 'diffPreferences')
+ .firstElementChild as HTMLInputElement;
+ assert.equal(lineWrappingInput.checked, diffPreferences.line_wrapping);
+
+ const lineLengthInput = valueOf('Diff width', 'diffPreferences')
+ .firstElementChild as IronInputElement;
+ assert.equal(lineLengthInput.bindValue, `${diffPreferences.line_length}`);
+
+ const tabSizeInput = valueOf('Tab width', 'diffPreferences')
+ .firstElementChild as IronInputElement;
+ assert.equal(tabSizeInput.bindValue, `${diffPreferences.tab_size}`);
+
+ const fontSizeInput = valueOf('Font size', 'diffPreferences')
+ .firstElementChild as IronInputElement;
+ assert.equal(fontSizeInput.bindValue, `${diffPreferences.font_size}`);
+
+ const showTabsInput = valueOf('Show tabs', 'diffPreferences')
+ .firstElementChild as HTMLInputElement;
+ assert.equal(showTabsInput.checked, diffPreferences.show_tabs);
+
+ const showWhitespaceErrorsInput = valueOf(
+ 'Show trailing whitespace',
+ 'diffPreferences'
+ ).firstElementChild as HTMLInputElement;
+ assert.equal(
+ showWhitespaceErrorsInput.checked,
+ diffPreferences.show_whitespace_errors
+ );
+
+ const syntaxHighlightingInput = valueOf(
+ 'Syntax highlighting',
+ 'diffPreferences'
+ ).firstElementChild as HTMLInputElement;
+ assert.equal(
+ syntaxHighlightingInput.checked,
+ diffPreferences.syntax_highlighting
+ );
+
+ const manualReviewInput = valueOf(
+ 'Automatically mark viewed files reviewed',
+ 'diffPreferences'
+ ).firstElementChild as HTMLInputElement;
+ assert.equal(manualReviewInput.checked, !diffPreferences.manual_review);
+
+ const ignoreWhitespaceInput = valueOf(
+ 'Ignore Whitespace',
+ 'diffPreferences'
+ ).firstElementChild as GrSelect;
+ assert.equal(
+ ignoreWhitespaceInput.bindValue,
+ diffPreferences.ignore_whitespace
+ );
+
+ assert.isFalse(element.hasUnsavedChanges);
+ });
+
+ test('save changes', async () => {
+ const showTrailingWhitespaceCheckbox = valueOf(
+ 'Show trailing whitespace',
+ 'diffPreferences'
+ ).firstElementChild as HTMLInputElement;
+ showTrailingWhitespaceCheckbox.checked = false;
+ element._handleShowTrailingWhitespaceTap();
+
+ assert.isTrue(element.hasUnsavedChanges);
+
+ // Save the change.
+ await element.save();
+ assert.isFalse(element.hasUnsavedChanges);
+ });
+});
diff --git a/polygerrit-ui/app/services/comments/comments-model.ts b/polygerrit-ui/app/services/comments/comments-model.ts
index b26ec9b..bbc7b1a 100644
--- a/polygerrit-ui/app/services/comments/comments-model.ts
+++ b/polygerrit-ui/app/services/comments/comments-model.ts
@@ -111,7 +111,7 @@
if (!drafts[draft.path]) drafts[draft.path] = [] as DraftInfo[];
else drafts[draft.path] = [...drafts[draft.path]];
const index = drafts[draft.path].findIndex(
- d => d.__draftID === draft.__draftID || d.id === draft.id
+ d => (d.__draftID && d.__draftID === draft.__draftID) || d.id === draft.id
);
if (index !== -1) {
drafts[draft.path][index] = draft;
@@ -127,7 +127,7 @@
nextState.drafts = {...nextState.drafts};
const drafts = nextState.drafts;
const index = (drafts[draft.path] || []).findIndex(
- d => d.__draftID === draft.__draftID || d.id === draft.id
+ d => (d.__draftID && d.__draftID === draft.__draftID) || d.id === draft.id
);
if (index === -1) return;
drafts[draft.path] = [...drafts[draft.path]];
diff --git a/polygerrit-ui/app/styles/themes/app-theme.ts b/polygerrit-ui/app/styles/themes/app-theme.ts
index 134003b..1996800 100644
--- a/polygerrit-ui/app/styles/themes/app-theme.ts
+++ b/polygerrit-ui/app/styles/themes/app-theme.ts
@@ -128,20 +128,20 @@
--error-foreground: var(--red-700);
--error-background: var(--red-50);
- --error-background-hover: linear-gradient(var(--red-700-04), var(--red-700-04), var(--red-50));
- --error-background-focus: linear-gradient(var(--red-700-12), var(--red-700-12), var(--red-50));
+ --error-background-hover: linear-gradient(var(--red-700-04), var(--red-700-04)), var(--red-50);
+ --error-background-focus: linear-gradient(var(--red-700-12), var(--red-700-12)), var(--red-50);
--error-ripple: var(--red-700-10);
--warning-foreground: var(--orange-700);
--warning-background: var(--orange-50);
- --warning-background-hover: linear-gradient(var(--orange-700-04), var(--orange-700-04), var(--orange-50));
- --warning-background-focus: linear-gradient(var(--orange-700-12), var(--orange-700-12), var(--orange-50));
+ --warning-background-hover: linear-gradient(var(--orange-700-04), var(--orange-700-04)), var(--orange-50);
+ --warning-background-focus: linear-gradient(var(--orange-700-12), var(--orange-700-12)), var(--orange-50);
--warning-ripple: var(--orange-700-10);
--info-foreground: var(--blue-700);
--info-background: var(--blue-50);
- --info-background-hover: linear-gradient(var(--blue-700-04), var(--blue-700-04), var(--blue-50));
- --info-background-focus: linear-gradient(var(--blue-700-12), var(--blue-700-12), var(--blue-50));
+ --info-background-hover: linear-gradient(var(--blue-700-04), var(--blue-700-04)), var(--blue-50);
+ --info-background-focus: linear-gradient(var(--blue-700-12), var(--blue-700-12)), var(--blue-50);
--info-ripple: var(--blue-700-10);
--primary-button-text-color: white;
@@ -154,14 +154,14 @@
--success-foreground: var(--green-700);
--success-background: var(--green-50);
- --success-background-hover: linear-gradient(var(--green-700-04), var(--green-700-04), var(--green-50));
- --success-background-focus: linear-gradient(var(--green-700-12), var(--green-700-12), var(--green-50));
+ --success-background-hover: linear-gradient(var(--green-700-04), var(--green-700-04)), var(--green-50);
+ --success-background-focus: linear-gradient(var(--green-700-12), var(--green-700-12)), var(--green-50);
--success-ripple: var(--green-700-10);
--gray-foreground: var(--gray-700);
--gray-background: var(--gray-100);
- --gray-background-hover: linear-gradient(var(--gray-700-04), var(--gray-700-04), var(--gray-100));
- --gray-background-focus: linear-gradient(var(--gray-700-12), var(--gray-700-12), var(--gray-100));
+ --gray-background-hover: linear-gradient(var(--gray-700-04), var(--gray-700-04)), var(--gray-100);
+ --gray-background-focus: linear-gradient(var(--gray-700-12), var(--gray-700-12)), var(--gray-100);
--gray-ripple: var(--gray-700-10);
--disabled-foreground: var(--gray-800-38);
diff --git a/polygerrit-ui/app/styles/themes/dark-theme.ts b/polygerrit-ui/app/styles/themes/dark-theme.ts
index 69256b2..926b02d 100644
--- a/polygerrit-ui/app/styles/themes/dark-theme.ts
+++ b/polygerrit-ui/app/styles/themes/dark-theme.ts
@@ -38,20 +38,20 @@
--error-foreground: var(--red-200);
--error-background: var(--red-tonal);
- --error-background-hover: linear-gradient(var(--white-04), var(--white-04), var(--red-tonal));
- --error-background-focus: linear-gradient(var(--white-12), var(--white-12), var(--red-tonal));
+ --error-background-hover: linear-gradient(var(--white-04), var(--white-04)), var(--red-tonal);
+ --error-background-focus: linear-gradient(var(--white-12), var(--white-12)), var(--red-tonal);
--error-ripple: var(--white-10);
--warning-foreground: var(--orange-200);
--warning-background: var(--orange-tonal);
- --warning-background-hover: linear-gradient(var(--white-04), var(--white-04), var(--orange-tonal));
- --warning-background-focus: linear-gradient(var(--white-12), var(--white-12), var(--orange-tonal));
+ --warning-background-hover: linear-gradient(var(--white-04), var(--white-04)), var(--orange-tonal);
+ --warning-background-focus: linear-gradient(var(--white-12), var(--white-12)), var(--orange-tonal);
--warning-ripple: var(--white-10);
--info-foreground: var(--blue-200);
--info-background: var(--blue-tonal);
- --info-background-hover: linear-gradient(var(--white-04), var(--white-04), var(--blue-tonal));
- --info-background-focus: linear-gradient(var(--white-12), var(--white-12), var(--blue-tonal));
+ --info-background-hover: linear-gradient(var(--white-04), var(--white-04)), var(--blue-tonal);
+ --info-background-focus: linear-gradient(var(--white-12), var(--white-12)), var(--blue-tonal);
--info-ripple: var(--white-10);
--primary-button-text-color: black;
@@ -64,14 +64,14 @@
--success-foreground: var(--green-200);
--success-background: var(--green-tonal);
- --success-background-hover: linear-gradient(var(--white-04), var(--white-04), var(--green-tonal));
- --success-background-focus: linear-gradient(var(--white-12), var(--white-12), var(--green-tonal));
+ --success-background-hover: linear-gradient(var(--white-04), var(--white-04)), var(--green-tonal);
+ --success-background-focus: linear-gradient(var(--white-12), var(--white-12)), var(--green-tonal);
--success-ripple: var(--white-10);
--gray-foreground: var(--gray-300);
--gray-background: var(--gray-tonal);
- --gray-background-hover: linear-gradient(var(--white-04), var(--white-04), var(--gray-tonal));
- --gray-background-focus: linear-gradient(var(--white-12), var(--white-12), var(--gray-tonal));
+ --gray-background-hover: linear-gradient(var(--white-04), var(--white-04)), var(--gray-tonal);
+ --gray-background-focus: linear-gradient(var(--white-12), var(--white-12)), var(--gray-tonal);
--gray-ripple: var(--white-10);
--disabled-foreground: var(--gray-200-38);
diff --git a/proto/entities.proto b/proto/entities.proto
index 84c7fbd..d4ff736 100644
--- a/proto/entities.proto
+++ b/proto/entities.proto
@@ -35,7 +35,6 @@
message Change {
required Change_Id change_id = 1;
optional Change_Key change_key = 2;
- optional int32 row_version = 3;
optional fixed64 created_on = 4;
optional fixed64 last_updated_on = 5;
optional Account_Id owner_account_id = 7;
@@ -54,6 +53,7 @@
optional PatchSet_Id cherry_pick_of = 24;
// Deleted fields, should not be reused:
+ reserved 3; // row_version
reserved 6; // sortkey
reserved 9; // open
reserved 11; // nbrPatchSets
