Merge changes I06f395b5,If4fa6aa4,Idd840114
* changes:
Return submit requirements for legacy submit records for closed changes
Do not store legacy submit records as submit requirements upon merge
Move #getLegacyRequirements to SubmitRequirementsAdapter
diff --git a/java/com/google/gerrit/server/notedb/StoreSubmitRequirementsOp.java b/java/com/google/gerrit/server/notedb/StoreSubmitRequirementsOp.java
index 57a3cd7..1a7d5af 100644
--- a/java/com/google/gerrit/server/notedb/StoreSubmitRequirementsOp.java
+++ b/java/com/google/gerrit/server/notedb/StoreSubmitRequirementsOp.java
@@ -50,7 +50,9 @@
// patchset to the user before it was merged.
ChangeData changeData = changeDataFactory.create(ctx.getProject(), ctx.getChange().getId());
ChangeUpdate update = ctx.getUpdate(ctx.getChange().currentPatchSetId());
- update.putSubmitRequirementResults(evaluator.evaluateAllRequirements(changeData).values());
+ // We do not want to store submit requirements in NoteDb for legacy submit records
+ update.putSubmitRequirementResults(
+ evaluator.evaluateAllRequirements(changeData, /* includeLegacy= */ false).values());
return !changeData.submitRequirements().isEmpty();
}
}
diff --git a/java/com/google/gerrit/server/project/SubmitRequirementsAdapter.java b/java/com/google/gerrit/server/project/SubmitRequirementsAdapter.java
index f028def..63a29cc 100644
--- a/java/com/google/gerrit/server/project/SubmitRequirementsAdapter.java
+++ b/java/com/google/gerrit/server/project/SubmitRequirementsAdapter.java
@@ -25,8 +25,12 @@
import com.google.gerrit.entities.SubmitRequirementExpressionResult;
import com.google.gerrit.entities.SubmitRequirementExpressionResult.Status;
import com.google.gerrit.entities.SubmitRequirementResult;
+import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.ChangeQueryBuilder;
import java.util.List;
+import java.util.Map;
+import java.util.function.Function;
+import java.util.stream.Collectors;
import org.eclipse.jgit.lib.ObjectId;
/**
@@ -38,7 +42,25 @@
private SubmitRequirementsAdapter() {}
- public static List<SubmitRequirementResult> createResult(
+ /**
+ * Retrieve legacy submit records (created by label functions and other {@link
+ * com.google.gerrit.server.rules.SubmitRule}s) and convert them to submit requirement results.
+ */
+ public static Map<SubmitRequirement, SubmitRequirementResult> getLegacyRequirements(
+ SubmitRuleEvaluator.Factory evaluator, ChangeData cd) {
+ // We use SubmitRuleOptions.defaults() which does not recompute submit rules for closed changes.
+ // This doesn't have an effect since we never call this class (i.e. to evaluate submit
+ // requirements) for closed changes.
+ List<SubmitRecord> records = evaluator.create(SubmitRuleOptions.defaults()).evaluate(cd);
+ List<LabelType> labelTypes = cd.getLabelTypes().getLabelTypes();
+ ObjectId commitId = cd.currentPatchSet().commitId();
+ return records.stream()
+ .map(r -> createResult(r, labelTypes, commitId))
+ .flatMap(List::stream)
+ .collect(Collectors.toMap(sr -> sr.submitRequirement(), Function.identity()));
+ }
+
+ static List<SubmitRequirementResult> createResult(
SubmitRecord record, List<LabelType> labelTypes, ObjectId psCommitId) {
List<SubmitRequirementResult> results;
if (record.ruleName.equals("gerrit~DefaultSubmitRule")) {
diff --git a/java/com/google/gerrit/server/project/SubmitRequirementsEvaluator.java b/java/com/google/gerrit/server/project/SubmitRequirementsEvaluator.java
index b3ac380..402bb51 100644
--- a/java/com/google/gerrit/server/project/SubmitRequirementsEvaluator.java
+++ b/java/com/google/gerrit/server/project/SubmitRequirementsEvaluator.java
@@ -26,8 +26,13 @@
/**
* Evaluate and return all submit requirement results for a change. Submit requirements are read
* from the project config of the project containing the change as well as parent projects.
+ *
+ * @param cd change data corresponding to a specific gerrit change
+ * @param includeLegacy if set to true, evaluate legacy {@link
+ * com.google.gerrit.entities.SubmitRecord}s and convert them to submit requirements.
*/
- Map<SubmitRequirement, SubmitRequirementResult> evaluateAllRequirements(ChangeData cd);
+ Map<SubmitRequirement, SubmitRequirementResult> evaluateAllRequirements(
+ ChangeData cd, boolean includeLegacy);
/** Evaluate a single {@link SubmitRequirement} using change data. */
SubmitRequirementResult evaluateRequirement(SubmitRequirement sr, ChangeData cd);
diff --git a/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java b/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java
index 9be50c7..151ee7b 100644
--- a/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java
+++ b/java/com/google/gerrit/server/project/SubmitRequirementsEvaluatorImpl.java
@@ -17,8 +17,6 @@
import static com.google.gerrit.server.project.ProjectCache.illegalState;
import com.google.common.collect.ImmutableMap;
-import com.google.gerrit.entities.LabelType;
-import com.google.gerrit.entities.SubmitRecord;
import com.google.gerrit.entities.SubmitRequirement;
import com.google.gerrit.entities.SubmitRequirementExpression;
import com.google.gerrit.entities.SubmitRequirementExpressionResult;
@@ -36,12 +34,8 @@
import com.google.inject.Provider;
import com.google.inject.Scopes;
import java.util.HashMap;
-import java.util.List;
import java.util.Map;
import java.util.Optional;
-import java.util.function.Function;
-import java.util.stream.Collectors;
-import org.eclipse.jgit.lib.ObjectId;
/** Evaluates submit requirements for different change data. */
public class SubmitRequirementsEvaluatorImpl implements SubmitRequirementsEvaluator {
@@ -81,12 +75,14 @@
}
@Override
- public Map<SubmitRequirement, SubmitRequirementResult> evaluateAllRequirements(ChangeData cd) {
+ public Map<SubmitRequirement, SubmitRequirementResult> evaluateAllRequirements(
+ ChangeData cd, boolean includeLegacy) {
Map<SubmitRequirement, SubmitRequirementResult> result = getRequirements(cd);
- if (experimentFeatures.isFeatureEnabled(
- ExperimentFeaturesConstants
- .GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_LEGACY_SUBMIT_REQUIREMENTS)) {
- result.putAll(getLegacyRequirements(cd));
+ if (includeLegacy
+ && experimentFeatures.isFeatureEnabled(
+ ExperimentFeaturesConstants
+ .GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_LEGACY_SUBMIT_REQUIREMENTS)) {
+ result.putAll(SubmitRequirementsAdapter.getLegacyRequirements(legacyEvaluator, cd));
}
return ImmutableMap.copyOf(result);
}
@@ -140,23 +136,6 @@
return result;
}
- /**
- * Convert and return legacy submit records (created by label functions and other {@link
- * com.google.gerrit.server.rules.SubmitRule}s to submit requirement results.
- */
- private Map<SubmitRequirement, SubmitRequirementResult> getLegacyRequirements(ChangeData cd) {
- // We use SubmitRuleOptions.defaults() which does not recompute submit rules for closed changes.
- // This doesn't have an effect since we never call this class (i.e. to evaluate submit
- // requirements) for closed changes.
- List<SubmitRecord> records = legacyEvaluator.create(SubmitRuleOptions.defaults()).evaluate(cd);
- List<LabelType> labelTypes = cd.getLabelTypes().getLabelTypes();
- ObjectId commitId = cd.currentPatchSet().commitId();
- return records.stream()
- .map(r -> SubmitRequirementsAdapter.createResult(r, labelTypes, commitId))
- .flatMap(List::stream)
- .collect(Collectors.toMap(sr -> sr.submitRequirement(), Function.identity()));
- }
-
/** Evaluate the predicate recursively using change data. */
private PredicateResult evaluatePredicateTree(
Predicate<ChangeData> predicate, ChangeData changeData) {
diff --git a/java/com/google/gerrit/server/query/change/ChangeData.java b/java/com/google/gerrit/server/query/change/ChangeData.java
index c551cd2..6e14a13 100644
--- a/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -75,6 +75,8 @@
import com.google.gerrit.server.change.PureRevert;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.TrackingFooters;
+import com.google.gerrit.server.experiments.ExperimentFeatures;
+import com.google.gerrit.server.experiments.ExperimentFeaturesConstants;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.notedb.ChangeNotes;
@@ -87,6 +89,7 @@
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectCache;
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.SubmitRuleEvaluator;
import com.google.gerrit.server.project.SubmitRuleOptions;
@@ -268,7 +271,7 @@
ChangeData cd =
new ChangeData(
null, null, null, null, null, null, null, null, null, null, null, null, null, null,
- null, null, project, id, null, null);
+ null, null, null, project, id, null, null);
cd.currentPatchSet =
PatchSet.builder()
.id(PatchSet.id(id, currentPatchSetId))
@@ -286,6 +289,7 @@
private final ChangeMessagesUtil cmUtil;
private final ChangeNotes.Factory notesFactory;
private final CommentsUtil commentsUtil;
+ private final ExperimentFeatures experimentFeatures;
private final GitRepositoryManager repoManager;
private final MergeUtil.Factory mergeUtilFactory;
private final MergeabilityCache mergeabilityCache;
@@ -367,6 +371,7 @@
ChangeMessagesUtil cmUtil,
ChangeNotes.Factory notesFactory,
CommentsUtil commentsUtil,
+ ExperimentFeatures experimentFeatures,
GitRepositoryManager repoManager,
MergeUtil.Factory mergeUtilFactory,
MergeabilityCache mergeabilityCache,
@@ -386,6 +391,7 @@
this.cmUtil = cmUtil;
this.notesFactory = notesFactory;
this.commentsUtil = commentsUtil;
+ this.experimentFeatures = experimentFeatures;
this.repoManager = repoManager;
this.mergeUtilFactory = mergeUtilFactory;
this.mergeabilityCache = mergeabilityCache;
@@ -946,13 +952,33 @@
return Collections.emptyMap();
}
Change c = change();
- if (c != null && c.isClosed()) {
+ if (c == null || !c.isClosed()) {
+ // Open changes: Evaluate submit requirements online.
submitRequirements =
- notes().getSubmitRequirementsResult().stream()
- .collect(Collectors.toMap(r -> r.submitRequirement(), Function.identity()));
- } else {
- submitRequirements = submitRequirementsEvaluator.evaluateAllRequirements(this);
+ submitRequirementsEvaluator.evaluateAllRequirements(this, /* includeLegacy= */ true);
+ return submitRequirements;
}
+ // Closed changes: Load submit requirement results from NoteDb.
+ Map<SubmitRequirement, SubmitRequirementResult> projectConfigRequirements =
+ notes().getSubmitRequirementsResult().stream()
+ .filter(r -> !r.legacy())
+ .collect(Collectors.toMap(r -> r.submitRequirement(), Function.identity()));
+ if (!experimentFeatures.isFeatureEnabled(
+ ExperimentFeaturesConstants
+ .GERRIT_BACKEND_REQUEST_FEATURE_ENABLE_LEGACY_SUBMIT_REQUIREMENTS)) {
+ 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));
}
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 4c17800..ed9f2f3 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -4582,6 +4582,7 @@
// 4. Merge the change. Submit requirements status is presented from NoteDb.
gApi.changes().id(changeId).current().submit();
change = gApi.changes().id(changeId).get();
+ // Legacy submit records are returned as submit requirements.
assertThat(change.submitRequirements).hasSize(2);
assertSubmitRequirementStatus(
change.submitRequirements, "Code-Review", Status.SATISFIED, /* isLegacy= */ true);
