Merge "Bump Bazel version to 0.27.0 rc5"
diff --git a/java/com/google/gerrit/httpd/raw/IndexHtmlUtil.java b/java/com/google/gerrit/httpd/raw/IndexHtmlUtil.java
index 905c013..d7e9e44 100644
--- a/java/com/google/gerrit/httpd/raw/IndexHtmlUtil.java
+++ b/java/com/google/gerrit/httpd/raw/IndexHtmlUtil.java
@@ -49,10 +49,13 @@
String canonicalURL,
String cdnPath,
String faviconPath,
+ Map<String, String[]> urlParameterMap,
Function<String, SanitizedContent> urlInScriptTagOrdainer)
throws URISyntaxException, RestApiException {
return ImmutableMap.<String, Object>builder()
- .putAll(staticTemplateData(canonicalURL, cdnPath, faviconPath, urlInScriptTagOrdainer))
+ .putAll(
+ staticTemplateData(
+ canonicalURL, cdnPath, faviconPath, urlParameterMap, urlInScriptTagOrdainer))
.putAll(dynamicTemplateData(gerritApi))
.build();
}
@@ -94,6 +97,7 @@
String canonicalURL,
String cdnPath,
String faviconPath,
+ Map<String, String[]> urlParameterMap,
Function<String, SanitizedContent> urlInScriptTagOrdainer)
throws URISyntaxException {
String canonicalPath = computeCanonicalPath(canonicalURL);
@@ -116,6 +120,9 @@
if (faviconPath != null) {
data.put("faviconPath", faviconPath);
}
+ if (urlParameterMap.containsKey("p2")) {
+ data.put("polymer2", "true");
+ }
return data.build();
}
diff --git a/java/com/google/gerrit/httpd/raw/IndexServlet.java b/java/com/google/gerrit/httpd/raw/IndexServlet.java
index 8299bfc..978f3eb 100644
--- a/java/com/google/gerrit/httpd/raw/IndexServlet.java
+++ b/java/com/google/gerrit/httpd/raw/IndexServlet.java
@@ -17,6 +17,7 @@
import static java.nio.charset.StandardCharsets.UTF_8;
import static javax.servlet.http.HttpServletResponse.SC_OK;
+import com.google.common.collect.ImmutableMap;
import com.google.common.io.Resources;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.api.GerritApi;
@@ -28,6 +29,7 @@
import java.io.IOException;
import java.io.OutputStream;
import java.net.URISyntaxException;
+import java.util.Map;
import java.util.function.Function;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
@@ -67,14 +69,16 @@
protected void doGet(HttpServletRequest req, HttpServletResponse rsp) throws IOException {
SoyTofu.Renderer renderer;
try {
+ Map<String, String[]> parameterMap = req.getParameterMap();
// TODO(hiesel): Remove URL ordainer as parameter once Soy is consistent
+ ImmutableMap<String, Object> templateData =
+ IndexHtmlUtil.templateData(
+ gerritApi, canonicalUrl, cdnPath, faviconPath, parameterMap, urlOrdainer);
renderer =
soyTofu
.newRenderer("com.google.gerrit.httpd.raw.Index")
.setContentKind(SanitizedContent.ContentKind.HTML)
- .setData(
- IndexHtmlUtil.templateData(
- gerritApi, canonicalUrl, cdnPath, faviconPath, urlOrdainer));
+ .setData(templateData);
} catch (URISyntaxException | RestApiException e) {
throw new IOException(e);
}
diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
index efdf5ba..45e4749 100644
--- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
+++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java
@@ -310,7 +310,10 @@
logger.atFinest().log(
"Received REST request: %s %s (parameters: %s)",
req.getMethod(), req.getRequestURI(), getParameterNames(req));
- logger.atFinest().log("Calling user: %s", globals.currentUser.get().getLoggableName());
+ logger.atFinest().log(
+ "Calling user: %s (groups = %s)",
+ globals.currentUser.get().getLoggableName(),
+ globals.currentUser.get().getEffectiveGroups().getKnownGroups());
if (isCorsPreflight(req)) {
doCorsPreflight(req, res);
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index 98ed97a..f69e364 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -545,7 +545,9 @@
// Process as many commands as possible, but may leave some commands in state NOT_ATTEMPTED.
private void processCommandsUnsafe(
Collection<ReceiveCommand> commands, MultiProgressMonitor progress) {
- logger.atFinest().log("Calling user: %s", user.getLoggableName());
+ logger.atFine().log(
+ "Calling user: %s (groups = %s)",
+ user.getLoggableName(), user.getEffectiveGroups().getKnownGroups());
if (!projectState.getProject().getState().permitsWrite()) {
for (ReceiveCommand cmd : commands) {
diff --git a/java/com/google/gerrit/server/permissions/DefaultRefFilter.java b/java/com/google/gerrit/server/permissions/DefaultRefFilter.java
index 6f9f75a..5c17be8 100644
--- a/java/com/google/gerrit/server/permissions/DefaultRefFilter.java
+++ b/java/com/google/gerrit/server/permissions/DefaultRefFilter.java
@@ -49,6 +49,8 @@
import com.google.gerrit.server.git.TagCache;
import com.google.gerrit.server.git.TagMatcher;
import com.google.gerrit.server.group.InternalGroup;
+import com.google.gerrit.server.logging.TraceContext;
+import com.google.gerrit.server.logging.TraceContext.TraceTimer;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeNotes.Factory.ChangeNotesResult;
import com.google.gerrit.server.permissions.PermissionBackend.RefFilterOptions;
@@ -129,52 +131,82 @@
/** Filters given refs and tags by visibility. */
Map<String, Ref> filter(Map<String, Ref> refs, Repository repo, RefFilterOptions opts)
throws PermissionBackendException {
+ logger.atFinest().log(
+ "Filter refs for repository %s by visibility (options = %s, refs = %s)",
+ projectState.getNameKey(), opts, refs);
+ logger.atFinest().log(
+ "Calling user: %s (groups = %s)",
+ user.getLoggableName(), user.getEffectiveGroups().getKnownGroups());
+ logger.atFinest().log(
+ "auth.skipFullRefEvaluationIfAllRefsAreVisible = %s",
+ skipFullRefEvaluationIfAllRefsAreVisible);
+ logger.atFinest().log(
+ "Project state %s permits read = %s",
+ projectState.getProject().getState(), projectState.statePermitsRead());
+
// See if we can get away with a single, cheap ref evaluation.
if (refs.size() == 1) {
String refName = Iterables.getOnlyElement(refs.values()).getName();
if (opts.filterMeta() && isMetadata(refName)) {
+ logger.atFinest().log("Filter out metadata ref %s", refName);
return ImmutableMap.of();
}
if (RefNames.isRefsChanges(refName)) {
- return canSeeSingleChangeRef(refName) ? refs : ImmutableMap.of();
+ boolean isChangeRefVisisble = canSeeSingleChangeRef(refName);
+ if (isChangeRefVisisble) {
+ logger.atFinest().log("Change ref %s is visible", refName);
+ return refs;
+ }
+ logger.atFinest().log("Filter out non-visible change ref %s", refName);
+ return ImmutableMap.of();
}
}
// Perform an initial ref filtering with all the refs the caller asked for. If we find tags that
- // we have to investigate (deferred tags) separately then perform a reachability check starting
+ // we have to investigate separately (deferred tags) then perform a reachability check starting
// from all visible branches (refs/heads/*).
Result initialRefFilter = filterRefs(refs, repo, opts);
Map<String, Ref> visibleRefs = initialRefFilter.visibleRefs();
if (!initialRefFilter.deferredTags().isEmpty()) {
- Result allVisibleBranches = filterRefs(getTaggableRefsMap(repo), repo, opts);
- checkState(
- allVisibleBranches.deferredTags().isEmpty(),
- "unexpected tags found when filtering refs/heads/* " + allVisibleBranches.deferredTags());
+ try (TraceTimer traceTimer = TraceContext.newTimer("Check visibility of deferred tags")) {
+ Result allVisibleBranches = filterRefs(getTaggableRefsMap(repo), repo, opts);
+ checkState(
+ allVisibleBranches.deferredTags().isEmpty(),
+ "unexpected tags found when filtering refs/heads/* "
+ + allVisibleBranches.deferredTags());
- TagMatcher tags =
- tagCache
- .get(projectState.getNameKey())
- .matcher(tagCache, repo, allVisibleBranches.visibleRefs().values());
- for (Ref tag : initialRefFilter.deferredTags()) {
- try {
- if (tags.isReachable(tag)) {
- visibleRefs.put(tag.getName(), tag);
+ TagMatcher tags =
+ tagCache
+ .get(projectState.getNameKey())
+ .matcher(tagCache, repo, allVisibleBranches.visibleRefs().values());
+ for (Ref tag : initialRefFilter.deferredTags()) {
+ try {
+ if (tags.isReachable(tag)) {
+ logger.atFinest().log("Include reachable tag %s", tag.getName());
+ visibleRefs.put(tag.getName(), tag);
+ } else {
+ logger.atFinest().log("Filter out non-reachable tag %s", tag.getName());
+ }
+ } catch (IOException e) {
+ throw new PermissionBackendException(e);
}
- } catch (IOException e) {
- throw new PermissionBackendException(e);
}
}
}
+
+ logger.atFinest().log("visible refs = %s", visibleRefs);
return visibleRefs;
}
/**
* Filters refs by visibility. Returns tags where visibility can't be trivially computed
- * separately for later ref-walk-based visibility computation. Tags where visibility is trivial to
+ * separately for later rev-walk-based visibility computation. Tags where visibility is trivial to
* compute will be returned as part of {@link Result#visibleRefs()}.
*/
Result filterRefs(Map<String, Ref> refs, Repository repo, RefFilterOptions opts)
throws PermissionBackendException {
+ logger.atFinest().log("Filter refs (refs = %s)", refs);
+
if (projectState.isAllUsers()) {
refs = addUsersSelfSymref(refs);
}
@@ -182,16 +214,22 @@
// TODO(hiesel): Remove when optimization is done.
boolean hasReadOnRefsStar =
checkProjectPermission(permissionBackendForProject, ProjectPermission.READ);
+ logger.atFinest().log("User has READ on refs/* = %s", hasReadOnRefsStar);
if (skipFullRefEvaluationIfAllRefsAreVisible && !projectState.isAllUsers()) {
if (projectState.statePermitsRead() && hasReadOnRefsStar) {
skipFilterCount.increment();
+ logger.atFinest().log(
+ "Fast path, all refs are visible because user has READ on refs/*: %s", refs);
return new AutoValue_DefaultRefFilter_Result(refs, ImmutableList.of());
} else if (projectControl.allRefsAreVisible(ImmutableSet.of(RefNames.REFS_CONFIG))) {
skipFilterCount.increment();
- return new AutoValue_DefaultRefFilter_Result(
- fastHideRefsMetaConfig(refs), ImmutableList.of());
+ refs = fastHideRefsMetaConfig(refs);
+ logger.atFinest().log(
+ "Fast path, all refs except %s are visible: %s", RefNames.REFS_CONFIG, refs);
+ return new AutoValue_DefaultRefFilter_Result(refs, ImmutableList.of());
}
}
+ logger.atFinest().log("Doing full ref filtering");
fullFilterCount.increment();
boolean viewMetadata;
@@ -204,36 +242,53 @@
isAdmin = withUser.testOrFalse(GlobalPermission.ADMINISTRATE_SERVER);
identifiedUser = user.asIdentifiedUser();
userId = identifiedUser.getAccountId();
+ logger.atFinest().log(
+ "Account = %d; can view metadata = %s; is admin = %s",
+ userId.get(), viewMetadata, isAdmin);
} else {
+ logger.atFinest().log("User is anonymous");
viewMetadata = false;
isAdmin = false;
userId = null;
identifiedUser = null;
}
- Map<String, Ref> result = new HashMap<>();
+ Map<String, Ref> resultRefs = new HashMap<>();
List<Ref> deferredTags = new ArrayList<>();
for (Ref ref : refs.values()) {
String name = ref.getName();
Change.Id changeId;
Account.Id accountId;
AccountGroup.UUID accountGroupUuid;
- if (name.startsWith(REFS_CACHE_AUTOMERGE) || (opts.filterMeta() && isMetadata(name))) {
+ if (name.startsWith(REFS_CACHE_AUTOMERGE)) {
+ logger.atFinest().log("Filter out ref %s", name);
+ continue;
+ } else if (opts.filterMeta() && isMetadata(name)) {
+ logger.atFinest().log("Filter out metadata ref %s", name);
continue;
} else if (RefNames.isRefsEdit(name)) {
// Edits are visible only to the owning user, if change is visible.
if (viewMetadata || visibleEdit(repo, name)) {
- result.put(name, ref);
+ logger.atFinest().log("Include edit ref %s", name);
+ resultRefs.put(name, ref);
+ } else {
+ logger.atFinest().log("Filter out edit ref %s", name);
}
} else if ((changeId = Change.Id.fromRef(name)) != null) {
// Change ref is visible only if the change is visible.
if (viewMetadata || visible(repo, changeId)) {
- result.put(name, ref);
+ logger.atFinest().log("Include change ref %s", name);
+ resultRefs.put(name, ref);
+ } else {
+ logger.atFinest().log("Filter out change ref %s", name);
}
} else if ((accountId = Account.Id.fromRef(name)) != null) {
// Account ref is visible only to the corresponding account.
if (viewMetadata || (accountId.equals(userId) && canReadRef(name))) {
- result.put(name, ref);
+ logger.atFinest().log("Include user ref %s", name);
+ resultRefs.put(name, ref);
+ } else {
+ logger.atFinest().log("Filter out user ref %s", name);
}
} else if ((accountGroupUuid = AccountGroup.UUID.fromRef(name)) != null) {
// Group ref is visible only to the corresponding owner group.
@@ -242,7 +297,10 @@
|| (group != null
&& isGroupOwner(group, identifiedUser, isAdmin)
&& canReadRef(name))) {
- result.put(name, ref);
+ logger.atFinest().log("Include group ref %s", name);
+ resultRefs.put(name, ref);
+ } else {
+ logger.atFinest().log("Filter out group ref %s", name);
}
} else if (isTag(ref)) {
if (hasReadOnRefsStar) {
@@ -255,39 +313,56 @@
// (e.g. a private change ref) if a tag was attached to it. Tags are meant to be used on
// the regular Git tree that users interact with, not on any of the Gerrit trees, so this
// is a negligible risk.
- result.put(name, ref);
+ logger.atFinest().log("Include tag ref %s because user has read on refs/*", name);
+ resultRefs.put(name, ref);
} else {
// If its a tag, consider it later.
if (ref.getObjectId() != null) {
+ logger.atFinest().log("Defer tag ref %s", name);
deferredTags.add(ref);
+ } else {
+ logger.atFinest().log("Filter out tag ref %s that is not a tag", name);
}
}
} else if (name.startsWith(RefNames.REFS_SEQUENCES)) {
// Sequences are internal database implementation details.
if (viewMetadata) {
- result.put(name, ref);
+ logger.atFinest().log("Include sequence ref %s", name);
+ resultRefs.put(name, ref);
+ } else {
+ logger.atFinest().log("Filter out sequence ref %s", name);
}
} else if (projectState.isAllUsers()
&& (name.equals(RefNames.REFS_EXTERNAL_IDS) || name.equals(RefNames.REFS_GROUPNAMES))) {
// The notes branches with the external IDs / group names must not be exposed to normal
// users.
if (viewMetadata) {
- result.put(name, ref);
+ logger.atFinest().log("Include external IDs branch %s", name);
+ resultRefs.put(name, ref);
+ } else {
+ logger.atFinest().log("Filter out external IDs branch %s", name);
}
} else if (canReadRef(ref.getLeaf().getName())) {
// Use the leaf to lookup the control data. If the reference is
// symbolic we want the control around the final target. If its
// not symbolic then getLeaf() is a no-op returning ref itself.
- result.put(name, ref);
+ logger.atFinest().log(
+ "Include ref %s because its leaf %s is readable", name, ref.getLeaf().getName());
+ resultRefs.put(name, ref);
} else if (isRefsUsersSelf(ref)) {
// viewMetadata allows to see all account refs, hence refs/users/self should be included as
// well
if (viewMetadata) {
- result.put(name, ref);
+ logger.atFinest().log("Include ref %s", REFS_USERS_SELF);
+ resultRefs.put(name, ref);
}
+ } else {
+ logger.atFinest().log("Filter out ref %s", name);
}
}
- return new AutoValue_DefaultRefFilter_Result(result, deferredTags);
+ Result result = new AutoValue_DefaultRefFilter_Result(resultRefs, deferredTags);
+ logger.atFinest().log("Result of ref filtering = %s", result);
+ return result;
}
/**
@@ -324,12 +399,17 @@
private Map<String, Ref> addUsersSelfSymref(Map<String, Ref> refs) {
if (user.isIdentifiedUser()) {
- Ref r = refs.get(RefNames.refsUsers(user.getAccountId()));
- if (r != null) {
- SymbolicRef s = new SymbolicRef(REFS_USERS_SELF, r);
- refs = new HashMap<>(refs);
- refs.put(s.getName(), s);
+ String refName = RefNames.refsUsers(user.getAccountId());
+ Ref r = refs.get(refName);
+ if (r == null) {
+ logger.atWarning().log("User ref %s not found", refName);
+ return refs;
}
+
+ SymbolicRef s = new SymbolicRef(REFS_USERS_SELF, r);
+ refs = new HashMap<>(refs);
+ refs.put(s.getName(), s);
+ logger.atFinest().log("Added %s as alias for user ref %s", REFS_USERS_SELF, refName);
}
return refs;
}
@@ -341,36 +421,44 @@
} else {
visibleChanges = visibleChangesBySearch();
}
+ logger.atFinest().log("Visible changes: %s", visibleChanges.keySet());
}
return visibleChanges.containsKey(changeId);
}
private boolean visibleEdit(Repository repo, String name) throws PermissionBackendException {
Change.Id id = Change.Id.fromEditRefPart(name);
- // Initialize if it wasn't yet
- if (visibleChanges == null) {
- visible(repo, id);
- }
if (id == null) {
+ logger.atWarning().log("Couldn't extract change ID from edit ref %s", name);
return false;
}
if (user.isIdentifiedUser()
&& name.startsWith(RefNames.refsEditPrefix(user.asIdentifiedUser().getAccountId()))
&& visible(repo, id)) {
+ logger.atFinest().log("Own change edit ref is visible: %s", name);
return true;
}
+
+ // Initialize visibleChanges if it wasn't initialized yet.
+ if (visibleChanges == null) {
+ visible(repo, id);
+ }
if (visibleChanges.containsKey(id)) {
try {
// Default to READ_PRIVATE_CHANGES as there is no special permission for reading edits.
permissionBackendForProject
.ref(visibleChanges.get(id).branch())
.check(RefPermission.READ_PRIVATE_CHANGES);
+ logger.atFinest().log("Foreign change edit ref is visible: %s", name);
return true;
} catch (AuthException e) {
+ logger.atFinest().log("Foreign change edit ref is not visible: %s", name);
return false;
}
}
+
+ logger.atFinest().log("Change %d of change edit ref %s is not visible", id.get(), name);
return false;
}
@@ -442,7 +530,9 @@
}
private boolean isMetadata(String name) {
- return RefNames.isRefsChanges(name) || RefNames.isRefsEdit(name);
+ boolean isMetaData = RefNames.isRefsChanges(name) || RefNames.isRefsEdit(name);
+ logger.atFinest().log("ref %s is " + (isMetaData ? "" : "not ") + "a metadata ref", name);
+ return isMetaData;
}
private static boolean isTag(Ref ref) {
@@ -478,8 +568,10 @@
requireNonNull(group);
// Keep this logic in sync with GroupControl#isOwner().
- return isAdmin
- || (user != null && user.getEffectiveGroups().contains(group.getOwnerGroupUUID()));
+ boolean isGroupOwner =
+ isAdmin || (user != null && user.getEffectiveGroups().contains(group.getOwnerGroupUUID()));
+ logger.atFinest().log("User is owner of group %s = %s", group.getGroupUUID(), isGroupOwner);
+ return isGroupOwner;
}
/**
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index 18d668a..ef38b05 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -136,6 +136,7 @@
import java.util.Optional;
import java.util.OptionalInt;
import java.util.Set;
+import java.util.stream.Collectors;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
@@ -145,7 +146,7 @@
extends RetryingRestModifyView<RevisionResource, ReviewInput, Response<ReviewResult>> {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
- public static final String ERROR_ADDING_REVIEWER = "error adding reviewer";
+ private static final String ERROR_ADDING_REVIEWER = "error adding reviewer";
public static final String ERROR_WIP_READY_MUTUALLY_EXCLUSIVE =
"work_in_progress and ready are mutually exclusive";
@@ -290,7 +291,7 @@
Account.Id id = revision.getUser().getAccountId();
boolean ccOrReviewer = false;
if (input.labels != null && !input.labels.isEmpty()) {
- ccOrReviewer = input.labels.values().stream().filter(v -> v != 0).findFirst().isPresent();
+ ccOrReviewer = input.labels.values().stream().anyMatch(v -> v != 0);
}
if (!ccOrReviewer) {
@@ -792,7 +793,10 @@
}
}
- /** Used to compare Comments with CommentInput comments. */
+ /**
+ * Used to compare existing {@link Comment}-s with {@link CommentInput} comments by copying only
+ * the fields to compare.
+ */
@AutoValue
abstract static class CommentSetEntry {
private static CommentSetEntry create(
@@ -858,8 +862,7 @@
user = ctx.getIdentifiedUser();
notes = ctx.getNotes();
ps = psUtil.get(ctx.getNotes(), psId);
- boolean dirty = false;
- dirty |= insertComments(ctx);
+ boolean dirty = insertComments(ctx);
dirty |= insertRobotComments(ctx);
dirty |= updateLabels(projectState, ctx);
dirty |= insertMessage(ctx);
@@ -889,13 +892,17 @@
private boolean insertComments(ChangeContext ctx)
throws UnprocessableEntityException, PatchListNotAvailableException {
- Map<String, List<CommentInput>> map = in.comments;
- if (map == null) {
- map = Collections.emptyMap();
+ Map<String, List<CommentInput>> inputComments = in.comments;
+ if (inputComments == null) {
+ inputComments = Collections.emptyMap();
}
- Map<String, Comment> drafts = Collections.emptyMap();
- if (!map.isEmpty() || in.drafts != DraftHandling.KEEP) {
+ // HashMap instead of Collections.emptyMap() avoids warning about remove() on immutable
+ // object.
+ Map<String, Comment> drafts = new HashMap<>();
+ // If there are inputComments we need the deduplication loop below, so we have to read (and
+ // publish) drafts here.
+ if (!inputComments.isEmpty() || in.drafts != DraftHandling.KEEP) {
if (in.drafts == DraftHandling.PUBLISH_ALL_REVISIONS) {
drafts = changeDrafts(ctx);
} else {
@@ -905,30 +912,42 @@
List<Comment> toPublish = new ArrayList<>();
- Set<CommentSetEntry> existingIds =
+ Set<CommentSetEntry> existingComments =
in.omitDuplicateComments ? readExistingComments(ctx) : Collections.emptySet();
- for (Map.Entry<String, List<CommentInput>> ent : map.entrySet()) {
- String path = ent.getKey();
- for (CommentInput c : ent.getValue()) {
- String parent = Url.decode(c.inReplyTo);
- Comment e = drafts.remove(Url.decode(c.id));
- if (e == null) {
- e = commentsUtil.newComment(ctx, path, psId, c.side(), c.message, c.unresolved, parent);
+ // Deduplication:
+ // - Ignore drafts with the same ID as an inputComment here. These are deleted later.
+ // - Swallow comments that already exist.
+ for (Map.Entry<String, List<CommentInput>> entry : inputComments.entrySet()) {
+ String path = entry.getKey();
+ for (CommentInput inputComment : entry.getValue()) {
+ Comment comment = drafts.remove(Url.decode(inputComment.id));
+ if (comment == null) {
+ String parent = Url.decode(inputComment.inReplyTo);
+ comment =
+ commentsUtil.newComment(
+ ctx,
+ path,
+ psId,
+ inputComment.side(),
+ inputComment.message,
+ inputComment.unresolved,
+ parent);
} else {
- e.writtenOn = ctx.getWhen();
- e.side = c.side();
- e.message = c.message;
+ // In ChangeUpdate#putComment() the draft with the same ID will be deleted.
+ comment.writtenOn = ctx.getWhen();
+ comment.side = inputComment.side();
+ comment.message = inputComment.message;
}
- setCommentCommitId(e, patchListCache, ctx.getChange(), ps);
- e.setLineNbrAndRange(c.line, c.range);
- e.tag = in.tag;
+ setCommentCommitId(comment, patchListCache, ctx.getChange(), ps);
+ comment.setLineNbrAndRange(inputComment.line, inputComment.range);
+ comment.tag = in.tag;
- if (existingIds.contains(CommentSetEntry.create(e))) {
+ if (existingComments.contains(CommentSetEntry.create(comment))) {
continue;
}
- toPublish.add(e);
+ toPublish.add(comment);
}
}
@@ -942,8 +961,8 @@
default:
break;
}
- ChangeUpdate u = ctx.getUpdate(psId);
- commentsUtil.putComments(u, Status.PUBLISHED, toPublish);
+ ChangeUpdate changeUpdate = ctx.getUpdate(psId);
+ commentsUtil.putComments(changeUpdate, Status.PUBLISHED, toPublish);
comments.addAll(toPublish);
return !toPublish.isEmpty();
}
@@ -1042,21 +1061,19 @@
}
private Map<String, Comment> changeDrafts(ChangeContext ctx) {
- Map<String, Comment> drafts = new HashMap<>();
- for (Comment c : commentsUtil.draftByChangeAuthor(ctx.getNotes(), user.getAccountId())) {
- c.tag = in.tag;
- drafts.put(c.key.uuid, c);
- }
- return drafts;
+ return commentsUtil.draftByChangeAuthor(ctx.getNotes(), user.getAccountId()).stream()
+ .collect(
+ Collectors.toMap(
+ c -> c.key.uuid,
+ c -> {
+ c.tag = in.tag;
+ return c;
+ }));
}
private Map<String, Comment> patchSetDrafts(ChangeContext ctx) {
- Map<String, Comment> drafts = new HashMap<>();
- for (Comment c :
- commentsUtil.draftByPatchSetAuthor(psId, user.getAccountId(), ctx.getNotes())) {
- drafts.put(c.key.uuid, c);
- }
- return drafts;
+ return commentsUtil.draftByPatchSetAuthor(psId, user.getAccountId(), ctx.getNotes()).stream()
+ .collect(Collectors.toMap(c -> c.key.uuid, c -> c));
}
private Map<String, Short> approvalsByKey(Collection<PatchSetApproval> patchsetApprovals) {
@@ -1104,10 +1121,7 @@
}
ChangeData cd = changeDataFactory.create(ctx.getNotes());
ReviewerSet reviewers = cd.reviewers();
- if (reviewers.byState(REVIEWER).contains(ctx.getAccountId())) {
- return true;
- }
- return false;
+ return reviewers.byState(REVIEWER).contains(ctx.getAccountId());
}
private boolean updateLabels(ProjectState projectState, ChangeContext ctx)
diff --git a/javatests/com/google/gerrit/httpd/raw/IndexHtmlUtilTest.java b/javatests/com/google/gerrit/httpd/raw/IndexHtmlUtilTest.java
index a4b6ab2..d695c48 100644
--- a/javatests/com/google/gerrit/httpd/raw/IndexHtmlUtilTest.java
+++ b/javatests/com/google/gerrit/httpd/raw/IndexHtmlUtilTest.java
@@ -17,21 +17,42 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.httpd.raw.IndexHtmlUtil.staticTemplateData;
+import com.google.common.collect.ImmutableMap;
import com.google.template.soy.data.SanitizedContent;
import com.google.template.soy.data.UnsafeSanitizedContentOrdainer;
+import java.util.HashMap;
import org.junit.Test;
public class IndexHtmlUtilTest {
@Test
+ public void polymer2() throws Exception {
+ assertThat(
+ staticTemplateData(
+ "http://example.com/",
+ null,
+ null,
+ ImmutableMap.of("p2", new String[0]),
+ IndexHtmlUtilTest::ordain))
+ .containsExactly("canonicalPath", "", "polymer2", "true", "staticResourcePath", ordain(""));
+ }
+
+ @Test
public void noPathAndNoCDN() throws Exception {
- assertThat(staticTemplateData("http://example.com/", null, null, IndexHtmlUtilTest::ordain))
+ assertThat(
+ staticTemplateData(
+ "http://example.com/", null, null, new HashMap<>(), IndexHtmlUtilTest::ordain))
.containsExactly("canonicalPath", "", "staticResourcePath", ordain(""));
}
@Test
public void pathAndNoCDN() throws Exception {
assertThat(
- staticTemplateData("http://example.com/gerrit/", null, null, IndexHtmlUtilTest::ordain))
+ staticTemplateData(
+ "http://example.com/gerrit/",
+ null,
+ null,
+ new HashMap<>(),
+ IndexHtmlUtilTest::ordain))
.containsExactly("canonicalPath", "/gerrit", "staticResourcePath", ordain("/gerrit"));
}
@@ -42,6 +63,7 @@
"http://example.com/",
"http://my-cdn.com/foo/bar/",
null,
+ new HashMap<>(),
IndexHtmlUtilTest::ordain))
.containsExactly(
"canonicalPath", "", "staticResourcePath", ordain("http://my-cdn.com/foo/bar/"));
@@ -54,6 +76,7 @@
"http://example.com/gerrit",
"http://my-cdn.com/foo/bar/",
null,
+ new HashMap<>(),
IndexHtmlUtilTest::ordain))
.containsExactly(
"canonicalPath", "/gerrit", "staticResourcePath", ordain("http://my-cdn.com/foo/bar/"));
diff --git a/lib/js/npm.bzl b/lib/js/npm.bzl
index 8a9e1ee..5a6a8c0 100644
--- a/lib/js/npm.bzl
+++ b/lib/js/npm.bzl
@@ -1,11 +1,11 @@
NPM_VERSIONS = {
"bower": "1.8.8",
"crisper": "2.0.2",
- "polymer-bundler": "4.0.2",
+ "polymer-bundler": "4.0.9",
}
NPM_SHA1S = {
"bower": "82544be34a33aeae7efb8bdf9905247b2cffa985",
"crisper": "7183c58cea33632fb036c91cefd1b43e390d22a2",
- "polymer-bundler": "6b296b6099ab5a0e93ca914cbe93e753f2395910",
+ "polymer-bundler": "c80c9815690d76656d1fa6a231481850b4fa3874",
}
diff --git a/polygerrit-ui/app/elements/gr-app-p2.html b/polygerrit-ui/app/elements/gr-app-p2.html
new file mode 100644
index 0000000..2ce5ed8
--- /dev/null
+++ b/polygerrit-ui/app/elements/gr-app-p2.html
@@ -0,0 +1,40 @@
+<!--
+@license
+Copyright (C) 2019 The Android Open Source Project
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+-->
+<script>
+ window.Gerrit = window.Gerrit || {};
+</script>
+
+<link rel="import" href="/bower_components/polymer/polymer.html">
+<link rel="import" href="/bower_components/polymer-resin/polymer-resin.html">
+<link rel="import" href="/bower_components/polymer/lib/legacy/legacy-data-mixin.html">
+<link rel="import" href="/bower_components/shadycss/apply-shim.html">
+<link rel="import" href="../behaviors/safe-types-behavior/safe-types-behavior.html">
+<script>
+ security.polymer_resin.install({
+ allowedIdentifierPrefixes: [''],
+ reportHandler: security.polymer_resin.CONSOLE_LOGGING_REPORT_HANDLER,
+ safeTypesBridge: Gerrit.SafeTypes.safeTypesBridge,
+ });
+</script>
+
+<link rel="import" href="./gr-app-element.html">
+<dom-module id="gr-app-p2">
+ <template>
+ <gr-app-element id="app-element"></gr-app-element>
+ </template>
+ <script src="gr-app-p2.js" crossorigin="anonymous"></script>
+</dom-module>
diff --git a/polygerrit-ui/app/elements/gr-app-p2.js b/polygerrit-ui/app/elements/gr-app-p2.js
new file mode 100644
index 0000000..2163c02
--- /dev/null
+++ b/polygerrit-ui/app/elements/gr-app-p2.js
@@ -0,0 +1,23 @@
+/**
+ * @license
+ * Copyright (C) 2019 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+(function() {
+ 'use strict';
+
+ Polymer({
+ is: 'gr-app-p2',
+ });
+})();
diff --git a/polygerrit-ui/app/elements/gr-app.html b/polygerrit-ui/app/elements/gr-app.html
index 1a31d4c..e2d2680 100644
--- a/polygerrit-ui/app/elements/gr-app.html
+++ b/polygerrit-ui/app/elements/gr-app.html
@@ -15,25 +15,22 @@
limitations under the License.
-->
<script>
- if (!window.POLYMER2) {
- // This must be set prior to loading Polymer for the first time.
- if (localStorage.getItem('USE_SHADOW_DOM') === 'true') {
- window.Polymer = {
- dom: 'shadow',
- passiveTouchGestures: true,
- };
- } else if (!window.Polymer) {
- window.Polymer = {
- passiveTouchGestures: true,
- };
- }
+ // This must be set prior to loading Polymer for the first time.
+ if (localStorage.getItem('USE_SHADOW_DOM') === 'true') {
+ window.Polymer = {
+ dom: 'shadow',
+ passiveTouchGestures: true,
+ };
+ } else if (!window.Polymer) {
+ window.Polymer = {
+ passiveTouchGestures: true,
+ };
}
window.Gerrit = window.Gerrit || {};
</script>
<link rel="import" href="/bower_components/polymer/polymer.html">
<link rel="import" href="/bower_components/polymer-resin/standalone/polymer-resin.html">
-<link rel="import" href="/bower_components/polymer/lib/legacy/legacy-data-mixin.html">
<link rel="import" href="../behaviors/safe-types-behavior/safe-types-behavior.html">
<script>
security.polymer_resin.install({
diff --git a/polygerrit-ui/app/elements/gr-app.js b/polygerrit-ui/app/elements/gr-app.js
index 2298b2f..5c74659 100644
--- a/polygerrit-ui/app/elements/gr-app.js
+++ b/polygerrit-ui/app/elements/gr-app.js
@@ -21,9 +21,7 @@
// requestAnimationFrame.)
// @see https://github.com/Polymer/polymer/issues/3851
// @see Issue 4699
- if (!window.POLYMER2) {
- Polymer.RenderStatus._makeReady();
- }
+ Polymer.RenderStatus._makeReady();
Polymer({
is: 'gr-app',
diff --git a/resources/com/google/gerrit/httpd/raw/PolyGerritIndexHtml.soy b/resources/com/google/gerrit/httpd/raw/PolyGerritIndexHtml.soy
index bca05db..c907ae9 100644
--- a/resources/com/google/gerrit/httpd/raw/PolyGerritIndexHtml.soy
+++ b/resources/com/google/gerrit/httpd/raw/PolyGerritIndexHtml.soy
@@ -79,7 +79,13 @@
<link rel="preload" href="{$staticResourcePath}/fonts/Roboto-Medium.woff" as="font" type="font/woff" crossorigin="anonymous">{\n}
<link rel="stylesheet" href="{$staticResourcePath}/styles/fonts.css">{\n}
<link rel="stylesheet" href="{$staticResourcePath}/styles/main.css">{\n}
- <script src="{$staticResourcePath}/bower_components/webcomponentsjs/webcomponents-lite.js"></script>{\n}
+
+ {if $polymer2}
+ <script src="{$staticResourcePath}/bower_components/webcomponentsjs-p2/webcomponents-lite.js"></script>{\n}
+ {else}
+ <script src="{$staticResourcePath}/bower_components/webcomponentsjs/webcomponents-lite.js"></script>{\n}
+ {/if}
+
// Content between webcomponents-lite and the load of the main app element
// run before polymer-resin is installed so may have security consequences.
// Contact your local security engineer if you have any questions, and
@@ -90,9 +96,18 @@
<link rel="import" href="{$assetsPath}/{$assetsBundle}">{\n}
{/if}
- <link rel="preload" href="{$staticResourcePath}/elements/gr-app.js" as="script" crossorigin="anonymous">{\n}
- <link rel="import" href="{$staticResourcePath}/elements/gr-app.html">{\n}
+ {if $polymer2}
+ <link rel="preload" href="{$staticResourcePath}/elements/gr-app-p2.js" as="script" crossorigin="anonymous">{\n}
+ <link rel="import" href="{$staticResourcePath}/elements/gr-app-p2.html">{\n}
+ {else}
+ <link rel="preload" href="{$staticResourcePath}/elements/gr-app.js" as="script" crossorigin="anonymous">{\n}
+ <link rel="import" href="{$staticResourcePath}/elements/gr-app.html">{\n}
+ {/if}
<body unresolved>{\n}
- <gr-app id="app"></gr-app>{\n}
+ {if $polymer2}
+ <gr-app-p2 id="app"></gr-app-p2>{\n}
+ {else}
+ <gr-app id="app"></gr-app>{\n}
+ {/if}
{/template}
