diff options
author | Diego Zambelli Sessona <diego.sessona@gmail.com> | 2023-11-16 13:49:32 +0100 |
---|---|---|
committer | Diego Zambelli Sessona <diego.sessona@gmail.com> | 2023-11-16 13:50:57 +0100 |
commit | dc5ffa8c22b5483cc7b9c483331c60dab27eea49 (patch) | |
tree | a1326808c5f1b025bbdfe6555d30e7b0e974c3ed | |
parent | 44eb389915a391a4de38b29ec09802ca5cdabdb5 (diff) | |
parent | fbc8e8f0e73fd0aa172a5adaf0a68f3d21fc4b79 (diff) |
Merge branch 'stable-3.6' into stable-3.7
* stable-3.6:
Make gr-comment-thread test reliable in stable-3.6
Bazel: Remove left over --java_toolchain option
Optionally skip submit rules evaluation on closed changes
Optimise the way that `SubmitRuleEvaluator.evaluate` loads objects
SubmitRuleEvaluator: Simply logic that checks that change and project exist
Update Jetty to 9.4.53.v20231009 for security updates
Bump JGit to v5.13.2.202306221912-r (5aa8a7e27)
Release-Notes: skip
Change-Id: I7eea63100cf461b7889fbeced0afee7dc8688f56
12 files changed, 198 insertions, 22 deletions
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index fd4a4cc1ef..72519daf3e 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -1662,6 +1662,21 @@ If 0 the update polling is disabled. + Default is 5 minutes. +[[change.skipCurrentRulesEvaluationOnClosedChanges]] ++ +If false, Gerrit will always take latest project configuration to +compute submit labels. This means that, closed changes (either merged +or abandoned) will be evaluated against the latest configuration which +may produce different results. Especially for merged changes, they may +look like they didn't meet the submit requirements. ++ +When true, evaluation will be skipped and Gerrit will show the +exact status of submit labels when change was submitted. Post-review +votes will only be allowed on labels that were configured when change +was closed. ++ +Default it false. + [[changeCleanup]] === Section changeCleanup diff --git a/Documentation/dev-bazel.txt b/Documentation/dev-bazel.txt index d1a5bcf23f..08529dfcba 100644 --- a/Documentation/dev-bazel.txt +++ b/Documentation/dev-bazel.txt @@ -54,7 +54,7 @@ To check the installed version of Java, open a terminal window and run: To build Gerrit with Java 11 language level, run: ``` - $ bazel build --java_toolchain=//tools:error_prone_warnings_toolchain_java11 :release + $ bazel build :release ``` [[java-17]] diff --git a/java/com/google/gerrit/pgm/util/BatchProgramModule.java b/java/com/google/gerrit/pgm/util/BatchProgramModule.java index cfd2ba6e0f..f58387e367 100644 --- a/java/com/google/gerrit/pgm/util/BatchProgramModule.java +++ b/java/com/google/gerrit/pgm/util/BatchProgramModule.java @@ -61,6 +61,7 @@ import com.google.gerrit.server.config.EnablePeerIPInReflogRecordProvider; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.GitReceivePackGroups; import com.google.gerrit.server.config.GitUploadPackGroups; +import com.google.gerrit.server.config.SkipCurrentRulesEvaluationOnClosedChangesModule; import com.google.gerrit.server.config.SysExecutorModule; import com.google.gerrit.server.extensions.events.AttentionSetObserver; import com.google.gerrit.server.extensions.events.EventUtil; @@ -208,6 +209,7 @@ public class BatchProgramModule extends FactoryModule { modules.add(new PrologModule(getConfig())); modules.add(new DefaultSubmitRuleModule()); modules.add(new IgnoreSelfApprovalRuleModule()); + modules.add(new SkipCurrentRulesEvaluationOnClosedChangesModule()); // Global submit requirements DynamicSet.setOf(binder(), SubmitRequirement.class); diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java index e5b063be8a..13024a207e 100644 --- a/java/com/google/gerrit/server/config/GerritGlobalModule.java +++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java @@ -293,6 +293,7 @@ public class GerritGlobalModule extends FactoryModule { install(ThreadLocalRequestContext.module()); install(new ApprovalModule()); install(new MailSoySauceModule()); + install(new SkipCurrentRulesEvaluationOnClosedChangesModule()); factory(CapabilityCollection.Factory.class); factory(ChangeData.AssistedFactory.class); diff --git a/java/com/google/gerrit/server/config/SkipCurrentRulesEvaluationOnClosedChanges.java b/java/com/google/gerrit/server/config/SkipCurrentRulesEvaluationOnClosedChanges.java new file mode 100644 index 0000000000..b812710ca1 --- /dev/null +++ b/java/com/google/gerrit/server/config/SkipCurrentRulesEvaluationOnClosedChanges.java @@ -0,0 +1,25 @@ +// Copyright (C) 2023 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.config; + +import static java.lang.annotation.RetentionPolicy.RUNTIME; + +import com.google.inject.BindingAnnotation; +import java.lang.annotation.Retention; + +/** Marker on a {@link Boolean} holding the evaluation of current Prolog rules on closed changes. */ +@Retention(RUNTIME) +@BindingAnnotation +public @interface SkipCurrentRulesEvaluationOnClosedChanges {} diff --git a/java/com/google/gerrit/server/config/SkipCurrentRulesEvaluationOnClosedChangesModule.java b/java/com/google/gerrit/server/config/SkipCurrentRulesEvaluationOnClosedChangesModule.java new file mode 100644 index 0000000000..9eaae021e0 --- /dev/null +++ b/java/com/google/gerrit/server/config/SkipCurrentRulesEvaluationOnClosedChangesModule.java @@ -0,0 +1,27 @@ +// Copyright (C) 2023 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.config; + +import com.google.inject.AbstractModule; + +public class SkipCurrentRulesEvaluationOnClosedChangesModule extends AbstractModule { + + @Override + protected void configure() { + bind(Boolean.class) + .annotatedWith(SkipCurrentRulesEvaluationOnClosedChanges.class) + .toProvider(SkipCurrentRulesEvaluationOnClosedChangesProvider.class); + } +} diff --git a/java/com/google/gerrit/server/config/SkipCurrentRulesEvaluationOnClosedChangesProvider.java b/java/com/google/gerrit/server/config/SkipCurrentRulesEvaluationOnClosedChangesProvider.java new file mode 100644 index 0000000000..bc25a3353f --- /dev/null +++ b/java/com/google/gerrit/server/config/SkipCurrentRulesEvaluationOnClosedChangesProvider.java @@ -0,0 +1,35 @@ +// Copyright (C) 2023 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.config; + +import com.google.inject.Inject; +import com.google.inject.Provider; +import com.google.inject.Singleton; +import org.eclipse.jgit.lib.Config; + +@Singleton +public class SkipCurrentRulesEvaluationOnClosedChangesProvider implements Provider<Boolean> { + private final Boolean value; + + @Inject + SkipCurrentRulesEvaluationOnClosedChangesProvider(@GerritServerConfig Config config) { + value = config.getBoolean("change", null, "skipCurrentRulesEvaluationOnClosedChanges", false); + } + + @Override + public Boolean get() { + return value; + } +} diff --git a/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java b/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java index 513303bc1a..c5928f6d7a 100644 --- a/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java +++ b/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java @@ -18,7 +18,6 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import com.google.common.collect.Streams; import com.google.common.flogger.FluentLogger; -import com.google.gerrit.entities.Change; import com.google.gerrit.entities.Project; import com.google.gerrit.entities.SubmitRecord; import com.google.gerrit.entities.SubmitTypeRecord; @@ -118,25 +117,12 @@ public class SubmitRuleEvaluator { "Evaluate submit rules for change %d (caller: %s)", cd.change().getId().get(), callerFinder.findCallerLazy()); try (Timer0.Context ignored = metrics.submitRuleEvaluationLatency.start()) { - Change change; - ProjectState projectState; - try { - change = cd.change(); - if (change == null) { - throw new StorageException("Change not found"); - } - - Project.NameKey name = cd.project(); - Optional<ProjectState> projectStateOptional = projectCache.get(name); - if (!projectStateOptional.isPresent()) { - throw new NoSuchProjectException(name); - } - projectState = projectStateOptional.get(); - } catch (NoSuchProjectException e) { - throw new IllegalStateException("Unable to find project while evaluating submit rule", e); + if (cd.change() == null) { + throw new StorageException("Change not found"); } - if (change.isClosed() && (!opts.recomputeOnClosedChanges() || OnlineReindexMode.isActive())) { + if (cd.change().isClosed() + && (!opts.recomputeOnClosedChanges() || OnlineReindexMode.isActive())) { return cd.notes().getSubmitRecords().stream() .map( r -> { @@ -150,6 +136,15 @@ public class SubmitRuleEvaluator { .collect(toImmutableList()); } + ProjectState projectState = + projectCache + .get(cd.project()) + .orElseThrow( + () -> + new IllegalStateException( + "Unable to find project while evaluating submit rule", + new NoSuchProjectException(cd.project()))); + // We evaluate all the plugin-defined evaluators, // and then we collect the results in one list. return Streams.stream(submitRules) diff --git a/java/com/google/gerrit/server/query/change/ChangeData.java b/java/com/google/gerrit/server/query/change/ChangeData.java index 97eb0fde96..9a32a0344c 100644 --- a/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/java/com/google/gerrit/server/query/change/ChangeData.java @@ -76,6 +76,7 @@ import com.google.gerrit.server.change.MergeabilityCache; import com.google.gerrit.server.change.PureRevert; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.config.GerritServerId; +import com.google.gerrit.server.config.SkipCurrentRulesEvaluationOnClosedChanges; import com.google.gerrit.server.config.TrackingFooters; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.MergeUtilFactory; @@ -332,6 +333,7 @@ public class ChangeData { null, serverId, virtualIdAlgo, + false, project, id, null, @@ -364,6 +366,7 @@ public class ChangeData { private final SubmitRequirementsEvaluator submitRequirementsEvaluator; private final SubmitRequirementsUtil submitRequirementsUtil; private final SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory; + private final boolean skipCurrentRulesEvaluationOnClosedChanges; // Required assisted injected fields. private final Project.NameKey project; @@ -456,6 +459,7 @@ public class ChangeData { SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory, @GerritServerId String gerritServerId, ChangeNumberVirtualIdAlgorithm virtualIdFunc, + @SkipCurrentRulesEvaluationOnClosedChanges Boolean skipCurrentRulesEvaluationOnClosedChange, @Assisted Project.NameKey project, @Assisted Change.Id id, @Assisted @Nullable Change change, @@ -477,6 +481,7 @@ public class ChangeData { this.submitRequirementsEvaluator = submitRequirementsEvaluator; this.submitRequirementsUtil = submitRequirementsUtil; this.submitRuleEvaluatorFactory = submitRuleEvaluatorFactory; + this.skipCurrentRulesEvaluationOnClosedChanges = skipCurrentRulesEvaluationOnClosedChange; this.project = project; this.legacyId = id; @@ -1151,6 +1156,9 @@ public class ChangeData { project(), getId().get()); return Collections.emptyList(); } + if (skipCurrentRulesEvaluationOnClosedChanges && change().isClosed()) { + return notes().getSubmitRecords(); + } records = submitRuleEvaluatorFactory.create(options).evaluate(this); submitRecords.put(options, records); if (!change().isClosed() && submitRecords.size() == 1) { diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java index bd156f4e42..c9c5c2c1c8 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -31,6 +31,7 @@ import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.b import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.labelPermissionKey; import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.permissionKey; import static com.google.gerrit.entities.RefNames.changeMetaRef; +import static com.google.gerrit.extensions.client.ChangeStatus.ABANDONED; import static com.google.gerrit.extensions.client.ChangeStatus.MERGED; import static com.google.gerrit.extensions.client.ListChangesOption.ALL_REVISIONS; import static com.google.gerrit.extensions.client.ListChangesOption.CHANGE_ACTIONS; @@ -3893,6 +3894,74 @@ public class ChangeIT extends AbstractDaemonTest { } @Test + @GerritConfig(name = "change.skipCurrentRulesEvaluationOnClosedChanges", value = "true") + public void checkLabelsNotUpdatedForMergedChange() throws Exception { + PushOneCommit.Result r = createChange(); + gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve()); + gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).submit(); + + ChangeInfo change = gApi.changes().id(r.getChangeId()).get(); + assertThat(change.status).isEqualTo(MERGED); + assertThat(change.submissionId).isNotNull(); + assertThat(change.labels.keySet()).containsExactly(LabelId.CODE_REVIEW); + assertThat(change.permittedLabels.keySet()).containsExactly(LabelId.CODE_REVIEW); + assertPermitted(change, LabelId.CODE_REVIEW, 2); + + LabelType verified = TestLabels.verified(); + AccountGroup.UUID registeredUsers = systemGroupBackend.getGroup(REGISTERED_USERS).getUUID(); + String heads = RefNames.REFS_HEADS + "*"; + + // add new label and assert that it's returned for existing changes + try (ProjectConfigUpdate u = updateProject(project)) { + u.getConfig().upsertLabelType(verified); + u.save(); + } + projectOperations + .project(project) + .forUpdate() + .add(allowLabel(verified.getName()).ref(heads).group(registeredUsers).range(-1, 1)) + .update(); + + change = gApi.changes().id(r.getChangeId()).get(); + assertThat(change.labels.keySet()).containsExactly(LabelId.CODE_REVIEW); + assertThat(change.permittedLabels.keySet()) + .containsExactly(LabelId.CODE_REVIEW, LabelId.VERIFIED); + assertPermitted(change, LabelId.CODE_REVIEW, 2); + } + + @Test + @GerritConfig(name = "change.skipCurrentRulesEvaluationOnClosedChanges", value = "true") + public void checkLabelsNotUpdatedForAbandonedChange() throws Exception { + PushOneCommit.Result r = createChange(); + gApi.changes().id(r.getChangeId()).abandon(); + + ChangeInfo change = gApi.changes().id(r.getChangeId()).get(); + assertThat(change.status).isEqualTo(ABANDONED); + assertThat(change.labels.keySet()).isEmpty(); + assertThat(change.submitRecords).isEmpty(); + + LabelType verified = TestLabels.verified(); + AccountGroup.UUID registeredUsers = systemGroupBackend.getGroup(REGISTERED_USERS).getUUID(); + String heads = RefNames.REFS_HEADS + "*"; + + // add new label and assert that it's returned for existing changes + try (ProjectConfigUpdate u = updateProject(project)) { + u.getConfig().upsertLabelType(verified); + u.save(); + } + projectOperations + .project(project) + .forUpdate() + .add(allowLabel(verified.getName()).ref(heads).group(registeredUsers).range(-1, 1)) + .update(); + + change = gApi.changes().id(r.getChangeId()).get(); + assertThat(change.labels.keySet()).isEmpty(); + assertThat(change.permittedLabels.keySet()).isEmpty(); + assertThat(change.submitRecords).isEmpty(); + } + + @Test public void notifyConfigForDirectoryTriggersEmail() throws Exception { // Configure notifications on project level. RevCommit oldHead = projectOperations.project(project).getHead("master"); diff --git a/tools/eclipse/project.py b/tools/eclipse/project.py index d574ecf7b7..dc136ed2cc 100755 --- a/tools/eclipse/project.py +++ b/tools/eclipse/project.py @@ -96,8 +96,7 @@ def _build_bazel_cmd(*args): build = True cmd.append(arg) if custom_java: - cmd.append('--host_java_toolchain=@bazel_tools//tools/jdk:toolchain_java%s' % custom_java) - cmd.append('--java_toolchain=@bazel_tools//tools/jdk:toolchain_java%s' % custom_java) + cmd.append('--config=java%s' % custom_java) return cmd diff --git a/tools/maven/package.bzl b/tools/maven/package.bzl index eacb02b9f4..b25656da7f 100644 --- a/tools/maven/package.bzl +++ b/tools/maven/package.bzl @@ -40,7 +40,7 @@ def maven_package( src = {}, doc = {}, war = {}): - build_cmd = ["bazel_cmd", "build", "--java_toolchain=//tools:error_prone_warnings_toolchain_java11"] + build_cmd = ["bazel_cmd", "build"] mvn_cmd = ["python", "tools/maven/mvn.py", "-v", version] api_cmd = mvn_cmd[:] api_targets = [] |