Mark submit requirements as bypassed if change is pushed with %submit If a change is uploaded for review with the "%submit" option, submit rules/requirements are bypassed. To enable after the fact auditing, we used to store an extra marker submit record with status=FORCED to indicate that all other records were forced. In this change, we implement the same behaviour for submit requirements. We also bypass submit requirements by setting a "forced" boolean in SubmitRequirementResult. After the change is merged, we return the "FORCED" status for all SRs on the API. Change-Id: I3305bfeee661cdb7af0b954c7b4d9a9348a75a58
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index 870e194..1b11ba5 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt
@@ -8280,7 +8280,8 @@ Description of the submit requirement. |`status`|| Status describing the result of evaluating the submit requirement. The status -is one of (`SATISFIED`, `UNSATISFIED`, `OVERRIDDEN`, `NOT_APPLICABLE`, `ERROR`). +is one of (`SATISFIED`, `UNSATISFIED`, `OVERRIDDEN`, `NOT_APPLICABLE`, `ERROR`, +`FORCED`). |`is_legacy`|| If true, this submit requirement result was created from a legacy link:#submit-record[SubmitRecord]. Otherwise, it was created by evaluating a
diff --git a/java/com/google/gerrit/entities/SubmitRequirementResult.java b/java/com/google/gerrit/entities/SubmitRequirementResult.java index 865e22e..4bdce80 100644 --- a/java/com/google/gerrit/entities/SubmitRequirementResult.java +++ b/java/com/google/gerrit/entities/SubmitRequirementResult.java
@@ -53,9 +53,17 @@ return legacy().orElse(false); } + /** + * Boolean indicating if the "submit requirement" was bypassed during submission, e.g. by + * performing a push with the %submit option. + */ + public abstract Optional<Boolean> forced(); + @Memoized public Status status() { - if (assertError(submittabilityExpressionResult()) + if (forced().orElse(false)) { + return Status.FORCED; + } else if (assertError(submittabilityExpressionResult()) || assertError(applicabilityExpressionResult()) || assertError(overrideExpressionResult())) { return Status.ERROR; @@ -74,7 +82,10 @@ @Memoized public boolean fulfilled() { Status s = status(); - return s == Status.SATISFIED || s == Status.OVERRIDDEN || s == Status.NOT_APPLICABLE; + return s == Status.SATISFIED + || s == Status.OVERRIDDEN + || s == Status.NOT_APPLICABLE + || s == Status.FORCED; } public static Builder builder() { @@ -113,7 +124,13 @@ * Any of the applicability, submittability or override expressions contain invalid syntax and * are not parsable. */ - ERROR + ERROR, + + /** + * The "submit requirement" was bypassed during submission, e.g. by pushing for review with the + * %submit option. + */ + FORCED } @AutoValue.Builder @@ -132,6 +149,8 @@ public abstract Builder legacy(Optional<Boolean> value); + public abstract Builder forced(Optional<Boolean> value); + public abstract SubmitRequirementResult build(); }
diff --git a/java/com/google/gerrit/extensions/common/SubmitRequirementResultInfo.java b/java/com/google/gerrit/extensions/common/SubmitRequirementResultInfo.java index 3d50f13..7b87be8 100644 --- a/java/com/google/gerrit/extensions/common/SubmitRequirementResultInfo.java +++ b/java/com/google/gerrit/extensions/common/SubmitRequirementResultInfo.java
@@ -41,7 +41,13 @@ * Any of the applicability, submittability or override expressions contain invalid syntax and * are not parsable. */ - ERROR + ERROR, + + /** + * The "submit requirement" was bypassed during submission, e.g. by pushing for review with the + * %submit option. + */ + FORCED } /** Submit requirement name. */
diff --git a/java/com/google/gerrit/server/notedb/ChangeNoteJson.java b/java/com/google/gerrit/server/notedb/ChangeNoteJson.java index f770186..0dfd494 100644 --- a/java/com/google/gerrit/server/notedb/ChangeNoteJson.java +++ b/java/com/google/gerrit/server/notedb/ChangeNoteJson.java
@@ -75,12 +75,15 @@ @Override public Optional<Boolean> read(JsonReader in) throws IOException { JsonElement parsed = new JsonParser().parse(in); + if (parsed == null) { + return Optional.empty(); + } if (parsed.isJsonObject()) { // If it's not a JSON object, then the boolean value is available directly in the Json // element. parsed = parsed.getAsJsonObject().get("value"); } - if (parsed.isJsonNull()) { + if (parsed == null || parsed.isJsonNull()) { return Optional.empty(); } return Optional.of(parsed.getAsBoolean());
diff --git a/java/com/google/gerrit/server/notedb/SubmitRequirementProtoConverter.java b/java/com/google/gerrit/server/notedb/SubmitRequirementProtoConverter.java index 3caa4d4..dac71ea 100644 --- a/java/com/google/gerrit/server/notedb/SubmitRequirementProtoConverter.java +++ b/java/com/google/gerrit/server/notedb/SubmitRequirementProtoConverter.java
@@ -36,6 +36,8 @@ SubmitRequirementResultProto.getDescriptor().findFieldByNumber(4); private static final FieldDescriptor SR_LEGACY_FIELD = SubmitRequirementResultProto.getDescriptor().findFieldByNumber(6); + private static final FieldDescriptor SR_FORCED_FIELD = + SubmitRequirementResultProto.getDescriptor().findFieldByNumber(7); @Override public SubmitRequirementResultProto toProto(SubmitRequirementResult r) { @@ -46,6 +48,9 @@ if (r.legacy().isPresent()) { builder.setLegacy(r.legacy().get()); } + if (r.forced().isPresent()) { + builder.setForced(r.forced().get()); + } if (r.applicabilityExpressionResult().isPresent()) { builder.setApplicabilityExpressionResult( SubmitRequirementExpressionResultSerializer.serialize( @@ -71,6 +76,9 @@ if (proto.hasField(SR_LEGACY_FIELD)) { builder.legacy(Optional.of(proto.getLegacy())); } + if (proto.hasField(SR_FORCED_FIELD)) { + builder.forced(Optional.of(proto.getForced())); + } if (proto.hasField(SR_APPLICABILITY_EXPR_RESULT_FIELD)) { builder.applicabilityExpressionResult( Optional.of(
diff --git a/java/com/google/gerrit/server/project/SubmitRequirementsAdapter.java b/java/com/google/gerrit/server/project/SubmitRequirementsAdapter.java index 83c4bab..547f640 100644 --- a/java/com/google/gerrit/server/project/SubmitRequirementsAdapter.java +++ b/java/com/google/gerrit/server/project/SubmitRequirementsAdapter.java
@@ -52,10 +52,16 @@ // This doesn't have an effect since we never call this class (i.e. to evaluate submit // requirements) for closed changes. List<SubmitRecord> records = cd.submitRecords(SubmitRuleOptions.defaults()); + boolean areForced = + records.stream().anyMatch(record -> SubmitRecord.Status.FORCED.equals(record.status)); List<LabelType> labelTypes = cd.getLabelTypes().getLabelTypes(); ObjectId commitId = cd.currentPatchSet().commitId(); return records.stream() - .map(r -> createResult(r, labelTypes, commitId)) + // Filter out the "FORCED" submit record. This is a marker submit record that was just used + // to indicate that all other records were forced. "FORCED" means that the change was pushed + // with the %submit option bypassing submit rules. + .filter(r -> !SubmitRecord.Status.FORCED.equals(r.status)) + .map(r -> createResult(r, labelTypes, commitId, areForced)) .flatMap(List::stream) .collect( Collectors.toMap( @@ -79,19 +85,19 @@ } static List<SubmitRequirementResult> createResult( - SubmitRecord record, List<LabelType> labelTypes, ObjectId psCommitId) { + SubmitRecord record, List<LabelType> labelTypes, ObjectId psCommitId, boolean isForced) { List<SubmitRequirementResult> results; if (record.ruleName != null && record.ruleName.equals("gerrit~DefaultSubmitRule")) { - results = createFromDefaultSubmitRecord(record.labels, labelTypes, psCommitId); + results = createFromDefaultSubmitRecord(record.labels, labelTypes, psCommitId, isForced); } else { - results = createFromCustomSubmitRecord(record, psCommitId); + results = createFromCustomSubmitRecord(record, psCommitId, isForced); } logger.atFine().log("Converted submit record %s to submit requirements %s", record, results); return results; } private static List<SubmitRequirementResult> createFromDefaultSubmitRecord( - List<Label> labels, List<LabelType> labelTypes, ObjectId psCommitId) { + List<Label> labels, List<LabelType> labelTypes, ObjectId psCommitId, boolean isForced) { ImmutableList.Builder<SubmitRequirementResult> result = ImmutableList.builder(); for (Label label : labels) { Optional<LabelType> maybeLabelType = getLabelType(labelTypes, label.label); @@ -117,13 +123,14 @@ .submittabilityExpressionResult( createExpressionResult(toExpression(atoms), mapStatus(label), atoms)) .patchSetCommitId(psCommitId) + .forced(Optional.of(isForced)) .build()); } return result.build(); } private static List<SubmitRequirementResult> createFromCustomSubmitRecord( - SubmitRecord record, ObjectId psCommitId) { + SubmitRecord record, ObjectId psCommitId, boolean isForced) { String ruleName = record.ruleName != null ? record.ruleName : "Custom-Rule"; if (record.labels == null || record.labels.isEmpty()) { SubmitRequirement sr = @@ -141,6 +148,7 @@ createExpressionResult( sr.submittabilityExpression(), mapStatus(record), ImmutableList.of(ruleName))) .patchSetCommitId(psCommitId) + .forced(Optional.of(isForced)) .build()); } ImmutableList.Builder<SubmitRequirementResult> result = ImmutableList.builder();
diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java index fc862f3..c383e4b 100644 --- a/java/com/google/gerrit/server/submit/MergeOp.java +++ b/java/com/google/gerrit/server/submit/MergeOp.java
@@ -97,6 +97,7 @@ import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; @@ -325,6 +326,7 @@ case SATISFIED: case NOT_APPLICABLE: case OVERRIDDEN: + case FORCED: break; case ERROR: @@ -382,7 +384,7 @@ commitStatus.maybeFailVerbose(); } - private void bypassSubmitRules(ChangeSet cs) { + private void bypassSubmitRulesAndRequirements(ChangeSet cs) { checkArgument( !cs.furtherHiddenChanges(), "cannot bypass submit rules for topic with hidden change"); for (ChangeData cd : cs.changes()) { @@ -399,6 +401,15 @@ forced.status = SubmitRecord.Status.FORCED; records.add(forced); cd.setSubmitRecords(submitRuleOptions(/* allowClosed= */ false), records); + + // Also bypass submit requirements. Mark them as forced. + Map<SubmitRequirement, SubmitRequirementResult> forcedSRs = + cd.submitRequirements().entrySet().stream() + .collect( + Collectors.toMap( + Map.Entry::getKey, + entry -> entry.getValue().toBuilder().forced(Optional.of(true)).build())); + cd.setSubmitRequirements(forcedSRs); } } @@ -511,7 +522,7 @@ checkSubmitRulesAndState(filteredNoteDbChangeSet, isRetry); } else { logger.atFine().log("Bypassing submit rules"); - bypassSubmitRules(filteredNoteDbChangeSet); + bypassSubmitRulesAndRequirements(filteredNoteDbChangeSet); } integrateIntoHistory(filteredNoteDbChangeSet, submissionExecutor); return null;
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java index 35f7b45..9fdbcb6 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -6035,6 +6035,41 @@ } @Test + @GerritConfig( + name = "experiments.enabled", + values = { + ExperimentFeaturesConstants + .GERRIT_BACKEND_REQUEST_FEATURE_STORE_SUBMIT_REQUIREMENTS_ON_MERGE + }) + public void submitRequirements_forcedByDirectSubmission() throws Exception { + projectOperations + .project(project) + .forUpdate() + .add(allow(Permission.SUBMIT).ref("refs/for/refs/heads/master").group(REGISTERED_USERS)) + .update(); + + configSubmitRequirement( + project, + SubmitRequirement.builder() + .setName("My-Requirement") + // Submit requirement is always unsatisfied, but we are going to bypass it. + .setSubmittabilityExpression(SubmitRequirementExpression.create("is:false")) + .setAllowOverrideInChildProjects(false) + .build()); + + PushOneCommit.Result r = createChange(); + String changeId = r.getChangeId(); + pushFactory.create(admin.newIdent(), testRepo, changeId).to("refs/for/master%submit"); + + ChangeInfo change = gApi.changes().id(changeId).get(); + assertThat(change.submitRequirements).hasSize(2); + assertSubmitRequirementStatus( + change.submitRequirements, "My-Requirement", Status.FORCED, /* isLegacy= */ false); + assertSubmitRequirementStatus( + change.submitRequirements, "Code-Review", Status.FORCED, /* isLegacy= */ true); + } + + @Test public void fourByteEmoji() throws Exception { // U+1F601 GRINNING FACE WITH SMILING EYES String smile = new String(Character.toChars(0x1f601));
diff --git a/javatests/com/google/gerrit/server/cache/serialize/entities/SubmitRequirementJsonSerializerTest.java b/javatests/com/google/gerrit/server/cache/serialize/entities/SubmitRequirementJsonSerializerTest.java index 75a6882..5cd43af 100644 --- a/javatests/com/google/gerrit/server/cache/serialize/entities/SubmitRequirementJsonSerializerTest.java +++ b/javatests/com/google/gerrit/server/cache/serialize/entities/SubmitRequirementJsonSerializerTest.java
@@ -129,7 +129,8 @@ + "\"passingAtoms\":[]," + "\"failingAtoms\":[\"label:Override=+1\"]}}," + "\"patchSetCommitId\":\"4663ab9e9eb49a214e68e60f0fe5d0b6f44f763e\"," - + "\"legacy\":{\"value\":true}}"; + + "\"legacy\":{\"value\":true}," + + "\"forced\":{\"value\":null}}"; private static final Gson gson = new ChangeNoteJson().getGson();
diff --git a/javatests/com/google/gerrit/server/project/SubmitRequirementsAdapterTest.java b/javatests/com/google/gerrit/server/project/SubmitRequirementsAdapterTest.java index c6b8a33..45ccb03 100644 --- a/javatests/com/google/gerrit/server/project/SubmitRequirementsAdapterTest.java +++ b/javatests/com/google/gerrit/server/project/SubmitRequirementsAdapterTest.java
@@ -94,7 +94,8 @@ createLabel("Verified", Label.Status.OK))); List<SubmitRequirementResult> requirements = - SubmitRequirementsAdapter.createResult(submitRecord, labelTypes, psCommitId); + SubmitRequirementsAdapter.createResult( + submitRecord, labelTypes, psCommitId, /* isForced= */ false); assertThat(requirements).hasSize(2); assertResult( @@ -122,7 +123,8 @@ createLabel("Verified", Label.Status.NEED))); List<SubmitRequirementResult> requirements = - SubmitRequirementsAdapter.createResult(submitRecord, labelTypes, psCommitId); + SubmitRequirementsAdapter.createResult( + submitRecord, labelTypes, psCommitId, /* isForced= */ false); assertThat(requirements).hasSize(2); assertResult( @@ -140,6 +142,32 @@ } @Test + public void defaultSubmitRule_withOneLabelForced() { + SubmitRecord submitRecord = + createSubmitRecord( + "gerrit~DefaultSubmitRule", + Status.OK, + Arrays.asList(createLabel("Code-Review", Label.Status.NEED))); + + // Submit records that are forced are written with their initial status in NoteDb, e.g. NEED. + // If we do a force submit, the gerrit server appends an extra marker record with status=FORCED + // to indicate that all other records were forced, that's why we explicitly pass isForced=true + // to the "submit requirements adapter". The resulting submit requirement result has a + // status=FORCED. + List<SubmitRequirementResult> requirements = + SubmitRequirementsAdapter.createResult( + submitRecord, labelTypes, psCommitId, /* isForced= */ true); + + assertThat(requirements).hasSize(1); + assertResult( + requirements.get(0), + /* reqName= */ "Code-Review", + /* submitExpression= */ "label:Code-Review=MAX -label:Code-Review=MIN", + SubmitRequirementResult.Status.FORCED, + SubmitRequirementExpressionResult.Status.FAIL); + } + + @Test public void defaultSubmitRule_withLabelStatusNeed_labelHasIgnoreSelfApproval() throws Exception { SubmitRecord submitRecord = createSubmitRecord( @@ -148,7 +176,8 @@ Arrays.asList(createLabel("ISA-Label", Label.Status.NEED))); List<SubmitRequirementResult> requirements = - SubmitRequirementsAdapter.createResult(submitRecord, labelTypes, psCommitId); + SubmitRequirementsAdapter.createResult( + submitRecord, labelTypes, psCommitId, /* isForced= */ false); assertThat(requirements).hasSize(1); assertResult( @@ -168,7 +197,8 @@ Arrays.asList(createLabel("ISA-Label", Label.Status.OK))); List<SubmitRequirementResult> requirements = - SubmitRequirementsAdapter.createResult(submitRecord, labelTypes, psCommitId); + SubmitRequirementsAdapter.createResult( + submitRecord, labelTypes, psCommitId, /* isForced= */ false); assertThat(requirements).hasSize(1); assertResult( @@ -188,7 +218,8 @@ Arrays.asList(createLabel("Non-Existing", Label.Status.OK))); List<SubmitRequirementResult> requirements = - SubmitRequirementsAdapter.createResult(submitRecord, labelTypes, psCommitId); + SubmitRequirementsAdapter.createResult( + submitRecord, labelTypes, psCommitId, /* isForced= */ false); assertThat(requirements).isEmpty(); } @@ -204,7 +235,8 @@ createLabel("Code-Review", Label.Status.OK))); List<SubmitRequirementResult> requirements = - SubmitRequirementsAdapter.createResult(submitRecord, labelTypes, psCommitId); + SubmitRequirementsAdapter.createResult( + submitRecord, labelTypes, psCommitId, /* isForced= */ false); // The "Non-Existing" label was skipped since it does not exist in the project config. assertThat(requirements).hasSize(1); @@ -222,7 +254,8 @@ createSubmitRecord("gerrit~IgnoreSelfApprovalRule", Status.OK, Arrays.asList()); List<SubmitRequirementResult> requirements = - SubmitRequirementsAdapter.createResult(submitRecord, labelTypes, psCommitId); + SubmitRequirementsAdapter.createResult( + submitRecord, labelTypes, psCommitId, /* isForced= */ false); assertThat(requirements).hasSize(1); assertResult( @@ -239,7 +272,8 @@ createSubmitRecord("gerrit~IgnoreSelfApprovalRule", Status.OK, /* labels= */ null); List<SubmitRequirementResult> requirements = - SubmitRequirementsAdapter.createResult(submitRecord, labelTypes, psCommitId); + SubmitRequirementsAdapter.createResult( + submitRecord, labelTypes, psCommitId, /* isForced= */ false); assertThat(requirements).hasSize(1); assertResult( @@ -256,7 +290,8 @@ createSubmitRecord("gerrit~IgnoreSelfApprovalRule", Status.NOT_READY, Arrays.asList()); List<SubmitRequirementResult> requirements = - SubmitRequirementsAdapter.createResult(submitRecord, labelTypes, psCommitId); + SubmitRequirementsAdapter.createResult( + submitRecord, labelTypes, psCommitId, /* isForced= */ false); assertThat(requirements).hasSize(1); assertResult( @@ -278,7 +313,8 @@ createLabel("custom-label-2", Label.Status.REJECT))); List<SubmitRequirementResult> requirements = - SubmitRequirementsAdapter.createResult(submitRecord, labelTypes, psCommitId); + SubmitRequirementsAdapter.createResult( + submitRecord, labelTypes, psCommitId, /* isForced= */ false); assertThat(requirements).hasSize(2); assertResult( @@ -306,7 +342,8 @@ createLabel("custom-label-2", Label.Status.REJECT))); List<SubmitRequirementResult> requirements = - SubmitRequirementsAdapter.createResult(submitRecord, labelTypes, psCommitId); + SubmitRequirementsAdapter.createResult( + submitRecord, labelTypes, psCommitId, /* isForced= */ false); assertThat(requirements).hasSize(2); assertResult(
diff --git a/proto/cache.proto b/proto/cache.proto index 750bea1..1a0961d 100644 --- a/proto/cache.proto +++ b/proto/cache.proto
@@ -484,7 +484,7 @@ } // Serialized form of com.google.gerrit.entities.SubmitRequirementResult. -// Next ID: 7 +// Next ID: 8 message SubmitRequirementResultProto { SubmitRequirementProto submit_requirement = 1; SubmitRequirementExpressionResultProto applicability_expression_result = 2; @@ -496,6 +496,10 @@ // Whether this result was created from a legacy submit record. bool legacy = 6; + + // Whether the submit requirement was bypassed during submission (i.e. by + // performing a push with the %submit option). + bool forced = 7; } // Serialized form of com.google.gerrit.entities.SubmitRequirementExpressionResult.