Merge "Fix exception for Revert Submission"
diff --git a/Jenkinsfile b/Jenkinsfile
index 0193df3..f21c7897 100644
--- a/Jenkinsfile
+++ b/Jenkinsfile
@@ -185,9 +185,17 @@
def collectBuilds() {
def builds = [:]
- builds["Gerrit-codestyle"] = prepareBuildsForMode("Gerrit-codestyle")
- Builds.modes.each {
- builds["Gerrit-verification(${it})"] = prepareBuildsForMode("Gerrit-verifier-bazel", it)
+ if (env.GERRIT_CHANGE_NUMBER == "") {
+ builds["java8"] = { -> build "Gerrit-bazel-${env.BRANCH_NAME}" }
+
+ if (env.BRANCH_NAME == "master") {
+ builds["java11"] = { -> build "Gerrit-bazel-java11-${env.BRANCH_NAME}" }
+ }
+ } else {
+ builds["Gerrit-codestyle"] = prepareBuildsForMode("Gerrit-codestyle")
+ Builds.modes.each {
+ builds["Gerrit-verification(${it})"] = prepareBuildsForMode("Gerrit-verifier-bazel", it)
+ }
}
return builds
}
@@ -274,44 +282,48 @@
node ('master') {
- stage('Preparing'){
- gerritReview labels: ['Verified': 0, 'Code-Style': 0]
+ if (env.GERRIT_CHANGE_NUMBER != "") {
+ stage('Preparing'){
+ gerritReview labels: ['Verified': 0, 'Code-Style': 0]
- getChangeMetaData()
- collectBuildModes()
+ getChangeMetaData()
+ collectBuildModes()
+ }
}
parallel(collectBuilds())
- stage('Retry Flaky Builds'){
- def flakyBuildsModes = findFlakyBuilds()
- if (flakyBuildsModes.size() > 0){
- parallel flakyBuildsModes.collectEntries {
- ["Gerrit-verification(${it})" :
- prepareBuildsForMode("Gerrit-verifier-bazel", it, 3)]
+ if (env.GERRIT_CHANGE_NUMBER != "") {
+ stage('Retry Flaky Builds'){
+ def flakyBuildsModes = findFlakyBuilds()
+ if (flakyBuildsModes.size() > 0){
+ parallel flakyBuildsModes.collectEntries {
+ ["Gerrit-verification(${it})" :
+ prepareBuildsForMode("Gerrit-verifier-bazel", it, 3)]
+ }
}
}
- }
- stage('Report to Gerrit'){
- resCodeStyle = getLabelValue(1, Builds.codeStyle.result)
- gerritReview(
- labels: ['Code-Style': resCodeStyle],
- message: createCodeStyleMsgBody(Builds.codeStyle, resCodeStyle))
- postCheck(new GerritCheck("codestyle", Change.number, Change.sha1, Builds.codeStyle))
+ stage('Report to Gerrit'){
+ resCodeStyle = getLabelValue(1, Builds.codeStyle.result)
+ gerritReview(
+ labels: ['Code-Style': resCodeStyle],
+ message: createCodeStyleMsgBody(Builds.codeStyle, resCodeStyle))
+ postCheck(new GerritCheck("codestyle", Change.number, Change.sha1, Builds.codeStyle))
- def verificationResults = Builds.verification.collect { k, v -> v }
- def resVerify = verificationResults.inject(1) {
- acc, build -> getLabelValue(acc, build.result)
+ def verificationResults = Builds.verification.collect { k, v -> v }
+ def resVerify = verificationResults.inject(1) {
+ acc, build -> getLabelValue(acc, build.result)
+ }
+ gerritReview(
+ labels: ['Verified': resVerify],
+ message: createVerifyMsgBody(Builds.verification))
+
+ Builds.verification.each { type, build -> postCheck(
+ new GerritCheck(type, Change.number, Change.sha1, build)
+ )}
+
+ setResult(resVerify, resCodeStyle)
}
- gerritReview(
- labels: ['Verified': resVerify],
- message: createVerifyMsgBody(Builds.verification))
-
- Builds.verification.each { type, build -> postCheck(
- new GerritCheck(type, Change.number, Change.sha1, build)
- )}
-
- setResult(resVerify, resCodeStyle)
}
}
diff --git a/java/com/google/gerrit/httpd/restapi/RestApiMetrics.java b/java/com/google/gerrit/httpd/restapi/RestApiMetrics.java
index a378fa4..aa1b921 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiMetrics.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiMetrics.java
@@ -19,7 +19,7 @@
import com.google.gerrit.extensions.registration.PluginName;
import com.google.gerrit.httpd.restapi.RestApiServlet.ViewData;
import com.google.gerrit.metrics.Counter1;
-import com.google.gerrit.metrics.Counter2;
+import com.google.gerrit.metrics.Counter3;
import com.google.gerrit.metrics.Description;
import com.google.gerrit.metrics.Description.Units;
import com.google.gerrit.metrics.Field;
@@ -37,7 +37,7 @@
};
final Counter1<String> count;
- final Counter2<String, Integer> errorCount;
+ final Counter3<String, Integer, String> errorCount;
final Timer1<String> serverLatency;
final Histogram1<String> responseBytes;
@@ -60,6 +60,9 @@
viewField,
Field.ofInteger("error_code", Metadata.Builder::httpStatus)
.description("HTTP status code")
+ .build(),
+ Field.ofString("cause", Metadata.Builder::cause)
+ .description("The cause of the error.")
.build());
serverLatency =
diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index 1514df8..0535397 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -17,6 +17,7 @@
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.flogger.LazyArgs.lazy;
import static com.google.common.net.HttpHeaders.ACCESS_CONTROL_ALLOW_CREDENTIALS;
import static com.google.common.net.HttpHeaders.ACCESS_CONTROL_ALLOW_HEADERS;
@@ -103,6 +104,7 @@
import com.google.gerrit.server.AccessPath;
import com.google.gerrit.server.AnonymousUser;
import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.ExceptionHook;
import com.google.gerrit.server.OptionUtil;
import com.google.gerrit.server.RequestInfo;
import com.google.gerrit.server.RequestListener;
@@ -173,6 +175,7 @@
import java.util.Set;
import java.util.TreeMap;
import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
import java.util.regex.Pattern;
import java.util.stream.Stream;
import java.util.zip.GZIPOutputStream;
@@ -245,6 +248,7 @@
final DynamicSet<PerformanceLogger> performanceLoggers;
final ChangeFinder changeFinder;
final RetryHelper retryHelper;
+ final PluginSetContext<ExceptionHook> exceptionHooks;
@Inject
Globals(
@@ -259,7 +263,8 @@
@GerritServerConfig Config config,
DynamicSet<PerformanceLogger> performanceLoggers,
ChangeFinder changeFinder,
- RetryHelper retryHelper) {
+ RetryHelper retryHelper,
+ PluginSetContext<ExceptionHook> exceptionHooks) {
this.currentUser = currentUser;
this.webSession = webSession;
this.paramParser = paramParser;
@@ -272,6 +277,7 @@
this.performanceLoggers = performanceLoggers;
this.changeFinder = changeFinder;
this.retryHelper = retryHelper;
+ this.exceptionHooks = exceptionHooks;
allowOrigin = makeAllowOrigin(config);
}
@@ -286,7 +292,6 @@
private final Globals globals;
private final Provider<RestCollection<RestResource, RestResource>> members;
- private Optional<String> traceId = Optional.empty();
public RestApiServlet(
Globals globals, RestCollection<? extends RestResource, ? extends RestResource> members) {
@@ -312,6 +317,7 @@
res.setHeader("X-Content-Type-Options", "nosniff");
int status = SC_OK;
long responseBytes = -1;
+ Optional<Exception> cause = Optional.empty();
Response<?> response = null;
QueryParams qp = null;
Object inputRequestBody = null;
@@ -593,22 +599,28 @@
}
}
} catch (MalformedJsonException | JsonParseException e) {
+ cause = Optional.of(e);
responseBytes =
replyError(
req, res, status = SC_BAD_REQUEST, "Invalid " + JSON_TYPE + " in request", e);
} catch (BadRequestException e) {
+ cause = Optional.of(e);
responseBytes =
replyError(
req, res, status = SC_BAD_REQUEST, messageOr(e, "Bad Request"), e.caching(), e);
} catch (AuthException e) {
+ cause = Optional.of(e);
responseBytes =
replyError(req, res, status = SC_FORBIDDEN, messageOr(e, "Forbidden"), e.caching(), e);
} catch (AmbiguousViewException e) {
+ cause = Optional.of(e);
responseBytes = replyError(req, res, status = SC_NOT_FOUND, messageOr(e, "Ambiguous"), e);
} catch (ResourceNotFoundException e) {
+ cause = Optional.of(e);
responseBytes =
replyError(req, res, status = SC_NOT_FOUND, messageOr(e, "Not Found"), e.caching(), e);
} catch (MethodNotAllowedException e) {
+ cause = Optional.of(e);
responseBytes =
replyError(
req,
@@ -618,9 +630,11 @@
e.caching(),
e);
} catch (ResourceConflictException e) {
+ cause = Optional.of(e);
responseBytes =
replyError(req, res, status = SC_CONFLICT, messageOr(e, "Conflict"), e.caching(), e);
} catch (PreconditionFailedException e) {
+ cause = Optional.of(e);
responseBytes =
replyError(
req,
@@ -630,6 +644,7 @@
e.caching(),
e);
} catch (UnprocessableEntityException e) {
+ cause = Optional.of(e);
responseBytes =
replyError(
req,
@@ -639,19 +654,22 @@
e.caching(),
e);
} catch (NotImplementedException e) {
+ cause = Optional.of(e);
logger.atSevere().withCause(e).log("Error in %s %s", req.getMethod(), uriForLogging(req));
responseBytes =
replyError(req, res, status = SC_NOT_IMPLEMENTED, messageOr(e, "Not Implemented"), e);
} catch (UpdateException e) {
+ cause = Optional.of(e);
Throwable t = e.getCause();
if (t instanceof LockFailureException) {
logger.atSevere().withCause(t).log("Error in %s %s", req.getMethod(), uriForLogging(req));
responseBytes = replyError(req, res, status = SC_SERVICE_UNAVAILABLE, "Lock failure", e);
} else {
status = SC_INTERNAL_SERVER_ERROR;
- responseBytes = handleException(e, req, res);
+ responseBytes = handleException(traceContext, e, req, res);
}
} catch (QuotaException e) {
+ cause = Optional.of(e);
responseBytes =
replyError(
req,
@@ -661,13 +679,15 @@
e.caching(),
e);
} catch (Exception e) {
+ cause = Optional.of(e);
status = SC_INTERNAL_SERVER_ERROR;
- responseBytes = handleException(e, req, res);
+ responseBytes = handleException(traceContext, e, req, res);
} finally {
String metric = getViewName(viewData);
+ String formattedCause = cause.map(globals.retryHelper::formatCause).orElse("_none");
globals.metrics.count.increment(metric);
if (status >= SC_BAD_REQUEST) {
- globals.metrics.errorCount.increment(metric, status);
+ globals.metrics.errorCount.increment(metric, status, formattedCause);
}
if (responseBytes != -1) {
globals.metrics.responseBytes.record(metric, responseBytes);
@@ -794,6 +814,7 @@
ActionType actionType,
Action<T> action)
throws Exception {
+ AtomicReference<Optional<String>> traceId = new AtomicReference<>(Optional.empty());
RetryHelper.Options.Builder retryOptionsBuilder = RetryHelper.options().caller(caller);
if (!traceContext.isTracing()) {
// enable automatic retry with tracing in case of non-recoverable failure
@@ -802,7 +823,7 @@
.retryWithTrace(t -> !(t instanceof RestApiException))
.onAutoTrace(
autoTraceId -> {
- traceId = Optional.of(autoTraceId);
+ traceId.set(Optional.of(autoTraceId));
// Include details of the request into the trace.
traceRequestData(req);
@@ -818,7 +839,9 @@
// If auto-tracing got triggered due to a non-recoverable failure, also trace the rest of
// this request. This means logging is forced for all further log statements and the logs are
// associated with the same trace ID.
- traceId.ifPresent(tid -> traceContext.addTag(RequestId.Type.TRACE_ID, tid).forceLogging());
+ traceId
+ .get()
+ .ifPresent(tid -> traceContext.addTag(RequestId.Type.TRACE_ID, tid).forceLogging());
}
}
@@ -1640,13 +1663,25 @@
}
}
- private long handleException(Throwable err, HttpServletRequest req, HttpServletResponse res)
+ private long handleException(
+ TraceContext traceContext, Throwable err, HttpServletRequest req, HttpServletResponse res)
throws IOException {
logger.atSevere().withCause(err).log("Error in %s %s", req.getMethod(), uriForLogging(req));
if (!res.isCommitted()) {
res.reset();
- traceId.ifPresent(traceId -> res.addHeader(X_GERRIT_TRACE, traceId));
- return replyError(req, res, SC_INTERNAL_SERVER_ERROR, "Internal server error", err);
+ traceContext.getTraceId().ifPresent(traceId -> res.addHeader(X_GERRIT_TRACE, traceId));
+ StringBuilder msg = new StringBuilder("Internal server error");
+ ImmutableList<String> userMessages =
+ globals.exceptionHooks.stream()
+ .map(h -> h.getUserMessage(err))
+ .filter(Optional::isPresent)
+ .map(Optional::get)
+ .collect(toImmutableList());
+ if (!userMessages.isEmpty()) {
+ msg.append("\n");
+ userMessages.forEach(m -> msg.append("\n* ").append(m));
+ }
+ return replyError(req, res, SC_INTERNAL_SERVER_ERROR, msg.toString(), err);
}
return 0;
}
diff --git a/java/com/google/gerrit/server/CreateGroupPermissionSyncer.java b/java/com/google/gerrit/server/CreateGroupPermissionSyncer.java
index 996257c..5e7919f 100644
--- a/java/com/google/gerrit/server/CreateGroupPermissionSyncer.java
+++ b/java/com/google/gerrit/server/CreateGroupPermissionSyncer.java
@@ -115,11 +115,12 @@
.collect(toList()));
config.replace(createGroupAccessSection);
} else {
- Permission createGroupPermission = new Permission(Permission.CREATE);
- createGroupAccessSection.addPermission(createGroupPermission);
- createGroupsGlobal.forEach(createGroupPermission::add);
// The create permission is managed by Gerrit at this point only so there is no concern of
// overwriting user-defined permissions here.
+ Permission createGroupPermission = new Permission(Permission.CREATE);
+ createGroupAccessSection.remove(createGroupPermission);
+ createGroupAccessSection.addPermission(createGroupPermission);
+ createGroupsGlobal.forEach(createGroupPermission::add);
config.replace(createGroupAccessSection);
}
diff --git a/java/com/google/gerrit/server/ExceptionHook.java b/java/com/google/gerrit/server/ExceptionHook.java
index 6f05814..db44b4b 100644
--- a/java/com/google/gerrit/server/ExceptionHook.java
+++ b/java/com/google/gerrit/server/ExceptionHook.java
@@ -53,4 +53,15 @@
default Optional<String> formatCause(Throwable throwable) {
return Optional.empty();
}
+
+ /**
+ * Returns an error message that should be returned to the user.
+ *
+ * @param throwable throwable that was thrown while executing an operation
+ * @return error message that should be returned to the user, {@link Optional#empty()} if no
+ * message should be returned to the user
+ */
+ default Optional<String> getUserMessage(Throwable throwable) {
+ return Optional.empty();
+ }
}
diff --git a/java/com/google/gerrit/server/ExceptionHookImpl.java b/java/com/google/gerrit/server/ExceptionHookImpl.java
index b5edb24..9613b29 100644
--- a/java/com/google/gerrit/server/ExceptionHookImpl.java
+++ b/java/com/google/gerrit/server/ExceptionHookImpl.java
@@ -14,9 +14,9 @@
package com.google.gerrit.server;
-import com.google.gerrit.exceptions.StorageException;
+import com.google.common.base.Predicate;
+import com.google.common.base.Throwables;
import com.google.gerrit.git.LockFailureException;
-import com.google.gerrit.server.update.UpdateException;
import java.util.Optional;
import org.eclipse.jgit.lib.RefUpdate;
@@ -25,6 +25,11 @@
* a retry of the failed operation.
*/
public class ExceptionHookImpl implements ExceptionHook {
+ private static final String LOCK_FAILURE_USER_MESSAGE =
+ "Updating a ref failed with LOCK_FAILURE.\n"
+ + "This may be a temporary issue due to concurrent updates.\n"
+ + "Please retry later.";
+
@Override
public boolean shouldRetry(Throwable throwable) {
return isLockFailure(throwable);
@@ -38,10 +43,26 @@
return Optional.empty();
}
- private static boolean isLockFailure(Throwable throwable) {
- if (throwable instanceof UpdateException || throwable instanceof StorageException) {
- throwable = throwable.getCause();
+ @Override
+ public Optional<String> getUserMessage(Throwable throwable) {
+ if (isLockFailure(throwable)) {
+ return Optional.of(LOCK_FAILURE_USER_MESSAGE);
}
- return throwable instanceof LockFailureException;
+ return Optional.empty();
+ }
+
+ private static boolean isLockFailure(Throwable throwable) {
+ return isMatching(throwable, t -> t instanceof LockFailureException);
+ }
+
+ /**
+ * Check whether the given exception or any of its causes matches the given predicate.
+ *
+ * @param throwable Exception that should be tested
+ * @param predicate predicate to check if a throwable matches
+ * @return {@code true} if the given exception or any of its causes matches the given predicate
+ */
+ private static boolean isMatching(Throwable throwable, Predicate<Throwable> predicate) {
+ return Throwables.getCausalChain(throwable).stream().anyMatch(predicate);
}
}
diff --git a/java/com/google/gerrit/server/git/MergedByPushOp.java b/java/com/google/gerrit/server/git/MergedByPushOp.java
index c8b5e3f..9aebebf 100644
--- a/java/com/google/gerrit/server/git/MergedByPushOp.java
+++ b/java/com/google/gerrit/server/git/MergedByPushOp.java
@@ -52,6 +52,7 @@
MergedByPushOp create(
RequestScopePropagator requestScopePropagator,
PatchSet.Id psId,
+ @Assisted RequestId submissionId,
@Assisted("refName") String refName,
@Assisted("mergeResultRevId") String mergeResultRevId);
}
@@ -65,6 +66,7 @@
private final ChangeMerged changeMerged;
private final PatchSet.Id psId;
+ private final RequestId submissionId;
private final String refName;
private final String mergeResultRevId;
@@ -84,6 +86,7 @@
ChangeMerged changeMerged,
@Assisted RequestScopePropagator requestScopePropagator,
@Assisted PatchSet.Id psId,
+ @Assisted RequestId submissionId,
@Assisted("refName") String refName,
@Assisted("mergeResultRevId") String mergeResultRevId) {
this.patchSetInfoFactory = patchSetInfoFactory;
@@ -93,6 +96,7 @@
this.sendEmailExecutor = sendEmailExecutor;
this.changeMerged = changeMerged;
this.requestScopePropagator = requestScopePropagator;
+ this.submissionId = submissionId;
this.psId = psId;
this.refName = refName;
this.mergeResultRevId = mergeResultRevId;
@@ -133,7 +137,6 @@
}
change.setCurrentPatchSet(info);
change.setStatus(Change.Status.MERGED);
- RequestId submissionId = new RequestId(change.getId().toString());
change.setSubmissionId(submissionId.toStringForStorage());
// we cannot reconstruct the submit records for when this change was
// submitted, this is why we must fix the status and other details.
diff --git a/java/com/google/gerrit/server/git/TracingHook.java b/java/com/google/gerrit/server/git/TracingHook.java
index 63d8bc6..56eded0 100644
--- a/java/com/google/gerrit/server/git/TracingHook.java
+++ b/java/com/google/gerrit/server/git/TracingHook.java
@@ -14,8 +14,6 @@
package com.google.gerrit.server.git;
-import static com.google.common.base.Preconditions.checkState;
-
import com.google.gerrit.server.logging.TraceContext;
import java.util.List;
import java.util.Optional;
@@ -59,7 +57,10 @@
* @param serverOptionList list of provided server options
*/
private void maybeStartTrace(List<String> serverOptionList) {
- checkState(traceContext == null, "Trace was already started.");
+ if (traceContext != null) {
+ // Trace was already started
+ return;
+ }
Optional<String> traceOption = parseTraceOption(serverOptionList);
traceContext =
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index 7dd21e1..cec9e4e 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -3281,6 +3281,7 @@
int existingPatchSets = 0;
int newPatchSets = 0;
+ RequestId submissionId = null;
COMMIT:
for (RevCommit c; (c = rw.next()) != null; ) {
rw.parseBody(c);
@@ -3289,13 +3290,20 @@
receivePackRefCache.tipsFromObjectId(c.copy(), RefNames.REFS_CHANGES)) {
PatchSet.Id psId = PatchSet.Id.fromRef(ref.getName());
Optional<ChangeNotes> notes = getChangeNotes(psId.changeId());
+ if (submissionId == null) {
+ submissionId = new RequestId(psId.changeId().toString());
+ }
if (notes.isPresent() && notes.get().getChange().getDest().equals(branch)) {
existingPatchSets++;
bu.addOp(notes.get().getChangeId(), setPrivateOpFactory.create(false, null));
bu.addOp(
psId.changeId(),
mergedByPushOpFactory.create(
- requestScopePropagator, psId, refName, newTip.getId().getName()));
+ requestScopePropagator,
+ psId,
+ submissionId,
+ refName,
+ newTip.getId().getName()));
continue COMMIT;
}
}
@@ -3324,13 +3332,20 @@
logger.atFine().log("Not closing %s because validation failed", id);
continue;
}
+ if (submissionId == null) {
+ submissionId = new RequestId(id.toString());
+ }
req.addOps(bu, null);
bu.addOp(id, setPrivateOpFactory.create(false, null));
bu.addOp(
id,
mergedByPushOpFactory
.create(
- requestScopePropagator, req.psId, refName, newTip.getId().getName())
+ requestScopePropagator,
+ req.psId,
+ submissionId,
+ refName,
+ newTip.getId().getName())
.setPatchSetProvider(req.replaceOp::getPatchSet));
bu.addOp(id, new ChangeProgressOp(progress));
ids.add(id);
diff --git a/java/com/google/gerrit/server/git/receive/ReplaceOp.java b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
index e95cf3b..6c0d5d3 100644
--- a/java/com/google/gerrit/server/git/receive/ReplaceOp.java
+++ b/java/com/google/gerrit/server/git/receive/ReplaceOp.java
@@ -61,6 +61,7 @@
import com.google.gerrit.server.extensions.events.RevisionCreated;
import com.google.gerrit.server.git.MergedByPushOp;
import com.google.gerrit.server.git.receive.ReceiveCommits.MagicBranchInput;
+import com.google.gerrit.server.logging.RequestId;
import com.google.gerrit.server.mail.MailUtil.MailRecipients;
import com.google.gerrit.server.mail.send.ReplacePatchSetSender;
import com.google.gerrit.server.notedb.ChangeNotes;
@@ -236,7 +237,11 @@
if (mergedInto != null) {
mergedByPushOp =
mergedByPushOpFactory.create(
- requestScopePropagator, patchSetId, mergedInto, mergeResultRevId);
+ requestScopePropagator,
+ patchSetId,
+ new RequestId(patchSetId.changeId().toString()),
+ mergedInto,
+ mergeResultRevId);
}
}
diff --git a/java/com/google/gerrit/server/project/ProjectConfig.java b/java/com/google/gerrit/server/project/ProjectConfig.java
index 4d551a2..fa877af 100644
--- a/java/com/google/gerrit/server/project/ProjectConfig.java
+++ b/java/com/google/gerrit/server/project/ProjectConfig.java
@@ -923,9 +923,18 @@
lowerNames.put(lower, name);
List<LabelValue> values = new ArrayList<>();
+ Set<Short> allValues = new HashSet<>();
for (String value : rc.getStringList(LABEL, name, KEY_VALUE)) {
try {
- values.add(parseLabelValue(value));
+ LabelValue labelValue = parseLabelValue(value);
+ if (allValues.add(labelValue.getValue())) {
+ values.add(labelValue);
+ } else {
+ error(
+ new ValidationError(
+ PROJECT_CONFIG,
+ String.format("Duplicate %s \"%s\" for label \"%s\"", KEY_VALUE, value, name)));
+ }
} catch (IllegalArgumentException notValue) {
error(
new ValidationError(
diff --git a/java/com/google/gerrit/server/restapi/change/RelatedChangesSorter.java b/java/com/google/gerrit/server/restapi/change/RelatedChangesSorter.java
index 8040847..af65483 100644
--- a/java/com/google/gerrit/server/restapi/change/RelatedChangesSorter.java
+++ b/java/com/google/gerrit/server/restapi/change/RelatedChangesSorter.java
@@ -16,6 +16,7 @@
import static com.google.common.base.Preconditions.checkArgument;
import static java.util.Objects.requireNonNull;
+import static java.util.stream.Collectors.toMap;
import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
@@ -76,7 +77,15 @@
// Map of all patch sets, keyed by commit SHA-1.
Map<ObjectId, PatchSetData> byId = collectById(in);
PatchSetData start = byId.get(startPs.commitId());
- checkArgument(start != null, "%s not found in %s", startPs, in);
+ requireNonNull(
+ start,
+ () ->
+ String.format(
+ "commit %s of patch set %s not found in %s",
+ startPs.commitId().name(),
+ startPs.id(),
+ byId.entrySet().stream()
+ .collect(toMap(e -> e.getKey().name(), e -> e.getValue().patchSet().id()))));
// Map of patch set -> immediate parent.
ListMultimap<PatchSetData, PatchSetData> parents =
diff --git a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
index 9d2bd03..b44bb29 100644
--- a/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
+++ b/java/com/google/gerrit/server/restapi/change/RevertSubmission.java
@@ -161,10 +161,21 @@
"Revert this change and all changes that have been submitted together with this change")
.setVisible(
and(
- change.isMerged() && change.getSubmissionId() != null && projectStatePermitsWrite,
+ change.isMerged()
+ && change.getSubmissionId() != null
+ && isChangePartOfSubmission(change.getSubmissionId())
+ && projectStatePermitsWrite,
permissionBackend
.user(rsrc.getUser())
.ref(change.getDest())
.testCond(CREATE_CHANGE)));
}
+
+ /**
+ * @param submissionId the submission id of the change.
+ * @return True if the submission has more than one change, false otherwise.
+ */
+ private Boolean isChangePartOfSubmission(String submissionId) {
+ return (queryProvider.get().setLimit(2).bySubmissionId(submissionId).size() > 1);
+ }
}
diff --git a/java/com/google/gerrit/server/restapi/project/LabelDefinitionInputParser.java b/java/com/google/gerrit/server/restapi/project/LabelDefinitionInputParser.java
index a45c67f..1e288f4 100644
--- a/java/com/google/gerrit/server/restapi/project/LabelDefinitionInputParser.java
+++ b/java/com/google/gerrit/server/restapi/project/LabelDefinitionInputParser.java
@@ -24,10 +24,12 @@
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.server.project.RefPattern;
import java.util.ArrayList;
+import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Optional;
+import java.util.Set;
public class LabelDefinitionInputParser {
public static LabelFunction parseFunction(String functionString) throws BadRequestException {
@@ -39,6 +41,7 @@
public static List<LabelValue> parseValues(Map<String, String> values)
throws BadRequestException {
List<LabelValue> valueList = new ArrayList<>();
+ Set<Short> allValues = new HashSet<>();
for (Entry<String, String> e : values.entrySet()) {
short value;
try {
@@ -46,6 +49,9 @@
} catch (NumberFormatException ex) {
throw new BadRequestException("invalid value: " + e.getKey(), ex);
}
+ if (!allValues.add(value)) {
+ throw new BadRequestException("duplicate value: " + value);
+ }
String valueDescription = e.getValue().trim();
if (valueDescription.isEmpty()) {
throw new BadRequestException("description for value '" + e.getKey() + "' cannot be empty");
diff --git a/java/com/google/gerrit/server/update/RetryHelper.java b/java/com/google/gerrit/server/update/RetryHelper.java
index 6f8ef12..4f9a67c 100644
--- a/java/com/google/gerrit/server/update/RetryHelper.java
+++ b/java/com/google/gerrit/server/update/RetryHelper.java
@@ -34,8 +34,6 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.restapi.RestApiException;
-import com.google.gerrit.metrics.Counter1;
-import com.google.gerrit.metrics.Counter2;
import com.google.gerrit.metrics.Counter3;
import com.google.gerrit.metrics.Description;
import com.google.gerrit.metrics.Field;
@@ -128,8 +126,8 @@
@VisibleForTesting
@Singleton
public static class Metrics {
- final Counter2<ActionType, String> attemptCounts;
- final Counter1<ActionType> timeoutCount;
+ final Counter3<ActionType, String, String> attemptCounts;
+ final Counter3<ActionType, String, String> timeoutCount;
final Counter3<ActionType, String, String> autoRetryCount;
final Counter3<ActionType, String, String> failuresOnAutoRetryCount;
@@ -137,6 +135,19 @@
Metrics(MetricMaker metricMaker) {
Field<ActionType> actionTypeField =
Field.ofEnum(ActionType.class, "action_type", Metadata.Builder::actionType).build();
+ Field<String> operationNameField =
+ Field.ofString("operation_name", Metadata.Builder::operationName)
+ .description("The name of the operation that was retried.")
+ .build();
+ Field<String> lastAttemptCauseField =
+ Field.ofString("cause", Metadata.Builder::cause)
+ .description("The cause for the last attempt.")
+ .build();
+ Field<String> causeField =
+ Field.ofString("cause", Metadata.Builder::cause)
+ .description("The cause for the retry.")
+ .build();
+
attemptCounts =
metricMaker.newCounter(
"action/retry_attempt_count",
@@ -146,9 +157,8 @@
.setCumulative()
.setUnit("attempts"),
actionTypeField,
- Field.ofString("cause", Metadata.Builder::cause)
- .description("The cause for the last attempt.")
- .build());
+ operationNameField,
+ lastAttemptCauseField);
timeoutCount =
metricMaker.newCounter(
"action/retry_timeout_count",
@@ -156,7 +166,9 @@
"Number of action executions of RetryHelper that ultimately timed out")
.setCumulative()
.setUnit("timeouts"),
- actionTypeField);
+ actionTypeField,
+ operationNameField,
+ lastAttemptCauseField);
autoRetryCount =
metricMaker.newCounter(
"action/auto_retry_count",
@@ -164,12 +176,8 @@
.setCumulative()
.setUnit("retries"),
actionTypeField,
- Field.ofString("operation_name", Metadata.Builder::operationName)
- .description("The name of the operation that was retried.")
- .build(),
- Field.ofString("cause", Metadata.Builder::cause)
- .description("The cause for the retry.")
- .build());
+ operationNameField,
+ causeField);
failuresOnAutoRetryCount =
metricMaker.newCounter(
"action/failures_on_auto_retry_count",
@@ -177,12 +185,8 @@
.setCumulative()
.setUnit("failures"),
actionTypeField,
- Field.ofString("operation_name", Metadata.Builder::operationName)
- .description("The name of the operation that was retried.")
- .build(),
- Field.ofString("cause", Metadata.Builder::cause)
- .description("The cause for the retry.")
- .build());
+ operationNameField,
+ causeField);
}
}
@@ -355,19 +359,20 @@
return false;
});
retryerBuilder.withRetryListener(listener);
- return executeWithTimeoutCount(actionType, action, retryerBuilder.build());
+ return executeWithTimeoutCount(actionType, action, opts, retryerBuilder.build());
} finally {
if (listener.getAttemptCount() > 1) {
logger.atFine().log("%s was attempted %d times", actionType, listener.getAttemptCount());
metrics.attemptCounts.incrementBy(
actionType,
+ opts.caller().orElse("N/A"),
listener.getCause().map(this::formatCause).orElse("_unknown"),
listener.getAttemptCount() - 1);
}
}
}
- private String formatCause(Throwable t) {
+ public String formatCause(Throwable t) {
while ((t instanceof UpdateException
|| t instanceof StorageException
|| t instanceof ExecutionException)
@@ -396,18 +401,22 @@
*
* @param actionType the type of the action
* @param action the action which should be executed and retried on failure
+ * @param opts options for retrying the action on failure
* @param retryer the retryer
* @return the result of executing the action
* @throws Throwable any error or exception that made the action fail, callers are expected to
* catch and inspect this Throwable to decide carefully whether it should be re-thrown
*/
- private <T> T executeWithTimeoutCount(ActionType actionType, Action<T> action, Retryer<T> retryer)
- throws Throwable {
+ private <T> T executeWithTimeoutCount(
+ ActionType actionType, Action<T> action, Options opts, Retryer<T> retryer) throws Throwable {
try {
return retryer.call(action::call);
} catch (ExecutionException | RetryException e) {
if (e instanceof RetryException) {
- metrics.timeoutCount.increment(actionType);
+ metrics.timeoutCount.increment(
+ actionType,
+ opts.caller().orElse("N/A"),
+ e.getCause() != null ? formatCause(e.getCause()) : "_unknown");
}
if (e.getCause() != null) {
throw e.getCause();
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 7ecb07e..477ec38 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -3573,6 +3573,28 @@
}
@Test
+ public void checkSubmissionIdForAutoClosedChange() throws Exception {
+ PushOneCommit.Result first = createChange();
+ PushOneCommit.Result second = createChange();
+
+ PushOneCommit push = pushFactory.create(admin.newIdent(), testRepo);
+
+ PushOneCommit.Result result = push.to("refs/heads/master");
+ result.assertOkStatus();
+
+ ChangeInfo firstChange = gApi.changes().id(first.getChangeId()).get();
+ assertThat(firstChange.status).isEqualTo(ChangeStatus.MERGED);
+ assertThat(firstChange.submissionId).isNotNull();
+
+ ChangeInfo secondChange = gApi.changes().id(second.getChangeId()).get();
+ assertThat(secondChange.status).isEqualTo(ChangeStatus.MERGED);
+ assertThat(secondChange.submissionId).isNotNull();
+
+ assertThat(secondChange.submissionId).isEqualTo(firstChange.submissionId);
+ assertThat(gApi.changes().id(second.getChangeId()).submittedTogether()).hasSize(2);
+ }
+
+ @Test
public void maxPermittedValueAllowed() throws Exception {
final int minPermittedValue = -2;
final int maxPermittedValue = +2;
diff --git a/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java b/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java
index 78c8209..2801b36 100644
--- a/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java
@@ -69,11 +69,16 @@
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.index.IndexExecutor;
import com.google.gerrit.server.project.CommentLinkInfoImpl;
+import com.google.gerrit.server.project.ProjectConfig;
import com.google.inject.AbstractModule;
import com.google.inject.Inject;
import com.google.inject.Module;
import java.util.HashMap;
import java.util.Map;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.transport.PushResult;
import org.eclipse.jgit.transport.RemoteRefUpdate.Status;
@@ -851,6 +856,58 @@
assertCommentLinks(getConfig(project), expected);
}
+ @Test
+ public void cannotPushLabelDefinitionWithDuplicateValues() throws Exception {
+ Config cfg = new Config();
+ cfg.fromText(projectOperations.project(allProjects).getConfig().toText());
+ cfg.setStringList(
+ "label",
+ "Code-Review",
+ "value",
+ ImmutableList.of("+1 LGTM", "1 LGTM", "0 No Value", "-1 Looks Bad"));
+
+ TestRepository<InMemoryRepository> repo = cloneProject(allProjects);
+ GitUtil.fetch(repo, RefNames.REFS_CONFIG + ":" + RefNames.REFS_CONFIG);
+ repo.reset(RefNames.REFS_CONFIG);
+ PushOneCommit.Result r =
+ pushFactory
+ .create(admin.newIdent(), repo, "Subject", "project.config", cfg.toText())
+ .to(RefNames.REFS_CONFIG);
+ r.assertErrorStatus("invalid project configuration");
+ r.assertMessage("project.config: duplicate value \"1 lgtm\" for label \"code-review\"");
+ }
+
+ @Test
+ public void getProjectThatHasLabelDefinitionWithDuplicateValues() throws Exception {
+ // Update the definition of the Code-Review label so that it has the value "+1 LGTM" twice.
+ // This update bypasses all validation checks so that the duplicate label value doesn't get
+ // rejected.
+ Config cfg = new Config();
+ cfg.fromText(projectOperations.project(allProjects).getConfig().toText());
+ cfg.setStringList(
+ "label",
+ "Code-Review",
+ "value",
+ ImmutableList.of("+1 LGTM", "1 LGTM", "0 No Value", "-1 Looks Bad"));
+
+ try (TestRepository<Repository> repo =
+ new TestRepository<>(repoManager.openRepository(allProjects))) {
+ repo.update(
+ RefNames.REFS_CONFIG,
+ repo.commit()
+ .message("Set label with duplicate value")
+ .parent(getHead(repo.getRepository(), RefNames.REFS_CONFIG))
+ .add(ProjectConfig.PROJECT_CONFIG, cfg.toText()));
+ }
+
+ // Verify that project info can be retrieved and that the label value "+1 LGTM" appears only
+ // once.
+ ProjectInfo projectInfo = gApi.projects().name(allProjects.get()).get();
+ assertThat(projectInfo.labels.keySet()).containsExactly("Code-Review");
+ assertThat(projectInfo.labels.get("Code-Review").values)
+ .containsExactly("+1", "LGTM", " 0", "No Value", "-1", "Looks Bad");
+ }
+
private CommentLinkInfo commentLinkInfo(String name, String match, String link) {
return new CommentLinkInfoImpl(name, match, link, null /*html*/, null /*enabled*/);
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java
index 8b51e7f..911a04d 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/ActionsIT.java
@@ -72,13 +72,28 @@
}
@Test
- public void changeActionOneMergedChangeHasReverts() throws Exception {
+ public void changeActionOneMergedChangeHasOnlyNormalRevert() throws Exception {
String changeId = createChangeWithTopic().getChangeId();
gApi.changes().id(changeId).current().review(ReviewInput.approve());
gApi.changes().id(changeId).current().submit();
Map<String, ActionInfo> actions = getChangeActions(changeId);
assertThat(actions).containsKey("revert");
- assertThat(actions).containsKey("revert_submission");
+ assertThat(actions).doesNotContainKey("revert_submission");
+ }
+
+ @Test
+ public void changeActionTwoMergedChangesHaveReverts() throws Exception {
+ String changeId1 = createChangeWithTopic().getChangeId();
+ String changeId2 = createChangeWithTopic().getChangeId();
+ gApi.changes().id(changeId1).current().review(ReviewInput.approve());
+ gApi.changes().id(changeId2).current().review(ReviewInput.approve());
+ gApi.changes().id(changeId2).current().submit();
+ Map<String, ActionInfo> actions1 = getChangeActions(changeId1);
+ assertThat(actions1).containsKey("revert");
+ assertThat(actions1).containsKey("revert_submission");
+ Map<String, ActionInfo> actions2 = getChangeActions(changeId2);
+ assertThat(actions2).containsKey("revert");
+ assertThat(actions2).containsKey("revert_submission");
}
@Test
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java b/javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java
index 91a10ca..3e9b1f6 100644
--- a/javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java
@@ -604,7 +604,7 @@
}
@Test
- public void syncCreateGroupPermission() throws Exception {
+ public void syncCreateGroupPermission_addAndRemoveCreateGroupCapability() throws Exception {
// Grant CREATE_GROUP to Registered Users
ProjectAccessInput accessInput = newProjectAccessInput();
AccessSectionInfo accessSection = newAccessSectionInfo();
@@ -642,6 +642,44 @@
}
@Test
+ public void syncCreateGroupPermission_addCreateGroupCapabilityToMultipleGroups()
+ throws Exception {
+ PermissionRuleInfo pri = new PermissionRuleInfo(PermissionRuleInfo.Action.ALLOW, false);
+
+ // Grant CREATE_GROUP to Registered Users
+ ProjectAccessInput accessInput = newProjectAccessInput();
+ AccessSectionInfo accessSection = newAccessSectionInfo();
+ PermissionInfo createGroup = newPermissionInfo();
+ createGroup.rules.put(SystemGroupBackend.REGISTERED_USERS.get(), pri);
+ accessSection.permissions.put(GlobalCapability.CREATE_GROUP, createGroup);
+ accessInput.add.put(AccessSection.GLOBAL_CAPABILITIES, accessSection);
+ gApi.projects().name(allProjects.get()).access(accessInput);
+
+ // Grant CREATE_GROUP to Administrators
+ accessInput = newProjectAccessInput();
+ accessSection = newAccessSectionInfo();
+ createGroup = newPermissionInfo();
+ createGroup.rules.put(adminGroupUuid().get(), pri);
+ accessSection.permissions.put(GlobalCapability.CREATE_GROUP, createGroup);
+ accessInput.add.put(AccessSection.GLOBAL_CAPABILITIES, accessSection);
+ gApi.projects().name(allProjects.get()).access(accessInput);
+
+ // Assert that the permissions were synced from All-Projects (global) to All-Users (ref)
+ Map<String, AccessSectionInfo> local = gApi.projects().name("All-Users").access().local;
+ assertThat(local).isNotNull();
+ assertThat(local).containsKey(RefNames.REFS_GROUPS + "*");
+ Map<String, PermissionInfo> permissions = local.get(RefNames.REFS_GROUPS + "*").permissions;
+ assertThat(permissions).hasSize(2);
+ // READ is the default permission and should be preserved by the syncer
+ assertThat(permissions.keySet()).containsExactly(Permission.READ, Permission.CREATE);
+ Map<String, PermissionRuleInfo> rules = permissions.get(Permission.CREATE).rules;
+ assertThat(rules.keySet())
+ .containsExactly(SystemGroupBackend.REGISTERED_USERS.get(), adminGroupUuid().get());
+ assertThat(rules.get(SystemGroupBackend.REGISTERED_USERS.get())).isEqualTo(pri);
+ assertThat(rules.get(adminGroupUuid().get())).isEqualTo(pri);
+ }
+
+ @Test
public void addAccessSectionForInvalidRef() throws Exception {
ProjectAccessInput accessInput = newProjectAccessInput();
AccessSectionInfo accessSectionInfo = createDefaultAccessSectionInfo();
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/CreateLabelIT.java b/javatests/com/google/gerrit/acceptance/rest/project/CreateLabelIT.java
index 28e8b14..57a1e56 100644
--- a/javatests/com/google/gerrit/acceptance/rest/project/CreateLabelIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/project/CreateLabelIT.java
@@ -169,6 +169,21 @@
}
@Test
+ public void cannotCreateLabelWithDuplicateValues() throws Exception {
+ LabelDefinitionInput input = new LabelDefinitionInput();
+ // Positive values can be specified as '<value>' or '+<value>'.
+ input.values =
+ ImmutableMap.of(
+ "+1", "Looks Good", "1", "Looks Good", "0", "Don't Know", "-1", "Looks Bad");
+
+ BadRequestException thrown =
+ assertThrows(
+ BadRequestException.class,
+ () -> gApi.projects().name(allProjects.get()).label("Foo").create(input));
+ assertThat(thrown).hasMessageThat().contains("duplicate value: 1");
+ }
+
+ @Test
public void cannotCreateLabelWithInvalidDefaultValue() throws Exception {
LabelDefinitionInput input = new LabelDefinitionInput();
input.values = ImmutableMap.of("+1", "Looks Good", "0", "Don't Know", "-1", "Looks Bad");
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/SetLabelIT.java b/javatests/com/google/gerrit/acceptance/rest/project/SetLabelIT.java
index 9cba930..97b795f 100644
--- a/javatests/com/google/gerrit/acceptance/rest/project/SetLabelIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/project/SetLabelIT.java
@@ -323,6 +323,21 @@
}
@Test
+ public void cannotSetDuplicateValues() throws Exception {
+ LabelDefinitionInput input = new LabelDefinitionInput();
+ // Positive values can be specified as '<value>' or '+<value>'.
+ input.values =
+ ImmutableMap.of(
+ "+1", "Looks Good", "1", "Looks Good", "0", "Don't Know", "-1", "Looks Bad");
+
+ BadRequestException thrown =
+ assertThrows(
+ BadRequestException.class,
+ () -> gApi.projects().name(allProjects.get()).label("Code-Review").update(input));
+ assertThat(thrown).hasMessageThat().contains("duplicate value: 1");
+ }
+
+ @Test
public void updateDefaultValue() throws Exception {
LabelDefinitionInput input = new LabelDefinitionInput();
input.defaultValue = 1;
diff --git a/plugins/replication b/plugins/replication
index d7c09fb..4689b41 160000
--- a/plugins/replication
+++ b/plugins/replication
@@ -1 +1 @@
-Subproject commit d7c09fbb4c18b1743d6060d361171c2a5237f22b
+Subproject commit 4689b419eab61ea204daf2dfca47296667ac317c
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html
index b7e615c..f1338fb 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html
@@ -92,7 +92,6 @@
a {
color: inherit;
cursor: pointer;
- display: inline-block;
text-decoration: none;
}
a:hover {
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-api-utils.js b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-api-utils.js
index 0ec3d6a..2d66cfa 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-api-utils.js
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-api-utils.js
@@ -50,11 +50,7 @@
return url.pathname;
}
const base = Gerrit.BaseUrlBehavior.getBaseUrl();
- let pathname = url.pathname.replace(base, '');
- // Load from ASSETS_PATH
- if (window.ASSETS_PATH && url.href.includes(window.ASSETS_PATH)) {
- pathname = url.href.replace(window.ASSETS_PATH, '');
- }
+ const pathname = url.pathname.replace(base, '');
// Site theme is server from predefined path.
if (pathname === '/static/gerrit-theme.html') {
return 'gerrit-theme';
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-api-utils_test.html b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-api-utils_test.html
index b43796f..128738d 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-api-utils_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-api-utils_test.html
@@ -72,15 +72,6 @@
'gerrit-theme'
);
});
-
- test('with ASSETS_PATH', () => {
- window.ASSETS_PATH = 'http://cdn.com/2';
- assert.equal(
- getPluginNameFromUrl(`${window.ASSETS_PATH}/plugins/a.html`),
- 'a'
- );
- window.ASSETS_PATH = undefined;
- });
});
});
</script>
\ No newline at end of file
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-loader.js b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-loader.js
index 96d8411..4be38b6 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-loader.js
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-loader.js
@@ -113,7 +113,7 @@
this._pluginListLoaded = true;
plugins.forEach(path => {
- const url = this._urlFor(path, window.ASSETS_PATH);
+ const url = this._urlFor(path);
// Skip if preloaded, for bundling.
if (this.isPluginPreloaded(url)) return;
@@ -128,11 +128,11 @@
});
if (this._isPathEndsWith(url, '.html')) {
- this._importHtmlPlugin(path, opts && opts[path]);
+ this._importHtmlPlugin(url, opts && opts[path]);
} else if (this._isPathEndsWith(url, '.js')) {
- this._loadJsPlugin(path);
+ this._loadJsPlugin(url);
} else {
- this._failToLoad(`Unrecognized plugin path ${path}`, path);
+ this._failToLoad(`Unrecognized plugin url ${url}`, url);
}
});
@@ -181,15 +181,14 @@
return;
}
- const url = this._urlFor(src);
- const pluginObject = this.getPlugin(url);
+ const pluginObject = this.getPlugin(src);
let plugin = pluginObject && pluginObject.plugin;
if (!plugin) {
- plugin = new Plugin(url);
+ plugin = new Plugin(src);
}
try {
callback(plugin);
- this._pluginInstalled(url, plugin);
+ this._pluginInstalled(src, plugin);
} catch (e) {
this._failToLoad(`${e.name}: ${e.message}`, src);
}
@@ -314,79 +313,38 @@
}
_importHtmlPlugin(pluginUrl, opts = {}) {
- const urlWithAP = this._urlFor(pluginUrl, window.ASSETS_PATH);
- const urlWithoutAP = this._urlFor(pluginUrl);
- let onerror = null;
- if (urlWithAP !== urlWithoutAP) {
- onerror = () => this._loadHtmlPlugin(urlWithoutAP, opts.sync);
- }
- this._loadHtmlPlugin(urlWithAP, opts.sync, onerror);
- }
-
- _loadHtmlPlugin(url, sync, onerror) {
- if (!onerror) {
- onerror = () => {
- this._failToLoad(`${pluginUrl} import error`, pluginUrl);
- };
- }
-
+ // onload (second param) needs to be a function. When null or undefined
+ // were passed, plugins were not loaded correctly.
(Polymer.importHref || Polymer.Base.importHref)(
- url, () => {},
- onerror,
- !sync);
+ this._urlFor(pluginUrl), () => {},
+ () => this._failToLoad(`${pluginUrl} import error`, pluginUrl),
+ !opts.sync);
}
_loadJsPlugin(pluginUrl) {
- const urlWithAP = this._urlFor(pluginUrl, window.ASSETS_PATH);
- const urlWithoutAP = this._urlFor(pluginUrl);
- let onerror = null;
- if (urlWithAP !== urlWithoutAP) {
- onerror = () => this._createScriptTag(urlWithoutAP);
- }
-
- this._createScriptTag(urlWithAP, onerror);
+ this._createScriptTag(this._urlFor(pluginUrl));
}
- _createScriptTag(url, onerror) {
- if (!onerror) {
- onerror = () => this._failToLoad(`${url} load error`, url);
- }
-
+ _createScriptTag(url) {
const el = document.createElement('script');
el.defer = true;
el.setAttribute('src', url);
- el.onerror = onerror;
+ el.onerror = () => this._failToLoad(`${url} load error`, url);
return document.body.appendChild(el);
}
- _urlFor(pathOrUrl, assetsPath) {
+ _urlFor(pathOrUrl) {
if (!pathOrUrl) {
return pathOrUrl;
}
-
- // theme is per host, should always load from assetsPath
- const isThemeFile = pathOrUrl.endsWith('static/gerrit-theme.html');
- const shouldTryLoadFromAssetsPathFirst = !isThemeFile && assetsPath;
if (pathOrUrl.startsWith(PRELOADED_PROTOCOL) ||
pathOrUrl.startsWith('http')) {
// Plugins are loaded from another domain or preloaded.
- if (pathOrUrl.includes(location.host)
- && shouldTryLoadFromAssetsPathFirst) {
- // if is loading from host server, try replace with cdn when assetsPath provided
- return pathOrUrl
- .replace(location.origin, assetsPath);
- }
return pathOrUrl;
}
-
if (!pathOrUrl.startsWith('/')) {
pathOrUrl = '/' + pathOrUrl;
}
-
- if (shouldTryLoadFromAssetsPathFirst) {
- return assetsPath + pathOrUrl;
- }
-
return window.location.origin + getBaseUrl() + pathOrUrl;
}
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-loader_test.html b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-loader_test.html
index 08e7e18..8c1ec96 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-loader_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-loader_test.html
@@ -325,11 +325,11 @@
let loadJsPluginStub;
setup(() => {
importHtmlPluginStub = sandbox.stub();
- sandbox.stub(Gerrit._pluginLoader, '_loadHtmlPlugin', url => {
+ sandbox.stub(Gerrit._pluginLoader, '_importHtmlPlugin', url => {
importHtmlPluginStub(url);
});
loadJsPluginStub = sandbox.stub();
- sandbox.stub(Gerrit._pluginLoader, '_createScriptTag', url => {
+ sandbox.stub(Gerrit._pluginLoader, '_loadJsPlugin', url => {
loadJsPluginStub(url);
});
});
@@ -346,8 +346,8 @@
assert.isTrue(failToLoadStub.calledOnce);
assert.isTrue(failToLoadStub.calledWithExactly(
- 'Unrecognized plugin path foo/bar',
- 'foo/bar'
+ `Unrecognized plugin url ${url}/foo/bar`,
+ `${url}/foo/bar`
));
});
@@ -407,72 +407,6 @@
});
});
- suite('With ASSETS_PATH', () => {
- let importHtmlPluginStub;
- let loadJsPluginStub;
- setup(() => {
- window.ASSETS_PATH = 'https://cdn.com';
- importHtmlPluginStub = sandbox.stub();
- sandbox.stub(Gerrit._pluginLoader, '_loadHtmlPlugin', url => {
- importHtmlPluginStub(url);
- });
- loadJsPluginStub = sandbox.stub();
- sandbox.stub(Gerrit._pluginLoader, '_createScriptTag', url => {
- loadJsPluginStub(url);
- });
- });
-
- teardown(() => {
- window.ASSETS_PATH = '';
- });
-
- test('Should try load plugins from assets path instead', () => {
- Gerrit._loadPlugins([
- 'foo/bar.js',
- 'foo/bar.html',
- ]);
-
- assert.isTrue(importHtmlPluginStub.calledOnce);
- assert.isTrue(
- importHtmlPluginStub.calledWithExactly(`https://cdn.com/foo/bar.html`)
- );
- assert.isTrue(loadJsPluginStub.calledOnce);
- assert.isTrue(
- loadJsPluginStub.calledWithExactly(`https://cdn.com/foo/bar.js`));
- });
-
- test('Should honor original path if exists', () => {
- Gerrit._loadPlugins([
- 'http://e.com/foo/bar.html',
- 'http://e.com/foo/bar.js',
- ]);
-
- assert.isTrue(importHtmlPluginStub.calledOnce);
- assert.isTrue(
- importHtmlPluginStub.calledWithExactly(`http://e.com/foo/bar.html`)
- );
- assert.isTrue(loadJsPluginStub.calledOnce);
- assert.isTrue(
- loadJsPluginStub.calledWithExactly(`http://e.com/foo/bar.js`));
- });
-
- test('Should try replace current host with assetsPath', () => {
- const host = window.location.origin;
- Gerrit._loadPlugins([
- `${host}/foo/bar.html`,
- `${host}/foo/bar.js`,
- ]);
-
- assert.isTrue(importHtmlPluginStub.calledOnce);
- assert.isTrue(
- importHtmlPluginStub.calledWithExactly(`https://cdn.com/foo/bar.html`)
- );
- assert.isTrue(loadJsPluginStub.calledOnce);
- assert.isTrue(
- loadJsPluginStub.calledWithExactly(`https://cdn.com/foo/bar.js`));
- });
- });
-
test('adds js plugins will call the body', () => {
Gerrit._loadPlugins([
'http://e.com/foo/bar.js',
@@ -555,10 +489,12 @@
test('installing preloaded plugin', () => {
let plugin;
+ window.ASSETS_PATH = 'http://blips.com/chitz';
Gerrit.install(p => { plugin = p; }, '0.1', 'preloaded:foo');
assert.strictEqual(plugin.getPluginName(), 'foo');
assert.strictEqual(plugin.url('/some/thing.html'),
- 'preloaded:foo/plugins/foo/some/thing.html');
+ 'http://blips.com/chitz/plugins/foo/some/thing.html');
+ delete window.ASSETS_PATH;
});
});
});
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-public-js-api.js b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-public-js-api.js
index 0aaeaa1..6c306d9 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-public-js-api.js
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-public-js-api.js
@@ -17,6 +17,8 @@
(function(window) {
'use strict';
+ const PRELOADED_PROTOCOL = 'preloaded:';
+
const PANEL_ENDPOINTS_MAPPING = {
CHANGE_SCREEN_BELOW_COMMIT_INFO_BLOCK: 'change-view-integration',
CHANGE_SCREEN_BELOW_CHANGE_INFO_BLOCK: 'change-metadata-item',
@@ -64,6 +66,13 @@
this._url = new URL(opt_url);
this._name = getPluginNameFromUrl(this._url);
+ if (this._url.protocol === PRELOADED_PROTOCOL) {
+ // Original plugin URL is used in plugin assets URLs calculation.
+ const assetsBaseUrl = window.ASSETS_PATH ||
+ (window.location.origin + Gerrit.BaseUrlBehavior.getBaseUrl());
+ this._url = new URL(assetsBaseUrl + '/plugins/' + this._name +
+ '/static/' + this._name + '.js');
+ }
}
Plugin._sharedAPIElement = document.createElement('gr-js-api-interface');
