Submit requirements: merge legacy and non-legacy results
When experiment "enable_legacy_submit_requirements" is enabled, we
convert legacy submit records to submit requirements and return them
with ChangeInfo#submit_requirements.
In order to allow for a smoother migration to new submit requirements,
we can do this in two steps:
1) Add new submit requirements in project.config, track and monitor
their evaluation and make sure they behave correctly.
2) Remove the legacy submit rules (prolog code, label functions).
We would also like to allow creating submit requirements in
project.config with the same name as legacy submit requirements. If this
happens, we don't want the submit requirements API to return results
with duplicate names. In this change, we detect this case and favour
returning the project config's requirement if both results have the same
fulfillment status (i.e. they both allow or disallow change submission).
This change updates both code paths of open changes (in
SubmitRequirmentsEvaluatorImpl) and closed changes (ChangeData).
Change-Id: Id5db11e36aca7824d73cdfaf87f6d8f01dbc761b
diff --git a/java/com/google/gerrit/entities/SubmitRequirementResult.java b/java/com/google/gerrit/entities/SubmitRequirementResult.java
index 4f2f2f6..b7fa398 100644
--- a/java/com/google/gerrit/entities/SubmitRequirementResult.java
+++ b/java/com/google/gerrit/entities/SubmitRequirementResult.java
@@ -64,6 +64,13 @@
}
}
+ /** Returns true if the submit requirement is fulfilled and can allow change submission. */
+ @Memoized
+ public boolean fulfilled() {
+ Status s = status();
+ return s == Status.SATISFIED || s == Status.OVERRIDDEN || s == Status.NOT_APPLICABLE;
+ }
+
public static Builder builder() {
return new AutoValue_SubmitRequirementResult.Builder();
}
diff --git a/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java b/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java
index 151ee7b..de637b4 100644
--- a/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java
+++ b/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java
@@ -77,12 +77,17 @@
@Override
public Map<SubmitRequirement, SubmitRequirementResult> evaluateAllRequirements(
ChangeData cd, boolean includeLegacy) {
- Map<SubmitRequirement, SubmitRequirementResult> result = getRequirements(cd);
+ Map<SubmitRequirement, SubmitRequirementResult> projectConfigRequirements = getRequirements(cd);
+ Map<SubmitRequirement, SubmitRequirementResult> result = projectConfigRequirements;
if (includeLegacy
&& experimentFeatures.isFeatureEnabled(
ExperimentFeaturesConstants
.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_LEGACY_SUBMIT_REQUIREMENTS)) {
- result.putAll(SubmitRequirementsAdapter.getLegacyRequirements(legacyEvaluator, cd));
+ Map<SubmitRequirement, SubmitRequirementResult> legacyReqs =
+ SubmitRequirementsAdapter.getLegacyRequirements(legacyEvaluator, cd);
+ result =
+ SubmitRequirementsUtil.mergeLegacyAndNonLegacyRequirements(
+ projectConfigRequirements, legacyReqs);
}
return ImmutableMap.copyOf(result);
}
diff --git a/java/com/google/gerrit/server/project/SubmitRequirementsUtil.java b/java/com/google/gerrit/server/project/SubmitRequirementsUtil.java
new file mode 100644
index 0000000..2e43eac
--- /dev/null
+++ b/java/com/google/gerrit/server/project/SubmitRequirementsUtil.java
@@ -0,0 +1,71 @@
+// Copyright (C) 2021 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.project;
+
+import com.google.gerrit.entities.SubmitRequirement;
+import com.google.gerrit.entities.SubmitRequirementResult;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+/**
+ * A utility class for different operations related to {@link
+ * com.google.gerrit.entities.SubmitRequirement}s.
+ */
+public class SubmitRequirementsUtil {
+
+ private SubmitRequirementsUtil() {}
+
+ /**
+ * Merge legacy and non-legacy submit requirement results. If both input maps have submit
+ * requirements with the same name and fulfillment status (according to {@link
+ * SubmitRequirementResult#fulfilled()}), we eliminate the entry from the {@code
+ * legacyRequirements} input map and only include the one from the {@code
+ * projectConfigRequirements} in the result.
+ *
+ * @param projectConfigRequirements map of {@link SubmitRequirement} to {@link
+ * SubmitRequirementResult} containing results for submit requirements stored in the
+ * project.config.
+ * @param legacyRequirements map of {@link SubmitRequirement} to {@link SubmitRequirementResult}
+ * containing the results of converting legacy submit records to submit requirements.
+ * @return a map that is the result of merging both input maps, while eliminating requirements
+ * with the same name and status.
+ */
+ public static Map<SubmitRequirement, SubmitRequirementResult> mergeLegacyAndNonLegacyRequirements(
+ Map<SubmitRequirement, SubmitRequirementResult> projectConfigRequirements,
+ Map<SubmitRequirement, SubmitRequirementResult> legacyRequirements) {
+ Map<SubmitRequirement, SubmitRequirementResult> result = new HashMap<>();
+ result.putAll(projectConfigRequirements);
+ Map<String, SubmitRequirementResult> requirementsByName =
+ projectConfigRequirements.entrySet().stream()
+ .collect(Collectors.toMap(sr -> sr.getKey().name(), sr -> sr.getValue()));
+ for (Map.Entry<SubmitRequirement, SubmitRequirementResult> legacy :
+ legacyRequirements.entrySet()) {
+ String name = legacy.getKey().name();
+ SubmitRequirementResult projectConfigResult = requirementsByName.get(name);
+ SubmitRequirementResult legacyResult = legacy.getValue();
+ if (projectConfigResult != null && matchByStatus(projectConfigResult, legacyResult)) {
+ continue;
+ }
+ result.put(legacy.getKey(), legacy.getValue());
+ }
+ return result;
+ }
+
+ /** Returns true if both input results are equal in allowing/disallowing change submission. */
+ private static boolean matchByStatus(SubmitRequirementResult r1, SubmitRequirementResult r2) {
+ return r1.fulfilled() == r2.fulfilled();
+ }
+}
diff --git a/java/com/google/gerrit/server/query/change/ChangeData.java b/java/com/google/gerrit/server/query/change/ChangeData.java
index 4112579..b8c8c07 100644
--- a/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -91,6 +91,7 @@
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.project.SubmitRequirementsAdapter;
import com.google.gerrit.server.project.SubmitRequirementsEvaluator;
+import com.google.gerrit.server.project.SubmitRequirementsUtil;
import com.google.gerrit.server.project.SubmitRuleEvaluator;
import com.google.gerrit.server.project.SubmitRuleOptions;
import com.google.gerrit.server.util.time.TimeUtil;
@@ -969,16 +970,11 @@
submitRequirements = projectConfigRequirements;
return submitRequirements;
}
- // Get legacy submit requirements, i.e. those created from submit records.
Map<SubmitRequirement, SubmitRequirementResult> legacyRequirements =
SubmitRequirementsAdapter.getLegacyRequirements(submitRuleEvaluatorFactory, this);
- // Combine projectConfigRequirements with legacyRequirements
submitRequirements =
- Stream.of(projectConfigRequirements, legacyRequirements)
- .flatMap(map -> map.entrySet().stream())
- .collect(
- ImmutableMap.toImmutableMap(
- Map.Entry::getKey, Map.Entry::getValue, (value1, value2) -> value1));
+ SubmitRequirementsUtil.mergeLegacyAndNonLegacyRequirements(
+ projectConfigRequirements, legacyRequirements);
}
return submitRequirements;
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index ed9f2f3..52202d7 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -4539,6 +4539,129 @@
value =
ExperimentFeaturesConstants
.GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_LEGACY_SUBMIT_REQUIREMENTS)
+ public void
+ submitRequirements_returnOneEntryForMatchingLegacyAndNonLegacyResultsWithTheSameName_ifLegacySubmitRecordsAreEnabled()
+ throws Exception {
+ // Configure a legacy submit requirement: label with a max with block function
+ configLabel("build-cop-override", LabelFunction.MAX_WITH_BLOCK);
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ allowLabel("build-cop-override")
+ .ref("refs/heads/master")
+ .group(REGISTERED_USERS)
+ .range(-1, 1))
+ .update();
+
+ // Configure a submit requirement with the same name.
+ configSubmitRequirement(
+ project,
+ SubmitRequirement.builder()
+ .setName("build-cop-override")
+ .setSubmittabilityExpression(
+ SubmitRequirementExpression.create(
+ "label:build-cop-override=MAX -label:build-cop-override=MIN"))
+ .setAllowOverrideInChildProjects(false)
+ .build());
+
+ // Create a change. Vote to fulfill all requirements.
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+ voteLabel(changeId, "build-cop-override", 1);
+ voteLabel(changeId, "Code-Review", 2);
+
+ // Project has two legacy requirements: Code-Review and bco, and a non-legacy requirement: bco.
+ // Only non-legacy bco is returned.
+ ChangeInfo change = gApi.changes().id(changeId).get();
+ assertThat(change.submitRequirements).hasSize(2);
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "Code-Review", Status.SATISFIED, /* isLegacy= */ true);
+ assertSubmitRequirementStatus(
+ change.submitRequirements,
+ "build-cop-override",
+ Status.SATISFIED,
+ /* isLegacy= */ false,
+ /* submittabilityCondition= */ "label:build-cop-override=MAX -label:build-cop-override=MIN");
+
+ // Merge the change. Submit requirements are still the same.
+ gApi.changes().id(changeId).current().submit();
+ change = gApi.changes().id(changeId).get();
+ assertThat(change.submitRequirements).hasSize(2);
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "Code-Review", Status.SATISFIED, /* isLegacy= */ true);
+ assertSubmitRequirementStatus(
+ change.submitRequirements,
+ "build-cop-override",
+ Status.SATISFIED,
+ /* isLegacy= */ false,
+ /* submittabilityCondition= */ "label:build-cop-override=MAX -label:build-cop-override=MIN");
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value =
+ ExperimentFeaturesConstants
+ .GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_LEGACY_SUBMIT_REQUIREMENTS)
+ public void
+ submitRequirements_returnTwoEntriesForMismatchingLegacyAndNonLegacyResultsWithTheSameName_ifLegacySubmitRecordsAreEnabled()
+ throws Exception {
+ // Configure a legacy submit requirement: label with a max with block function
+ configLabel("build-cop-override", LabelFunction.MAX_WITH_BLOCK);
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ allowLabel("build-cop-override")
+ .ref("refs/heads/master")
+ .group(REGISTERED_USERS)
+ .range(-1, 1))
+ .update();
+
+ // Configure a submit requirement with the same name.
+ configSubmitRequirement(
+ project,
+ SubmitRequirement.builder()
+ .setName("build-cop-override")
+ .setSubmittabilityExpression(
+ SubmitRequirementExpression.create("label:build-cop-override=MIN"))
+ .setAllowOverrideInChildProjects(false)
+ .build());
+
+ // Create a change
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+ voteLabel(changeId, "build-cop-override", 1);
+ voteLabel(changeId, "Code-Review", 2);
+
+ // Project has two legacy requirements: Code-Review and bco, and a non-legacy requirement: bco.
+ // Two instances of bco will be returned since their status is not matching.
+ ChangeInfo change = gApi.changes().id(changeId).get();
+ assertThat(change.submitRequirements).hasSize(3);
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "Code-Review", Status.SATISFIED, /* isLegacy= */ true);
+ assertSubmitRequirementStatus(
+ change.submitRequirements,
+ "build-cop-override",
+ Status.SATISFIED,
+ /* isLegacy= */ true,
+ // MAX_WITH_BLOCK function was translated to a submittability expression.
+ /* submittabilityCondition= */ "label:build-cop-override=MAX -label:build-cop-override=MIN");
+ assertSubmitRequirementStatus(
+ change.submitRequirements,
+ "build-cop-override",
+ Status.UNSATISFIED,
+ /* isLegacy= */ false,
+ /* submittabilityCondition= */ "label:build-cop-override=MIN");
+ }
+
+ @Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ value =
+ ExperimentFeaturesConstants
+ .GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_LEGACY_SUBMIT_REQUIREMENTS)
public void submitRequirements_returnForLegacySubmitRecords_ifEnabled() throws Exception {
configLabel("build-cop-override", LabelFunction.MAX_WITH_BLOCK);
projectOperations
@@ -5197,6 +5320,30 @@
Collection<SubmitRequirementResultInfo> results,
String requirementName,
SubmitRequirementResultInfo.Status status,
+ boolean isLegacy,
+ String submittabilityCondition) {
+ for (SubmitRequirementResultInfo result : results) {
+ if (result.name.equals(requirementName)
+ && result.status == status
+ && result.isLegacy == isLegacy
+ && result.submittabilityExpressionResult.expression.equals(submittabilityCondition)) {
+ return;
+ }
+ }
+ throw new AssertionError(
+ String.format(
+ "Could not find submit requirement %s with status %s (results = %s)",
+ requirementName,
+ status,
+ results.stream()
+ .map(r -> String.format("%s=%s", r.name, r.status))
+ .collect(toImmutableList())));
+ }
+
+ private void assertSubmitRequirementStatus(
+ Collection<SubmitRequirementResultInfo> results,
+ String requirementName,
+ SubmitRequirementResultInfo.Status status,
boolean isLegacy) {
for (SubmitRequirementResultInfo result : results) {
if (result.name.equals(requirementName)
