Optionally skip submit rules evaluation on closed changes

Adds a `change.skipCurrentRulesEvaluationOnClosedChanges` configuration
option to control submit rule evaluation on closed (merged and abandoned)
changes.

The default value for this option is `true` to keep the current
behaviour of Gerrit. This means that each time closed change is viewed,
submit rules will be revaluated. This may result in labels being added
or removed depending on the most recent project configuration. This also
means that labels on the closed changes may not represent the stale of
them when it was submitted.

When `change.skipCurrentRulesEvaluationOnClosedChanges` is set to `true`
Gerrit will show the exact state of review labels the change was merged
with.

Bug: Issue 298084094
Release-Notes: Allow disable rules evaluation on closed changes
Change-Id: I2499271b84edceba0c34c3347dea2f8508eca00f

8 files changed, 182 insertions, 1 deletions

diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 4b89c6b575..524b17159e 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/java/com/google/gerrit/pgm/util/BatchProgramModule.java b/java/com/google/gerrit/pgm/util/BatchProgramModule.java
index 78a162bbc7..fe5966c5f7 100644
--- a/java/com/google/gerrit/pgm/util/BatchProgramModule.java
+++ b/java/com/google/gerrit/pgm/util/BatchProgramModule.java
@@ -60,6 +60,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.EventUtil;
 import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
@@ -209,6 +210,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 644915537c..d6baa7f8a8 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -290,6 +290,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/query/change/ChangeData.java b/java/com/google/gerrit/server/query/change/ChangeData.java
index cc08254669..3f5f63b1e1 100644
--- a/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -75,6 +75,7 @@ import com.google.gerrit.server.change.CommentThreads;
 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.SkipCurrentRulesEvaluationOnClosedChanges;
 import com.google.gerrit.server.config.TrackingFooters;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.MergeUtil;
@@ -284,7 +285,7 @@ public class ChangeData {
     ChangeData cd =
         new ChangeData(
             null, null, null, null, null, null, null, null, null, null, null, null, null, null,
-            null, null, null, project, id, null, null);
+            null, null, null, false, project, id, null, null);
     cd.currentPatchSet =
         PatchSet.builder()
             .id(PatchSet.id(id, currentPatchSetId))
@@ -313,6 +314,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;
@@ -400,6 +402,7 @@ public class ChangeData {
       SubmitRequirementsEvaluator submitRequirementsEvaluator,
       SubmitRequirementsUtil submitRequirementsUtil,
       SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory,
+      @SkipCurrentRulesEvaluationOnClosedChanges Boolean skipCurrentRulesEvaluationOnClosedChange,
       @Assisted Project.NameKey project,
       @Assisted Change.Id id,
       @Assisted @Nullable Change change,
@@ -421,6 +424,7 @@ public class ChangeData {
     this.submitRequirementsEvaluator = submitRequirementsEvaluator;
     this.submitRequirementsUtil = submitRequirementsUtil;
     this.submitRuleEvaluatorFactory = submitRuleEvaluatorFactory;
+    this.skipCurrentRulesEvaluationOnClosedChanges = skipCurrentRulesEvaluationOnClosedChange;
 
     this.project = project;
     this.legacyId = id;
@@ -1083,6 +1087,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 c5f0d232fb..86691c6fc8 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;
@@ -3806,6 +3807,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");