Merge "Submit requirements: merge legacy and non-legacy results"
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)
