Merge changes I0fd82d6f,I21620607 * changes: coverage.sh: Fix warning when running find coverage.sh: Fix iteration over plugins
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index fc4b00a..82cb3f9 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt
@@ -1171,15 +1171,6 @@ + Default is `ALL`. -[[change.api.excludeMergeableInChangeInfo]]change.api.excludeMergeableInChangeInfo:: -+ -If true, the mergeability bit in -link:rest-api-changes.html#change-info[ChangeInfo] will never be set. It can -be requested separately through the -link:rest-api-changes.html#get-mergeable[get-mergeable] endpoint. -+ -Default is false. - [[change.cacheAutomerge]]change.cacheAutomerge:: + When reviewing diff commits, the left-hand side shows the output of the @@ -1195,6 +1186,15 @@ + Default is true. +[[change.commentSizeLimit]]change.commentSizeLimit:: ++ +Maximum allowed size in characters of a regular (non-robot) comment. Comments +which exceed this size will be rejected. Size computation is approximate and may +be off by roughly 1%. Common unit suffixes of 'k', 'm', or 'g' are supported. +The value must be positive. ++ +The default limit is 16kB. + [[change.disablePrivateChanges]]change.disablePrivateChanges:: + If set to true, users are not allowed to create private changes. @@ -1212,6 +1212,13 @@ + By default 500. +[[change.maxComments]]change.maxComments:: ++ +Maximum number of comments (regular plus robot) allowed per change. Additional +comments are rejected. ++ +By default 5,000. + [[change.maxUpdates]]change.maxUpdates:: + Maximum number of updates to a change. Counts only updates to the main NoteDb @@ -1232,6 +1239,34 @@ By default 1000. +[[change.mergeabilityComputationBehavior]]change.mergeabilityComputationBehavior:: ++ +This setting determines when Gerrit computes if a change is mergeable or not. +This computation is expensive, especially when the repository is large or when +there are many open changes. ++ +This config can have the following states: ++ +* `API_REF_UPDATED_AND_CHANGE_REINDEX`: Gerrit indexes `mergeability` enabling + the `is:mergeable` predicate in change search and allowing fast retrieval of + this bit in query responses. Gerrit will always serve `mergeable` in + link:rest-api-changes.html#change-info[ChangeInfo] objects. + Gerrit will reindex all open changes when the target ref advances (expensive). +* `REF_UPDATED_AND_CHANGE_REINDEX`: Gerrit indexes `mergeability` enabling the + `is:mergeable` predicate in change search and allowing fast retrieval of this + bit in query responses. Gerrit will never serve `mergeable` in + link:rest-api-changes.html#change-info[ChangeInfo] + objects. This state can be a final state for instances that only want to + optimize the read path, but not the write path for speed or serve as an + intermediary step for instances that want to optimize both and need to migrate + callers of their API. + Gerrit will reindex all open changes when the target ref advances (expensive). +* `NEVER`: Gerrit does not index `mergeable`, so `is:mergeable` is disabled as + query operator. Gerrit does not serve `mergeable` in + link:rest-api-changes.html#change-info[ChangeInfo]. + +Default is `REF_UPDATED_AND_CHANGE_REINDEX`. + [[change.move]]change.move:: + Whether the link:rest-api-changes.html#move-change[Move Change] REST @@ -1266,11 +1301,10 @@ [[change.robotCommentSizeLimit]]change.robotCommentSizeLimit:: + -Maximum allowed size of a robot comment that will be accepted. Robot comments -which exceed the indicated size will be rejected on addition. The specified -value is interpreted as the maximum size in bytes of the JSON representation of -the robot comment. Common unit suffixes of 'k', 'm', or 'g' are supported. -Zero or negative values allow robot comments of unlimited size. +Maximum allowed size in characters of a robot comment. Robot comments which +exceed this size will be rejected on addition. Size computation is approximate +and may be off by roughly 1%. Common unit suffixes of 'k', 'm', or 'g' are +supported. Zero or negative values allow robot comments of unlimited size. + The default limit is 1024kB. @@ -1400,9 +1434,10 @@ + By default `true`, meaning mergeable changes are auto-abandoned. + -If link:#index.change.indexMergeable[`index.change.indexMergeable`] -is disabled, setting this option to `false` has no effect and it -behaves as though it were set to `true`. +If +link:#change.mergeabilityComputationBehavior[`change.mergeabilityComputationBehavior`] +is set to `NEVER`, setting this option to `false` has no effect and it behaves +as though it were set to `true`. [[changeCleanup.cleanupAccountPatchReview]]changeCleanup.cleanupAccountPatchReview:: + @@ -2776,22 +2811,6 @@ If not set or set to a zero, defaults to the number of logical CPUs as returned by the JVM. If set to a negative value, defaults to a direct executor. -[[index.change.indexMergeable]]index.change.indexMergeable:: -+ -Specifies if `mergeable` should be index or not. Indexing this field enables -queries that contain the mergeability operator (`is:mergeable`). If enabled, -Gerrit will check if the change is mergeable into the target branch when -reindexing a change. This is an expensive operation. -+ -If true, Gerrit will reindex all open changes when the target ref advances. -Depending on the frequency of updates to the ref and the number of open changes, -this can be very expensive. -+ -When this setting is changed from `false` to `true`, all changes need to be -reindexed. -+ -Defaults to true. - [[index.onlineUpgrade]]index.onlineUpgrade:: + Whether to upgrade to new index schema versions while the server is @@ -3102,7 +3121,7 @@ events. + This can be set to the set of minimal options that consumers of Gerrit's -events need. A minimal set would be (`SKIP_MERGEABLE`,`SKIP_DIFFSTAT`). +events need. A minimal set would be (`SKIP_DIFFSTAT`). + Every option that gets added here will have a performance impact. The general recommendation is therefore to set this to a minimal set of
diff --git a/Documentation/rest-api-accounts.txt b/Documentation/rest-api-accounts.txt index e3e9009..bb83d95 100644 --- a/Documentation/rest-api-accounts.txt +++ b/Documentation/rest-api-accounts.txt
@@ -2428,8 +2428,8 @@ |Field Name |Description |`change` | link:rest-api-changes.html#change-info[ChangeInfo] entity describing the change -on which one or more comments was deleted. Populated with only the -link:rest-api-changes.html#skip_mergeable[SKIP_MERGEABLE] option. +on which one or more comments was deleted. Populated with no change list +options. |`deleted` | List of link:rest-api-changes.html#comment-info[CommentInfo] entities for each comment that was deleted.
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 8e2f873..0f36763 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt
@@ -306,26 +306,6 @@ from the change owner, i.e. this change would show up in the results of link:user-search.html#reviewedby[reviewedby:self]. -- - -[[skip_mergeable]] --- -* `SKIP_MERGEABLE`: skip the `mergeable` field in -link:#change-info[ChangeInfo]. For fast moving projects, this field must -be recomputed often, which is slow for projects with big trees. -+ -When link:config-gerrit.html#change.api.excludeMergeableInChangeInfo[ -`change.api.excludeMergeableInChangeInfo`] is set in the `gerrit.config`, -the `mergeable` field will always be omitted and `SKIP_MERGEABLE` has no -effect. -+ -When link:config-gerrit.html#index.change.indexMergeable[ -`index.change.indexMergeable`] is set to `false` in the `gerrit.config`, -the `mergeable` field will always be omitted when querying changes and -`SKIP_MERGEABLE` has no effect. -+ -A change's mergeability can be requested separately by calling the -link:#get-mergeable[get-mergeable] endpoint. --- [[skip_diffstat]] -- * `SKIP_DIFFSTAT`: skip the 'insertions' and 'deletions' field in link:#change-info[ChangeInfo]. @@ -5964,10 +5944,9 @@ Not set for merged changes. |`mergeable` |optional| Whether the change is mergeable. + -Not set for merged changes, if the change has not yet been tested, or -if the link:#skip_mergeable[skip_mergeable] option is set or when -link:config-gerrit.html#change.api.excludeMergeableInChangeInfo[change.api.excludeMergeableInChangeInfo] -is set. +Only set for open changes if +link:config-gerrit.html#change.mergeabilityComputationBehavior[change.mergeabilityComputationBehavior] +is `API_REF_UPDATED_AND_CHANGE_REINDEX`. |`submittable` |optional| Whether the change has been approved by the project submit rules. + Only set if link:#submittable[requested].
diff --git a/Documentation/rest-api-config.txt b/Documentation/rest-api-config.txt index e9e3b94..828674f 100644 --- a/Documentation/rest-api-config.txt +++ b/Documentation/rest-api-config.txt
@@ -1566,10 +1566,11 @@ the whole topic is submitted]. |`disable_private_changes` |not set if `false`| Returns true if private changes are disabled. -|`exclude_mergeable_in_change_info` |not set if `false`| -Value of the link:config-gerrit.html#change.api.excludeMergeableInChangeInfo[ +|`mergeability_computation_behavior` || +Value of the link:config-gerrit.html#change.mergeabilityComputationBehavior[ configuration parameter] that controls whether the mergeability bit in -link:rest-api-changes.html#change-info[ChangeInfo] will never be set. +link:rest-api-changes.html#change-info[ChangeInfo] will never be set and if the +bit is indexed. |============================= [[change-index-config-info]]
diff --git a/java/com/google/gerrit/acceptance/GerritServer.java b/java/com/google/gerrit/acceptance/GerritServer.java index f90df67..95afad7 100644 --- a/java/com/google/gerrit/acceptance/GerritServer.java +++ b/java/com/google/gerrit/acceptance/GerritServer.java
@@ -571,8 +571,8 @@ cfg.setInt("sshd", null, "commandStartThreads", 1); cfg.setInt("receive", null, "threadPoolSize", 1); cfg.setInt("index", null, "threads", 1); - if (cfg.getString("index", "change", "indexMergeable") == null) { - cfg.setBoolean("index", "change", "indexMergeable", false); + if (cfg.getString("index", null, "mergeabilityComputationBehavior") == null) { + cfg.setString("index", null, "mergeabilityComputationBehavior", "NEVER"); } }
diff --git a/java/com/google/gerrit/common/data/CommentDetail.java b/java/com/google/gerrit/common/data/CommentDetail.java index d69f0bb..55e0143 100644 --- a/java/com/google/gerrit/common/data/CommentDetail.java +++ b/java/com/google/gerrit/common/data/CommentDetail.java
@@ -18,10 +18,7 @@ import com.google.gerrit.entities.Comment; import com.google.gerrit.entities.PatchSet; import java.util.ArrayList; -import java.util.Collections; -import java.util.HashMap; import java.util.List; -import java.util.Map; public class CommentDetail { protected List<Comment> a; @@ -29,8 +26,6 @@ private transient PatchSet.Id idA; private transient PatchSet.Id idB; - private transient Map<Integer, List<Comment>> forA; - private transient Map<Integer, List<Comment>> forB; public CommentDetail(PatchSet.Id idA, PatchSet.Id idB) { this.a = new ArrayList<>(); @@ -67,88 +62,4 @@ public boolean isEmpty() { return a.isEmpty() && b.isEmpty(); } - - public List<Comment> getForA(int lineNbr) { - if (forA == null) { - forA = index(a); - } - return get(forA, lineNbr); - } - - public List<Comment> getForB(int lineNbr) { - if (forB == null) { - forB = index(b); - } - return get(forB, lineNbr); - } - - private static List<Comment> get(Map<Integer, List<Comment>> m, int i) { - List<Comment> r = m.get(i); - return r != null ? orderComments(r) : Collections.emptyList(); - } - - /** - * Order the comments based on their parent_uuid parent. It is possible to do this by iterating - * over the list only once but it's probably overkill since the number of comments on a given line - * will be small most of the time. - * - * @param comments The list of comments for a given line. - * @return The comments sorted as they should appear in the UI - */ - private static List<Comment> orderComments(List<Comment> comments) { - // Map of comments keyed by their parent. The values are lists of comments since it is - // possible for several comments to have the same parent (this can happen if two reviewers - // click Reply on the same comment at the same time). Such comments will be displayed under - // their correct parent in chronological order. - Map<String, List<Comment>> parentMap = new HashMap<>(); - - // It's possible to have more than one root comment if two reviewers create a comment on the - // same line at the same time - List<Comment> rootComments = new ArrayList<>(); - - // Store all the comments in parentMap, keyed by their parent - for (Comment c : comments) { - String parentUuid = c.parentUuid; - List<Comment> l = parentMap.get(parentUuid); - if (l == null) { - l = new ArrayList<>(); - parentMap.put(parentUuid, l); - } - l.add(c); - if (parentUuid == null) { - rootComments.add(c); - } - } - - // Add the comments in the list, starting with the head and then going through all the - // comments that have it as a parent, and so on - List<Comment> result = new ArrayList<>(); - addChildren(parentMap, rootComments, result); - - return result; - } - - /** Add the comments to {@code outResult}, depth first */ - private static void addChildren( - Map<String, List<Comment>> parentMap, List<Comment> children, List<Comment> outResult) { - if (children != null) { - for (Comment c : children) { - outResult.add(c); - addChildren(parentMap, parentMap.get(c.key.uuid), outResult); - } - } - } - - private Map<Integer, List<Comment>> index(List<Comment> in) { - HashMap<Integer, List<Comment>> r = new HashMap<>(); - for (Comment p : in) { - List<Comment> l = r.get(p.lineNbr); - if (l == null) { - l = new ArrayList<>(); - r.put(p.lineNbr, l); - } - l.add(p); - } - return r; - } }
diff --git a/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java b/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java index 084c2ec..f659bca 100644 --- a/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java +++ b/java/com/google/gerrit/elasticsearch/ElasticChangeIndex.java
@@ -49,6 +49,7 @@ import com.google.gerrit.server.ReviewerByEmailSet; import com.google.gerrit.server.ReviewerSet; import com.google.gerrit.server.StarredChangesUtil; +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; @@ -111,7 +112,7 @@ this.idField = this.schema.useLegacyNumericFields() ? ChangeField.LEGACY_ID : ChangeField.LEGACY_ID_STR; this.skipFields = - gerritConfig.getBoolean("index", "change", "indexMergeable", true) + MergeabilityComputationBehavior.fromConfig(gerritConfig).includeInIndex() ? ImmutableSet.of() : ImmutableSet.of(ChangeField.MERGEABLE.getName()); }
diff --git a/java/com/google/gerrit/entities/Comment.java b/java/com/google/gerrit/entities/Comment.java index 55d739a..9c58fef 100644 --- a/java/com/google/gerrit/entities/Comment.java +++ b/java/com/google/gerrit/entities/Comment.java
@@ -29,6 +29,8 @@ * * <p>Changing fields in this class changes the storage format of inline comments in NoteDb and may * require a corresponding data migration (adding new optional fields is generally okay). + * + * <p>Consider updating {@link #getApproximateSize()} when adding/changing fields. */ public class Comment { public enum Status { @@ -56,7 +58,7 @@ } } - public static class Key { + public static final class Key { public String uuid; public String filename; public int patchSetId; @@ -97,7 +99,7 @@ } } - public static class Identity { + public static final class Identity { int id; public Identity(Account.Id id) { @@ -147,7 +149,7 @@ * </ul> * </ul> */ - public static class Range implements Comparable<Range> { + public static final class Range implements Comparable<Range> { private static final Comparator<Range> RANGE_COMPARATOR = Comparator.<Range>comparingInt(range -> range.startLine) .thenComparingInt(range -> range.startChar) @@ -220,10 +222,12 @@ public Range range; public String tag; - // Hex commit SHA1 of the commit of the patchset to which this comment applies. Other classes call - // this "commitId", but this class uses the old ReviewDb term "revId", and this field name is - // serialized into JSON in NoteDb, so it can't easily be changed. Callers do not access this field - // directly, and instead use the public getter/setter that wraps an ObjectId. + /** + * Hex commit SHA1 of the commit of the patchset to which this comment applies. Other classes call + * this "commitId", but this class uses the old ReviewDb term "revId", and this field name is + * serialized into JSON in NoteDb, so it can't easily be changed. Callers do not access this field + * directly, and instead use the public getter/setter that wraps an ObjectId. + */ private String revId; public String serverId; @@ -293,6 +297,23 @@ return realAuthor != null ? realAuthor : author; } + /** + * Returns the comment's approximate size. This is used to enforce size limits and should + * therefore include all unbounded fields (e.g. String-s). + */ + public int getApproximateSize() { + return nullableLength(message, parentUuid, tag, revId, serverId) + + (key != null ? nullableLength(key.filename, key.uuid) : 0); + } + + static int nullableLength(String... strings) { + int length = 0; + for (String s : strings) { + length += s == null ? 0 : s.length(); + } + return length; + } + @Override public boolean equals(Object o) { if (!(o instanceof Comment)) {
diff --git a/java/com/google/gerrit/entities/FixReplacement.java b/java/com/google/gerrit/entities/FixReplacement.java index 046300e..fbbf746 100644 --- a/java/com/google/gerrit/entities/FixReplacement.java +++ b/java/com/google/gerrit/entities/FixReplacement.java
@@ -14,10 +14,10 @@ package com.google.gerrit.entities; -public class FixReplacement { - public String path; - public Comment.Range range; - public String replacement; +public final class FixReplacement { + public final String path; + public final Comment.Range range; + public final String replacement; public FixReplacement(String path, Comment.Range range, String replacement) { this.path = path; @@ -38,4 +38,9 @@ + '\'' + '}'; } + + /** Returns this instance's approximate size in bytes for the purpose of applying size limits. */ + int getApproximateSize() { + return path.length() + replacement.length(); + } }
diff --git a/java/com/google/gerrit/entities/FixSuggestion.java b/java/com/google/gerrit/entities/FixSuggestion.java index ac4e720..c88afe7 100644 --- a/java/com/google/gerrit/entities/FixSuggestion.java +++ b/java/com/google/gerrit/entities/FixSuggestion.java
@@ -16,10 +16,10 @@ import java.util.List; -public class FixSuggestion { - public String fixId; - public String description; - public List<FixReplacement> replacements; +public final class FixSuggestion { + public final String fixId; + public final String description; + public final List<FixReplacement> replacements; public FixSuggestion(String fixId, String description, List<FixReplacement> replacements) { this.fixId = fixId; @@ -40,4 +40,11 @@ + replacements + '}'; } + + /** Returns this instance's approximate size in bytes for the purpose of applying size limits. */ + int getApproximateSize() { + return fixId.length() + + description.length() + + replacements.stream().map(FixReplacement::getApproximateSize).reduce(0, Integer::sum); + } }
diff --git a/java/com/google/gerrit/entities/RobotComment.java b/java/com/google/gerrit/entities/RobotComment.java index a7951ad..4a17566 100644 --- a/java/com/google/gerrit/entities/RobotComment.java +++ b/java/com/google/gerrit/entities/RobotComment.java
@@ -19,7 +19,7 @@ import java.util.Map; import java.util.Objects; -public class RobotComment extends Comment { +public final class RobotComment extends Comment { public String robotId; public String robotRunId; public String url; @@ -41,6 +41,22 @@ } @Override + public int getApproximateSize() { + int approximateSize = super.getApproximateSize() + nullableLength(robotId, robotRunId, url); + approximateSize += + properties != null + ? properties.entrySet().stream() + .map(entry -> nullableLength(entry.getKey(), entry.getValue())) + .reduce(0, Integer::sum) + : 0; + approximateSize += + fixSuggestions != null + ? fixSuggestions.stream().map(FixSuggestion::getApproximateSize).reduce(0, Integer::sum) + : 0; + return approximateSize; + } + + @Override public String toString() { return toStringHelper() .add("robotId", robotId)
diff --git a/java/com/google/gerrit/extensions/api/changes/ChangeApi.java b/java/com/google/gerrit/extensions/api/changes/ChangeApi.java index 119d941..96455a6 100644 --- a/java/com/google/gerrit/extensions/api/changes/ChangeApi.java +++ b/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
@@ -259,17 +259,12 @@ * * <ul> * <li>{@code CHECK} is omitted, to skip consistency checks. - * <li>{@code SKIP_MERGEABLE} is omitted, so the {@code mergeable} bit <em>is</em> set. * <li>{@code SKIP_DIFFSTAT} is omitted to ensure diffstat calculations. * </ul> */ default ChangeInfo get() throws RestApiException { return get( - EnumSet.complementOf( - EnumSet.of( - ListChangesOption.CHECK, - ListChangesOption.SKIP_MERGEABLE, - ListChangesOption.SKIP_DIFFSTAT))); + EnumSet.complementOf(EnumSet.of(ListChangesOption.CHECK, ListChangesOption.SKIP_DIFFSTAT))); } /** {@link #get(ListChangesOption...)} with no options included. */
diff --git a/java/com/google/gerrit/extensions/client/ListChangesOption.java b/java/com/google/gerrit/extensions/client/ListChangesOption.java index 425265b..6071cc7 100644 --- a/java/com/google/gerrit/extensions/client/ListChangesOption.java +++ b/java/com/google/gerrit/extensions/client/ListChangesOption.java
@@ -74,7 +74,11 @@ /** If tracking Ids are included, include detailed tracking Ids info. */ TRACKING_IDS(21), - /** Skip mergeability data */ + /** + * Use {@code gerrit.config} instead to turn this off for your instance. See {@code + * change.mergeabilityComputationBehavior}. + */ + @Deprecated SKIP_MERGEABLE(22), /**
diff --git a/java/com/google/gerrit/extensions/common/ChangeConfigInfo.java b/java/com/google/gerrit/extensions/common/ChangeConfigInfo.java index 67d0b0d9..66a2dd5 100644 --- a/java/com/google/gerrit/extensions/common/ChangeConfigInfo.java +++ b/java/com/google/gerrit/extensions/common/ChangeConfigInfo.java
@@ -24,5 +24,5 @@ public String replyTooltip; public int updateDelay; public Boolean submitWholeTopic; - public Boolean excludeMergeableInChangeInfo; + public String mergeabilityComputationBehavior; }
diff --git a/java/com/google/gerrit/extensions/common/ChangeIndexConfigInfo.java b/java/com/google/gerrit/extensions/common/ChangeIndexConfigInfo.java deleted file mode 100644 index 0cce106..0000000 --- a/java/com/google/gerrit/extensions/common/ChangeIndexConfigInfo.java +++ /dev/null
@@ -1,22 +0,0 @@ -// Copyright (C) 2019 The Android Open Source Project -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.gerrit.extensions.common; - -/** - * API response containing values from the {@code index.change} section of {@code gerrit.config}. - */ -public class ChangeIndexConfigInfo { - public Boolean indexMergeable; -}
diff --git a/java/com/google/gerrit/extensions/common/IndexConfigInfo.java b/java/com/google/gerrit/extensions/common/IndexConfigInfo.java deleted file mode 100644 index f3f510b..0000000 --- a/java/com/google/gerrit/extensions/common/IndexConfigInfo.java +++ /dev/null
@@ -1,20 +0,0 @@ -// Copyright (C) 2019 The Android Open Source Project -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.gerrit.extensions.common; - -/** API response containing values from the {@code index} section of {@code gerrit.config}. */ -public class IndexConfigInfo { - public ChangeIndexConfigInfo change; -}
diff --git a/java/com/google/gerrit/extensions/common/ServerInfo.java b/java/com/google/gerrit/extensions/common/ServerInfo.java index 555c363..bc7fcfd 100644 --- a/java/com/google/gerrit/extensions/common/ServerInfo.java +++ b/java/com/google/gerrit/extensions/common/ServerInfo.java
@@ -21,7 +21,6 @@ public ChangeConfigInfo change; public DownloadInfo download; public GerritInfo gerrit; - public IndexConfigInfo index; public Boolean noteDbEnabled; public PluginConfigInfo plugin; public SshdInfo sshd;
diff --git a/java/com/google/gerrit/extensions/validators/CommentForValidation.java b/java/com/google/gerrit/extensions/validators/CommentForValidation.java index 51ae5ae..9b91dff 100644 --- a/java/com/google/gerrit/extensions/validators/CommentForValidation.java +++ b/java/com/google/gerrit/extensions/validators/CommentForValidation.java
@@ -17,13 +17,21 @@ import com.google.auto.value.AutoValue; /** - * Holds a comment's text and {@link CommentType} in order to pass it to a validation plugin. + * Holds a comment's text and some metadata in order to pass it to a validation plugin. * * @see CommentValidator */ @AutoValue public abstract class CommentForValidation { + /** The creator of the comment. */ + public enum CommentSource { + /** A regular user comment. */ + HUMAN, + /** A robot comment. */ + ROBOT + } + /** The type of comment. */ public enum CommentType { /** A regular (inline) comment. */ @@ -34,14 +42,27 @@ CHANGE_MESSAGE } - public static CommentForValidation create(CommentType type, String text) { - return new AutoValue_CommentForValidation(type, text); + public static CommentForValidation create( + CommentSource source, CommentType type, String text, long size) { + return new AutoValue_CommentForValidation(source, type, text, size); } + public abstract CommentSource getSource(); + public abstract CommentType getType(); + /** + * Returns the comment text. Note that especially for robot comments the total size may be + * significantly larger and should be determined by using {@link #getApproximateSize()}. + */ public abstract String getText(); + /** + * Returns this instance's approximate size in bytes for the purpose of applying size limits. For + * robot comments this may be significantly larger than the size of the comment text. + */ + public abstract long getApproximateSize(); + public CommentValidationFailure failValidation(String message) { return CommentValidationFailure.create(this, message); }
diff --git a/java/com/google/gerrit/extensions/validators/CommentValidationContext.java b/java/com/google/gerrit/extensions/validators/CommentValidationContext.java index 1cb00e3..db08058 100644 --- a/java/com/google/gerrit/extensions/validators/CommentValidationContext.java +++ b/java/com/google/gerrit/extensions/validators/CommentValidationContext.java
@@ -34,16 +34,7 @@ /** Returns the project the comment is being added to. */ public abstract String getProject(); - public static Builder builder() { - return new AutoValue_CommentValidationContext.Builder(); - } - - @AutoValue.Builder - public abstract static class Builder { - public abstract Builder changeId(int value); - - public abstract Builder project(String value); - - public abstract CommentValidationContext build(); + public static CommentValidationContext create(int changeId, String project) { + return new AutoValue_CommentValidationContext(changeId, project); } }
diff --git a/java/com/google/gerrit/extensions/validators/CommentValidator.java b/java/com/google/gerrit/extensions/validators/CommentValidator.java index ba73e46..2dfd6e2 100644 --- a/java/com/google/gerrit/extensions/validators/CommentValidator.java +++ b/java/com/google/gerrit/extensions/validators/CommentValidator.java
@@ -25,7 +25,10 @@ public interface CommentValidator { /** - * Validate the specified comments. + * Validate the specified comments. This method will be called with all new comments that need to + * be validated. This allows validators to statelessly count the new comments. Note that the + * method may be called separately for texts that are not comments, but similar in nature and also + * subject to size limits, such as a change message. * * @return An empty list if all comments are valid, or else a list of validation failures. */
diff --git a/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/java/com/google/gerrit/lucene/LuceneChangeIndex.java index 3b277dd..375967b 100644 --- a/java/com/google/gerrit/lucene/LuceneChangeIndex.java +++ b/java/com/google/gerrit/lucene/LuceneChangeIndex.java
@@ -55,6 +55,7 @@ import com.google.gerrit.index.query.ResultSet; import com.google.gerrit.proto.Protos; import com.google.gerrit.server.StarredChangesUtil; +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.IndexExecutor; @@ -182,7 +183,7 @@ this.changeDataFactory = changeDataFactory; this.schema = schema; this.skipFields = - cfg.getBoolean("index", "change", "indexMergeable", true) + MergeabilityComputationBehavior.fromConfig(cfg).includeInIndex() ? ImmutableSet.of() : ImmutableSet.of(ChangeField.MERGEABLE.getName());
diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java index 974cb47..70e7967 100644 --- a/java/com/google/gerrit/server/change/ChangeJson.java +++ b/java/com/google/gerrit/server/change/ChangeJson.java
@@ -29,7 +29,6 @@ import static com.google.gerrit.extensions.client.ListChangesOption.REVIEWED; import static com.google.gerrit.extensions.client.ListChangesOption.REVIEWER_UPDATES; import static com.google.gerrit.extensions.client.ListChangesOption.SKIP_DIFFSTAT; -import static com.google.gerrit.extensions.client.ListChangesOption.SKIP_MERGEABLE; import static com.google.gerrit.extensions.client.ListChangesOption.SUBMITTABLE; import static com.google.gerrit.extensions.client.ListChangesOption.TRACKING_IDS; import static com.google.gerrit.server.ChangeMessagesUtil.createChangeMessageInfo; @@ -219,7 +218,7 @@ private final Metrics metrics; private final RevisionJson revisionJson; private final Optional<PluginDefinedAttributesFactory> pluginDefinedAttributesFactory; - private final boolean excludeMergeableInChangeInfo; + private final boolean includeMergeable; private final boolean lazyLoad; private AccountLoader accountLoader; @@ -257,8 +256,7 @@ this.metrics = metrics; this.revisionJson = revisionJsonFactory.create(options); this.options = Sets.immutableEnumSet(options); - this.excludeMergeableInChangeInfo = - cfg.getBoolean("change", "api", "excludeMergeableInChangeInfo", false); + this.includeMergeable = MergeabilityComputationBehavior.fromConfig(cfg).includeInApi(); this.lazyLoad = containsAnyOf(this.options, REQUIRE_LAZY_LOAD); this.pluginDefinedAttributesFactory = pluginDefinedAttributesFactory; @@ -525,7 +523,7 @@ if (str.isOk()) { out.submitType = str.type; } - if (!excludeMergeableInChangeInfo && !has(SKIP_MERGEABLE)) { + if (includeMergeable) { out.mergeable = cd.isMergeable(); } if (has(SUBMITTABLE)) {
diff --git a/java/com/google/gerrit/server/change/MergeabilityComputationBehavior.java b/java/com/google/gerrit/server/change/MergeabilityComputationBehavior.java new file mode 100644 index 0000000..26e7a89 --- /dev/null +++ b/java/com/google/gerrit/server/change/MergeabilityComputationBehavior.java
@@ -0,0 +1,58 @@ +// Copyright (C) 2020 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.change; + +import org.eclipse.jgit.lib.Config; + +/** + * State that is used to decide if {@code mergeable} should be included in the REST API or the + * change index. + * + * <p>Computing mergeability of a change is an expensive operation - especially for Gerrit + * installations with a large number of open changes or large repositories. Therefore, we want to + * control when this computation is performed. + */ +public enum MergeabilityComputationBehavior { + NEVER(false, false), + REF_UPDATED_AND_CHANGE_REINDEX(true, false), + API_REF_UPDATED_AND_CHANGE_REINDEX(true, true); + + private final boolean includeInIndex; + private final boolean includeInApi; + + MergeabilityComputationBehavior(boolean includeInIndex, boolean includeInApi) { + this.includeInIndex = includeInIndex; + this.includeInApi = includeInApi; + } + + /** Returns a {@link MergeabilityComputationBehavior} constructed from a Gerrit server config. */ + public static MergeabilityComputationBehavior fromConfig(Config cfg) { + return cfg.getEnum( + "change", + null, + "mergeabilityComputationBehavior", + MergeabilityComputationBehavior.API_REF_UPDATED_AND_CHANGE_REINDEX); + } + + /** Whether {@code mergeable} should be included in the change API. */ + public boolean includeInApi() { + return includeInApi; + } + + /** Whether {@code mergeable} should be included in the change index. */ + public boolean includeInIndex() { + return includeInIndex; + } +}
diff --git a/java/com/google/gerrit/server/config/ChangeCleanupConfig.java b/java/com/google/gerrit/server/config/ChangeCleanupConfig.java index afd159c..bd51fc7 100644 --- a/java/com/google/gerrit/server/config/ChangeCleanupConfig.java +++ b/java/com/google/gerrit/server/config/ChangeCleanupConfig.java
@@ -17,6 +17,7 @@ import com.google.common.base.Strings; import com.google.common.flogger.FluentLogger; import com.google.gerrit.extensions.registration.DynamicItem; +import com.google.gerrit.server.change.MergeabilityComputationBehavior; import com.google.gerrit.server.config.ScheduleConfig.Schedule; import com.google.inject.Inject; import com.google.inject.Singleton; @@ -51,7 +52,7 @@ this.urlFormatter = urlFormatter; schedule = ScheduleConfig.createSchedule(cfg, SECTION); abandonAfter = readAbandonAfter(cfg); - boolean indexMergeable = cfg.getBoolean("index", "change", "indexMergeable", true); + boolean indexMergeable = MergeabilityComputationBehavior.fromConfig(cfg).includeInIndex(); if (!indexMergeable) { if (!readAbandonIfMergeable(cfg)) { logger.atWarning().log(
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java index 7df3902..012e4cd 100644 --- a/java/com/google/gerrit/server/config/GerritGlobalModule.java +++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -128,7 +128,8 @@ import com.google.gerrit.server.git.TagCache; import com.google.gerrit.server.git.TransferConfig; import com.google.gerrit.server.git.receive.ReceiveCommitsModule; -import com.google.gerrit.server.git.validators.CommentLimitsValidator; +import com.google.gerrit.server.git.validators.CommentCountValidator; +import com.google.gerrit.server.git.validators.CommentSizeValidator; import com.google.gerrit.server.git.validators.CommitValidationListener; import com.google.gerrit.server.git.validators.MergeValidationListener; import com.google.gerrit.server.git.validators.MergeValidators; @@ -410,8 +411,11 @@ DynamicSet.setOf(binder(), UploadValidationListener.class); bind(CommentValidator.class) - .annotatedWith(Exports.named(CommentLimitsValidator.class.getSimpleName())) - .to(CommentLimitsValidator.class); + .annotatedWith(Exports.named(CommentCountValidator.class.getSimpleName())) + .to(CommentCountValidator.class); + bind(CommentValidator.class) + .annotatedWith(Exports.named(CommentSizeValidator.class.getSimpleName())) + .to(CommentSizeValidator.class); DynamicMap.mapOf(binder(), DynamicOptions.DynamicBean.class); DynamicMap.mapOf(binder(), ChangeQueryBuilder.ChangeOperatorFactory.class);
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index d8aa054..4b6a4cd 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -92,6 +92,7 @@ import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.UnprocessableEntityException; import com.google.gerrit.extensions.validators.CommentForValidation; +import com.google.gerrit.extensions.validators.CommentForValidation.CommentSource; import com.google.gerrit.extensions.validators.CommentForValidation.CommentType; import com.google.gerrit.extensions.validators.CommentValidationContext; import com.google.gerrit.extensions.validators.CommentValidationFailure; @@ -126,6 +127,8 @@ import com.google.gerrit.server.git.ReceivePackInitializer; import com.google.gerrit.server.git.TagCache; import com.google.gerrit.server.git.ValidationError; +import com.google.gerrit.server.git.validators.CommentCountValidator; +import com.google.gerrit.server.git.validators.CommentSizeValidator; import com.google.gerrit.server.git.validators.CommitValidationMessage; import com.google.gerrit.server.git.validators.RefOperationValidationException; import com.google.gerrit.server.git.validators.RefOperationValidators; @@ -1441,11 +1444,18 @@ final ReceiveCommand cmd; final LabelTypes labelTypes; /** - * Result of running {@link CommentValidator}-s on drafts that are published with the commit - * (which happens iff {@code --publish-comments} is set). Remains {@code true} if none are - * installed. + * Draft comments are published with the commit iff {@code --publish-comments} is set. All + * drafts are withheld (overriding the option) if at least one of the following conditions are + * met: + * + * <ul> + * <li>Installed {@link CommentValidator} plugins reject one or more draft comments. + * <li>One or more comments exceed the maximum comment size (see {@link + * CommentSizeValidator}). + * <li>The maximum number of comments would be exceeded (see {@link CommentCountValidator}). + * </ul> */ - private boolean commentsValid = true; + private boolean withholdComments = false; BranchNameKey dest; PermissionBackend.ForRef perm; @@ -1642,18 +1652,19 @@ .collect(toImmutableSet()); } - void setCommentsValid(boolean commentsValid) { - this.commentsValid = commentsValid; + void setWithholdComments(boolean withholdComments) { + this.withholdComments = withholdComments; } boolean shouldPublishComments() { - if (!commentsValid) { + if (withholdComments) { // Validation messages of type WARNING have already been added, now withhold the comments. return false; } if (publishComments) { return true; - } else if (noPublishComments) { + } + if (noPublishComments) { return false; } return defaultPublishComments; @@ -2010,19 +2021,18 @@ .map( comment -> CommentForValidation.create( + CommentSource.HUMAN, comment.lineNbr > 0 ? CommentType.INLINE_COMMENT : CommentType.FILE_COMMENT, - comment.message)) + comment.message, + comment.message.length())) .collect(toImmutableList()); CommentValidationContext ctx = - CommentValidationContext.builder() - .changeId(change.getChangeId()) - .project(change.getProject().get()) - .build(); + CommentValidationContext.create(change.getChangeId(), change.getProject().get()); ImmutableList<CommentValidationFailure> commentValidationFailures = PublishCommentUtil.findInvalidComments(ctx, commentValidators, draftsForValidation); - magicBranch.setCommentsValid(commentValidationFailures.isEmpty()); + magicBranch.setWithholdComments(!commentValidationFailures.isEmpty()); commentValidationFailures.forEach( failure -> addMessage(
diff --git a/java/com/google/gerrit/server/git/validators/CommentCountValidator.java b/java/com/google/gerrit/server/git/validators/CommentCountValidator.java new file mode 100644 index 0000000..2c939c8 --- /dev/null +++ b/java/com/google/gerrit/server/git/validators/CommentCountValidator.java
@@ -0,0 +1,61 @@ +// Copyright (C) 2020 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.git.validators; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Iterables; +import com.google.gerrit.entities.Change; +import com.google.gerrit.entities.Project; +import com.google.gerrit.extensions.validators.CommentForValidation; +import com.google.gerrit.extensions.validators.CommentValidationContext; +import com.google.gerrit.extensions.validators.CommentValidationFailure; +import com.google.gerrit.extensions.validators.CommentValidator; +import com.google.gerrit.server.config.GerritServerConfig; +import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.inject.Inject; +import org.eclipse.jgit.lib.Config; + +/** Limits number of comments to prevent space/time complexity issues. */ +public class CommentCountValidator implements CommentValidator { + private final int maxComments; + private final ChangeNotes.Factory notesFactory; + + @Inject + CommentCountValidator(@GerritServerConfig Config serverConfig, ChangeNotes.Factory notesFactory) { + this.notesFactory = notesFactory; + maxComments = serverConfig.getInt("change", "maxComments", 5_000); + } + + @Override + public ImmutableList<CommentValidationFailure> validateComments( + CommentValidationContext ctx, ImmutableList<CommentForValidation> comments) { + ImmutableList.Builder<CommentValidationFailure> failures = ImmutableList.builder(); + ChangeNotes notes = + notesFactory.createChecked(Project.nameKey(ctx.getProject()), Change.id(ctx.getChangeId())); + int numExistingComments = notes.getComments().size() + notes.getRobotComments().size(); + if (!comments.isEmpty() && numExistingComments + comments.size() > maxComments) { + // This warning really applies to the set of all comments, but we need to pick one to attach + // the message to. + CommentForValidation commentForFailureMessage = Iterables.getLast(comments); + + failures.add( + commentForFailureMessage.failValidation( + String.format( + "Exceeding maximum number of comments: %d (existing) + %d (new) > %d", + numExistingComments, comments.size(), maxComments))); + } + return failures.build(); + } +}
diff --git a/java/com/google/gerrit/server/git/validators/CommentLimitsValidator.java b/java/com/google/gerrit/server/git/validators/CommentLimitsValidator.java deleted file mode 100644 index 3a8bcac..0000000 --- a/java/com/google/gerrit/server/git/validators/CommentLimitsValidator.java +++ /dev/null
@@ -1,47 +0,0 @@ -// Copyright (C) 2020 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.git.validators; - -import com.google.common.collect.ImmutableList; -import com.google.gerrit.extensions.validators.CommentForValidation; -import com.google.gerrit.extensions.validators.CommentValidationContext; -import com.google.gerrit.extensions.validators.CommentValidationFailure; -import com.google.gerrit.extensions.validators.CommentValidator; -import com.google.gerrit.server.config.GerritServerConfig; -import com.google.inject.Inject; -import org.eclipse.jgit.lib.Config; - -/** Limits the size of comments to prevent large comments from causing issues. */ -public class CommentLimitsValidator implements CommentValidator { - private final int maxCommentLength; - - @Inject - CommentLimitsValidator(@GerritServerConfig Config serverConfig) { - maxCommentLength = serverConfig.getInt("change", null, "maxCommentLength", 16 << 10); - } - - @Override - public ImmutableList<CommentValidationFailure> validateComments( - CommentValidationContext ctx, ImmutableList<CommentForValidation> comments) { - return comments.stream() - .filter(c -> c.getText().length() > maxCommentLength) - .map( - c -> - c.failValidation( - String.format( - "Comment too large (%d > %d)", c.getText().length(), maxCommentLength))) - .collect(ImmutableList.toImmutableList()); - } -}
diff --git a/java/com/google/gerrit/server/git/validators/CommentSizeValidator.java b/java/com/google/gerrit/server/git/validators/CommentSizeValidator.java new file mode 100644 index 0000000..58b0cb1 --- /dev/null +++ b/java/com/google/gerrit/server/git/validators/CommentSizeValidator.java
@@ -0,0 +1,71 @@ +// Copyright (C) 2020 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.git.validators; + +import com.google.common.collect.ImmutableList; +import com.google.gerrit.extensions.validators.CommentForValidation; +import com.google.gerrit.extensions.validators.CommentValidationContext; +import com.google.gerrit.extensions.validators.CommentValidationFailure; +import com.google.gerrit.extensions.validators.CommentValidator; +import com.google.gerrit.server.config.GerritServerConfig; +import com.google.inject.Inject; +import org.eclipse.jgit.lib.Config; + +/** Limits the size of comments to prevent space/time complexity issues. */ +public class CommentSizeValidator implements CommentValidator { + private final int commentSizeLimit; + private final int robotCommentSizeLimit; + + @Inject + CommentSizeValidator(@GerritServerConfig Config serverConfig) { + commentSizeLimit = serverConfig.getInt("change", "commentSizeLimit", 16 << 10); + robotCommentSizeLimit = serverConfig.getInt("change", "robotCommentSizeLimit", 1 << 20); + } + + @Override + public ImmutableList<CommentValidationFailure> validateComments( + CommentValidationContext ctx, ImmutableList<CommentForValidation> comments) { + return comments.stream() + .filter(this::exceedsSizeLimit) + .map(c -> c.failValidation(buildErrorMessage(c))) + .collect(ImmutableList.toImmutableList()); + } + + private boolean exceedsSizeLimit(CommentForValidation comment) { + switch (comment.getSource()) { + case HUMAN: + return comment.getApproximateSize() > commentSizeLimit; + case ROBOT: + return robotCommentSizeLimit > 0 && comment.getApproximateSize() > robotCommentSizeLimit; + } + throw new RuntimeException( + "Unknown comment source (should not have compiled): " + comment.getSource()); + } + + private String buildErrorMessage(CommentForValidation comment) { + switch (comment.getSource()) { + case HUMAN: + return String.format( + "Comment size exceeds limit (%d > %d)", comment.getApproximateSize(), commentSizeLimit); + + case ROBOT: + return String.format( + "Size %d (bytes) of robot comment is greater than limit %d (bytes)", + comment.getApproximateSize(), robotCommentSizeLimit); + } + throw new RuntimeException( + "Unknown comment source (should not have compiled): " + comment.getSource()); + } +}
diff --git a/java/com/google/gerrit/server/git/validators/CommitValidators.java b/java/com/google/gerrit/server/git/validators/CommitValidators.java index 160248c..4d2dbef 100644 --- a/java/com/google/gerrit/server/git/validators/CommitValidators.java +++ b/java/com/google/gerrit/server/git/validators/CommitValidators.java
@@ -151,6 +151,7 @@ new ProjectStateValidationListener(projectState), new AmendedGerritMergeCommitValidationListener(perm, gerritIdent), new AuthorUploaderValidator(user, perm, urlFormatter.get()), + new FileCountValidator(patchListCache, config), new CommitterUploaderValidator(user, perm, urlFormatter.get()), new SignedOffByValidator(user, perm, projectState), new ChangeIdValidator( @@ -414,7 +415,11 @@ diffSummary.getPaths().size(), maxFileCount)); } } catch (PatchListNotAvailableException e) { - logger.atWarning().withCause(e).log("Failed to validate file count"); + // This happens e.g. for cherrypicks. + if (!receiveEvent.command.getRefName().startsWith(REFS_CHANGES)) { + logger.atWarning().withCause(e).log( + "Failed to validate file count for commit: %s", receiveEvent.commit.toString()); + } } return Collections.emptyList(); }
diff --git a/java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java b/java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java index 4e27859..19bb423 100644 --- a/java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java +++ b/java/com/google/gerrit/server/index/change/ReindexAfterRefUpdate.java
@@ -28,6 +28,7 @@ import com.google.gerrit.entities.RefNames; import com.google.gerrit.extensions.events.GitReferenceUpdatedListener; import com.google.gerrit.server.account.AccountCache; +import com.google.gerrit.server.change.MergeabilityComputationBehavior; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.git.QueueProvider.QueueType; @@ -85,7 +86,7 @@ this.accountCache = accountCache; this.indexer = indexer; this.executor = executor; - this.enabled = cfg.getBoolean("index", "change", "indexMergeable", true); + this.enabled = MergeabilityComputationBehavior.fromConfig(cfg).includeInIndex(); } @Override
diff --git a/java/com/google/gerrit/server/mail/receive/MailProcessor.java b/java/com/google/gerrit/server/mail/receive/MailProcessor.java index e79696a..9c3dd02 100644 --- a/java/com/google/gerrit/server/mail/receive/MailProcessor.java +++ b/java/com/google/gerrit/server/mail/receive/MailProcessor.java
@@ -19,6 +19,7 @@ import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Iterables; import com.google.common.flogger.FluentLogger; import com.google.gerrit.entities.Account; import com.google.gerrit.entities.Change; @@ -243,7 +244,7 @@ sendRejectionEmail(message, InboundEmailRejectionSender.Error.INTERNAL_EXCEPTION); return; } - ChangeData cd = changeDataList.get(0); + ChangeData cd = Iterables.getOnlyElement(changeDataList); if (existingMessageIds(cd).contains(message.id())) { logger.atInfo().log("Message %s was already processed. Will delete message.", message.id()); return; @@ -285,14 +286,14 @@ .map( comment -> CommentForValidation.create( + CommentForValidation.CommentSource.HUMAN, MAIL_COMMENT_TYPE_TO_VALIDATION_TYPE.get(comment.getType()), - comment.getMessage())) + comment.getMessage(), + comment.getMessage().length())) .collect(ImmutableList.toImmutableList()); CommentValidationContext commentValidationCtx = - CommentValidationContext.builder() - .changeId(cd.change().getChangeId()) - .project(cd.change().getProject().get()) - .build(); + CommentValidationContext.create( + cd.change().getChangeId(), cd.change().getProject().get()); ImmutableList<CommentValidationFailure> commentValidationFailures = PublishCommentUtil.findInvalidComments( commentValidationCtx, commentValidators, parsedCommentsForValidation);
diff --git a/java/com/google/gerrit/server/notedb/LimitExceededException.java b/java/com/google/gerrit/server/notedb/LimitExceededException.java index 69f9241..0a57d96 100644 --- a/java/com/google/gerrit/server/notedb/LimitExceededException.java +++ b/java/com/google/gerrit/server/notedb/LimitExceededException.java
@@ -23,12 +23,13 @@ * <ul> * <li>The number of NoteDb updates per change. * <li>The number of patch sets per change. + * <li>The number of files per change. * </ul> */ public class LimitExceededException extends StorageException { private static final long serialVersionUID = 1L; - LimitExceededException(String message) { + public LimitExceededException(String message) { super(message); } }
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java index 11fb0d6..327c21f 100644 --- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java +++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -63,6 +63,7 @@ import com.google.gerrit.server.account.VersionedAccountDestinations; import com.google.gerrit.server.account.VersionedAccountQueries; import com.google.gerrit.server.change.ChangeTriplet; +import com.google.gerrit.server.change.MergeabilityComputationBehavior; import com.google.gerrit.server.config.AllProjectsName; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.config.GerritServerConfig; @@ -293,7 +294,7 @@ groupMembers, anonymousUserProvider, operatorAliasConfig, - gerritConfig.getBoolean("index", "change", "indexMergeable", true)); + MergeabilityComputationBehavior.fromConfig(gerritConfig).includeInIndex()); } private Arguments(
diff --git a/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java b/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java index a6fbb10..4b505c6 100644 --- a/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java +++ b/java/com/google/gerrit/server/restapi/account/DeleteDraftComments.java
@@ -27,7 +27,6 @@ import com.google.gerrit.entities.Project; import com.google.gerrit.extensions.api.accounts.DeleteDraftCommentsInput; import com.google.gerrit.extensions.api.accounts.DeletedDraftCommentInfo; -import com.google.gerrit.extensions.client.ListChangesOption; import com.google.gerrit.extensions.common.CommentInfo; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; @@ -190,9 +189,7 @@ if (dirty) { result = new DeletedDraftCommentInfo(); result.change = - changeJsonFactory - .create(ListChangesOption.SKIP_MERGEABLE) - .format(changeDataFactory.create(ctx.getNotes())); + changeJsonFactory.noOptions().format(changeDataFactory.create(ctx.getNotes())); result.deleted = comments.build(); } return dirty;
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java index ddb112a2..129df59 100644 --- a/java/com/google/gerrit/server/restapi/change/PostReview.java +++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -80,7 +80,6 @@ import com.google.gerrit.extensions.validators.CommentValidationContext; import com.google.gerrit.extensions.validators.CommentValidationFailure; import com.google.gerrit.extensions.validators.CommentValidator; -import com.google.gerrit.json.OutputFormat; import com.google.gerrit.mail.Address; import com.google.gerrit.server.ApprovalsUtil; import com.google.gerrit.server.ChangeMessagesUtil; @@ -125,11 +124,9 @@ import com.google.gerrit.server.update.UpdateException; import com.google.gerrit.server.util.LabelVote; import com.google.gerrit.server.util.time.TimeUtil; -import com.google.gson.Gson; import com.google.inject.Inject; import com.google.inject.Singleton; import java.io.IOException; -import java.nio.charset.StandardCharsets; import java.sql.Timestamp; import java.util.ArrayList; import java.util.Collection; @@ -141,7 +138,6 @@ import java.util.Map; import java.util.Objects; import java.util.Optional; -import java.util.OptionalInt; import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -159,9 +155,6 @@ public static final String START_REVIEW_MESSAGE = "This change is ready for review."; - private static final Gson GSON = OutputFormat.JSON_COMPACT.newGson(); - private static final int DEFAULT_ROBOT_COMMENT_SIZE_LIMIT_IN_BYTES = 1024 * 1024; - private final BatchUpdate.Factory updateFactory; private final ChangeResource.Factory changeResourceFactory; private final ChangeData.Factory changeDataFactory; @@ -177,7 +170,6 @@ private final ReviewerAdder reviewerAdder; private final AddReviewersEmail addReviewersEmail; private final NotifyResolver notifyResolver; - private final Config gerritConfig; private final WorkInProgressOp.Factory workInProgressOpFactory; private final ProjectCache projectCache; private final PermissionBackend permissionBackend; @@ -221,7 +213,6 @@ this.reviewerAdder = reviewerAdder; this.addReviewersEmail = addReviewersEmail; this.notifyResolver = notifyResolver; - this.gerritConfig = gerritConfig; this.workInProgressOpFactory = workInProgressOpFactory; this.projectCache = projectCache; this.permissionBackend = permissionBackend; @@ -672,41 +663,15 @@ for (Map.Entry<String, List<RobotCommentInput>> e : in.entrySet()) { String commentPath = e.getKey(); for (RobotCommentInput c : e.getValue()) { - ensureSizeOfJsonInputIsWithinBounds(c); ensureRobotIdIsSet(c.robotId, commentPath); ensureRobotRunIdIsSet(c.robotRunId, commentPath); ensureFixSuggestionsAreAddable(c.fixSuggestions, commentPath); + // Size is validated later, in CommentLimitsValidator. } } checkComments(revision, in); } - private void ensureSizeOfJsonInputIsWithinBounds(RobotCommentInput robotCommentInput) - throws BadRequestException { - OptionalInt robotCommentSizeLimit = getRobotCommentSizeLimit(); - if (robotCommentSizeLimit.isPresent()) { - int sizeLimit = robotCommentSizeLimit.getAsInt(); - byte[] robotCommentBytes = GSON.toJson(robotCommentInput).getBytes(StandardCharsets.UTF_8); - int robotCommentSize = robotCommentBytes.length; - if (robotCommentSize > sizeLimit) { - throw new BadRequestException( - String.format( - "Size %d (bytes) of robot comment is greater than limit %d (bytes)", - robotCommentSize, sizeLimit)); - } - } - } - - private OptionalInt getRobotCommentSizeLimit() { - int robotCommentSizeLimit = - gerritConfig.getInt( - "change", "robotCommentSizeLimit", DEFAULT_ROBOT_COMMENT_SIZE_LIMIT_IN_BYTES); - if (robotCommentSizeLimit <= 0) { - return OptionalInt.empty(); - } - return OptionalInt.of(robotCommentSizeLimit); - } - private static void ensureRobotIdIsSet(String robotId, String commentPath) throws BadRequestException { if (robotId == null) { @@ -910,8 +875,10 @@ user = ctx.getIdentifiedUser(); notes = ctx.getNotes(); ps = psUtil.get(ctx.getNotes(), psId); - boolean dirty = insertComments(ctx); - dirty |= insertRobotComments(ctx); + List<RobotComment> newRobotComments = + in.robotComments == null ? ImmutableList.of() : getNewRobotComments(ctx); + boolean dirty = insertComments(ctx, newRobotComments); + dirty |= insertRobotComments(ctx, newRobotComments); dirty |= updateLabels(projectState, ctx); dirty |= insertMessage(ctx); return dirty; @@ -938,7 +905,7 @@ ctx.getWhen()); } - private boolean insertComments(ChangeContext ctx) + private boolean insertComments(ChangeContext ctx, List<RobotComment> newRobotComments) throws UnprocessableEntityException, PatchListNotAvailableException, CommentsRejectedException { Map<String, List<CommentInput>> inputComments = in.comments; @@ -1002,21 +969,21 @@ } CommentValidationContext commentValidationCtx = - CommentValidationContext.builder() - .changeId(ctx.getChange().getChangeId()) - .project(ctx.getChange().getProject().get()) - .build(); + CommentValidationContext.create( + ctx.getChange().getChangeId(), ctx.getChange().getProject().get()); switch (in.drafts) { case PUBLISH: case PUBLISH_ALL_REVISIONS: validateComments( - commentValidationCtx, Streams.concat(drafts.values().stream(), toPublish.stream())); + commentValidationCtx, + Streams.concat( + drafts.values().stream(), toPublish.stream(), newRobotComments.stream())); publishCommentUtil.publish(ctx, ctx.getUpdate(psId), drafts.values(), in.tag); comments.addAll(drafts.values()); break; case KEEP: - default: - validateComments(commentValidationCtx, toPublish.stream()); + validateComments( + commentValidationCtx, Stream.concat(toPublish.stream(), newRobotComments.stream())); break; } ChangeUpdate changeUpdate = ctx.getUpdate(psId); @@ -1032,10 +999,14 @@ .map( comment -> CommentForValidation.create( + comment instanceof RobotComment + ? CommentForValidation.CommentSource.ROBOT + : CommentForValidation.CommentSource.HUMAN, comment.lineNbr > 0 ? CommentForValidation.CommentType.INLINE_COMMENT : CommentForValidation.CommentType.FILE_COMMENT, - comment.message)) + comment.message, + comment.getApproximateSize())) .collect(toImmutableList()); ImmutableList<CommentValidationFailure> draftValidationFailures = PublishCommentUtil.findInvalidComments(ctx, commentValidators, draftsForValidation); @@ -1044,12 +1015,10 @@ } } - private boolean insertRobotComments(ChangeContext ctx) throws PatchListNotAvailableException { + private boolean insertRobotComments(ChangeContext ctx, List<RobotComment> newRobotComments) { if (in.robotComments == null) { return false; } - - List<RobotComment> newRobotComments = getNewRobotComments(ctx); commentsUtil.putRobotComments(ctx.getUpdate(psId), newRobotComments); comments.addAll(newRobotComments); return !newRobotComments.isEmpty(); @@ -1425,17 +1394,18 @@ } if (!msg.isEmpty()) { CommentValidationContext commentValidationCtx = - CommentValidationContext.builder() - .changeId(ctx.getChange().getChangeId()) - .project(ctx.getChange().getProject().get()) - .build(); + CommentValidationContext.create( + ctx.getChange().getChangeId(), ctx.getChange().getProject().get()); ImmutableList<CommentValidationFailure> messageValidationFailure = PublishCommentUtil.findInvalidComments( commentValidationCtx, commentValidators, ImmutableList.of( CommentForValidation.create( - CommentForValidation.CommentType.CHANGE_MESSAGE, msg))); + CommentForValidation.CommentSource.HUMAN, + CommentForValidation.CommentType.CHANGE_MESSAGE, + msg, + msg.length()))); if (!messageValidationFailure.isEmpty()) { throw new CommentsRejectedException(messageValidationFailure); }
diff --git a/java/com/google/gerrit/server/restapi/config/GetServerInfo.java b/java/com/google/gerrit/server/restapi/config/GetServerInfo.java index 2d4bfe6..bbe38a3 100644 --- a/java/com/google/gerrit/server/restapi/config/GetServerInfo.java +++ b/java/com/google/gerrit/server/restapi/config/GetServerInfo.java
@@ -23,11 +23,9 @@ import com.google.gerrit.extensions.common.AccountsInfo; import com.google.gerrit.extensions.common.AuthInfo; import com.google.gerrit.extensions.common.ChangeConfigInfo; -import com.google.gerrit.extensions.common.ChangeIndexConfigInfo; import com.google.gerrit.extensions.common.DownloadInfo; import com.google.gerrit.extensions.common.DownloadSchemeInfo; import com.google.gerrit.extensions.common.GerritInfo; -import com.google.gerrit.extensions.common.IndexConfigInfo; import com.google.gerrit.extensions.common.PluginConfigInfo; import com.google.gerrit.extensions.common.ReceiveInfo; import com.google.gerrit.extensions.common.ServerInfo; @@ -45,6 +43,7 @@ import com.google.gerrit.server.account.Realm; import com.google.gerrit.server.avatar.AvatarProvider; import com.google.gerrit.server.change.ArchiveFormat; +import com.google.gerrit.server.change.MergeabilityComputationBehavior; import com.google.gerrit.server.config.AllProjectsName; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.config.AnonymousCowardName; @@ -143,7 +142,6 @@ info.change = getChangeInfo(); info.download = getDownloadInfo(); info.gerrit = getGerritInfo(); - info.index = getIndexInfo(); info.noteDbEnabled = true; info.plugin = getPluginInfo(); info.defaultTheme = getDefaultTheme(); @@ -230,10 +228,10 @@ info.updateDelay = (int) ConfigUtil.getTimeUnit(config, "change", null, "updateDelay", 300, TimeUnit.SECONDS); info.submitWholeTopic = toBoolean(MergeSuperSet.wholeTopicEnabled(config)); - info.excludeMergeableInChangeInfo = - toBoolean(this.config.getBoolean("change", "api", "excludeMergeableInChangeInfo", false)); info.disablePrivateChanges = toBoolean(this.config.getBoolean("change", null, "disablePrivateChanges", false)); + info.mergeabilityComputationBehavior = + MergeabilityComputationBehavior.fromConfig(config).name(); return info; } @@ -299,14 +297,6 @@ return info; } - private IndexConfigInfo getIndexInfo() { - ChangeIndexConfigInfo change = new ChangeIndexConfigInfo(); - change.indexMergeable = toBoolean(config.getBoolean("index", "change", "indexMergeable", true)); - IndexConfigInfo index = new IndexConfigInfo(); - index.change = change; - return index; - } - private String getDocUrl() { String docUrl = config.getString("gerrit", null, "docUrl"); if (Strings.isNullOrEmpty(docUrl)) {
diff --git a/java/com/google/gerrit/testing/IndexConfig.java b/java/com/google/gerrit/testing/IndexConfig.java index fb6c926..a8ed5be 100644 --- a/java/com/google/gerrit/testing/IndexConfig.java +++ b/java/com/google/gerrit/testing/IndexConfig.java
@@ -14,6 +14,7 @@ package com.google.gerrit.testing; +import com.google.gerrit.server.change.MergeabilityComputationBehavior; import org.eclipse.jgit.lib.Config; public class IndexConfig { @@ -23,9 +24,10 @@ public static Config createFromExistingConfig(Config cfg) { cfg.setInt("index", null, "maxPages", 10); - // To avoid this flakiness indexMergeable is switched off for the tests as it incurs background + // To avoid this flakiness indexing mergeable is disabled for the tests as it incurs background // reindex calls. - cfg.setBoolean("index", "change", "indexMergeable", false); + cfg.setEnum( + "change", null, "mergeabilityComputationBehavior", MergeabilityComputationBehavior.NEVER); cfg.setString("trackingid", "query-bug", "footer", "Bug:"); cfg.setString("trackingid", "query-bug", "match", "QUERY\\d{2,8}"); cfg.setString("trackingid", "query-bug", "system", "querytests");
diff --git a/javatests/com/google/gerrit/acceptance/api/change/AbandonIT.java b/javatests/com/google/gerrit/acceptance/api/change/AbandonIT.java index 7ecbe69..d04eebd 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/AbandonIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/AbandonIT.java
@@ -143,7 +143,9 @@ @UseClockStep @GerritConfig(name = "changeCleanup.abandonAfter", value = "1w") @GerritConfig(name = "changeCleanup.abandonIfMergeable", value = "false") - @GerritConfig(name = "index.change.indexMergeable", value = "true") + @GerritConfig( + name = "change.mergeabilityComputationBehavior", + value = "API_REF_UPDATED_AND_CHANGE_REINDEX") public void notAbandonedIfMergeableWhenMergeableOperatorIsEnabled() throws Exception { ObjectId initial = repo().exactRef(HEAD).getLeaf().getObjectId(); @@ -180,14 +182,14 @@ } /** - * When indexMergeable is disabled then the abandonIfMergeable option is ineffective and the auto - * abandon behaves as though it were set to its default value (true). + * When indexing mergeable is disabled then the abandonIfMergeable option is ineffective and the + * auto abandon behaves as though it were set to its default value (true). */ @Test @UseClockStep @GerritConfig(name = "changeCleanup.abandonAfter", value = "1w") @GerritConfig(name = "changeCleanup.abandonIfMergeable", value = "false") - @GerritConfig(name = "index.change.indexMergeable", value = "false") + @GerritConfig(name = "change.mergeabilityComputationBehavior", value = "NEVER") public void abandonedIfMergeableWhenMergeableOperatorIsDisabled() throws Exception { ObjectId initial = repo().exactRef(HEAD).getLeaf().getObjectId();
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java index 74f753d..d5686d1 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -295,19 +295,7 @@ } @Test - public void skipMergeable() throws Exception { - PushOneCommit.Result r = createChange(); - String triplet = project.get() + "~master~" + r.getChangeId(); - ChangeInfo c = - gApi.changes().id(triplet).get(ImmutableList.of(ListChangesOption.SKIP_MERGEABLE)); - assertThat(c.mergeable).isNull(); - - c = gApi.changes().id(triplet).get(); - assertThat(c.mergeable).isTrue(); - } - - @Test - @GerritConfig(name = "change.api.excludeMergeableInChangeInfo", value = "true") + @GerritConfig(name = "change.mergeabilityComputationBehavior", value = "NEVER") public void excludeMergeableInChangeInfo() throws Exception { PushOneCommit.Result r = createChange(); ChangeInfo c = gApi.changes().id(r.getChangeId()).get(); @@ -4425,7 +4413,6 @@ ListChangesOption.MESSAGES, ListChangesOption.SUBMITTABLE, ListChangesOption.WEB_LINKS, - ListChangesOption.SKIP_MERGEABLE, ListChangesOption.SKIP_DIFFSTAT); PushOneCommit.Result change = createChange(); @@ -4438,14 +4425,16 @@ } @Test - @GerritConfig(name = "index.change.indexMergeable", value = "true") + @GerritConfig( + name = "change.mergeabilityComputationBehavior", + value = "API_REF_UPDATED_AND_CHANGE_REINDEX") public void changeQueryReturnsMergeableWhenGerritIndexMergeable() throws Exception { String changeId = createChange().getChangeId(); assertThat(gApi.changes().query(changeId).get().get(0).mergeable).isTrue(); } @Test - @GerritConfig(name = "index.change.indexMergeable", value = "false") + @GerritConfig(name = "change.mergeabilityComputationBehavior", value = "NEVER") public void changeQueryDoesNotReturnMergeableWhenGerritDoesNotIndexMergeable() throws Exception { String changeId = createChange().getChangeId(); assertThat(gApi.changes().query(changeId).get().get(0).mergeable).isNull();
diff --git a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java index 524a05e..707076c 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
@@ -16,7 +16,10 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.gerrit.testing.GerritJUnit.assertThrows; +import static java.util.stream.Collectors.toList; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.argThat; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.clearInvocations; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -25,19 +28,23 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; +import com.google.common.truth.Correspondence; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.PushOneCommit; +import com.google.gerrit.acceptance.config.GerritConfig; import com.google.gerrit.extensions.annotations.Exports; import com.google.gerrit.extensions.api.changes.DraftInput; import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput; import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling; +import com.google.gerrit.extensions.api.changes.ReviewInput.RobotCommentInput; import com.google.gerrit.extensions.client.Side; import com.google.gerrit.extensions.common.ChangeMessageInfo; +import com.google.gerrit.extensions.common.RobotCommentInfo; import com.google.gerrit.extensions.config.FactoryModule; import com.google.gerrit.extensions.restapi.BadRequestException; +import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.validators.CommentForValidation; -import com.google.gerrit.extensions.validators.CommentForValidation.CommentType; import com.google.gerrit.extensions.validators.CommentValidationContext; import com.google.gerrit.extensions.validators.CommentValidator; import com.google.gerrit.server.restapi.change.PostReview; @@ -46,9 +53,12 @@ import com.google.inject.Inject; import com.google.inject.Module; import java.sql.Timestamp; +import java.util.Collection; +import java.util.List; import org.junit.Before; import org.junit.Test; import org.mockito.ArgumentCaptor; +import org.mockito.ArgumentMatcher; import org.mockito.Captor; /** Tests for comment validation in {@link PostReview}. */ @@ -57,8 +67,43 @@ @Inject private TestCommentHelper testCommentHelper; private static final String COMMENT_TEXT = "The comment text"; + private static final CommentForValidation FILE_COMMENT_FOR_VALIDATION = + CommentForValidation.create( + CommentForValidation.CommentSource.HUMAN, + CommentForValidation.CommentType.FILE_COMMENT, + COMMENT_TEXT, + COMMENT_TEXT.length()); + private static final CommentForValidation INLINE_COMMENT_FOR_VALIDATION = + CommentForValidation.create( + CommentForValidation.CommentSource.HUMAN, + CommentForValidation.CommentType.INLINE_COMMENT, + COMMENT_TEXT, + COMMENT_TEXT.length()); + private static final CommentForValidation CHANGE_MESSAGE_FOR_VALIDATION = + CommentForValidation.create( + CommentForValidation.CommentSource.HUMAN, + CommentForValidation.CommentType.CHANGE_MESSAGE, + COMMENT_TEXT, + COMMENT_TEXT.length()); - @Captor private ArgumentCaptor<ImmutableList<CommentForValidation>> capture; + @Captor private ArgumentCaptor<ImmutableList<CommentForValidation>> captor; + + private static class SingleCommentMatcher + implements ArgumentMatcher<ImmutableList<CommentForValidation>> { + final CommentForValidation left; + + SingleCommentMatcher(CommentForValidation left) { + this.left = left; + } + + @Override + public boolean matches(ImmutableList<CommentForValidation> rightList) { + CommentForValidation right = Iterables.getOnlyElement(rightList); + return left.getSource() == right.getSource() + && left.getType() == right.getType() + && left.getText().equals(right.getText()); + } + } @Override public Module createModule() { @@ -84,13 +129,7 @@ public void validateCommentsInInput_commentOK() throws Exception { PushOneCommit.Result r = createChange(); when(mockCommentValidator.validateComments( - CommentValidationContext.builder() - .changeId(r.getChange().getId().get()) - .project(r.getChange().project().get()) - .build(), - ImmutableList.of( - CommentForValidation.create( - CommentForValidation.CommentType.FILE_COMMENT, COMMENT_TEXT)))) + eq(contextFor(r)), argThat(new SingleCommentMatcher(FILE_COMMENT_FOR_VALIDATION)))) .thenReturn(ImmutableList.of()); ReviewInput input = new ReviewInput(); @@ -107,15 +146,9 @@ @Test public void validateCommentsInInput_commentRejected() throws Exception { PushOneCommit.Result r = createChange(); - CommentForValidation commentForValidation = - CommentForValidation.create(CommentType.FILE_COMMENT, COMMENT_TEXT); when(mockCommentValidator.validateComments( - CommentValidationContext.builder() - .changeId(r.getChange().getId().get()) - .project(r.getChange().project().get()) - .build(), - ImmutableList.of(CommentForValidation.create(CommentType.FILE_COMMENT, COMMENT_TEXT)))) - .thenReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!"))); + eq(contextFor(r)), argThat(new SingleCommentMatcher(FILE_COMMENT_FOR_VALIDATION)))) + .thenReturn(ImmutableList.of(FILE_COMMENT_FOR_VALIDATION.failValidation("Oh no!"))); ReviewInput input = new ReviewInput(); CommentInput comment = newComment(r.getChange().currentFilePaths().get(0)); @@ -161,19 +194,13 @@ public void validateDrafts_draftOK() throws Exception { PushOneCommit.Result r = createChange(); when(mockCommentValidator.validateComments( - CommentValidationContext.builder() - .changeId(r.getChange().getId().get()) - .project(r.getChange().project().get()) - .build(), - ImmutableList.of( - CommentForValidation.create( - CommentForValidation.CommentType.INLINE_COMMENT, COMMENT_TEXT)))) + eq(contextFor(r)), argThat(new SingleCommentMatcher((INLINE_COMMENT_FOR_VALIDATION))))) .thenReturn(ImmutableList.of()); DraftInput draft = testCommentHelper.newDraft( r.getChange().currentFilePaths().get(0), Side.REVISION, 1, COMMENT_TEXT); - gApi.changes().id(r.getChangeId()).revision(r.getCommit().getName()).createDraft(draft).get(); + gApi.changes().id(r.getChangeId()).revision(r.getCommit().getName()).createDraft(draft); assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).isEmpty(); ReviewInput input = new ReviewInput(); @@ -186,17 +213,9 @@ @Test public void validateDrafts_draftRejected() throws Exception { PushOneCommit.Result r = createChange(); - CommentForValidation commentForValidation = - CommentForValidation.create(CommentType.INLINE_COMMENT, COMMENT_TEXT); when(mockCommentValidator.validateComments( - CommentValidationContext.builder() - .changeId(r.getChange().getId().get()) - .project(r.getChange().project().get()) - .build(), - ImmutableList.of( - CommentForValidation.create( - CommentForValidation.CommentType.INLINE_COMMENT, COMMENT_TEXT)))) - .thenReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!"))); + eq(contextFor(r)), argThat(new SingleCommentMatcher(INLINE_COMMENT_FOR_VALIDATION)))) + .thenReturn(ImmutableList.of(INLINE_COMMENT_FOR_VALIDATION.failValidation("Oh no!"))); DraftInput draft = testCommentHelper.newDraft( @@ -233,7 +252,7 @@ testCommentHelper.addDraft(r.getChangeId(), r.getCommit().getName(), draftFile); assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).isEmpty(); - when(mockCommentValidator.validateComments(any(), capture.capture())) + when(mockCommentValidator.validateComments(any(), captor.capture())) .thenReturn(ImmutableList.of()); ReviewInput input = new ReviewInput(); @@ -241,25 +260,33 @@ gApi.changes().id(r.getChangeId()).current().review(input); assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).hasSize(2); - assertThat(capture.getAllValues()).hasSize(1); - assertThat(capture.getValue()) + assertThat(captor.getAllValues()).hasSize(1); + assertThat(captor.getValue()) + .comparingElementsUsing( + Correspondence.<CommentForValidation, CommentForValidation>from( + (a, b) -> + a.getSource() == b.getSource() + && a.getType() == b.getType() + && a.getText().equals(b.getText()), + "matches (ignoring size approximation)")) .containsExactly( CommentForValidation.create( - CommentForValidation.CommentType.INLINE_COMMENT, draftInline.message), + CommentForValidation.CommentSource.HUMAN, + CommentForValidation.CommentType.INLINE_COMMENT, + draftInline.message, + draftInline.message.length()), CommentForValidation.create( - CommentForValidation.CommentType.FILE_COMMENT, draftFile.message)); + CommentForValidation.CommentSource.HUMAN, + CommentForValidation.CommentType.FILE_COMMENT, + draftFile.message, + draftFile.message.length())); } @Test public void validateCommentsInChangeMessage_messageOK() throws Exception { PushOneCommit.Result r = createChange(); when(mockCommentValidator.validateComments( - CommentValidationContext.builder() - .changeId(r.getChange().getId().get()) - .project(r.getChange().project().get()) - .build(), - ImmutableList.of( - CommentForValidation.create(CommentType.CHANGE_MESSAGE, COMMENT_TEXT)))) + contextFor(r), ImmutableList.of(CHANGE_MESSAGE_FOR_VALIDATION))) .thenReturn(ImmutableList.of()); ReviewInput input = new ReviewInput().message(COMMENT_TEXT); @@ -274,16 +301,9 @@ @Test public void validateCommentsInChangeMessage_messageRejected() throws Exception { PushOneCommit.Result r = createChange(); - CommentForValidation commentForValidation = - CommentForValidation.create(CommentType.CHANGE_MESSAGE, COMMENT_TEXT); when(mockCommentValidator.validateComments( - CommentValidationContext.builder() - .changeId(r.getChange().getId().get()) - .project(r.getChange().project().get()) - .build(), - ImmutableList.of( - CommentForValidation.create(CommentType.CHANGE_MESSAGE, COMMENT_TEXT)))) - .thenReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!"))); + contextFor(r), ImmutableList.of(CHANGE_MESSAGE_FOR_VALIDATION))) + .thenReturn(ImmutableList.of(CHANGE_MESSAGE_FOR_VALIDATION.failValidation("Oh no!"))); ReviewInput input = new ReviewInput().message(COMMENT_TEXT); assertThat(gApi.changes().id(r.getChangeId()).get().messages) @@ -308,7 +328,56 @@ assertThat(message.message).doesNotContain(COMMENT_TEXT); } + @Test + @GerritConfig(name = "change.maxComments", value = "4") + public void restrictNumberOfComments() throws Exception { + when(mockCommentValidator.validateComments(any(), any())).thenReturn(ImmutableList.of()); + + PushOneCommit.Result r = createChange(); + String filePath = r.getChange().currentFilePaths().get(0); + CommentInput commentInput = new CommentInput(); + commentInput.line = 1; + commentInput.message = "foo"; + commentInput.path = filePath; + RobotCommentInput robotCommentInput = + TestCommentHelper.createRobotCommentInputWithMandatoryFields(filePath); + ReviewInput reviewInput = new ReviewInput(); + reviewInput.comments = ImmutableMap.of(filePath, ImmutableList.of(commentInput)); + reviewInput.robotComments = ImmutableMap.of(filePath, ImmutableList.of(robotCommentInput)); + gApi.changes().id(r.getChangeId()).current().review(reviewInput); + + // reviewInput still has both a user and a robot comment (and deduplication is false). We also + // create a draft so that in total there would be 5 comments. The limit is set to 4, so this + // verifies that all three channels are considered. + DraftInput draftInline = testCommentHelper.newDraft(filePath, Side.REVISION, 1, "a draft"); + testCommentHelper.addDraft(r.getChangeId(), r.getPatchSetId().getId(), draftInline); + reviewInput.drafts = DraftHandling.PUBLISH; + reviewInput.omitDuplicateComments = false; + + BadRequestException exception = + assertThrows( + BadRequestException.class, + () -> gApi.changes().id(r.getChangeId()).current().review(reviewInput)); + assertThat(exception) + .hasMessageThat() + .contains("Exceeding maximum number of comments: 2 (existing) + 3 (new) > 4"); + + assertThat(testCommentHelper.getPublishedComments(r.getChangeId())).hasSize(1); + assertThat(getRobotComments(r.getChangeId())).hasSize(1); + } + + private List<RobotCommentInfo> getRobotComments(String changeId) throws RestApiException { + return gApi.changes().id(changeId).robotComments().values().stream() + .flatMap(Collection::stream) + .collect(toList()); + } + private static CommentInput newComment(String path) { return TestCommentHelper.populate(new CommentInput(), path, PostReviewIT.COMMENT_TEXT); } + + private static CommentValidationContext contextFor(PushOneCommit.Result result) { + return CommentValidationContext.create( + result.getChange().getId().get(), result.getChange().project().get()); + } }
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java index e6b2190..36b7265 100644 --- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java +++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
@@ -1102,7 +1102,9 @@ } @Test - @GerritConfig(name = "index.change.indexMergeable", value = "true") + @GerritConfig( + name = "change.mergeabilityComputationBehavior", + value = "API_REF_UPDATED_AND_CHANGE_REINDEX") public void mergeable() throws Exception { ObjectId initial = repo().exactRef(HEAD).getLeaf().getObjectId();
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java index c35ded6..6b92c16 100644 --- a/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java +++ b/javatests/com/google/gerrit/acceptance/api/revision/RobotCommentsIT.java
@@ -173,9 +173,8 @@ @Test public void hugeRobotCommentIsRejected() { - int defaultSizeLimit = 1024 * 1024; - int sizeOfRest = 451; - fixReplacementInfo.replacement = getStringFor(defaultSizeLimit - sizeOfRest + 1); + int defaultSizeLimit = 1 << 20; + fixReplacementInfo.replacement = getStringFor(defaultSizeLimit + 1); BadRequestException thrown = assertThrows( @@ -186,9 +185,9 @@ @Test public void reasonablyLargeRobotCommentIsAccepted() throws Exception { - int defaultSizeLimit = 1024 * 1024; - int sizeOfRest = 451; - fixReplacementInfo.replacement = getStringFor(defaultSizeLimit - sizeOfRest); + int defaultSizeLimit = 1 << 20; + // Allow for a few hundred bytes in other fields. + fixReplacementInfo.replacement = getStringFor(defaultSizeLimit - 666); testCommentHelper.addRobotComment(changeId, withFixRobotCommentInput); @@ -199,7 +198,7 @@ @Test @GerritConfig(name = "change.robotCommentSizeLimit", value = "10k") public void maximumAllowedSizeOfRobotCommentCanBeAdjusted() { - int sizeLimit = 10 * 1024; + int sizeLimit = 10 << 20; fixReplacementInfo.replacement = getStringFor(sizeLimit); BadRequestException thrown = @@ -212,8 +211,8 @@ @Test @GerritConfig(name = "change.robotCommentSizeLimit", value = "0") public void zeroForMaximumAllowedSizeOfRobotCommentRemovesRestriction() throws Exception { - int defaultSizeLimit = 1024 * 1024; - fixReplacementInfo.replacement = getStringFor(defaultSizeLimit); + int defaultSizeLimit = 1 << 20; + fixReplacementInfo.replacement = getStringFor(2 * defaultSizeLimit); testCommentHelper.addRobotComment(changeId, withFixRobotCommentInput); @@ -225,8 +224,8 @@ @GerritConfig(name = "change.robotCommentSizeLimit", value = "-1") public void negativeValueForMaximumAllowedSizeOfRobotCommentRemovesRestriction() throws Exception { - int defaultSizeLimit = 1024 * 1024; - fixReplacementInfo.replacement = getStringFor(defaultSizeLimit); + int defaultSizeLimit = 1 << 20; + fixReplacementInfo.replacement = getStringFor(2 * defaultSizeLimit); testCommentHelper.addRobotComment(changeId, withFixRobotCommentInput);
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java index 0c0a8ed..cdffa3c 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
@@ -1205,7 +1205,9 @@ } @Test - @GerritConfig(name = "index.change.indexMergeable", value = "true") + @GerritConfig( + name = "change.mergeabilityComputationBehavior", + value = "API_REF_UPDATED_AND_CHANGE_REINDEX") public void submitSchedulesOpenChangesOfSameBranchForReindexing() throws Throwable { // Create a merged change. PushOneCommit push =
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/IndexChangeIT.java b/javatests/com/google/gerrit/acceptance/rest/change/IndexChangeIT.java index ea3a6a0..61dc4d4 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/IndexChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/IndexChangeIT.java
@@ -59,7 +59,9 @@ } @Test - @GerritConfig(name = "index.change.indexMergeable", value = "true") + @GerritConfig( + name = "change.mergeabilityComputationBehavior", + value = "API_REF_UPDATED_AND_CHANGE_REINDEX") public void indexChangeAfterOwnerLosesVisibility() throws Exception { // Create a test group with 2 users as members TestAccount user2 = accountCreator.user2();
diff --git a/javatests/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java b/javatests/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java index 9573eb0..14a04cd 100644 --- a/javatests/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java
@@ -169,7 +169,8 @@ assertThat(i.change.updateDelay).isEqualTo(300); assertThat(i.change.disablePrivateChanges).isNull(); assertThat(i.change.submitWholeTopic).isNull(); - assertThat(i.change.excludeMergeableInChangeInfo).isNull(); + assertThat(i.change.mergeabilityComputationBehavior) + .isEqualTo("API_REF_UPDATED_AND_CHANGE_REINDEX"); // download assertThat(i.download.archives).containsExactly("tar", "tbz2", "tgz", "txz"); @@ -180,9 +181,6 @@ assertThat(i.gerrit.allUsers).isEqualTo(AllUsersNameProvider.DEFAULT); assertThat(i.gerrit.reportBugUrl).isNull(); - // index - assertThat(i.index.change.indexMergeable).isNull(); // false in all tests - // plugin assertThat(i.plugin.jsResourcePaths).isEmpty(); @@ -204,16 +202,9 @@ } @Test - @GerritConfig(name = "change.api.excludeMergeableInChangeInfo", value = "true") - public void serverConfigWithExcludeMergeableInChangeInfo() throws Exception { + @GerritConfig(name = "change.mergeabilityComputationBehavior", value = "NEVER") + public void mergeabilityComputationBehavior_neverCompute() throws Exception { ServerInfo i = gApi.config().server().getInfo(); - assertThat(i.change.excludeMergeableInChangeInfo).isTrue(); - } - - @Test - @GerritConfig(name = "index.change.indexMergeable", value = "true") - public void indexMergeableIsTrueWhenTrueInConfig() throws Exception { - ServerInfo i = gApi.config().server().getInfo(); - assertThat(i.index.change.indexMergeable).isTrue(); + assertThat(i.change.mergeabilityComputationBehavior).isEqualTo("NEVER"); } }
diff --git a/javatests/com/google/gerrit/acceptance/server/event/EventPayloadIT.java b/javatests/com/google/gerrit/acceptance/server/event/EventPayloadIT.java index d973934..3bc863f 100644 --- a/javatests/com/google/gerrit/acceptance/server/event/EventPayloadIT.java +++ b/javatests/com/google/gerrit/acceptance/server/event/EventPayloadIT.java
@@ -32,32 +32,26 @@ @Test public void defaultOptions() throws Exception { RevisionCreatedListener listener = - new RevisionCreatedListener() { - @Override - public void onRevisionCreated(Event event) { - assertThat(event.getChange().submittable).isNotNull(); - assertThat(event.getRevision().files).isNotEmpty(); - } + event -> { + assertThat(event.getChange().submittable).isNotNull(); + assertThat(event.getRevision().files).isNotEmpty(); }; - try (Registration registration = extensionRegistry.newRegistration().add(listener)) { + try (Registration ignored = extensionRegistry.newRegistration().add(listener)) { createChange(); } } @Test - @GerritConfig(name = "event.payload.listChangeOptions", value = "SKIP_MERGEABLE") + @GerritConfig(name = "event.payload.listChangeOptions", value = "SKIP_DIFFSTAT") public void configuredOptions() throws Exception { RevisionCreatedListener listener = - new RevisionCreatedListener() { - @Override - public void onRevisionCreated(Event event) { - assertThat(event.getChange().submittable).isNull(); - assertThat(event.getChange().mergeable).isNull(); - assertThat(event.getRevision().files).isNull(); - assertThat(event.getChange().subject).isNotEmpty(); - } + event -> { + assertThat(event.getChange().submittable).isNull(); + assertThat(event.getChange().insertions).isNull(); + assertThat(event.getRevision().files).isNull(); + assertThat(event.getChange().subject).isNotEmpty(); }; - try (Registration registration = extensionRegistry.newRegistration().add(listener)) { + try (Registration ignored = extensionRegistry.newRegistration().add(listener)) { createChange(); } }
diff --git a/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java b/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java index ccfe783..eb1f907 100644 --- a/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java +++ b/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsCommentValidationIT.java
@@ -31,7 +31,6 @@ import com.google.gerrit.extensions.client.Side; import com.google.gerrit.extensions.config.FactoryModule; import com.google.gerrit.extensions.validators.CommentForValidation; -import com.google.gerrit.extensions.validators.CommentForValidation.CommentType; import com.google.gerrit.extensions.validators.CommentValidationContext; import com.google.gerrit.extensions.validators.CommentValidator; import com.google.gerrit.testing.TestCommentHelper; @@ -49,9 +48,15 @@ @Inject private CommentValidator mockCommentValidator; @Inject private TestCommentHelper testCommentHelper; - private static final int MAX_COMMENT_LENGTH = 666; + private static final int COMMENT_SIZE_LIMIT = 666; private static final String COMMENT_TEXT = "The comment text"; + private static final CommentForValidation COMMENT_FOR_VALIDATION = + CommentForValidation.create( + CommentForValidation.CommentSource.HUMAN, + CommentForValidation.CommentType.FILE_COMMENT, + COMMENT_TEXT, + COMMENT_TEXT.length()); @Captor private ArgumentCaptor<ImmutableList<CommentForValidation>> capture; @Captor private ArgumentCaptor<CommentValidationContext> captureCtx; @@ -82,13 +87,9 @@ String changeId = result.getChangeId(); String revId = result.getCommit().getName(); when(mockCommentValidator.validateComments( - CommentValidationContext.builder() - .changeId(result.getChange().getId().get()) - .project(result.getChange().project().get()) - .build(), - ImmutableList.of( - CommentForValidation.create( - CommentForValidation.CommentType.FILE_COMMENT, COMMENT_TEXT)))) + CommentValidationContext.create( + result.getChange().getId().get(), result.getChange().project().get()), + ImmutableList.of(COMMENT_FOR_VALIDATION))) .thenReturn(ImmutableList.of()); DraftInput comment = testCommentHelper.newDraft(COMMENT_TEXT); testCommentHelper.addDraft(changeId, revId, comment); @@ -101,20 +102,14 @@ @Test public void validateComments_commentRejected() throws Exception { - CommentForValidation commentForValidation = - CommentForValidation.create(CommentType.FILE_COMMENT, COMMENT_TEXT); PushOneCommit.Result result = createChange(); String changeId = result.getChangeId(); String revId = result.getCommit().getName(); when(mockCommentValidator.validateComments( - CommentValidationContext.builder() - .changeId(result.getChange().getId().get()) - .project(result.getChange().project().get()) - .build(), - ImmutableList.of( - CommentForValidation.create( - CommentForValidation.CommentType.FILE_COMMENT, COMMENT_TEXT)))) - .thenReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!"))); + CommentValidationContext.create( + result.getChange().getId().get(), result.getChange().project().get()), + ImmutableList.of(COMMENT_FOR_VALIDATION))) + .thenReturn(ImmutableList.of(COMMENT_FOR_VALIDATION.failValidation("Oh no!"))); DraftInput comment = testCommentHelper.newDraft(COMMENT_TEXT); testCommentHelper.addDraft(changeId, revId, comment); assertThat(testCommentHelper.getPublishedComments(result.getChangeId())).isEmpty(); @@ -149,18 +144,24 @@ assertThat(capture.getAllValues().get(0)) .containsExactly( CommentForValidation.create( - CommentForValidation.CommentType.INLINE_COMMENT, draftInline.message), + CommentForValidation.CommentSource.HUMAN, + CommentForValidation.CommentType.INLINE_COMMENT, + draftInline.message, + draftInline.message.length()), CommentForValidation.create( - CommentForValidation.CommentType.FILE_COMMENT, draftFile.message)); + CommentForValidation.CommentSource.HUMAN, + CommentForValidation.CommentType.FILE_COMMENT, + draftFile.message, + draftFile.message.length())); } @Test - @GerritConfig(name = "change.maxCommentLength", value = "" + MAX_COMMENT_LENGTH) + @GerritConfig(name = "change.commentSizeLimit", value = "" + COMMENT_SIZE_LIMIT) public void validateComments_enforceLimits_commentTooLarge() throws Exception { when(mockCommentValidator.validateComments(any(), any())).thenReturn(ImmutableList.of()); PushOneCommit.Result result = createChange(); String changeId = result.getChangeId(); - int commentLength = MAX_COMMENT_LENGTH + 1; + int commentLength = COMMENT_SIZE_LIMIT + 1; DraftInput comment = testCommentHelper.newDraft(new String(new char[commentLength]).replace("\0", "x")); testCommentHelper.addDraft(changeId, result.getCommit().getName(), comment); @@ -169,7 +170,33 @@ Result amendResult = amendChange(changeId, "refs/for/master%publish-comments", admin, testRepo); amendResult.assertOkStatus(); amendResult.assertMessage( - String.format("Comment too large (%d > %d)", commentLength, MAX_COMMENT_LENGTH)); + String.format("Comment size exceeds limit (%d > %d)", commentLength, COMMENT_SIZE_LIMIT)); assertThat(testCommentHelper.getPublishedComments(result.getChangeId())).isEmpty(); } + + @Test + @GerritConfig(name = "change.maxComments", value = "3") + public void countComments_limitNumberOfComments() throws Exception { + when(mockCommentValidator.validateComments(any(), any())).thenReturn(ImmutableList.of()); + PushOneCommit.Result result = createChange(); + String changeId = result.getChangeId(); + String revId = result.getCommit().getName(); + String filePath = result.getChange().currentFilePaths().get(0); + DraftInput draftInline = testCommentHelper.newDraft(filePath, Side.REVISION, 1, COMMENT_TEXT); + testCommentHelper.addDraft(changeId, revId, draftInline); + amendChange(changeId, "refs/for/master%publish-comments", admin, testRepo); + assertThat(testCommentHelper.getPublishedComments(result.getChangeId())).hasSize(1); + + for (int i = 1; i < 3; ++i) { + testCommentHelper.addRobotComment( + changeId, + TestCommentHelper.createRobotCommentInput(result.getChange().currentFilePaths().get(0))); + } + + draftInline = testCommentHelper.newDraft(filePath, Side.REVISION, 1, COMMENT_TEXT); + testCommentHelper.addDraft(changeId, revId, draftInline); + Result amendResult = amendChange(changeId, "refs/for/master%publish-comments", admin, testRepo); + assertThat(testCommentHelper.getPublishedComments(result.getChangeId())).hasSize(1); + amendResult.assertMessage("exceeding maximum number of comments"); + } }
diff --git a/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsLimitsIT.java b/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsLimitsIT.java new file mode 100644 index 0000000..c32827a --- /dev/null +++ b/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsLimitsIT.java
@@ -0,0 +1,67 @@ +// Copyright (C) 2020 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.git.receive; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.cache.Cache; +import com.google.common.collect.ImmutableMap; +import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.PushOneCommit; +import com.google.gerrit.acceptance.config.GerritConfig; +import com.google.gerrit.server.patch.DiffSummary; +import com.google.gerrit.server.patch.DiffSummaryKey; +import com.google.inject.Inject; +import com.google.inject.name.Named; +import org.junit.Test; + +/** Tests for applying limits to e.g. number of files per change. */ +public class ReceiveCommitsLimitsIT extends AbstractDaemonTest { + + @Inject + private @Named("diff_summary") Cache<DiffSummaryKey, DiffSummary> diffSummaryCache; + + @Test + @GerritConfig(name = "change.maxFiles", value = "1") + public void limitFileCount() throws Exception { + PushOneCommit.Result result = + pushFactory + .create( + admin.newIdent(), + testRepo, + "foo", + ImmutableMap.of("foo", "foo-1.0", "bar", "bar-1.0")) + .to("refs/for/master"); + result.assertErrorStatus("Exceeding maximum number of files per change (2 > 1)"); + } + + @Test + public void cacheKeyMatches() throws Exception { + int cacheSizeBefore = diffSummaryCache.asMap().size(); + PushOneCommit.Result result = + pushFactory + .create( + admin.newIdent(), + testRepo, + "foo", + ImmutableMap.of("foo", "foo-1.0", "bar", "bar-1.0")) + .to("refs/for/master"); + result.assertOkStatus(); + + // Assert that we only performed the diff computation once. This would e.g. catch + // bugs/deviations in the computation of the cache key. + assertThat(diffSummaryCache.asMap()).hasSize(cacheSizeBefore + 1); + } +}
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/AbstractMailIT.java b/javatests/com/google/gerrit/acceptance/server/mail/AbstractMailIT.java index 768c269..3b0e27b 100644 --- a/javatests/com/google/gerrit/acceptance/server/mail/AbstractMailIT.java +++ b/javatests/com/google/gerrit/acceptance/server/mail/AbstractMailIT.java
@@ -33,7 +33,9 @@ public class AbstractMailIT extends AbstractDaemonTest { @Inject private RequestScopeOperations requestScopeOperations; - protected MailMessage.Builder messageBuilderWithDefaultFields() { + static final String FILE_NAME = "gerrit-server/test.txt"; + + MailMessage.Builder messageBuilderWithDefaultFields() { MailMessage.Builder b = MailMessage.builder(); b.id("some id"); b.from(user.getEmailAddress()); @@ -43,16 +45,15 @@ return b; } - protected String createChangeWithReview() throws Exception { + String createChangeWithReview() throws Exception { return createChangeWithReview(admin); } - protected String createChangeWithReview(TestAccount reviewer) throws Exception { + String createChangeWithReview(TestAccount reviewer) throws Exception { // Create change - String file = "gerrit-server/test.txt"; String contents = "contents \nlorem \nipsum \nlorem"; PushOneCommit push = - pushFactory.create(admin.newIdent(), testRepo, "first subject", file, contents); + pushFactory.create(admin.newIdent(), testRepo, "first subject", FILE_NAME, contents); PushOneCommit.Result r = push.to("refs/for/master"); String changeId = r.getChangeId(); @@ -61,8 +62,8 @@ ReviewInput input = new ReviewInput(); input.message = "I have two comments"; input.comments = new HashMap<>(); - CommentInput c1 = newComment(file, Side.REVISION, 0, "comment on file"); - CommentInput c2 = newComment(file, Side.REVISION, 2, "inline comment"); + CommentInput c1 = newComment(FILE_NAME, Side.REVISION, 0, "comment on file"); + CommentInput c2 = newComment(FILE_NAME, Side.REVISION, 2, "inline comment"); input.comments.put(c1.path, ImmutableList.of(c1, c2)); revision(r).review(input); return changeId; @@ -94,7 +95,7 @@ * @param fc1 Comment in reply to a comment of file 1. * @return A string with all inline comments and the original quoted email. */ - protected static String newPlaintextBody( + static String newPlaintextBody( String changeURL, String changeMessage, String c1, String f1, String fc1) { return (changeMessage == null ? "" : changeMessage + "\n") + "> Foo Bar has posted comments on this change. ( \n" @@ -137,7 +138,7 @@ + "> \n"; } - protected static String textFooterForChange(int changeNumber, String timestamp) { + static String textFooterForChange(int changeNumber, String timestamp) { return "Gerrit-Change-Number: " + changeNumber + "\n"
diff --git a/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java b/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java index 2409f52..502194a 100644 --- a/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java +++ b/javatests/com/google/gerrit/acceptance/server/mail/MailProcessorIT.java
@@ -21,9 +21,14 @@ import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; +import com.google.gerrit.acceptance.config.GerritConfig; import com.google.gerrit.acceptance.testsuite.account.AccountOperations; import com.google.gerrit.extensions.annotations.Exports; +import com.google.gerrit.extensions.api.changes.ReviewInput; +import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput; +import com.google.gerrit.extensions.api.changes.ReviewInput.RobotCommentInput; import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.common.ChangeMessageInfo; import com.google.gerrit.extensions.common.CommentInfo; @@ -341,17 +346,53 @@ assertThat(message.body()).contains("rejected one or more comments"); } + @Test + @GerritConfig(name = "change.maxComments", value = "4") + public void limitNumberOfComments() throws Exception { + String changeId = createChangeWithReview(); + CommentInput commentInput = new CommentInput(); + commentInput.line = 1; + commentInput.message = "foo"; + commentInput.path = FILE_NAME; + RobotCommentInput robotCommentInput = + TestCommentHelper.createRobotCommentInputWithMandatoryFields(FILE_NAME); + ReviewInput reviewInput = new ReviewInput(); + reviewInput.comments = ImmutableMap.of(FILE_NAME, ImmutableList.of(commentInput)); + reviewInput.robotComments = ImmutableMap.of(FILE_NAME, ImmutableList.of(robotCommentInput)); + gApi.changes().id(changeId).current().review(reviewInput); + + ChangeInfo changeInfo = gApi.changes().id(changeId).get(); + List<CommentInfo> comments = gApi.changes().id(changeId).current().commentsAsList(); + String ts = + MailProcessingUtil.rfcDateformatter.format( + ZonedDateTime.ofInstant(comments.get(0).updated.toInstant(), ZoneId.of("UTC"))); + + MailMessage.Builder b = messageBuilderWithDefaultFields(); + String txt = newPlaintextBody(getChangeUrl(changeInfo) + "/1", null, null, COMMENT_TEXT, null); + b.textContent(txt + textFooterForChange(changeInfo._number, ts)); + + Collection<CommentInfo> commentsBefore = testCommentHelper.getPublishedComments(changeId); + mailProcessor.process(b.build()); + assertThat(testCommentHelper.getPublishedComments(changeId)).isEqualTo(commentsBefore); + + assertNotifyTo(user); + Message message = sender.nextMessage(); + assertThat(message.body()).contains("rejected one or more comments"); + } + private String getChangeUrl(ChangeInfo changeInfo) { return canonicalWebUrl.get() + "c/" + changeInfo.project + "/+/" + changeInfo._number; } private void setupFailValidation( CommentForValidation.CommentType type, String failProject, int failChange) { - CommentForValidation commentForValidation = CommentForValidation.create(type, COMMENT_TEXT); + CommentForValidation commentForValidation = + CommentForValidation.create( + CommentForValidation.CommentSource.HUMAN, type, COMMENT_TEXT, COMMENT_TEXT.length()); when(mockCommentValidator.validateComments( - CommentValidationContext.builder().changeId(failChange).project(failProject).build(), - ImmutableList.of(CommentForValidation.create(type, COMMENT_TEXT)))) + CommentValidationContext.create(failChange, failProject), + ImmutableList.of(commentForValidation))) .thenReturn(ImmutableList.of(commentForValidation.failValidation("Oh no!"))); } }
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java index 9c12052..38e4ca4 100644 --- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java +++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -2025,7 +2025,9 @@ } @Test - @GerritConfig(name = "index.change.indexMergeable", value = "true") + @GerritConfig( + name = "change.mergeabilityComputationBehavior", + value = "API_REF_UPDATED_AND_CHANGE_REINDEX") public void mergeable() throws Exception { TestRepository<Repo> repo = createProject("repo"); RevCommit commit1 = repo.parseBody(repo.commit().add("file1", "contents1").create()); @@ -2043,7 +2045,7 @@ // If a change gets submitted, the remaining open changes get reindexed asynchronously to update // their mergeability information. If the further assertions in this test are done before the // asynchronous reindex completed they fail because the mergeability information in the index - // was not updated yet. To avoid this flakiness indexMergeable is switched off for the + // was not updated yet. To avoid this flakiness indexing mergeable is switched off for the // tests and we index change2 synchronously here. gApi.changes().id(change2.getChangeId()).index(); @@ -3083,7 +3085,7 @@ } @Test - @GerritConfig(name = "index.change.indexMergeable", value = "false") + @GerritConfig(name = "change.mergeabilityComputationBehavior", value = "NEVER") public void mergeableFailsWhenNotIndexed() throws Exception { TestRepository<Repo> repo = createProject("repo"); RevCommit commit1 = repo.parseBody(repo.commit().add("file1", "contents1").create());
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js index 472a7ea..4aefe46 100644 --- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js +++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
@@ -1001,7 +1001,6 @@ this.ListChangesOption.MESSAGES, this.ListChangesOption.SUBMITTABLE, this.ListChangesOption.WEB_LINKS, - this.ListChangesOption.SKIP_MERGEABLE, this.ListChangesOption.SKIP_DIFFSTAT, ]; return this.getConfig(false).then(config => { @@ -1024,7 +1023,6 @@ const optionsHex = this.listChangesOptionsToHex( this.ListChangesOption.ALL_COMMITS, this.ListChangesOption.ALL_REVISIONS, - this.ListChangesOption.SKIP_MERGEABLE, this.ListChangesOption.SKIP_DIFFSTAT ); return this._getChangeDetail(changeNum, optionsHex, opt_errFn,
diff --git a/resources/com/google/gerrit/server/mail/InboundEmailRejection.soy b/resources/com/google/gerrit/server/mail/InboundEmailRejection.soy index e88c424..16c5c8d 100644 --- a/resources/com/google/gerrit/server/mail/InboundEmailRejection.soy +++ b/resources/com/google/gerrit/server/mail/InboundEmailRejection.soy
@@ -64,6 +64,7 @@ {/template} {template .InboundEmailRejection_COMMENT_REJECTED kind="text"} - Gerrit Code Review rejected one or more comments because they did not pass validation. + Gerrit Code Review rejected one or more comments because they did not pass validation, or + because the maximum number of comments per change would be exceeded. {call .InboundEmailRejectionFooter /} {/template}
diff --git a/resources/com/google/gerrit/server/mail/InboundEmailRejectionHtml.soy b/resources/com/google/gerrit/server/mail/InboundEmailRejectionHtml.soy index e17508d..8762e10 100644 --- a/resources/com/google/gerrit/server/mail/InboundEmailRejectionHtml.soy +++ b/resources/com/google/gerrit/server/mail/InboundEmailRejectionHtml.soy
@@ -81,7 +81,8 @@ {template .InboundEmailRejectionHtml_COMMENT_REJECTED} <p> - Gerrit Code Review rejected one or more comments because they did not pass validation. + Gerrit Code Review rejected one or more comments because they did not pass validation, or + because the maximum number of comments per change would be exceeded. </p> {call .InboundEmailRejectionFooterHtml /} {/template}