Assignee in ReviewDb.

Change-Id: I27c8823b349c79804c64207585d42f608d4319a8
This commit is contained in:
Gustaf Lundh
2016-09-22 17:38:06 +02:00
committed by Sven Selberg
parent ff7679320a
commit 138aec1244
18 changed files with 80 additions and 93 deletions

View File

@@ -36,11 +36,6 @@ import java.util.Set;
@NoHttpd
public class AssigneeIT extends AbstractDaemonTest {
@Before
public void before() {
assume().that(notesMigration.readChanges()).isTrue();
}
@BeforeClass
public static void setTimeForTesting() {
TestTimeUtil.resetWithClockStep(1, SECONDS);
@@ -75,6 +70,7 @@ public class AssigneeIT extends AbstractDaemonTest {
@Test
public void testGetPastAssignees() throws Exception {
assume().that(notesMigration.readChanges()).isTrue();
PushOneCommit.Result r = createChange();
setAssignee(r, user.email);
setAssignee(r, admin.email);

View File

@@ -24,7 +24,6 @@ import static com.google.gerrit.server.index.change.ChangeField.PROJECT;
import static com.google.gerrit.server.index.change.ChangeIndexRewriter.CLOSED_STATUSES;
import static com.google.gerrit.server.index.change.ChangeIndexRewriter.OPEN_STATUSES;
import com.google.common.base.Optional;
import com.google.common.base.Throwables;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.FluentIterable;
@@ -119,7 +118,6 @@ public class LuceneChangeIndex implements ChangeIndex {
private static final String ADDED_FIELD = ChangeField.ADDED.getName();
private static final String APPROVAL_FIELD = ChangeField.APPROVAL.getName();
private static final String ASSIGNEE_FIELD = ChangeField.ASSIGNEE.getName();
private static final String CHANGE_FIELD = ChangeField.CHANGE.getName();
private static final String DELETED_FIELD = ChangeField.DELETED.getName();
private static final String MERGEABLE_FIELD = ChangeField.MERGEABLE.getName();
@@ -475,9 +473,6 @@ public class LuceneChangeIndex implements ChangeIndex {
if (fields.contains(REVIEWEDBY_FIELD)) {
decodeReviewedBy(doc, cd);
}
if(fields.contains(ASSIGNEE_FIELD)) {
decodeAssignee(doc, cd);
}
if (fields.contains(HASHTAG_FIELD)) {
decodeHashtags(doc, cd);
}
@@ -549,18 +544,6 @@ public class LuceneChangeIndex implements ChangeIndex {
}
}
private void decodeAssignee(Multimap<String, IndexableField> doc, ChangeData cd) {
IndexableField af = Iterables.getFirst(doc.get(ASSIGNEE_FIELD), null);
Account.Id assignee = null;
if (af != null) {
int id = af.numericValue().intValue();
if (id > 0) {
assignee = new Account.Id(id);
}
}
cd.setAssignee(Optional.fromNullable(assignee));
}
private void decodeHashtags(Multimap<String, IndexableField> doc, ChangeData cd) {
Collection<IndexableField> hashtag = doc.get(HASHTAG_FIELD);
Set<String> hashtags = Sets.newHashSetWithExpectedSize(hashtag.size());

View File

@@ -481,6 +481,13 @@ public final class Change {
@Column(id = 18, notNull = false)
protected String submissionId;
/**
* Allows assigning a change to a user.
*/
@Column(id = 19, notNull = false)
protected Account.Id assignee;
/** @see com.google.gerrit.server.notedb.NoteDbChangeState */
@Column(id = 101, notNull = false, length = Integer.MAX_VALUE)
protected String noteDbState;
@@ -500,6 +507,7 @@ public final class Change {
}
public Change(Change other) {
assignee = other.assignee;
changeId = other.changeId;
changeKey = other.changeKey;
rowVersion = other.rowVersion;
@@ -535,6 +543,14 @@ public final class Change {
changeKey = k;
}
public Account.Id getAssignee() {
return assignee;
}
public void setAssignee(Account.Id a) {
assignee = a;
}
public Timestamp getCreatedOn() {
return createdOn;
}

View File

@@ -442,8 +442,8 @@ public class ChangeJson {
out.branch = in.getDest().getShortName();
out.topic = in.getTopic();
if (indexes.getSearchIndex().getSchema().hasField(ChangeField.ASSIGNEE)) {
if (cd.assignee().isPresent()) {
out.assignee = accountLoader.get(cd.assignee().get());
if (in.getAssignee() != null) {
out.assignee = accountLoader.get(in.getAssignee());
}
}
out.hashtags = cd.hashtags();

View File

@@ -14,15 +14,14 @@
package com.google.gerrit.server.change;
import com.google.common.base.Optional;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.extensions.common.AccountInfo;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ChangeMessagesUtil;
@@ -35,7 +34,6 @@ import com.google.gerrit.server.git.BatchUpdate;
import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
import com.google.gerrit.server.git.UpdateException;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -47,21 +45,18 @@ public class DeleteAssignee implements RestModifyView<ChangeResource, Input> {
}
private final BatchUpdate.Factory batchUpdateFactory;
private final NotesMigration notesMigration;
private final ChangeMessagesUtil cmUtil;
private final Provider<ReviewDb> db;
private final AccountInfoCacheFactory.Factory accountInfos;
private final String anonymousCowardName;
@Inject
DeleteAssignee(NotesMigration notesMigration,
BatchUpdate.Factory batchUpdateFactory,
DeleteAssignee(BatchUpdate.Factory batchUpdateFactory,
ChangeMessagesUtil cmUtil,
Provider<ReviewDb> db,
AccountInfoCacheFactory.Factory accountInfosFactory,
@AnonymousCowardName String anonymousCowardName) {
this.batchUpdateFactory = batchUpdateFactory;
this.notesMigration = notesMigration;
this.cmUtil = cmUtil;
this.db = db;
this.accountInfos = accountInfosFactory;
@@ -90,20 +85,20 @@ public class DeleteAssignee implements RestModifyView<ChangeResource, Input> {
@Override
public boolean updateChange(ChangeContext ctx)
throws RestApiException, OrmException{
if (!notesMigration.readChanges()) {
throw new BadRequestException(
"Cannot delete Assignee; NoteDb is disabled");
}
if (!ctx.getControl().canEditAssignee()) {
throw new AuthException("Delete Assignee not permitted");
}
ChangeUpdate update = ctx.getUpdate(ctx.getChange().currentPatchSetId());
Optional<Account.Id> currentAssigneeId = update.getNotes().getAssignee();
if (!currentAssigneeId.isPresent()) {
Change change = ctx.getChange();
ChangeUpdate update = ctx.getUpdate(change.currentPatchSetId());
Account.Id currentAssigneeId = change.getAssignee();
if (currentAssigneeId == null) {
return false;
}
Account account = accountInfos.create().get(currentAssigneeId.get());
Account account = accountInfos.create().get(currentAssigneeId);
// noteDb
update.removeAssignee();
// reviewDb
change.setAssignee(null);
addMessage(ctx, update, account);
deletedAssignee = account;
return true;

View File

@@ -38,7 +38,7 @@ public class GetAssignee implements RestReadView<ChangeResource> {
public Response<AccountInfo> apply(ChangeResource rsrc) throws OrmException {
Optional<Account.Id> assignee =
rsrc.getControl().getNotes().load().getAssignee();
Optional.fromNullable(rsrc.getChange().getAssignee());
if (assignee.isPresent()) {
Account account = accountInfo.create().get(assignee.get());
return Response.ok(AccountJson.toAccountInfo(account));

View File

@@ -32,7 +32,6 @@ import com.google.gerrit.server.account.AccountsCollection;
import com.google.gerrit.server.config.AnonymousCowardName;
import com.google.gerrit.server.git.BatchUpdate;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.server.validators.AssigneeValidationListener;
import com.google.gerrit.server.validators.ValidationException;
import com.google.gwtorm.server.OrmException;
@@ -48,23 +47,19 @@ public class SetAssigneeOp extends BatchUpdate.Op {
private final AccountsCollection accounts;
private final ChangeMessagesUtil cmUtil;
private final AccountInfoCacheFactory.Factory accountInfosFactory;
private final NotesMigration notesMigration;
private final String anonymousCowardName;
private final DynamicSet<AssigneeValidationListener> validationListeners;
private Change change;
private Account newAssignee;
@AssistedInject
SetAssigneeOp(AccountsCollection accounts,
NotesMigration notesMigration,
ChangeMessagesUtil cmUtil,
AccountInfoCacheFactory.Factory accountInfosFactory,
@AnonymousCowardName String anonymousCowardName,
@Assisted AssigneeInput input,
DynamicSet<AssigneeValidationListener> validationListeners) {
this.accounts = accounts;
this.notesMigration = notesMigration;
this.cmUtil = cmUtil;
this.accountInfosFactory = accountInfosFactory;
this.anonymousCowardName = anonymousCowardName;
@@ -75,15 +70,13 @@ public class SetAssigneeOp extends BatchUpdate.Op {
@Override
public boolean updateChange(BatchUpdate.ChangeContext ctx)
throws OrmException, RestApiException {
if (!notesMigration.readChanges()) {
throw new BadRequestException(
"Cannot add Assignee; NoteDb is disabled");
}
if (!ctx.getControl().canEditAssignee()) {
throw new AuthException("Changing Assignee not permitted");
}
ChangeUpdate update = ctx.getUpdate(ctx.getChange().currentPatchSetId());
Optional<Account.Id> oldAssigneeId = update.getNotes().getAssignee();
Change change = ctx.getChange();
ChangeUpdate update = ctx.getUpdate(change.currentPatchSetId());
Optional<Account.Id> oldAssigneeId =
Optional.fromNullable(ctx.getChange().getAssignee());
if (input.assignee == null) {
if (oldAssigneeId != null && oldAssigneeId.isPresent()) {
throw new AuthException("Cannot set Assignee to empty");
@@ -117,7 +110,10 @@ public class SetAssigneeOp extends BatchUpdate.Op {
} catch (ValidationException e) {
throw new BadRequestException(e.getMessage());
}
// notedb
update.setAssignee(newAssigneeUser.getAccountId());
// reviewdb
change.setAssignee(newAssigneeUser.getAccountId());
this.newAssignee = newAssigneeUser.getAccount();
addMessage(ctx, update, oldAssignee);
return true;

View File

@@ -296,12 +296,12 @@ public class ChangeField {
/** The user assigned to the change. */
public static final FieldDef<ChangeData, Integer> ASSIGNEE =
new FieldDef.Single<ChangeData, Integer>(
ChangeQueryBuilder.FIELD_ASSIGNEE, FieldType.INTEGER, true) {
ChangeQueryBuilder.FIELD_ASSIGNEE, FieldType.INTEGER, false) {
@Override
public Integer get(ChangeData input, FillArgs args)
throws OrmException {
Optional<Account.Id> id = input.assignee();
return id.isPresent() ? id.get().get() : NO_ASSIGNEE;
Account.Id id = input.change().getAssignee();
return id != null ? id.get() : NO_ASSIGNEE;
}
};

View File

@@ -224,7 +224,7 @@ public class ChangeBundle {
checkColumns(Change.Id.class, 1);
checkColumns(Change.class,
1, 2, 3, 4, 5, 7, 8, 10, 12, 13, 14, 17, 18, 101);
1, 2, 3, 4, 5, 7, 8, 10, 12, 13, 14, 17, 18, 19, 101);
checkColumns(ChangeMessage.Key.class, 1, 2);
checkColumns(ChangeMessage.class, 1, 2, 3, 4, 5, 6);
checkColumns(PatchSet.Id.class, 1, 2);

View File

@@ -22,7 +22,6 @@ import static com.google.gerrit.server.notedb.NoteDbTable.CHANGES;
import static java.util.Comparator.comparing;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Optional;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
@@ -377,13 +376,6 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
return state.reviewerUpdates();
}
/**
* @return an Account.Id of the user assigned to this change.
*/
public Optional<Account.Id> getAssignee() {
return Optional.fromNullable(state.assignee());
}
/**
* @return an ImmutableSet of Account.Ids of all users that have been assigned
* to this change.

View File

@@ -212,9 +212,9 @@ class ChangeNotesParser {
topic,
originalSubject,
submissionId,
assignee != null ? assignee.orNull() : null,
status,
assignee != null ? assignee.orNull() : null,
Sets.newLinkedHashSet(Lists.reverse(pastAssignees)),
hashtags,
patchSets,

View File

@@ -59,7 +59,6 @@ public abstract class ChangeNotesState {
return new AutoValue_ChangeNotesState(
change.getId(),
null,
null,
ImmutableSet.<Account.Id>of(),
ImmutableSet.<String>of(),
ImmutableSortedMap.<PatchSet.Id, PatchSet>of(),
@@ -85,8 +84,8 @@ public abstract class ChangeNotesState {
@Nullable String topic,
@Nullable String originalSubject,
@Nullable String submissionId,
@Nullable Change.Status status,
@Nullable Account.Id assignee,
@Nullable Change.Status status,
@Nullable Set<Account.Id> pastAssignees,
@Nullable Set<String> hashtags,
Map<PatchSet.Id, PatchSet> patchSets,
@@ -114,8 +113,8 @@ public abstract class ChangeNotesState {
topic,
originalSubject,
submissionId,
status),
assignee,
status),
ImmutableSet.copyOf(pastAssignees),
ImmutableSet.copyOf(hashtags),
ImmutableSortedMap.copyOf(patchSets, comparing(PatchSet.Id::get)),
@@ -150,6 +149,7 @@ public abstract class ChangeNotesState {
@Nullable abstract String topic();
@Nullable abstract String originalSubject();
@Nullable abstract String submissionId();
@Nullable abstract Account.Id assignee();
// TODO(dborowitz): Use a sensible default other than null
@Nullable abstract Change.Status status();
}
@@ -159,7 +159,6 @@ public abstract class ChangeNotesState {
@Nullable abstract ChangeColumns columns();
// Other related to this Change.
@Nullable abstract Account.Id assignee();
abstract ImmutableSet<Account.Id> pastAssignees();
abstract ImmutableSet<String> hashtags();
abstract ImmutableSortedMap<PatchSet.Id, PatchSet> patchSets();
@@ -187,6 +186,7 @@ public abstract class ChangeNotesState {
change.setLastUpdatedOn(c.lastUpdatedOn());
change.setOwner(c.owner());
change.setSubmissionId(c.submissionId());
change.setAssignee(c.assignee());
if (!patchSets().isEmpty()) {
change.setCurrentPatchSet(

View File

@@ -16,7 +16,6 @@ package com.google.gerrit.server.project;
import static com.google.common.base.Preconditions.checkArgument;
import com.google.common.base.Optional;
import com.google.common.collect.Lists;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.LabelType;
@@ -355,8 +354,8 @@ public class ChangeControl {
/** Is this user assigned to this change? */
public boolean isAssignee() {
Optional<Account.Id> currentAssignee = notes.getAssignee();
if (currentAssignee.isPresent() && getUser().isIdentifiedUser()) {
Account.Id currentAssignee = notes.getChange().getAssignee();
if (currentAssignee != null && getUser().isIdentifiedUser()) {
Account.Id id = getUser().getAccountId();
return id.equals(currentAssignee.get());
}

View File

@@ -14,7 +14,6 @@
package com.google.gerrit.server.query.change;
import com.google.common.base.Optional;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.index.change.ChangeField;
import com.google.gwtorm.server.OrmException;
@@ -30,10 +29,10 @@ class AssigneePredicate extends ChangeIndexPredicate {
@Override
public boolean match(final ChangeData object) throws OrmException {
if (id.get() == ChangeField.NO_ASSIGNEE) {
Optional<Account.Id> assignee = object.notes().load().getAssignee();
return assignee == null || !assignee.isPresent();
Account.Id assignee = object.change().getAssignee();
return assignee == null;
}
return id.equals(object.notes().load().getAssignee().get());
return id.equals(object.change().getAssignee());
}
@Override

View File

@@ -345,7 +345,6 @@ public class ChangeData {
private Optional<ChangedLines> changedLines;
private SubmitTypeRecord submitTypeRecord;
private Boolean mergeable;
private Optional<Account.Id> assignee;
private Set<String> hashtags;
private Set<Account.Id> editsByUser;
private Set<Account.Id> reviewedBy;
@@ -1158,20 +1157,6 @@ public class ChangeData {
this.reviewedBy = reviewedBy;
}
public Optional<Account.Id> assignee() throws OrmException {
if (assignee == null) {
if (!lazyLoad) {
return Optional.absent();
}
assignee = notes().getAssignee();
}
return assignee;
}
public void setAssignee(Optional<Account.Id> assignee) {
this.assignee = assignee;
}
public Set<String> hashtags() throws OrmException {
if (hashtags == null) {
if (!lazyLoad) {

View File

@@ -33,7 +33,7 @@ import java.util.List;
/** A version of the database schema. */
public abstract class SchemaVersion {
/** The current schema version. */
public static final Class<Schema_131> C = Schema_131.class;
public static final Class<Schema_132> C = Schema_132.class;
public static int getBinaryVersion() {
return guessVersion(C);

View File

@@ -0,0 +1,25 @@
// Copyright (C) 2016 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.schema;
import com.google.inject.Inject;
import com.google.inject.Provider;
public class Schema_132 extends SchemaVersion {
@Inject
Schema_132(Provider<Schema_131> prior) {
super(prior);
}
}

View File

@@ -591,14 +591,15 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
update.commit();
ChangeNotes notes = newNotes(c);
assertThat(notes.getAssignee().get()).isEqualTo(otherUserId);
assertThat(notes.getChange().getAssignee()).isEqualTo(otherUserId);
update = newUpdate(c, changeOwner);
update.setAssignee(changeOwner.getAccountId());
update.commit();
notes = newNotes(c);
assertThat(notes.getAssignee().get()).isEqualTo(changeOwner.getAccountId());
assertThat(notes.getChange().getAssignee())
.isEqualTo(changeOwner.getAccountId());
}
@Test