Merge "Support jumping to next/previous file with comments via shortcut"
diff --git a/Documentation/config-validation.txt b/Documentation/config-validation.txt
index cccfe5c..0d0717e 100644
--- a/Documentation/config-validation.txt
+++ b/Documentation/config-validation.txt
@@ -45,6 +45,18 @@
If the commit fails the validation, the plugin can throw an exception
which will cause the merge to fail.
+[[on-submit-validation]]
+== On submit validation
+
+
+Plugins implementing the `OnSubmitValidationListener` interface can
+perform additional validation checks against ref operations resuling
+from execution of submit operation before they are applied to any git
+repositories (there could be more than one in case of topic submits).
+
+Plugin can throw an exception which will cause submit operation to be
+aborted.
+
[[pre-upload-validation]]
== Pre-upload validation
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/GerritServer.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/GerritServer.java
index 4b57132..39d06f3 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/GerritServer.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/GerritServer.java
@@ -25,6 +25,8 @@
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.AsyncReceiveCommits;
import com.google.gerrit.server.ssh.NoSshModule;
+import com.google.gerrit.server.util.ManualRequestContext;
+import com.google.gerrit.server.util.OneOffRequestContext;
import com.google.gerrit.server.util.SocketUtil;
import com.google.gerrit.server.util.SystemLog;
import com.google.gerrit.testutil.FakeEmailSender;
@@ -62,6 +64,7 @@
static Description forTestClass(org.junit.runner.Description testDesc,
String configName) {
return new AutoValue_GerritServer_Description(
+ testDesc,
configName,
true, // @UseLocalDisk is only valid on methods.
!has(NoHttpd.class, testDesc.getTestClass()),
@@ -75,6 +78,7 @@
static Description forTestMethod(org.junit.runner.Description testDesc,
String configName) {
return new AutoValue_GerritServer_Description(
+ testDesc,
configName,
testDesc.getAnnotation(UseLocalDisk.class) == null,
testDesc.getAnnotation(NoHttpd.class) == null
@@ -97,6 +101,7 @@
return false;
}
+ abstract org.junit.runner.Description testDescription();
@Nullable abstract String configName();
abstract boolean memory();
abstract boolean httpd();
@@ -297,10 +302,7 @@
void stop() throws Exception {
try {
- if (NoteDbMode.get().equals(NoteDbMode.CHECK)) {
- testInjector.getInstance(NoteDbChecker.class)
- .rebuildAndCheckAllChanges();
- }
+ checkNoteDbState();
} finally {
daemon.getLifecycleManager().stop();
if (daemonService != null) {
@@ -312,6 +314,23 @@
}
}
+ private void checkNoteDbState() throws Exception {
+ NoteDbMode mode = NoteDbMode.get();
+ if (mode != NoteDbMode.CHECK && mode != NoteDbMode.PRIMARY) {
+ return;
+ }
+ NoteDbChecker checker = testInjector.getInstance(NoteDbChecker.class);
+ OneOffRequestContext oneOffRequestContext =
+ testInjector.getInstance(OneOffRequestContext.class);
+ try (ManualRequestContext ctx = oneOffRequestContext.open()) {
+ if (mode == NoteDbMode.CHECK) {
+ checker.rebuildAndCheckAllChanges();
+ } else if (mode == NoteDbMode.PRIMARY) {
+ checker.assertNoReviewDbChanges(desc.testDescription());
+ }
+ }
+ }
+
@Override
public String toString() {
return MoreObjects.toStringHelper(this).addValue(desc).toString();
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 02d74bf..1e12638 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -99,6 +99,7 @@
import com.google.gerrit.server.git.ChangeMessageModifier;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.group.SystemGroupBackend;
+import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.Util;
import com.google.gerrit.testutil.FakeEmailSender.Message;
@@ -1011,6 +1012,8 @@
public void addReviewerWithNoteDbWhenDummyApprovalInReviewDbExists()
throws Exception {
assume().that(notesMigration.enabled()).isTrue();
+ assume().that(notesMigration.changePrimaryStorage())
+ .isEqualTo(PrimaryStorage.REVIEW_DB);
PushOneCommit.Result r = createChange();
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/RefAdvertisementIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
index 521ccc4..a7002fc 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
@@ -43,6 +43,7 @@
import com.google.gerrit.server.git.TagCache;
import com.google.gerrit.server.git.VisibleRefFilter;
import com.google.gerrit.server.notedb.ChangeNoteUtil;
+import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.project.Util;
import com.google.gerrit.server.query.change.ChangeData;
@@ -447,8 +448,6 @@
@Test
public void receivePackOmitsMissingObject() throws Exception {
- // Use the tactic from ConsistencyCheckerIT to insert a new patch set with a
- // missing object.
String rev = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef";
try (Repository repo = repoManager.openRepository(project)) {
TestRepository<?> tr = new TestRepository<>(repo);
@@ -457,9 +456,11 @@
PatchSet.Id psId = new PatchSet.Id(c3.getId(), 2);
c.setCurrentPatchSet(psId, subject, c.getOriginalSubject());
- PatchSet ps = TestChanges.newPatchSet(psId, rev, admin.getId());
- db.patchSets().insert(Collections.singleton(ps));
- db.changes().update(Collections.singleton(c));
+ if (notesMigration.changePrimaryStorage() == PrimaryStorage.REVIEW_DB) {
+ PatchSet ps = TestChanges.newPatchSet(psId, rev, admin.getId());
+ db.patchSets().insert(Collections.singleton(ps));
+ db.changes().update(Collections.singleton(c));
+ }
if (notesMigration.commitChangeWrites()) {
PersonIdent committer = serverIdent.get();
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
index 45da3d8..2a39ffd 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
@@ -29,6 +29,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
@@ -45,6 +46,8 @@
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.LabelInfo;
+import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.extensions.registration.RegistrationHandle;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
@@ -56,6 +59,7 @@
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.change.RevisionResource;
@@ -63,8 +67,10 @@
import com.google.gerrit.server.git.BatchUpdate;
import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
import com.google.gerrit.server.git.ProjectConfig;
+import com.google.gerrit.server.git.validators.OnSubmitValidationListener;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.project.Util;
+import com.google.gerrit.server.validators.ValidationException;
import com.google.gerrit.testutil.ConfigSuite;
import com.google.gerrit.testutil.TestTimeUtil;
import com.google.gwtorm.server.OrmException;
@@ -86,9 +92,13 @@
import org.junit.Test;
import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.util.ArrayList;
import java.util.HashMap;
+import java.util.HashSet;
import java.util.List;
import java.util.Map;
+import java.util.concurrent.atomic.AtomicBoolean;
import java.util.stream.Collectors;
@NoHttpd
@@ -110,6 +120,10 @@
@Inject
private BatchUpdate.Factory updateFactory;
+ @Inject
+ private DynamicSet<OnSubmitValidationListener> onSubmitValidationListeners;
+ private RegistrationHandle onSubmitValidatorHandle;
+
private String systemTimeZone;
@Before
@@ -129,6 +143,13 @@
db.close();
}
+ @After
+ public void removeOnSubmitValidator(){
+ if (onSubmitValidatorHandle != null){
+ onSubmitValidatorHandle.remove();
+ }
+ }
+
protected abstract SubmitType getSubmitType();
@Test
@@ -647,6 +668,94 @@
assertThat(getRemoteHead()).isEqualTo(headAfterSubmit);
}
+ @Test
+ public void submitWithValidation() throws Exception {
+ AtomicBoolean called = new AtomicBoolean(false);
+ this.addOnSubmitValidationListener(new OnSubmitValidationListener() {
+ @Override
+ public void preBranchUpdate(Arguments args) throws ValidationException {
+ called.set(true);
+ HashSet<String> refs = Sets.newHashSet(args.getCommands().keySet());
+ assertThat(refs).contains("refs/heads/master");
+ refs.remove("refs/heads/master");
+ if (!refs.isEmpty()){
+ // Some submit strategies need to insert new patchset.
+ assertThat(refs).hasSize(1);
+ assertThat(refs.iterator().next()).startsWith(RefNames.REFS_CHANGES);
+ }
+ }
+ });
+
+ PushOneCommit.Result change = createChange();
+ approve(change.getChangeId());
+ submit(change.getChangeId());
+ assertThat(called.get()).isTrue();
+ }
+
+ @Test
+ public void submitWithValidationMultiRepo() throws Exception {
+ assume().that(isSubmitWholeTopicEnabled()).isTrue();
+ String topic = "test-topic";
+
+ // Create test projects
+ TestRepository<?> repoA =
+ createProjectWithPush("project-a", null, getSubmitType());
+ TestRepository<?> repoB =
+ createProjectWithPush("project-b", null, getSubmitType());
+
+ // Create changes on project-a
+ PushOneCommit.Result change1 =
+ createChange(repoA, "master", "Change 1", "a.txt", "content", topic);
+ PushOneCommit.Result change2 =
+ createChange(repoA, "master", "Change 2", "b.txt", "content", topic);
+
+ // Create changes on project-b
+ PushOneCommit.Result change3 =
+ createChange(repoB, "master", "Change 3", "a.txt", "content", topic);
+ PushOneCommit.Result change4 =
+ createChange(repoB, "master", "Change 4", "b.txt", "content", topic);
+
+ List<PushOneCommit.Result> changes =
+ Lists.newArrayList(change1, change2, change3, change4);
+ for (PushOneCommit.Result change : changes) {
+ approve(change.getChangeId());
+ }
+
+ // Construct validator which will throw on a second call.
+ // Since there are 2 repos, first submit attempt will fail, the second will
+ // succeed.
+ List<String> projectsCalled = new ArrayList<>(4);
+ this.addOnSubmitValidationListener(new OnSubmitValidationListener() {
+ @Override
+ public void preBranchUpdate(Arguments args) throws ValidationException {
+ assertThat(args.getCommands().keySet()).contains("refs/heads/master");
+ try (RevWalk rw = args.newRevWalk()) {
+ rw.parseBody(rw.parseCommit(
+ args.getCommands().get("refs/heads/master").getNewId()));
+ } catch (IOException e) {
+ assertThat(e).isNull();
+ }
+ projectsCalled.add(args.getProject().get());
+ if (projectsCalled.size() == 2) {
+ throw new ValidationException("time to fail");
+ }
+ }
+ });
+ submitWithConflict(change4.getChangeId(), "time to fail");
+ assertThat(projectsCalled).containsExactly(name("project-a"),
+ name("project-b"));
+ for (PushOneCommit.Result change : changes) {
+ change.assertChange(Change.Status.NEW, name(topic), admin);
+ }
+
+ submit(change4.getChangeId());
+ assertThat(projectsCalled).containsExactly(name("project-a"),
+ name("project-b"), name("project-a"), name("project-b"));
+ for (PushOneCommit.Result change : changes) {
+ change.assertChange(Change.Status.MERGED, name(topic), admin);
+ }
+ }
+
private void setChangeStatusToNew(PushOneCommit.Result... changes)
throws Exception {
for (PushOneCommit.Result change : changes) {
@@ -881,6 +990,11 @@
return getRemoteLog(project, "master");
}
+ protected void addOnSubmitValidationListener(OnSubmitValidationListener listener){
+ assertThat(onSubmitValidatorHandle).isNull();
+ onSubmitValidatorHandle = onSubmitValidationListeners.add(listener);
+ }
+
private String getLatestDiff(Repository repo) throws Exception {
ObjectId oldTreeId = repo.resolve("HEAD~1^{tree}");
ObjectId newTreeId = repo.resolve("HEAD^{tree}");
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java
new file mode 100644
index 0000000..c36c44b
--- /dev/null
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DeleteVoteIT.java
@@ -0,0 +1,109 @@
+// Copyright (C) 2017 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.rest.change;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.extensions.client.ReviewerState.REVIEWER;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
+import com.google.gson.reflect.TypeToken;
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.RestResponse;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.common.AccountInfo;
+import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.ChangeMessageInfo;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.testutil.FakeEmailSender;
+
+import org.junit.Test;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+
+public class DeleteVoteIT extends AbstractDaemonTest {
+ @Test
+ public void deleteVoteOnChange() throws Exception {
+ deleteVote(false);
+ }
+
+ @Test
+ public void deleteVoteOnRevision() throws Exception {
+ deleteVote(true);
+ }
+
+ private void deleteVote(boolean onRevisionLevel) throws Exception {
+ PushOneCommit.Result r = createChange();
+ gApi.changes()
+ .id(r.getChangeId())
+ .revision(r.getCommit().name())
+ .review(ReviewInput.approve());
+
+ PushOneCommit.Result r2 = amendChange(r.getChangeId());
+
+ setApiUser(user);
+ recommend(r.getChangeId());
+
+ sender.clear();
+ String endPoint = "/changes/" + r.getChangeId()
+ + (onRevisionLevel ? ("/revisions/" + r2.getCommit().getName()) : "")
+ + "/reviewers/" + user.getId().toString()
+ + "/votes/Code-Review";
+
+ RestResponse response = adminRestSession.delete(endPoint);
+ response.assertNoContent();
+
+ List<FakeEmailSender.Message> messages = sender.getMessages();
+ assertThat(messages).hasSize(1);
+ FakeEmailSender.Message msg = messages.get(0);
+ assertThat(msg.rcpt()).containsExactly(user.emailAddress);
+ assertThat(msg.body()).contains(
+ admin.fullName + " has removed a vote on this change.\n");
+ assertThat(msg.body()).contains("Removed Code-Review+1 by "
+ + user.fullName + " <" + user.email + ">" + "\n");
+
+ endPoint = "/changes/" + r.getChangeId()
+ + (onRevisionLevel ? ("/revisions/" + r2.getCommit().getName()) : "")
+ + "/reviewers/" + user.getId().toString()
+ + "/votes";
+
+ response = adminRestSession.get(endPoint);
+ response.assertOK();
+
+ Map<String, Short> m = newGson().fromJson(response.getReader(),
+ new TypeToken<Map<String, Short>>() {}.getType());
+
+ assertThat(m).containsExactly("Code-Review", Short.valueOf((short)0));
+
+ ChangeInfo c = gApi.changes()
+ .id(r.getChangeId())
+ .get();
+
+ ChangeMessageInfo message = Iterables.getLast(c.messages);
+ assertThat(message.author._accountId).isEqualTo(admin.getId().get());
+ assertThat(message.message).isEqualTo(
+ "Removed Code-Review+1 by User <user@example.com>\n");
+ assertThat(getReviewers(c.reviewers.get(REVIEWER)))
+ .containsExactlyElementsIn(
+ ImmutableSet.of(admin.getId(), user.getId()));
+ }
+
+ private Iterable<Account.Id> getReviewers(Collection<AccountInfo> r) {
+ return Iterables.transform(r, a -> new Account.Id(a._accountId));
+ }
+}
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java
index e33d163..fced72e 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java
@@ -50,6 +50,7 @@
import com.google.gerrit.server.git.BatchUpdate.RepoContext;
import com.google.gerrit.server.git.validators.CommitValidators;
import com.google.gerrit.server.notedb.ChangeNoteUtil;
+import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.testutil.InMemoryRepositoryManager;
import com.google.gerrit.testutil.TestChanges;
@@ -297,8 +298,10 @@
String rev = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef";
PatchSet ps = newPatchSet(psId, rev, adminId);
- db.changes().insert(singleton(c));
- db.patchSets().insert(singleton(ps));
+ if (notesMigration.changePrimaryStorage() == PrimaryStorage.REVIEW_DB) {
+ db.changes().insert(singleton(c));
+ db.patchSets().insert(singleton(ps));
+ }
addNoteDbCommit(
c.getId(),
"Create change\n"
@@ -824,10 +827,12 @@
Change c = new Change(ctl.getChange());
PatchSet.Id psId = nextPatchSetId(ctl);
c.setCurrentPatchSet(psId, subject, c.getOriginalSubject());
-
PatchSet ps = newPatchSet(psId, rev, adminId);
- db.patchSets().insert(singleton(ps));
- db.changes().update(singleton(c));
+
+ if (PrimaryStorage.of(c) == PrimaryStorage.REVIEW_DB) {
+ db.patchSets().insert(singleton(ps));
+ db.changes().update(singleton(c));
+ }
addNoteDbCommit(
c.getId(),
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/ExternalIdCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/ExternalIdCacheImpl.java
index 8585ffd..e55b15a 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/ExternalIdCacheImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/ExternalIdCacheImpl.java
@@ -160,7 +160,7 @@
}
}
- static class AllKey {
+ public static class AllKey {
static final AllKey ALL = new AllKey();
private AllKey() {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/SuggestChangeReviewers.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/SuggestChangeReviewers.java
index 131513b..0753769 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/SuggestChangeReviewers.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/SuggestChangeReviewers.java
@@ -15,10 +15,12 @@
package com.google.gerrit.server.change;
import com.google.gerrit.extensions.common.SuggestedReviewerInfo;
+import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.server.ReviewDb;
+import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.IdentifiedUser.GenericFactory;
import com.google.gerrit.server.ReviewersUtil;
@@ -42,18 +44,25 @@
usage = "exclude groups from query")
boolean excludeGroups;
+ private final Provider<CurrentUser> self;
+
@Inject
SuggestChangeReviewers(AccountVisibility av,
GenericFactory identifiedUserFactory,
Provider<ReviewDb> dbProvider,
+ Provider<CurrentUser> self,
@GerritServerConfig Config cfg,
ReviewersUtil reviewersUtil) {
super(av, identifiedUserFactory, dbProvider, cfg, reviewersUtil);
+ this.self = self;
}
@Override
public List<SuggestedReviewerInfo> apply(ChangeResource rsrc)
- throws BadRequestException, OrmException, IOException {
+ throws AuthException, BadRequestException, OrmException, IOException {
+ if (!self.get().isIdentifiedUser()) {
+ throw new AuthException("Authentication required");
+ }
return reviewersUtil.suggestReviewers(rsrc.getNotes(), this,
rsrc.getControl().getProjectControl(), getVisibility(rsrc), excludeGroups);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java
index df96a54..6012142 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -128,6 +128,8 @@
import com.google.gerrit.server.git.validators.MergeValidators.ProjectConfigValidator;
import com.google.gerrit.server.git.validators.RefOperationValidationListener;
import com.google.gerrit.server.git.validators.RefOperationValidators;
+import com.google.gerrit.server.git.validators.OnSubmitValidationListener;
+import com.google.gerrit.server.git.validators.OnSubmitValidators;
import com.google.gerrit.server.git.validators.UploadValidationListener;
import com.google.gerrit.server.git.validators.UploadValidators;
import com.google.gerrit.server.group.GroupInfoCache;
@@ -350,6 +352,7 @@
DynamicSet.setOf(binder(), CommitValidationListener.class);
DynamicSet.setOf(binder(), ChangeMessageModifier.class);
DynamicSet.setOf(binder(), RefOperationValidationListener.class);
+ DynamicSet.setOf(binder(), OnSubmitValidationListener.class);
DynamicSet.setOf(binder(), MergeValidationListener.class);
DynamicSet.setOf(binder(), ProjectCreationValidationListener.class);
DynamicSet.setOf(binder(), GroupCreationValidationListener.class);
@@ -391,6 +394,7 @@
factory(AbandonOp.Factory.class);
factory(RefOperationValidators.Factory.class);
+ factory(OnSubmitValidators.Factory.class);
factory(MergeValidators.Factory.class);
factory(ProjectConfigValidator.Factory.class);
factory(NotesBranchUtil.Factory.class);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java
index 5de4ff5..7419dab 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java
@@ -51,6 +51,7 @@
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
+import com.google.gerrit.server.git.validators.OnSubmitValidators;
import com.google.gerrit.server.index.change.ChangeIndexer;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
@@ -532,6 +533,7 @@
private BatchRefUpdate batchRefUpdate;
private boolean closeRepo;
private Order order;
+ private OnSubmitValidators onSubmitValidators;
private boolean updateChangesInParallel;
private RequestId requestId;
@@ -607,6 +609,15 @@
}
/**
+ * Add a validation step for intended ref operations, which will be performed
+ * at the end of {@link RepoOnlyOp#updateRepo(RepoContext)} step.
+ */
+ BatchUpdate setOnSubmitValidators(OnSubmitValidators onSubmitValidators) {
+ this.onSubmitValidators = onSubmitValidators;
+ return this;
+ }
+
+ /**
* Execute {@link Op#updateChange(ChangeContext)} in parallel for each change.
*/
public BatchUpdate updateChangesInParallel() {
@@ -692,6 +703,18 @@
op.updateRepo(ctx);
}
+ if (onSubmitValidators != null && commands != null
+ && !commands.isEmpty()) {
+ // Validation of refs has to take place here and not at the beginning
+ // executeRefUpdates. Otherwise failing validation in a second
+ // BatchUpdate object will happen *after* first object's
+ // executeRefUpdates has finished, hence after first repo's refs have
+ // been updated, which is too late.
+ onSubmitValidators.validate(project,
+ new ReadOnlyRepository(getRepository()),
+ ctx.getInserter().newReader(), commands.getCommands());
+ }
+
if (inserter != null) {
logDebug("Flushing inserter");
inserter.flush();
@@ -1049,18 +1072,45 @@
private ChangeContext newChangeContext(ReviewDb db, Repository repo,
RevWalk rw, Change.Id id) throws OrmException {
Change c = newChanges.get(id);
- if (c == null) {
+ boolean isNew = c != null;
+ PrimaryStorage defaultStorage = notesMigration.changePrimaryStorage();
+ if (isNew) {
+ // New change: populate noteDbState.
+ checkState(c.getNoteDbState() == null,
+ "noteDbState should not be filled in by callers");
+ if (defaultStorage == PrimaryStorage.NOTE_DB) {
+ c.setNoteDbState(NoteDbChangeState.NOTE_DB_PRIMARY_STATE);
+ }
+ } else {
+ // Existing change.
c = ChangeNotes.readOneReviewDbChange(db, id);
if (c == null) {
- logDebug("Failed to get change {} from unwrapped db", id);
- throw new NoSuchChangeException(id);
+ if (defaultStorage == PrimaryStorage.REVIEW_DB) {
+ logDebug("Failed to get change {} from unwrapped db", id);
+ throw new NoSuchChangeException(id);
+ }
+ // Not in ReviewDb, but new changes are created with default primary
+ // storage as NOTE_DB, so we can assume that a missing change is
+ // NoteDb primary. Pass a synthetic change into ChangeNotes.Factory,
+ // which lets ChangeNotes take care of the existence check.
+ //
+ // TODO(dborowitz): This assumption is potentially risky, because
+ // it means once we turn this option on and start creating changes
+ // without writing anything to ReviewDb, we can't turn this option
+ // back off without making those changes inaccessible. The problem
+ // is we have no way of distinguishing a change that only exists in
+ // NoteDb because it only ever existed in NoteDb, from a change that
+ // only exists in NoteDb because it used to exist in ReviewDb and
+ // deleting from ReviewDb succeeded but deleting from NoteDb failed.
+ //
+ // TODO(dborowitz): We actually still have that problem anyway. Maybe
+ // we need a cutoff timestamp? Or maybe we need to start leaving
+ // tombstones in ReviewDb?
+ c = ChangeNotes.Factory.newNoteDbOnlyChange(project, id);
}
NoteDbChangeState.checkNotReadOnly(c, skewMs);
}
- // Pass in preloaded change to controlFor, to avoid:
- // - reading from a db that does not belong to this update
- // - attempting to read a change that doesn't exist yet
- ChangeNotes notes = changeNotesFactory.createForBatchUpdate(c);
+ ChangeNotes notes = changeNotesFactory.createForBatchUpdate(c, !isNew);
ChangeControl ctl = changeControlFactory.controlFor(notes, user);
return new ChangeContext(ctl, new BatchUpdateReviewDb(db), repo, rw);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOpRepoManager.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOpRepoManager.java
index 5196ebe..1244ad3 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOpRepoManager.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOpRepoManager.java
@@ -22,6 +22,7 @@
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
+import com.google.gerrit.server.git.validators.OnSubmitValidators;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
@@ -107,7 +108,8 @@
if (update == null) {
update = batchUpdateFactory.create(db, getProjectName(), caller, ts)
.setRepository(repo, rw, ins)
- .setRequestId(submissionId);
+ .setRequestId(submissionId)
+ .setOnSubmitValidators(onSubmitValidatorsFactory.create());
}
return update;
}
@@ -157,6 +159,7 @@
private final Map<Project.NameKey, OpenRepo> openRepos;
private final BatchUpdate.Factory batchUpdateFactory;
+ private final OnSubmitValidators.Factory onSubmitValidatorsFactory;
private final GitRepositoryManager repoManager;
private final ProjectCache projectCache;
@@ -169,10 +172,12 @@
MergeOpRepoManager(
GitRepositoryManager repoManager,
ProjectCache projectCache,
- BatchUpdate.Factory batchUpdateFactory) {
+ BatchUpdate.Factory batchUpdateFactory,
+ OnSubmitValidators.Factory onSubmitValidatorsFactory) {
this.repoManager = repoManager;
this.projectCache = projectCache;
this.batchUpdateFactory = batchUpdateFactory;
+ this.onSubmitValidatorsFactory = onSubmitValidatorsFactory;
openRepos = new HashMap<>();
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java
index e0e2ae7..692bc98 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java
@@ -46,6 +46,7 @@
import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.git.SubmoduleOp;
import com.google.gerrit.server.git.TagCache;
+import com.google.gerrit.server.git.validators.OnSubmitValidators;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.ProjectCache;
@@ -118,6 +119,7 @@
final ProjectCache projectCache;
final PersonIdent serverIdent;
final RebaseChangeOp.Factory rebaseFactory;
+ final OnSubmitValidators.Factory onSubmitValidatorsFactory;
final TagCache tagCache;
final Branch.NameKey destBranch;
@@ -158,6 +160,7 @@
@GerritPersonIdent PersonIdent serverIdent,
ProjectCache projectCache,
RebaseChangeOp.Factory rebaseFactory,
+ OnSubmitValidators.Factory onSubmitValidatorsFactory,
TagCache tagCache,
@Assisted Branch.NameKey destBranch,
@Assisted CommitStatus commits,
@@ -212,6 +215,7 @@
"project not found: %s", destBranch.getParentKey());
this.mergeSorter = new MergeSorter(rw, alreadyAccepted, canMergeFlag);
this.mergeUtil = mergeUtilFactory.create(project);
+ this.onSubmitValidatorsFactory = onSubmitValidatorsFactory;
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/OnSubmitValidationListener.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/OnSubmitValidationListener.java
new file mode 100644
index 0000000..f2f397c
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/OnSubmitValidationListener.java
@@ -0,0 +1,85 @@
+// Copyright (C) 2017 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.git.validators;
+
+import com.google.gerrit.extensions.annotations.ExtensionPoint;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.reviewdb.client.Project.NameKey;
+import com.google.gerrit.server.validators.ValidationException;
+
+import org.eclipse.jgit.lib.ObjectReader;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.transport.ReceiveCommand;
+
+import java.util.Map;
+
+/**
+ * Listener to validate ref updates performed during submit operation.
+ *
+ * As submit strategies may generate new commits (e.g. Cherry Pick), this
+ * listener allows validation of resulting new commit before destination branch
+ * is updated and new patchset ref is created.
+ *
+ * If you only care about validating the change being submitted and not the
+ * resulting new commit, consider using {@link MergeValidationListener} instead.
+ */
+@ExtensionPoint
+public interface OnSubmitValidationListener {
+ public class Arguments {
+ private Project.NameKey project;
+ private Repository repository;
+ private ObjectReader objectReader;
+ private Map<String, ReceiveCommand> commands;
+
+ public Arguments(NameKey project, Repository repository,
+ ObjectReader objectReader, Map<String, ReceiveCommand> commands) {
+ this.project = project;
+ this.repository = repository;
+ this.objectReader = objectReader;
+ this.commands = commands;
+ }
+
+ public Project.NameKey getProject() {
+ return project;
+ }
+
+ /**
+ * @return a read only repository
+ */
+ public Repository getRepository() {
+ return repository;
+ }
+
+ public RevWalk newRevWalk() {
+ return new RevWalk(objectReader);
+ }
+
+ /**
+ * @return a map from ref to op on it covering all ref ops to be performed
+ * on this repository as part of ongoing submit operation.
+ */
+ public Map<String, ReceiveCommand> getCommands(){
+ return commands;
+ }
+ }
+
+ /**
+ * Called right before branch is updated with new commit or commits as a
+ * result of submit.
+ *
+ * If ValidationException is thrown, submitting is aborted.
+ */
+ void preBranchUpdate(Arguments args) throws ValidationException;
+}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/OnSubmitValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/OnSubmitValidators.java
new file mode 100644
index 0000000..568c597
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/OnSubmitValidators.java
@@ -0,0 +1,53 @@
+// Copyright (C) 2017 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.git.validators;
+
+import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.git.IntegrationException;
+import com.google.gerrit.server.git.validators.OnSubmitValidationListener.Arguments;
+import com.google.gerrit.server.validators.ValidationException;
+import com.google.inject.Inject;
+
+import org.eclipse.jgit.lib.ObjectReader;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.transport.ReceiveCommand;
+
+import java.util.Map;
+
+public class OnSubmitValidators {
+ public interface Factory {
+ OnSubmitValidators create();
+ }
+
+ private final DynamicSet<OnSubmitValidationListener> listeners;
+
+ @Inject
+ OnSubmitValidators(DynamicSet<OnSubmitValidationListener> listeners) {
+ this.listeners = listeners;
+ }
+
+ public void validate(Project.NameKey project, Repository repo,
+ ObjectReader objectReader, Map<String, ReceiveCommand> commands)
+ throws IntegrationException {
+ try {
+ for (OnSubmitValidationListener listener : this.listeners) {
+ listener.preBranchUpdate(
+ new Arguments(project, repo, objectReader, commands));
+ }
+ } catch (ValidationException e) {
+ throw new IntegrationException(e.getMessage());
+ }
+ }
+}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
index c00035c..c58d3fe 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
@@ -29,6 +29,7 @@
import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder;
+import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -174,7 +175,20 @@
return ref != null ? ref.getObjectId() : null;
}
- protected LoadHandle openHandle(Repository repo) throws IOException {
+ /**
+ * Open a handle for reading this entity from a repository.
+ * <p>
+ * Implementations may override this method to provide auto-rebuilding
+ * behavior.
+ *
+ * @param repo open repository.
+ * @return handle for reading the entity.
+ *
+ * @throws NoSuchChangeException change does not exist.
+ * @throws IOException a repo-level error occurred.
+ */
+ protected LoadHandle openHandle(Repository repo)
+ throws NoSuchChangeException, IOException {
return openHandle(repo, readRef(repo));
}
@@ -182,7 +196,7 @@
return LoadHandle.create(ChangeNotesCommit.newRevWalk(repo), id);
}
- public T reload() throws OrmException {
+ public T reload() throws NoSuchChangeException, OrmException {
loaded = false;
return load();
}
@@ -215,7 +229,7 @@
/** Set up the metadata, parsing any state from the loaded revision. */
protected abstract void onLoad(LoadHandle handle)
- throws IOException, ConfigInvalidException;
+ throws NoSuchChangeException, IOException, ConfigInvalidException;
@SuppressWarnings("unchecked")
protected final T self() {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
index 495641d..ee9f4f7 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -36,6 +36,7 @@
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.metrics.Timer1;
import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.Comment;
@@ -124,7 +125,14 @@
public ChangeNotes createChecked(ReviewDb db, Project.NameKey project,
Change.Id changeId) throws OrmException {
Change change = readOneReviewDbChange(db, changeId);
- if (change == null || !change.getProject().equals(project)) {
+ if (change == null) {
+ if (!args.migration.readChanges()) {
+ throw new NoSuchChangeException(changeId);
+ }
+ // Change isn't in ReviewDb, but its primary storage might be in NoteDb.
+ // Prepopulate the change exists with proper noteDbState field.
+ change = newNoteDbOnlyChange(project, changeId);
+ } else if (!change.getProject().equals(project)) {
throw new NoSuchChangeException(changeId);
}
return new ChangeNotes(args, change).load();
@@ -144,16 +152,33 @@
return changes.get(0).notes();
}
+ public static Change newNoteDbOnlyChange(
+ Project.NameKey project, Change.Id changeId) {
+ Change change = new Change(
+ null, changeId, null,
+ new Branch.NameKey(project, "INVALID_NOTE_DB_ONLY"),
+ null);
+ change.setNoteDbState(NoteDbChangeState.NOTE_DB_PRIMARY_STATE);
+ return change;
+ }
+
private Change loadChangeFromDb(ReviewDb db, Project.NameKey project,
Change.Id changeId) throws OrmException {
- Change change = readOneReviewDbChange(db, changeId);
checkArgument(project != null, "project is required");
- checkNotNull(change,
- "change %s not found in ReviewDb", changeId);
- checkArgument(change.getProject().equals(project),
- "passed project %s when creating ChangeNotes for %s, but actual"
- + " project is %s",
- project, changeId, change.getProject());
+ Change change = readOneReviewDbChange(db, changeId);
+
+ if (change == null && args.migration.readChanges()) {
+ // Change isn't in ReviewDb, but its primary storage might be in NoteDb.
+ // Prepopulate the change exists with proper noteDbState field.
+ change = newNoteDbOnlyChange(project, changeId);
+ } else {
+ checkNotNull(change, "change %s not found in ReviewDb", changeId);
+ checkArgument(change.getProject().equals(project),
+ "passed project %s when creating ChangeNotes for %s, but actual"
+ + " project is %s",
+ project, changeId, change.getProject());
+ }
+
// TODO: Throw NoSuchChangeException when the change is not found in the
// database
return change;
@@ -168,7 +193,7 @@
public ChangeNotes createWithAutoRebuildingDisabled(ReviewDb db,
Project.NameKey project, Change.Id changeId) throws OrmException {
return new ChangeNotes(
- args, loadChangeFromDb(db, project, changeId), false, null).load();
+ args, loadChangeFromDb(db, project, changeId), true, false, null).load();
}
/**
@@ -183,13 +208,14 @@
return new ChangeNotes(args, change);
}
- public ChangeNotes createForBatchUpdate(Change change) throws OrmException {
- return new ChangeNotes(args, change, false, null).load();
+ public ChangeNotes createForBatchUpdate(Change change, boolean shouldExist)
+ throws OrmException {
+ return new ChangeNotes(args, change, shouldExist, false, null).load();
}
public ChangeNotes createWithAutoRebuildingDisabled(Change change,
RefCache refs) throws OrmException {
- return new ChangeNotes(args, change, false, refs).load();
+ return new ChangeNotes(args, change, true, false, refs).load();
}
// TODO(ekempin): Remove when database backend is deleted
@@ -226,7 +252,7 @@
public List<ChangeNotes> create(ReviewDb db, Project.NameKey project,
Collection<Change.Id> changeIds, Predicate<ChangeNotes> predicate)
- throws OrmException {
+ throws OrmException {
List<ChangeNotes> notes = new ArrayList<>();
if (args.migration.enabled()) {
for (Change.Id cid : changeIds) {
@@ -302,13 +328,18 @@
Project.NameKey project) throws OrmException, IOException {
Set<Change.Id> ids = scan(repo);
List<ChangeNotes> changeNotes = new ArrayList<>(ids.size());
+ PrimaryStorage defaultStorage = args.migration.changePrimaryStorage();
for (Change.Id id : ids) {
Change change = readOneReviewDbChange(db, id);
if (change == null) {
- log.warn("skipping change {} found in project {} " +
- "but not in ReviewDb",
- id, project);
- continue;
+ if (defaultStorage == PrimaryStorage.REVIEW_DB) {
+ log.warn("skipping change {} found in project {} " +
+ "but not in ReviewDb",
+ id, project);
+ continue;
+ }
+ // TODO(dborowitz): See discussion in BatchUpdate#newChangeContext.
+ change = newNoteDbOnlyChange(project, id);
} else if (!change.getProject().equals(project)) {
log.error(
"skipping change {} found in project {} " +
@@ -337,6 +368,7 @@
}
}
+ private final boolean shouldExist;
private final RefCache refs;
private Change change;
@@ -358,13 +390,14 @@
@VisibleForTesting
public ChangeNotes(Args args, Change change) {
- this(args, change, true, null);
+ this(args, change, true, true, null);
}
- private ChangeNotes(Args args, Change change, boolean autoRebuild,
- @Nullable RefCache refs) {
+ private ChangeNotes(Args args, Change change, boolean shouldExist,
+ boolean autoRebuild, @Nullable RefCache refs) {
super(args, change.getId(), PrimaryStorage.of(change), autoRebuild);
this.change = new Change(change);
+ this.shouldExist = shouldExist;
this.refs = refs;
}
@@ -555,9 +588,14 @@
@Override
protected void onLoad(LoadHandle handle)
- throws IOException, ConfigInvalidException {
+ throws NoSuchChangeException, IOException, ConfigInvalidException {
ObjectId rev = handle.id();
if (rev == null) {
+ if (args.migration.readChanges()
+ && PrimaryStorage.of(change) == PrimaryStorage.NOTE_DB
+ && shouldExist) {
+ throw new NoSuchChangeException(getChangeId());
+ }
loadDefaults();
return;
}
@@ -587,12 +625,17 @@
}
@Override
- protected LoadHandle openHandle(Repository repo) throws IOException {
+ protected LoadHandle openHandle(Repository repo)
+ throws NoSuchChangeException, IOException {
if (autoRebuild) {
NoteDbChangeState state = NoteDbChangeState.parse(change);
ObjectId id = readRef(repo);
- if (state == null && id == null) {
- return super.openHandle(repo, id);
+ if (id == null) {
+ if (state == null) {
+ return super.openHandle(repo, id);
+ } else if (shouldExist) {
+ throw new NoSuchChangeException(getChangeId());
+ }
}
RefCache refs = this.refs != null ? this.refs : new RepoRefCache(repo);
if (!NoteDbChangeState.isChangeUpToDate(state, refs, getChangeId())) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ConfigNotesMigration.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ConfigNotesMigration.java
index 802359c..bf9bc4e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ConfigNotesMigration.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ConfigNotesMigration.java
@@ -20,6 +20,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.inject.AbstractModule;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -49,9 +50,11 @@
}
private static final String NOTE_DB = "noteDb";
+
+ private static final String PRIMARY_STORAGE = "primaryStorage";
private static final String READ = "read";
- private static final String WRITE = "write";
private static final String SEQUENCE = "sequence";
+ private static final String WRITE = "write";
private static void checkConfig(Config cfg) {
Set<String> keys = new HashSet<>();
@@ -81,6 +84,7 @@
private final boolean writeChanges;
private final boolean readChanges;
private final boolean readChangeSequence;
+ private final PrimaryStorage changePrimaryStorage;
private final boolean writeAccounts;
private final boolean readAccounts;
@@ -98,6 +102,9 @@
// NoteDb. This decision for the default may be reevaluated later.
readChangeSequence = cfg.getBoolean(NOTE_DB, CHANGES.key(), SEQUENCE, false);
+ changePrimaryStorage = cfg.getEnum(
+ NOTE_DB, CHANGES.key(), PRIMARY_STORAGE, PrimaryStorage.REVIEW_DB);
+
writeAccounts = cfg.getBoolean(NOTE_DB, ACCOUNTS.key(), WRITE, false);
readAccounts = cfg.getBoolean(NOTE_DB, ACCOUNTS.key(), READ, false);
}
@@ -118,6 +125,11 @@
}
@Override
+ public PrimaryStorage changePrimaryStorage() {
+ return changePrimaryStorage;
+ }
+
+ @Override
public boolean writeAccounts() {
return writeAccounts;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java
index 4feac2e..0408ffa 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java
@@ -188,7 +188,8 @@
}
@Override
- protected LoadHandle openHandle(Repository repo) throws IOException {
+ protected LoadHandle openHandle(Repository repo)
+ throws NoSuchChangeException, IOException {
if (rebuildResult != null) {
StagedResult sr = checkNotNull(rebuildResult.staged());
return LoadHandle.create(
@@ -216,7 +217,8 @@
return null;
}
- private LoadHandle rebuildAndOpen(Repository repo) throws IOException {
+ private LoadHandle rebuildAndOpen(Repository repo)
+ throws NoSuchChangeException, IOException {
Timer1.Context timer = args.metrics.autoRebuildLatency.start(CHANGES);
try {
Change.Id cid = getChangeId();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java
index 56b41d9..6afe87d 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NotesMigration.java
@@ -14,6 +14,8 @@
package com.google.gerrit.server.notedb;
+import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
+
/**
* Holds the current state of the NoteDb migration.
* <p>
@@ -71,6 +73,9 @@
*/
public abstract boolean readChangeSequence();
+ /** @return default primary storage for new changes. */
+ public abstract PrimaryStorage changePrimaryStorage();
+
public abstract boolean readAccounts();
public abstract boolean writeAccounts();
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index f9dd576..bc107ca 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -60,6 +60,7 @@
import com.google.gerrit.reviewdb.server.ReviewDbUtil;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.Sequences;
import com.google.gerrit.server.StarredChangesUtil;
import com.google.gerrit.server.account.AccountManager;
@@ -146,6 +147,7 @@
@Inject protected InternalChangeQuery internalChangeQuery;
@Inject protected ChangeNotes.Factory notesFactory;
@Inject protected PatchSetInserter.Factory patchSetFactory;
+ @Inject protected PatchSetUtil psUtil;
@Inject protected ChangeControl.GenericFactory changeControlFactory;
@Inject protected ChangeQueryProcessor queryProcessor;
@Inject protected SchemaCreator schemaCreator;
@@ -1579,9 +1581,13 @@
Account.Id user2 = createAccount("user2");
TestRepository<Repo> repo = createProject("repo");
Change change1 = insert(repo, newChange(repo));
- PatchSet ps1 = db.patchSets().get(change1.currentPatchSetId());
+ ChangeNotes notes1 =
+ notesFactory.create(db, change1.getProject(), change1.getId());
+ PatchSet ps1 = psUtil.get(db, notes1, change1.currentPatchSetId());
Change change2 = insert(repo, newChange(repo));
- PatchSet ps2 = db.patchSets().get(change2.currentPatchSetId());
+ ChangeNotes notes2 =
+ notesFactory.create(db, change2.getProject(), change2.getId());
+ PatchSet ps2 = psUtil.get(db, notes2, change2.currentPatchSetId());
requestContext.setContext(newRequestContext(user1));
assertQuery("has:edit");
@@ -1692,7 +1698,9 @@
Project.NameKey project = new Project.NameKey("repo");
TestRepository<Repo> repo = createProject(project.get());
Change change = insert(repo, newChange(repo));
- PatchSet ps = db.patchSets().get(change.currentPatchSetId());
+ ChangeNotes notes =
+ notesFactory.create(db, change.getProject(), change.getId());
+ PatchSet ps = psUtil.get(db, notes, change.currentPatchSetId());
requestContext.setContext(newRequestContext(user));
assertThat(changeEditModifier.createEdit(change, ps))
@@ -1714,6 +1722,9 @@
@Test
public void refStateFields() throws Exception {
+ // This test method manages primary storage manually.
+ assume().that(notesMigration.changePrimaryStorage())
+ .isEqualTo(PrimaryStorage.REVIEW_DB);
Account.Id user = createAccount("user");
Project.NameKey project = new Project.NameKey("repo");
TestRepository<Repo> repo = createProject(project.get());
@@ -1723,7 +1734,9 @@
Change change = insert(repo, newChangeForCommit(repo, commit));
Change.Id id = change.getId();
int c = id.get();
- PatchSet ps = db.patchSets().get(change.currentPatchSetId());
+ ChangeNotes notes =
+ notesFactory.create(db, change.getProject(), change.getId());
+ PatchSet ps = psUtil.get(db, notes, change.currentPatchSetId());
requestContext.setContext(newRequestContext(user));
// Ensure one of each type of supported ref is present for the change. If
diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/NoteDbChecker.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/NoteDbChecker.java
index c5f4301..ae0e515 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/testutil/NoteDbChecker.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/NoteDbChecker.java
@@ -39,6 +39,7 @@
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.Repository;
+import org.junit.runner.Description;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -124,6 +125,25 @@
}
}
+ public void assertNoReviewDbChanges(Description desc) throws Exception {
+ ReviewDb db = getUnwrappedDb();
+ assertThat(db.changes().all().toList())
+ .named("Changes in " + desc.getTestClass())
+ .isEmpty();
+ assertThat(db.changeMessages().all().toList())
+ .named("ChangeMessages in " + desc.getTestClass())
+ .isEmpty();
+ assertThat(db.patchSets().all().toList())
+ .named("PatchSets in " + desc.getTestClass())
+ .isEmpty();
+ assertThat(db.patchSetApprovals().all().toList())
+ .named("PatchSetApprovals in " + desc.getTestClass())
+ .isEmpty();
+ assertThat(db.patchComments().all().toList())
+ .named("PatchLineComments in " + desc.getTestClass())
+ .isEmpty();
+ }
+
private List<ChangeBundle> readExpected(Stream<Change.Id> changeIds)
throws Exception {
boolean old = notesMigration.readChanges();
diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/NoteDbMode.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/NoteDbMode.java
index 259973d..7db76fd 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/testutil/NoteDbMode.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/NoteDbMode.java
@@ -29,6 +29,9 @@
/** Reading and writing all data to NoteDb is enabled. */
READ_WRITE,
+ /** Changes are created with their primary storage as NoteDb. */
+ PRIMARY,
+
/**
* Run tests with NoteDb disabled, then convert ReviewDb to NoteDb and check
* that the results match.
@@ -59,6 +62,6 @@
}
public static boolean readWrite() {
- return get() == READ_WRITE;
+ return get() == READ_WRITE || get() == PRIMARY;
}
}
diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/TestNotesMigration.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/TestNotesMigration.java
index 2f9d67f..b11b2cd 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/testutil/TestNotesMigration.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/testutil/TestNotesMigration.java
@@ -14,6 +14,9 @@
package com.google.gerrit.testutil;
+import static com.google.common.base.Preconditions.checkNotNull;
+
+import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.notedb.NotesMigration;
import com.google.inject.Singleton;
@@ -22,6 +25,8 @@
public class TestNotesMigration extends NotesMigration {
private volatile boolean readChanges;
private volatile boolean writeChanges;
+ private volatile PrimaryStorage changePrimaryStorage =
+ PrimaryStorage.REVIEW_DB;
private volatile boolean failOnLoad;
@Override
@@ -36,6 +41,11 @@
return readChanges;
}
+ @Override
+ public PrimaryStorage changePrimaryStorage() {
+ return changePrimaryStorage;
+ }
+
// Increase visbility from superclass, as tests may want to check whether
// NoteDb data is written in specific migration scenarios.
@Override
@@ -68,6 +78,12 @@
return this;
}
+ public TestNotesMigration setChangePrimaryStorage(
+ PrimaryStorage changePrimaryStorage) {
+ this.changePrimaryStorage = checkNotNull(changePrimaryStorage);
+ return this;
+ }
+
public TestNotesMigration setFailOnLoad(boolean failOnLoad) {
this.failOnLoad = failOnLoad;
return this;
@@ -82,16 +98,24 @@
case READ_WRITE:
setWriteChanges(true);
setReadChanges(true);
+ setChangePrimaryStorage(PrimaryStorage.REVIEW_DB);
break;
case WRITE:
setWriteChanges(true);
setReadChanges(false);
+ setChangePrimaryStorage(PrimaryStorage.REVIEW_DB);
+ break;
+ case PRIMARY:
+ setWriteChanges(true);
+ setReadChanges(true);
+ setChangePrimaryStorage(PrimaryStorage.NOTE_DB);
break;
case CHECK:
case OFF:
default:
setWriteChanges(false);
setReadChanges(false);
+ setChangePrimaryStorage(PrimaryStorage.REVIEW_DB);
break;
}
return this;
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
index 4bd5141..099eb9e 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
@@ -253,7 +253,17 @@
<gr-change-star id="changeStar"
change="{{_change}}" hidden$="[[!_loggedIn]]"></gr-change-star>
<a href$="[[_computeChangePermalink(_change._number)]]">[[_change._number]]</a><!--
- --><span class="changeStatus">[[_computeChangeStatus(_change, _patchRange.patchNum)]]</span><!--
+ --><template is="dom-if" if="[[_changeStatus]]">
+ ([[_changeStatus]]<!--
+ --><template is="dom-if" if="[[_computeShowCommitInfo(_changeStatus)]]">
+ as
+ <gr-commit-info
+ change="[[_change]]"
+ commit-info="[[_computeMergedCommitInfo(_change.current_revision, _change.revisions)]]"
+ server-config="[[serverConfig]]"></gr-commit-info><!--
+ --></template><!--
+ -->)<!--
+ --></template><!--
-->:
[[_change.subject]]
</span>
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
index 910df6d..d66917c 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
@@ -127,6 +127,10 @@
value: true,
computed: '_computeReplyDisabled(serverConfig)',
},
+ _changeStatus: {
+ type: String,
+ computed: '_computeChangeStatus(_change, _patchRange.patchNum)',
+ },
},
behaviors: [
@@ -556,7 +560,17 @@
} else {
statusString = this.changeStatusString(change);
}
- return statusString ? ' (' + statusString + ')' : '';
+ return statusString || '';
+ },
+
+ _computeShowCommitInfo: function(changeStatus) {
+ return changeStatus === 'Merged';
+ },
+
+ _computeMergedCommitInfo: function(current_revision, revisions) {
+ var rev = revisions[current_revision];
+ if (!rev) { return {}; }
+ return rev.commit;
},
_computeChangeIdClass: function(displayChangeId) {
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
index dafff75..4f2731a 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
@@ -603,7 +603,26 @@
labels: {},
};
var status = element._computeChangeStatus(element._change, '1');
- assert.equal(status, ' (Draft)');
+ assert.equal(status, 'Draft');
+ });
+
+ test('change status merged', function() {
+ element._changeNum = '1';
+ element._patchRange = {
+ basePatchNum: 'PARENT',
+ patchNum: 1,
+ };
+ element._change = {
+ change_id: 'Iad9dc96274af6946f3632be53b106ef80f7ba6ca',
+ revisions: {
+ rev1: {_number: 1},
+ },
+ current_revision: 'rev1',
+ status: element.ChangeStatus.MERGED,
+ labels: {},
+ };
+ var status = element._computeChangeStatus(element._change, '1');
+ assert.equal(status, 'Merged');
});
test('revision status draft', function() {
@@ -626,7 +645,15 @@
labels: {},
};
var status = element._computeChangeStatus(element._change, '2');
- assert.equal(status, ' (Draft)');
+ assert.equal(status, 'Draft');
+ });
+
+ test('_computeMergedCommitInfo', function() {
+ var dummyRevObj = {
+ sha: {commit: 'test'},
+ };
+ assert.deepEqual(element._computeMergedCommitInfo('sha', dummyRevObj),
+ 'test');
});
test('get latest revision', function() {
