Protect writing fixSuggestions in HumanComments by flag
This allows to roll out the new code to production where some jobs
run old binaries who don't understand the new field while others
run the new code.
Forward-Compatible: checked
Release-Notes: skip
Change-Id: I664d80bcacb6fd76796aa857bc9892c43ee7d89d
diff --git a/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java b/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
index 951b6b1..61e3819 100644
--- a/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
+++ b/java/com/google/gerrit/server/experiments/ExperimentFeaturesConstants.java
@@ -59,4 +59,8 @@
/** Whether the rebase submit strategies should rebase merge commits. */
public static final String REBASE_MERGE_COMMITS = "GerritBackendFeature__rebase_merge_commits";
+
+ /** Whether we allow fix suggestions in HumanComments. */
+ public static final String ALLOW_FIX_SUGGESTIONS_IN_COMMENTS =
+ "GerritBackendFeature__allow_fix_suggestions_in_comments";
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeDraftNotesUpdate.java b/java/com/google/gerrit/server/notedb/ChangeDraftNotesUpdate.java
index 60212ac..92758fe 100644
--- a/java/com/google/gerrit/server/notedb/ChangeDraftNotesUpdate.java
+++ b/java/com/google/gerrit/server/notedb/ChangeDraftNotesUpdate.java
@@ -35,6 +35,8 @@
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.config.AllUsersName;
+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.logging.Metadata;
import com.google.gerrit.server.logging.TraceContext;
@@ -221,6 +223,7 @@
}
private final AllUsersName draftsProject;
+ private final ExperimentFeatures experimentFeatures;
private List<HumanComment> put = new ArrayList<>();
private Map<Key, DeleteReason> delete = new HashMap<>();
@@ -231,6 +234,7 @@
@GerritPersonIdent PersonIdent serverIdent,
AllUsersName allUsers,
ChangeNoteUtil noteUtil,
+ ExperimentFeatures experimentFeatures,
@Assisted ChangeNotes notes,
@Assisted("effective") Account.Id accountId,
@Assisted("real") Account.Id realAccountId,
@@ -238,6 +242,7 @@
@Assisted Instant when) {
super(noteUtil, serverIdent, notes, null, accountId, realAccountId, authorIdent, when);
this.draftsProject = allUsers;
+ this.experimentFeatures = experimentFeatures;
}
@AssistedInject
@@ -245,6 +250,7 @@
@GerritPersonIdent PersonIdent serverIdent,
AllUsersName allUsers,
ChangeNoteUtil noteUtil,
+ ExperimentFeatures experimentFeatures,
@Assisted Change change,
@Assisted("effective") Account.Id accountId,
@Assisted("real") Account.Id realAccountId,
@@ -252,6 +258,7 @@
@Assisted Instant when) {
super(noteUtil, serverIdent, null, change, accountId, realAccountId, authorIdent, when);
this.draftsProject = allUsers;
+ this.experimentFeatures = experimentFeatures;
}
@Override
@@ -313,6 +320,7 @@
authorIdent,
draftsProject,
noteUtil,
+ experimentFeatures,
new Change(getChange()),
accountId,
realAccountId,
@@ -330,6 +338,10 @@
RevisionNoteBuilder.Cache cache = new RevisionNoteBuilder.Cache(rnm);
for (HumanComment c : put) {
+ if (!experimentFeatures.isFeatureEnabled(
+ ExperimentFeaturesConstants.ALLOW_FIX_SUGGESTIONS_IN_COMMENTS)) {
+ checkState(c.fixSuggestions == null, "feature flag prohibits setting fixSuggestions");
+ }
if (!delete.keySet().contains(key(c))) {
cache.get(c.getCommitId()).putComment(c);
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/java/com/google/gerrit/server/notedb/ChangeUpdate.java
index 095bb58..2b2fbf9 100644
--- a/java/com/google/gerrit/server/notedb/ChangeUpdate.java
+++ b/java/com/google/gerrit/server/notedb/ChangeUpdate.java
@@ -83,6 +83,8 @@
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.account.ServiceUserClassifier;
import com.google.gerrit.server.approval.PatchSetApprovalUuidGenerator;
+import com.google.gerrit.server.experiments.ExperimentFeatures;
+import com.google.gerrit.server.experiments.ExperimentFeaturesConstants;
import com.google.gerrit.server.git.validators.TopicValidator;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.update.context.RefUpdateContext;
@@ -162,6 +164,7 @@
private final Map<Account.Id, ReviewerStateInternal> reviewers = new LinkedHashMap<>();
private final Map<Address, ReviewerStateInternal> reviewersByEmail = new LinkedHashMap<>();
private final List<HumanComment> comments = new ArrayList<>();
+ private final ExperimentFeatures experimentFeatures;
private String commitSubject;
private String subject;
@@ -213,6 +216,7 @@
ProjectCache projectCache,
ServiceUserClassifier serviceUserClassifier,
PatchSetApprovalUuidGenerator patchSetApprovalUuidGenerator,
+ ExperimentFeatures experimentFeatures,
@Assisted ChangeNotes notes,
@Assisted CurrentUser user,
@Assisted Instant when,
@@ -225,6 +229,7 @@
deleteCommentRewriterFactory,
serviceUserClassifier,
patchSetApprovalUuidGenerator,
+ experimentFeatures,
notes,
user,
when,
@@ -250,6 +255,7 @@
DeleteCommentRewriter.Factory deleteCommentRewriterFactory,
ServiceUserClassifier serviceUserClassifier,
PatchSetApprovalUuidGenerator patchSetApprovalUuidGenerator,
+ ExperimentFeatures experimentFeatures,
@Assisted ChangeNotes notes,
@Assisted CurrentUser user,
@Assisted Instant when,
@@ -262,6 +268,7 @@
this.deleteCommentRewriterFactory = deleteCommentRewriterFactory;
this.serviceUserClassifier = serviceUserClassifier;
this.patchSetApprovalUuidGenerator = patchSetApprovalUuidGenerator;
+ this.experimentFeatures = experimentFeatures;
this.approvals = approvals(labelNameComparator);
this.user = user;
}
@@ -612,6 +619,10 @@
RevisionNoteBuilder.Cache cache = new RevisionNoteBuilder.Cache(rnm);
for (HumanComment c : comments) {
c.tag = tag;
+ if (!experimentFeatures.isFeatureEnabled(
+ ExperimentFeaturesConstants.ALLOW_FIX_SUGGESTIONS_IN_COMMENTS)) {
+ checkState(c.fixSuggestions == null, "feature flag prohibits setting fixSuggestions");
+ }
cache.get(c.getCommitId()).putComment(c);
}
if (submitRequirementResults != null) {
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/CommentWithFixIT.java b/javatests/com/google/gerrit/acceptance/api/revision/CommentWithFixIT.java
index f4fb996..d411a21 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/CommentWithFixIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/CommentWithFixIT.java
@@ -52,6 +52,9 @@
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.testing.BinaryResultSubject;
+import com.google.gerrit.server.experiments.ExperimentFeatures;
+import com.google.gerrit.server.experiments.ExperimentFeaturesConstants;
+import com.google.gerrit.testing.ConfigSuite;
import com.google.gerrit.testing.TestCommentHelper;
import com.google.inject.Inject;
import java.util.Arrays;
@@ -60,6 +63,7 @@
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
+import org.eclipse.jgit.lib.Config;
import org.junit.Before;
import org.junit.Test;
@@ -68,6 +72,7 @@
@Inject private ChangeOperations changeOperations;
@Inject private AccountOperations accountOperations;
@Inject private RequestScopeOperations requestScopeOperations;
+ @Inject private ExperimentFeatures experimentFeatures;
private static final String PLAIN_TEXT_CONTENT_TYPE = "text/plain";
private static final String GERRIT_COMMIT_MESSAGE_TYPE = "text/x-gerrit-commit-message";
@@ -104,6 +109,17 @@
withFixCommentInput = TestCommentHelper.createCommentInput(FILE_NAME, fixSuggestionInfo);
}
+ @ConfigSuite.Default
+ public static Config setExperimentFlag() {
+ Config cfg = new Config();
+ cfg.setString(
+ "experiments",
+ null,
+ "enabled",
+ ExperimentFeaturesConstants.ALLOW_FIX_SUGGESTIONS_IN_COMMENTS);
+ return cfg;
+ }
+
@Test
public void fixSuggestionCannotPointToPatchsetLevel() throws Exception {
CommentInput input = TestCommentHelper.createCommentInput(FILE_NAME);
@@ -1176,6 +1192,18 @@
.containsExactly("Line 3", "Last line", "", footer, "");
}
+ @Test
+ @GerritConfig(
+ name = "experiments.disabled",
+ values = {ExperimentFeaturesConstants.ALLOW_FIX_SUGGESTIONS_IN_COMMENTS})
+ public void commentWithFixFailsToPersistWithoutFeatureFlag() {
+ IllegalStateException thrown =
+ assertThrows(
+ IllegalStateException.class,
+ () -> testCommentHelper.addComment(changeId, withFixCommentInput));
+ assertThat(thrown).hasMessageThat().contains("feature flag prohibits setting fixSuggestions");
+ }
+
private void updateCommitMessage(String changeId, String newCommitMessage) throws Exception {
gApi.changes().id(changeId).edit().create();
gApi.changes().id(changeId).edit().modifyCommitMessage(newCommitMessage);
diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
index 87fc94d..cb29873 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
@@ -33,6 +33,7 @@
import com.google.gerrit.acceptance.ExtensionRegistry;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
import com.google.gerrit.acceptance.testsuite.change.ChangeOperations;
import com.google.gerrit.acceptance.testsuite.change.TestHumanComment;
@@ -64,6 +65,7 @@
import com.google.gerrit.extensions.restapi.TopLevelResource;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.RevisionResource;
+import com.google.gerrit.server.experiments.ExperimentFeaturesConstants;
import com.google.gerrit.server.notedb.ChangeNoteUtil;
import com.google.gerrit.server.notedb.DeleteCommentRewriter;
import com.google.gerrit.server.restapi.change.ChangesCollection;
@@ -156,6 +158,9 @@
}
@Test
+ @GerritConfig(
+ name = "experiments.enabled",
+ values = {ExperimentFeaturesConstants.ALLOW_FIX_SUGGESTIONS_IN_COMMENTS})
public void createDraftWithFixSuggestions() throws Exception {
PushOneCommit.Result r = createChange();
String changeId = r.getChangeId();
@@ -189,6 +194,21 @@
}
@Test
+ public void createDraftWithFixSuggestionsFailsWithoutExperimentFlag() throws Exception {
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+ String revId = r.getCommit().getName();
+ String path = "file1";
+ DraftInput comment = CommentsUtil.newDraft(path, Side.REVISION, 0, "comment 1");
+ comment.fixSuggestions =
+ ImmutableList.of(
+ CommentsUtil.createFixSuggestionInfo(CommentsUtil.createFixReplacementInfo()));
+ IllegalStateException thrown =
+ assertThrows(IllegalStateException.class, () -> addDraft(changeId, revId, comment));
+ assertThat(thrown).hasMessageThat().contains("feature flag prohibits setting fixSuggestions");
+ }
+
+ @Test
public void createDraftWithFixInvalidSuggestions() throws Exception {
PushOneCommit.Result r = createChange();
String changeId = r.getChangeId();
diff --git a/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
index 8b17d94..6118d2e 100644
--- a/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
+++ b/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
@@ -57,6 +57,8 @@
import com.google.gerrit.server.config.GerritImportedServerIds;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.GerritServerId;
+import com.google.gerrit.server.experiments.ConfigExperimentFeatures;
+import com.google.gerrit.server.experiments.ExperimentFeatures;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.GitModule;
import com.google.gerrit.server.git.GitRepositoryManager;
@@ -225,6 +227,7 @@
bind(PatchSetApprovalUuidGenerator.class).to(TestPatchSetApprovalUuidGenerator.class);
bind(ChangeDraftUpdate.ChangeDraftUpdateFactory.class)
.to(ChangeDraftNotesUpdate.Factory.class);
+ bind(ExperimentFeatures.class).to(ConfigExperimentFeatures.class);
}
});
}
