opendev/gerrit
Code Issues Proposed changes

Read SubmitRecords back from NoteDb if allowClosed is false

Browse Source 
NoteDb stores SubmitRecords upon change submission. This enables us to
parse them back and present them to users instead of plainly recomputing
them each time. Recomputing submit records of a submitted change is
especially harmful when the rules have changed in the meantime.

Consider the case where we have added a new label: A change would
suddenly appear to not be submittable anymore while it was submitted
with all necessary approvals at the time.

This commit wires up the parsing logic in ChangeNotes with
SubmitRuleEvaluator in case we read from NoteDb and adds tests to ensure
the behavior is correct.

We change the status of the SR that we read back to CLOSED to reflect the
SRs actual status.

While at it, we rename allowClosed to recomputeOnClosedChanges. If this
flag is true, we compute a fresh SR even for a closed change. If it is
false, we read back data from NoteDb.

Change-Id: Icefecee32dac8a16c1a00c0633054f1df45145eb
This commit is contained in:
Patrick Hiesel
2018-09-11 10:39:20 +02:00
parent c677e7f0fa
commit 78e96009f7
9 changed files with 180 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files

31
java/com/google/gerrit/entities/SubmitRecord.java
View File

@@ -14,6 +14,9 @@
package com.google.gerrit.entities;
import static com.google.common.collect.ImmutableList.toImmutableList;
import com.google.common.collect.ImmutableList;
import java.util.Collection;
import java.util.List;
import java.util.Objects;
@@ -107,6 +110,17 @@ public class SubmitRecord {
public Status status;
public Account.Id appliedBy;
/**
* Returns a new instance of {@link Label} that contains a new instance for each mutable field.
*/
public Label deepCopy() {
Label copy = new Label();
copy.label = label;
copy.status = status;
copy.appliedBy = appliedBy;
return copy;
}
@Override
public String toString() {
StringBuilder sb = new StringBuilder();
@@ -134,6 +148,23 @@ public class SubmitRecord {
}
}
/**
* Returns a new instance of {@link SubmitRecord} that contains a new instance for each mutable
* field.
*/
public SubmitRecord deepCopy() {
SubmitRecord copy = new SubmitRecord();
copy.status = status;
copy.errorMessage = errorMessage;
if (labels != null) {
copy.labels = labels.stream().map(Label::deepCopy).collect(toImmutableList());
}
if (requirements != null) {
copy.requirements = ImmutableList.copyOf(requirements);
}
return copy;
}
@Override
public String toString() {
StringBuilder sb = new StringBuilder();

2
java/com/google/gerrit/server/index/change/ChangeField.java
View File

@@ -815,7 +815,7 @@ public class ChangeField {
});
public static final SubmitRuleOptions SUBMIT_RULE_OPTIONS_LENIENT =
SubmitRuleOptions.builder().allowClosed(true).build();
SubmitRuleOptions.builder().recomputeOnClosedChanges(true).build();
public static final SubmitRuleOptions SUBMIT_RULE_OPTIONS_STRICT =
SubmitRuleOptions.builder().build();

20
java/com/google/gerrit/server/project/SubmitRuleEvaluator.java
View File

@@ -14,6 +14,7 @@
package com.google.gerrit.server.project;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.gerrit.server.project.ProjectCache.noSuchProject;
import com.google.common.collect.Streams;
@@ -36,7 +37,6 @@ import com.google.inject.assistedinject.Assisted;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
import java.util.stream.Collectors;
/**
* Evaluates a submit-like Prolog rule found in the rules.pl file of the current project and filters
@@ -121,10 +121,18 @@ public class SubmitRuleEvaluator {
return Collections.singletonList(ruleError("Error looking up change " + cd.getId(), e));
}
if ((!opts.allowClosed() || OnlineReindexMode.isActive()) && change.isClosed()) {
SubmitRecord rec = new SubmitRecord();
rec.status = SubmitRecord.Status.CLOSED;
return Collections.singletonList(rec);
if (change.isClosed() && (!opts.recomputeOnClosedChanges() || OnlineReindexMode.isActive())) {
return cd.notes().getSubmitRecords().stream()
.map(
r -> {
SubmitRecord record = r.deepCopy();
if (record.status == SubmitRecord.Status.OK) {
// Submit records that were OK when they got merged are CLOSED now.
record.status = SubmitRecord.Status.CLOSED;
}
return record;
})
.collect(toImmutableList());
}
// We evaluate all the plugin-defined evaluators,
@@ -133,7 +141,7 @@ public class SubmitRuleEvaluator {
.map(c -> c.call(s -> s.evaluate(cd)))
.filter(Optional::isPresent)
.map(Optional::get)
.collect(Collectors.toList());
.collect(toImmutableList());
}
}

9
java/com/google/gerrit/server/project/SubmitRuleOptions.java
View File

@@ -25,7 +25,7 @@ import com.google.auto.value.AutoValue;
@AutoValue
public abstract class SubmitRuleOptions {
private static final SubmitRuleOptions defaults =
new AutoValue_SubmitRuleOptions.Builder().allowClosed(false).build();
new AutoValue_SubmitRuleOptions.Builder().recomputeOnClosedChanges(false).build();
public static SubmitRuleOptions defaults() {
return defaults;
@@ -35,13 +35,16 @@ public abstract class SubmitRuleOptions {
return defaults.toBuilder();
}
public abstract boolean allowClosed();
/**
* True if the submit rules should be recomputed even when the change is already closed (merged).
*/
public abstract boolean recomputeOnClosedChanges();
public abstract Builder toBuilder();
@AutoValue.Builder
public abstract static class Builder {
public abstract SubmitRuleOptions.Builder allowClosed(boolean allowClosed);
public abstract SubmitRuleOptions.Builder recomputeOnClosedChanges(boolean allowClosed);
public abstract SubmitRuleOptions build();
}

7
java/com/google/gerrit/server/query/change/ChangeData.java
View File

@@ -885,7 +885,12 @@ public class ChangeData {
submitRecords.put(options, records);
if (!change().isClosed() && submitRecords.size() == 1) {
// Cache the SubmitRecord with allowClosed = !allowClosed as the SubmitRecord are the same.
submitRecords.put(options.toBuilder().allowClosed(!options.allowClosed()).build(), records);
submitRecords.put(
options
.toBuilder()
.recomputeOnClosedChanges(!options.recomputeOnClosedChanges())
.build(),
records);
}
}
return records;

3
java/com/google/gerrit/server/query/change/OutputStreamQuery.java
View File

@@ -262,7 +262,8 @@ public class OutputStreamQuery {
}
if (includeSubmitRecords) {
SubmitRuleOptions options = SubmitRuleOptions.builder().allowClosed(true).build();
SubmitRuleOptions options =
SubmitRuleOptions.builder().recomputeOnClosedChanges(true).build();
eventFactory.addSubmitRecords(c, submitRuleEvaluatorFactory.create(options).evaluate(d));
}

2
java/com/google/gerrit/server/submit/MergeOp.java
View File

@@ -120,7 +120,7 @@ public class MergeOp implements AutoCloseable {
private static final SubmitRuleOptions SUBMIT_RULE_OPTIONS = SubmitRuleOptions.builder().build();
private static final SubmitRuleOptions SUBMIT_RULE_OPTIONS_ALLOW_CLOSED =
SUBMIT_RULE_OPTIONS.toBuilder().allowClosed(true).build();
SUBMIT_RULE_OPTIONS.toBuilder().recomputeOnClosedChanges(true).build();
public static class CommitStatus {
private final ImmutableMap<Change.Id, ChangeData> changes;

103
javatests/com/google/gerrit/acceptance/api/change/SubmitRuleIT.java Normal file
View File

@@ -0,0 +1,103 @@
// 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.acceptance.api.change;
import static com.google.common.truth.Truth.assertThat;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.entities.LabelFunction;
import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.LabelValue;
import com.google.gerrit.entities.SubmitRecord;
import com.google.gerrit.server.project.SubmitRuleEvaluator;
import com.google.gerrit.server.project.SubmitRuleOptions;
import com.google.inject.Inject;
import java.util.List;
import org.junit.Test;
public class SubmitRuleIT extends AbstractDaemonTest {
@Inject private SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory;
@Test
public void submitRecordsForClosedChanges_parsedBackByDefault() throws Exception {
SubmitRuleEvaluator submitRuleEvaluator =
submitRuleEvaluatorFactory.create(SubmitRuleOptions.defaults());
PushOneCommit.Result r = createChange();
approve(r.getChangeId());
List<SubmitRecord> recordsBeforeSubmission = submitRuleEvaluator.evaluate(r.getChange());
gApi.changes().id(r.getChangeId()).current().submit();
// Add a new label that blocks submission if not granted. In case we reevaluate the rules,
// this would show up as blocking submission.
setupCustomBlockingLabel();
List<SubmitRecord> recordsAfterSubmission = submitRuleEvaluator.evaluate(r.getChange());
recordsBeforeSubmission.forEach(
sr -> sr.status = SubmitRecord.Status.CLOSED); // Set status to closed
assertThat(recordsBeforeSubmission).isEqualTo(recordsAfterSubmission);
}
@Test
public void submitRecordsForClosedChanges_recomputedIfRequested() throws Exception {
SubmitRuleEvaluator submitRuleEvaluator =
submitRuleEvaluatorFactory.create(
SubmitRuleOptions.builder().recomputeOnClosedChanges(true).build());
PushOneCommit.Result r = createChange();
approve(r.getChangeId());
List<SubmitRecord> recordsBeforeSubmission = submitRuleEvaluator.evaluate(r.getChange());
gApi.changes().id(r.getChangeId()).current().submit();
// Add a new label that blocks submission if not granted. In case we reevaluate the rules,
// this would show up as blocking submission.
setupCustomBlockingLabel();
List<SubmitRecord> recordsAfterSubmission = submitRuleEvaluator.evaluate(r.getChange());
assertThat(recordsBeforeSubmission).isNotEqualTo(recordsAfterSubmission);
assertThat(recordsAfterSubmission).hasSize(1);
List<SubmitRecord.Label> recordLabels = recordsAfterSubmission.get(0).labels;
assertThat(recordLabels).hasSize(2);
assertCodeReviewApproved(recordLabels);
assertMyLabelNeed(recordLabels);
}
private void assertCodeReviewApproved(List<SubmitRecord.Label> recordLabels) {
SubmitRecord.Label haveCodeReview = new SubmitRecord.Label();
haveCodeReview.label = "Code-Review";
haveCodeReview.status = SubmitRecord.Label.Status.OK;
haveCodeReview.appliedBy = admin.id();
assertThat(recordLabels).contains(haveCodeReview);
}
private void assertMyLabelNeed(List<SubmitRecord.Label> recordLabels) {
SubmitRecord.Label needCustomLabel = new SubmitRecord.Label();
needCustomLabel.label = "My-Label";
needCustomLabel.status = SubmitRecord.Label.Status.NEED;
assertThat(recordLabels).contains(needCustomLabel);
}
private void setupCustomBlockingLabel() throws Exception {
try (ProjectConfigUpdate u = updateProject(project)) {
u.getConfig()
.upsertLabelType(
LabelType.builder(
"My-Label",
ImmutableList.of(
LabelValue.create((short) 0, "Not approved"),
LabelValue.create((short) 1, "Approved")))
.setFunction(LabelFunction.MAX_WITH_BLOCK)
.build());
u.save();
}
}
}

16
javatests/com/google/gerrit/entities/SubmitRecordTest.java
View File

@@ -16,6 +16,7 @@ package com.google.gerrit.entities;
import static com.google.common.truth.Truth.assertThat;
import com.google.common.collect.ImmutableList;
import java.util.ArrayList;
import java.util.Collection;
import org.junit.Test;
@@ -67,4 +68,19 @@ public class SubmitRecordTest {
assertThat(SubmitRecord.allRecordsOK(submitRecords)).isFalse();
}
@Test
public void deepCopy() {
SubmitRecord record = new SubmitRecord();
record.status = SubmitRecord.Status.CLOSED;
record.errorMessage = "ouch";
record.requirements =
ImmutableList.of(SubmitRequirement.builder().setFallbackText("foo").setType("baz").build());
SubmitRecord.Label label = new SubmitRecord.Label();
label.label = "Code-Review";
record.labels = ImmutableList.of(label);
assertThat(record).isNotSameInstanceAs(record.deepCopy());
assertThat(record).isEqualTo(record.deepCopy());
}
}