opendev/gerrit
Code Issues Proposed changes

Merge changes I7d559df1,I004ea0bb,Id36abd48,I7710befa,Ia365f71e

Browse Source 
* changes:
  ChangeControl: Add getId() method
  ChangeUtil: Reload changes in findChanges
  SetReviewersCommand: Use ChangeUtil#findChanges
  ChangeUtil: Return ChangeControls from findChanges
  Allow customizing the set of fields returned by the index
This commit is contained in:
Dave Borowitz 2015-11-23 18:55:20 +00:00 committed by Gerrit Code Review
commit 6f7ccf5409
33 changed files with 295 additions and 198 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
Documentation/cmd-set-reviewers.txt
View File

@ -10,17 +10,16 @@ gerrit set-reviewers - Add or remove reviewers to a change
[--add <REVIEWER> ... | -a <REVIEWER> ...]
[--remove <REVIEWER> ... | -r <REVIEWER> ...]
[--]
{COMMIT | CHANGE-ID}...
{CHANGE-ID}...
--
== DESCRIPTION
Adds or removes reviewers to the specified change, sending email
notifications when changes are made.
Changes should be specified as complete or abbreviated Change-Ids
such as 'Iac6b2ac2'. They may also be specified by numeric change
identifiers, such as '8242' or by complete or abbreviated commit
SHA-1s.
Changes can be specified in the
link:rest-api-changes.html#change-id[same format] supported by the REST
API.
== OPTIONS

86
gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java
View File

@ -131,7 +131,6 @@ public class LuceneChangeIndex implements ChangeIndex {
private static final String APPROVAL_FIELD = ChangeField.APPROVAL.getName();
private static final String CHANGE_FIELD = ChangeField.CHANGE.getName();
private static final String DELETED_FIELD = ChangeField.DELETED.getName();
private static final String ID_FIELD = ChangeField.LEGACY_ID2.getName();
private static final String MERGEABLE_FIELD = ChangeField.MERGEABLE.getName();
private static final String PATCH_SET_FIELD = ChangeField.PATCH_SET.getName();
private static final String REVIEWEDBY_FIELD =
@ -139,10 +138,6 @@ public class LuceneChangeIndex implements ChangeIndex {
private static final String UPDATED_SORT_FIELD =
sortFieldName(ChangeField.UPDATED);
private static final ImmutableSet<String> FIELDS = ImmutableSet.of(
ADDED_FIELD, APPROVAL_FIELD, CHANGE_FIELD, DELETED_FIELD, ID_FIELD,
MERGEABLE_FIELD, PATCH_SET_FIELD, REVIEWEDBY_FIELD);
private static final Map<String, String> CUSTOM_CHAR_MAPPING = ImmutableMap.of(
"_", " ", ".", " ");
@ -438,10 +433,12 @@ public class LuceneChangeIndex implements ChangeIndex {
List<ChangeData> result =
Lists.newArrayListWithCapacity(docs.scoreDocs.length);
Set<String> fields = fields(opts);
String idFieldName = idFieldName();
for (int i = opts.start(); i < docs.scoreDocs.length; i++) {
ScoreDoc sd = docs.scoreDocs[i];
Document doc = searchers[sd.shardIndex].doc(sd.doc, FIELDS);
result.add(toChangeData(doc));
Document doc = searchers[sd.shardIndex].doc(sd.doc, fields);
result.add(toChangeData(doc, fields, idFieldName));
}
final List<ChangeData> r = Collections.unmodifiableList(result);
@ -477,19 +474,62 @@ public class LuceneChangeIndex implements ChangeIndex {
}
}
private ChangeData toChangeData(Document doc) {
@SuppressWarnings("deprecation")
private Set<String> fields(QueryOptions opts) {
if (schemaHasRequestedField(ChangeField.LEGACY_ID2, opts.fields())
|| schemaHasRequestedField(ChangeField.CHANGE, opts.fields())
|| schemaHasRequestedField(ChangeField.LEGACY_ID, opts.fields())) {
return opts.fields();
}
// Request the numeric ID field even if the caller did not request it,
// otherwise we can't actually construct a ChangeData.
return Sets.union(opts.fields(), ImmutableSet.of(idFieldName()));
}
private boolean schemaHasRequestedField(FieldDef<ChangeData, ?> field,
Set<String> requested) {
return schema.hasField(field) && requested.contains(field.getName());
}
@SuppressWarnings("deprecation")
private String idFieldName() {
return schema.getField(ChangeField.LEGACY_ID2, ChangeField.LEGACY_ID).get()
.getName();
}
private ChangeData toChangeData(Document doc, Set<String> fields,
String idFieldName) {
ChangeData cd;
// Either change or the ID field was guaranteed to be included in the call
// to fields() above.
BytesRef cb = doc.getBinaryValue(CHANGE_FIELD);
if (cb == null) {
int id = doc.getField(ID_FIELD).numericValue().intValue();
return changeDataFactory.create(db.get(), new Change.Id(id));
if (cb != null) {
cd = changeDataFactory.create(db.get(),
ChangeProtoField.CODEC.decode(cb.bytes, cb.offset, cb.length));
} else {
int id = doc.getField(idFieldName).numericValue().intValue();
cd = changeDataFactory.create(db.get(), new Change.Id(id));
}
// Change proto.
Change change = ChangeProtoField.CODEC.decode(
cb.bytes, cb.offset, cb.length);
ChangeData cd = changeDataFactory.create(db.get(), change);
if (fields.contains(PATCH_SET_FIELD)) {
decodePatchSets(doc, cd);
}
if (fields.contains(APPROVAL_FIELD)) {
decodeApprovals(doc, cd);
}
if (fields.contains(ADDED_FIELD) && fields.contains(DELETED_FIELD)) {
decodeChangedLines(doc, cd);
}
if (fields.contains(MERGEABLE_FIELD)) {
decodeMergeable(doc, cd);
}
if (fields.contains(REVIEWEDBY_FIELD)) {
decodeReviewedBy(doc, cd);
}
return cd;
}
// Patch sets.
private void decodePatchSets(Document doc, ChangeData cd) {
List<PatchSet> patchSets =
decodeProtos(doc, PATCH_SET_FIELD, PatchSetProtoField.CODEC);
if (!patchSets.isEmpty()) {
@ -497,12 +537,14 @@ public class LuceneChangeIndex implements ChangeIndex {
// this cannot be valid since a change needs at least one patch set.
cd.setPatchSets(patchSets);
}
}
// Approvals.
private void decodeApprovals(Document doc, ChangeData cd) {
cd.setCurrentApprovals(
decodeProtos(doc, APPROVAL_FIELD, PatchSetApprovalProtoField.CODEC));
}
// Changed lines.
private void decodeChangedLines(Document doc, ChangeData cd) {
IndexableField added = doc.getField(ADDED_FIELD);
IndexableField deleted = doc.getField(DELETED_FIELD);
if (added != null && deleted != null) {
@ -510,16 +552,18 @@ public class LuceneChangeIndex implements ChangeIndex {
added.numericValue().intValue(),
deleted.numericValue().intValue());
}
}
// Mergeable.
private void decodeMergeable(Document doc, ChangeData cd) {
String mergeable = doc.get(MERGEABLE_FIELD);
if ("1".equals(mergeable)) {
cd.setMergeable(true);
} else if ("0".equals(mergeable)) {
cd.setMergeable(false);
}
}
// Reviewed-by.
private void decodeReviewedBy(Document doc, ChangeData cd) {
IndexableField[] reviewedBy = doc.getFields(REVIEWEDBY_FIELD);
if (reviewedBy.length > 0) {
Set<Account.Id> accounts =
@ -533,8 +577,6 @@ public class LuceneChangeIndex implements ChangeIndex {
}
cd.setReviewedBy(accounts);
}
return cd;
}
private static <T> List<T> decodeProtos(Document doc, String fieldName,

48
gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java
View File

@ -14,15 +14,13 @@
package com.google.gerrit.server;
import static com.google.gerrit.server.query.change.ChangeData.asChanges;
import com.google.common.base.Function;
import com.google.common.base.Optional;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Ordering;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
@ -43,6 +41,7 @@ import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.RefControl;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gerrit.server.util.IdGenerator;
import com.google.gwtorm.server.OrmConcurrencyException;
@ -73,6 +72,7 @@ import org.slf4j.LoggerFactory;
import java.io.IOException;
import java.text.MessageFormat;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Map;
@ -180,6 +180,7 @@ public class ChangeUtil {
private final Provider<IdentifiedUser> user;
private final Provider<ReviewDb> db;
private final Provider<InternalChangeQuery> queryProvider;
private final ChangeControl.GenericFactory changeControlFactory;
private final RevertedSender.Factory revertedSenderFactory;
private final ChangeInserter.Factory changeInserterFactory;
private final GitRepositoryManager gitManager;
@ -193,6 +194,7 @@ public class ChangeUtil {
ChangeUtil(Provider<IdentifiedUser> user,
Provider<ReviewDb> db,
Provider<InternalChangeQuery> queryProvider,
ChangeControl.GenericFactory changeControlFactory,
RevertedSender.Factory revertedSenderFactory,
ChangeInserter.Factory changeInserterFactory,
GitRepositoryManager gitManager,
@ -204,6 +206,7 @@ public class ChangeUtil {
this.user = user;
this.db = db;
this.queryProvider = queryProvider;
this.changeControlFactory = changeControlFactory;
this.revertedSenderFactory = revertedSenderFactory;
this.changeInserterFactory = changeInserterFactory;
this.gitManager = gitManager;
@ -425,34 +428,51 @@ public class ChangeUtil {
*
* @param id change identifier, either a numeric ID, a Change-Id, or
* project~branch~id triplet.
* @return all matching changes, even if they are not visible to the current
* user.
* @param user user to wrap in controls.
* @return possibly-empty list of controls for all matching changes,
* corresponding to the given user; may or may not be visible.
* @throws OrmException if an error occurred querying the database.
*/
public List<Change> findChanges(String id)
throws OrmException, ResourceNotFoundException {
public List<ChangeControl> findChanges(String id, CurrentUser user)
throws OrmException {
// Try legacy id
if (id.matches("^[1-9][0-9]*$")) {
Change c = db.get().changes().get(Change.Id.parse(id));
if (c != null) {
return ImmutableList.of(c);
try {
return ImmutableList.of(
changeControlFactory.controlFor(Change.Id.parse(id), user));
} catch (NoSuchChangeException e) {
return Collections.emptyList();
}
return Collections.emptyList();
}
// Use the index to search for changes, but don't return any stored fields,
// to force rereading in case the index is stale.
InternalChangeQuery query = queryProvider.get()
.setRequestedFields(ImmutableSet.<String> of());
// Try isolated changeId
if (!id.contains("~")) {
return asChanges(queryProvider.get().byKeyPrefix(id));
return asChangeControls(query.byKeyPrefix(id));
}
// Try change triplet
Optional<ChangeTriplet> triplet = ChangeTriplet.parse(id);
if (triplet.isPresent()) {
return asChanges(queryProvider.get().byBranchKey(
return asChangeControls(query.byBranchKey(
triplet.get().branch(),
triplet.get().id()));
}
throw new ResourceNotFoundException(id);
return Collections.emptyList();
}
private List<ChangeControl> asChangeControls(List<ChangeData> cds)
throws OrmException {
List<ChangeControl> ctls = new ArrayList<>(cds.size());
for (ChangeData cd : cds) {
ctls.add(cd.changeControl(user.get()));
}
return ctls;
}
private static void deleteOnlyDraftPatchSetPreserveRef(ReviewDb db,

4
gerrit-server/src/main/java/com/google/gerrit/server/account/StarredChanges.java
View File

@ -72,7 +72,7 @@ public class StarredChanges implements
user.asyncStarredChanges();
ChangeResource change = changes.parse(TopLevelResource.INSTANCE, id);
if (user.getStarredChanges().contains(change.getChange().getId())) {
if (user.getStarredChanges().contains(change.getId())) {
return new AccountResource.StarredChange(user, change);
}
throw new ResourceNotFoundException(id);
@ -141,7 +141,7 @@ public class StarredChanges implements
dbProvider.get().starredChanges().insert(Collections.singleton(
new StarredChange(new StarredChange.Key(
rsrc.getUser().getAccountId(),
change.getChange().getId()))));
change.getId()))));
} catch (OrmDuplicateKeyException e) {
return Response.none();
}

2
gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
View File

@ -132,7 +132,7 @@ class ChangeApiImpl implements ChangeApi {
@Override
public String id() {
return Integer.toString(change.getChange().getId().get());
return Integer.toString(change.getId().get());
}
@Override

2
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeEdits.java
View File

@ -177,7 +177,7 @@ public class ChangeEdits implements
if (edit.isPresent()) {
throw new ResourceConflictException(String.format(
"edit already exists for the change %s",
resource.getChange().getChangeId()));
resource.getId()));
}
edit = createEdit();
if (!Strings.isNullOrEmpty(path)) {

6
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeResource.java
View File

@ -49,6 +49,10 @@ public class ChangeResource implements RestResource, HasETag {
return control;
}
public Change.Id getId() {
return getControl().getId();
}
public Change getChange() {
return getControl().getChange();
}
@ -90,7 +94,7 @@ public class ChangeResource implements RestResource, HasETag {
public String getETag() {
CurrentUser user = control.getUser();
Hasher h = Hashing.md5().newHasher()
.putBoolean(user.getStarredChanges().contains(getChange().getId()));
.putBoolean(user.getStarredChanges().contains(getId()));
prepareETag(h, user);
return h.hash().toString();
}

22
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangesCollection.java
View File

@ -24,11 +24,11 @@ import com.google.gerrit.extensions.restapi.RestCollection;
import com.google.gerrit.extensions.restapi.RestView;
import com.google.gerrit.extensions.restapi.TopLevelResource;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.index.ChangeIndexer;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.query.change.QueryChanges;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@ -42,8 +42,8 @@ import java.util.List;
public class ChangesCollection implements
RestCollection<TopLevelResource, ChangeResource>,
AcceptsPost<TopLevelResource> {
private final Provider<ReviewDb> db;
private final Provider<CurrentUser> user;
private final ChangeControl.GenericFactory changeControlFactory;
private final Provider<QueryChanges> queryFactory;
private final DynamicMap<RestView<ChangeResource>> views;
private final ChangeUtil changeUtil;
@ -52,15 +52,15 @@ public class ChangesCollection implements
@Inject
ChangesCollection(
Provider<ReviewDb> db,
Provider<CurrentUser> user,
ChangeControl.GenericFactory changeControlFactory,
Provider<QueryChanges> queryFactory,
DynamicMap<RestView<ChangeResource>> views,
ChangeUtil changeUtil,
CreateChange createChange,
ChangeIndexer changeIndexer) {
this.db = db;
this.user = user;
this.changeControlFactory = changeControlFactory;
this.queryFactory = queryFactory;
this.views = views;
this.changeUtil = changeUtil;
@ -81,8 +81,8 @@ public class ChangesCollection implements
@Override
public ChangeResource parse(TopLevelResource root, IdString id)
throws ResourceNotFoundException, OrmException {
List<Change> changes = changeUtil.findChanges(id.encoded());
if (changes.isEmpty()) {
List<ChangeControl> ctls = changeUtil.findChanges(id.encoded(), user.get());
if (ctls.isEmpty()) {
Integer changeId = Ints.tryParse(id.get());
if (changeId != null) {
try {
@ -92,17 +92,15 @@ public class ChangesCollection implements
}
}
}
if (changes.size() != 1) {
if (ctls.size() != 1) {
throw new ResourceNotFoundException(id);
}
ChangeControl control;
try {
control = changeControlFactory.validateFor(changes.get(0), user.get());
} catch (NoSuchChangeException e) {
ChangeControl ctl = ctls.get(0);
if (!ctl.isVisible(db.get())) {
throw new ResourceNotFoundException(id);
}
return new ChangeResource(control);
return new ChangeResource(ctl);
}
public ChangeResource parse(Change.Id id)

15
gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java
View File

@ -44,6 +44,7 @@ import com.google.gerrit.server.git.BatchUpdate;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.UpdateException;
import com.google.gerrit.server.git.validators.CommitValidators;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.ProjectResource;
import com.google.gerrit.server.project.ProjectsCollection;
@ -154,19 +155,19 @@ public class CreateChange implements
ObjectId parentCommit;
List<String> groups;
if (input.baseChange != null) {
List<Change> changes = changeUtil.findChanges(input.baseChange);
if (changes.size() != 1) {
List<ChangeControl> ctls = changeUtil.findChanges(
input.baseChange, rsrc.getControl().getUser());
if (ctls.size() != 1) {
throw new InvalidChangeOperationException(
"Base change not found: " + input.baseChange);
}
Change change = Iterables.getOnlyElement(changes);
if (!rsrc.getControl().controlFor(change).isVisible(db.get())) {
ChangeControl ctl = Iterables.getOnlyElement(ctls);
if (!ctl.isVisible(db.get())) {
throw new InvalidChangeOperationException(
"Base change not found: " + input.baseChange);
}
PatchSet ps = db.get().patchSets().get(
new PatchSet.Id(change.getId(),
change.currentPatchSetId().get()));
PatchSet ps =
db.get().patchSets().get(ctl.getChange().currentPatchSetId());
parentCommit = ObjectId.fromString(ps.getRevision().get());
groups = ps.getGroups();
} else {

3
gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftChange.java
View File

@ -85,8 +85,7 @@ public class DeleteDraftChange implements
public UiAction.Description getDescription(ChangeResource rsrc) {
try {
return new UiAction.Description()
.setTitle(String.format("Delete draft change %d",
rsrc.getChange().getChangeId()))
.setTitle("Delete draft change " + rsrc.getId())
.setVisible(allowDrafts
&& rsrc.getChange().getStatus() == Status.DRAFT
&& rsrc.getControl().canDeleteDraft(dbProvider.get()));

6
gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewer.java
View File

@ -75,7 +75,7 @@ public class DeleteReviewer implements RestModifyView<ReviewerResource, Input> {
throws AuthException, ResourceNotFoundException, OrmException,
IOException {
ChangeControl control = rsrc.getControl();
Change.Id changeId = rsrc.getChange().getId();
Change.Id changeId = rsrc.getId();
ReviewDb db = dbProvider.get();
ChangeUpdate update = updateFactory.create(rsrc.getControl());
@ -103,13 +103,13 @@ public class DeleteReviewer implements RestModifyView<ReviewerResource, Input> {
if (del.isEmpty()) {
throw new ResourceNotFoundException();
}
ChangeUtil.bumpRowVersionNotLastUpdatedOn(rsrc.getChange().getId(), db);
ChangeUtil.bumpRowVersionNotLastUpdatedOn(rsrc.getId(), db);
db.patchSetApprovals().delete(del);
update.removeReviewer(rsrc.getUser().getAccountId());
if (msg.length() > 0) {
ChangeMessage changeMessage =
new ChangeMessage(new ChangeMessage.Key(rsrc.getChange().getId(),
new ChangeMessage(new ChangeMessage.Key(rsrc.getId(),
ChangeUtil.messageUUID(db)),
control.getUser().getAccountId(),
TimeUtil.nowTs(), rsrc.getChange().currentPatchSetId());

3
gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
View File

@ -237,8 +237,7 @@ public class PatchSetInserter extends BatchUpdate.Op {
if (message != null) {
changeMessage = new ChangeMessage(
new ChangeMessage.Key(
ctl.getChange().getId(), ChangeUtil.messageUUID(db)),
new ChangeMessage.Key(ctl.getId(), ChangeUtil.messageUUID(db)),
ctx.getUser().getAccountId(), ctx.getWhen(), patchSet.getId());
changeMessage.setMessage(message);
}

2
gerrit-server/src/main/java/com/google/gerrit/server/change/PostHashtags.java
View File

@ -52,7 +52,7 @@ public class PostHashtags
req.getChange().getProject(), req.getControl().getUser(),
TimeUtil.nowTs())) {
SetHashtagsOp op = hashtagsFactory.create(input);
bu.addOp(req.getChange().getId(), op);
bu.addOp(req.getId(), op);
bu.execute();
return Response.<ImmutableSortedSet<String>> ok(op.getUpdatedHashtags());
}

6
gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java
View File

@ -230,9 +230,9 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
ReviewDb db = dbProvider.get();
ChangeUpdate update = updateFactory.create(rsrc.getControl());
List<PatchSetApproval> added;
db.changes().beginTransaction(rsrc.getChange().getId());
db.changes().beginTransaction(rsrc.getId());
try {
ChangeUtil.bumpRowVersionNotLastUpdatedOn(rsrc.getChange().getId(), db);
ChangeUtil.bumpRowVersionNotLastUpdatedOn(rsrc.getId(), db);
added = approvalsUtil.addReviewers(db, rsrc.getNotes(), update,
rsrc.getControl().getLabelTypes(), rsrc.getChange(),
reviewers.keySet());
@ -243,7 +243,7 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
update.commit();
CheckedFuture<?, IOException> indexFuture =
indexer.indexAsync(rsrc.getChange().getId());
indexer.indexAsync(rsrc.getId());
result.reviewers = Lists.newArrayListWithCapacity(added.size());
for (PatchSetApproval psa : added) {
// New reviewers have value 0, don't bother normalizing.

2
gerrit-server/src/main/java/com/google/gerrit/server/change/PutTopic.java
View File

@ -77,7 +77,7 @@ public class PutTopic implements RestModifyView<ChangeResource, Input>,
Op op = new Op(ctl, input != null ? input : new Input());
try (BatchUpdate u = batchUpdateFactory.create(dbProvider.get(),
req.getChange().getProject(), ctl.getUser(), TimeUtil.nowTs())) {
u.addOp(req.getChange().getId(), op);
u.addOp(req.getId(), op);
u.execute();
}
return Strings.isNullOrEmpty(op.newTopicName)

2
gerrit-server/src/main/java/com/google/gerrit/server/change/Restore.java
View File

@ -87,7 +87,7 @@ public class Restore implements RestModifyView<ChangeResource, RestoreInput>,
Op op = new Op(input);
try (BatchUpdate u = batchUpdateFactory.create(dbProvider.get(),
req.getChange().getProject(), ctl.getUser(), TimeUtil.nowTs())) {
u.addOp(req.getChange().getId(), op).execute();
u.addOp(req.getId(), op).execute();
}
return json.create(ChangeJson.NO_OPTIONS).format(op.change);
}

9
gerrit-server/src/main/java/com/google/gerrit/server/change/Revisions.java
View File

@ -129,7 +129,7 @@ public class Revisions implements ChildCollection<ChangeResource, RevisionResour
// Chance of collision rises; look at all patch sets on the change.
List<RevisionResource> out = Lists.newArrayList();
for (PatchSet ps : dbProvider.get().patchSets()
.byChange(change.getChange().getId())) {
.byChange(change.getId())) {
if (ps.getRevision() != null && ps.getRevision().get().startsWith(id)) {
out.add(new RevisionResource(change, ps));
}
@ -141,7 +141,7 @@ public class Revisions implements ChildCollection<ChangeResource, RevisionResour
private List<RevisionResource> byLegacyPatchSetId(ChangeResource change,
String id) throws OrmException {
PatchSet ps = dbProvider.get().patchSets().get(new PatchSet.Id(
change.getChange().getId(),
change.getId(),
Integer.parseInt(id)));
if (ps != null) {
return Collections.singletonList(new RevisionResource(change, ps));
@ -161,8 +161,7 @@ public class Revisions implements ChildCollection<ChangeResource, RevisionResour
throws AuthException, IOException {
Optional<ChangeEdit> edit = editUtil.byChange(change.getChange());
if (edit.isPresent()) {
PatchSet ps = new PatchSet(new PatchSet.Id(
change.getChange().getId(), 0));
PatchSet ps = new PatchSet(new PatchSet.Id(change.getId(), 0));
ps.setRevision(edit.get().getRevision());
if (revid == null || edit.get().getRevision().equals(revid)) {
return Collections.singletonList(
@ -174,7 +173,7 @@ public class Revisions implements ChildCollection<ChangeResource, RevisionResour
private static List<RevisionResource> toResources(final ChangeResource change,
Iterable<PatchSet> patchSets) {
final Change.Id changeId = change.getChange().getId();
final Change.Id changeId = change.getId();
return FluentIterable.from(patchSets)
.filter(new Predicate<PatchSet>() {
@Override

2
gerrit-server/src/main/java/com/google/gerrit/server/git/LabelNormalizer.java
View File

@ -116,7 +116,7 @@ public class LabelNormalizer {
LabelTypes labelTypes = ctl.getLabelTypes();
for (PatchSetApproval psa : approvals) {
Change.Id changeId = psa.getKey().getParentKey().getParentKey();
checkArgument(changeId.equals(ctl.getChange().getId()),
checkArgument(changeId.equals(ctl.getId()),
"Approval %s does not match change %s",
psa.getKey(), ctl.getChange().getKey());
if (psa.isSubmit()) {

2
gerrit-server/src/main/java/com/google/gerrit/server/index/IndexedChangeQuery.java
View File

@ -50,7 +50,7 @@ public class IndexedChangeQuery extends Predicate<ChangeData>
int backendLimit = opts.config().maxLimit();
int limit = Ints.saturatedCast((long) opts.limit() + opts.start());
limit = Math.min(limit, backendLimit);
return QueryOptions.create(opts.config(), 0, limit);
return QueryOptions.create(opts.config(), 0, limit, opts.fields());
}
private final ChangeIndex index;

15
gerrit-server/src/main/java/com/google/gerrit/server/index/Schema.java
View File

@ -61,6 +61,8 @@ public class Schema<T> {
}
private final ImmutableMap<String, FieldDef<T, ?>> fields;
private final ImmutableMap<String, FieldDef<T, ?>> storedFields;
private int version;
protected Schema(Iterable<FieldDef<T, ?>> fields) {
@ -71,10 +73,15 @@ public class Schema<T> {
public Schema(int version, Iterable<FieldDef<T, ?>> fields) {
this.version = version;
ImmutableMap.Builder<String, FieldDef<T, ?>> b = ImmutableMap.builder();
ImmutableMap.Builder<String, FieldDef<T, ?>> sb = ImmutableMap.builder();
for (FieldDef<T, ?> f : fields) {
b.put(f.getName(), f);
if (f.isStored()) {
sb.put(f.getName(), f);
}
}
this.fields = b.build();
this.storedFields = sb.build();
}
public final int getVersion() {
@ -94,6 +101,14 @@ public class Schema<T> {
return fields;
}
/**
* @return all fields in this schema where {@link FieldDef#isStored()} is
* true.
*/
public final ImmutableMap<String, FieldDef<T, ?>> getStoredFields() {
return storedFields;
}
/**
* Look up fields in this schema.
*

3
gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java
View File

@ -86,8 +86,7 @@ public abstract class AbstractChangeUpdate extends VersionedMetaData {
}
public void setPatchSetId(PatchSet.Id psId) {
checkArgument(psId == null
|| psId.getParentKey().equals(getChange().getId()));
checkArgument(psId == null || psId.getParentKey().equals(ctl.getId()));
this.psId = psId;
}

4
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java
View File

@ -98,7 +98,7 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
IdentifiedUser user = ctl.getUser().asIdentifiedUser();
this.accountId = user.getAccountId();
this.changeNotes = getChangeNotes().load();
this.draftNotes = draftNotesFactory.create(ctl.getChange().getId(),
this.draftNotes = draftNotesFactory.create(ctl.getId(),
user.getAccountId());
this.upsertComments = Lists.newArrayList();
@ -273,7 +273,7 @@ public class ChangeDraftUpdate extends AbstractChangeUpdate {
@Override
protected String getRefName() {
return RefNames.refsDraftComments(accountId, getChange().getId());
return RefNames.refsDraftComments(accountId, ctl.getId());
}
@Override

2
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java
View File

@ -387,7 +387,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
@Override
protected String getRefName() {
return ChangeNoteUtil.changeRefName(getChange().getId());
return ChangeNoteUtil.changeRefName(ctl.getId());
}
@Override

6
gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
View File

@ -90,7 +90,7 @@ public class ChangeControl {
throws NoSuchChangeException, OrmException {
ChangeControl c = controlFor(change, user);
if (!c.isVisible(db.get())) {
throw new NoSuchChangeException(c.getChange().getId());
throw new NoSuchChangeException(c.getId());
}
return c;
}
@ -153,6 +153,10 @@ public class ChangeControl {
return getProjectControl().getProject();
}
public Change.Id getId() {
return notes.getChangeId();
}
public Change getChange() {
return notes.getChange();
}

19
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
View File

@ -416,7 +416,7 @@ public class ChangeData {
this.patchListCache = patchListCache;
this.notesMigration = notesMigration;
this.mergeabilityCache = mergeabilityCache;
legacyId = c.getChange().getId();
legacyId = c.getId();
change = c.getChange();
changeControl = c;
notes = c.getNotes();
@ -544,6 +544,23 @@ public class ChangeData {
return changeControl;
}
public ChangeControl changeControl(CurrentUser user) throws OrmException {
if (changeControl != null) {
throw new IllegalStateException(
"user already specified: " + changeControl.getUser());
}
try {
if (change != null) {
changeControl = changeControlFactory.controlFor(change, user);
} else {
changeControl = changeControlFactory.controlFor(legacyId, user);
}
} catch (NoSuchChangeException e) {
throw new OrmException(e);
}
return changeControl;
}
void cacheVisibleTo(ChangeControl ctl) {
visibleTo = ctl.getUser();
changeControl = ctl;

5
gerrit-server/src/main/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
View File

@ -106,6 +106,11 @@ public class InternalChangeQuery {
return this;
}
public InternalChangeQuery setRequestedFields(Set<String> fields) {
qp.setRequestedFields(fields);
return this;
}
public List<ChangeData> byKey(Change.Key key) throws OrmException {
return byKeyPrefix(key.get());
}

17
gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryOptions.java
View File

@ -17,29 +17,36 @@ package com.google.gerrit.server.query.change;
import static com.google.common.base.Preconditions.checkArgument;
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.server.index.IndexConfig;
import java.util.Set;
@AutoValue
public abstract class QueryOptions {
public static QueryOptions create(IndexConfig config, int start, int limit) {
public static QueryOptions create(IndexConfig config, int start, int limit,
Set<String> fields) {
checkArgument(start >= 0, "start must be nonnegative: %s", start);
checkArgument(limit > 0, "limit must be positive: %s", limit);
return new AutoValue_QueryOptions(config, start, limit);
return new AutoValue_QueryOptions(config, start, limit,
ImmutableSet.copyOf(fields));
}
public static QueryOptions oneResult() {
return create(IndexConfig.createDefault(), 0, 1);
return create(IndexConfig.createDefault(), 0, 1,
ImmutableSet.<String> of());
}
public abstract IndexConfig config();
public abstract int start();
public abstract int limit();
public abstract ImmutableSet<String> fields();
public QueryOptions withLimit(int newLimit) {
return create(config(), start(), newLimit);
return create(config(), start(), newLimit, fields());
}
public QueryOptions withStart(int newStart) {
return create(config(), newStart, limit());
return create(config(), newStart, limit(), fields());
}
}

26
gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java
View File

@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.server.query.change.ChangeStatusPredicate.open;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Ordering;
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.metrics.Description;
@ -25,6 +26,8 @@ import com.google.gerrit.metrics.MetricMaker;
import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.index.ChangeIndex;
import com.google.gerrit.server.index.IndexCollection;
import com.google.gerrit.server.index.IndexConfig;
import com.google.gerrit.server.index.IndexPredicate;
import com.google.gerrit.server.index.IndexRewriter;
@ -39,11 +42,13 @@ import com.google.inject.Singleton;
import java.util.ArrayList;
import java.util.List;
import java.util.Set;
public class QueryProcessor {
private final Provider<ReviewDb> db;
private final Provider<CurrentUser> userProvider;
private final ChangeControl.GenericFactory changeControlFactory;
private final IndexCollection indexes;
private final IndexRewriter rewriter;
private final IndexConfig indexConfig;
private final Metrics metrics;
@ -51,17 +56,20 @@ public class QueryProcessor {
private int limitFromCaller;
private int start;
private boolean enforceVisibility = true;
private Set<String> requestedFields;
@Inject
QueryProcessor(Provider<ReviewDb> db,
Provider<CurrentUser> userProvider,
ChangeControl.GenericFactory changeControlFactory,
IndexCollection indexes,
IndexRewriter rewriter,
IndexConfig indexConfig,
Metrics metrics) {
this.db = db;
this.userProvider = userProvider;
this.changeControlFactory = changeControlFactory;
this.indexes = indexes;
this.rewriter = rewriter;
this.indexConfig = indexConfig;
this.metrics = metrics;
@ -82,6 +90,11 @@ public class QueryProcessor {
return this;
}
public QueryProcessor setRequestedFields(Set<String> fields) {
requestedFields = fields;
return this;
}
/**
* Query for changes that match a structured query.
*
@ -150,7 +163,8 @@ public class QueryProcessor {
"Cannot go beyond page " + indexConfig.maxPages() + "of results");
}
QueryOptions opts = QueryOptions.create(indexConfig, start, limit + 1);
QueryOptions opts = QueryOptions.create(
indexConfig, start, limit + 1, getRequestedFields());
Predicate<ChangeData> s = rewriter.rewrite(q, opts);
if (!(s instanceof ChangeDataSource)) {
q = Predicate.and(open(), q);
@ -184,6 +198,16 @@ public class QueryProcessor {
return out;
}
private Set<String> getRequestedFields() {
if (requestedFields != null) {
return requestedFields;
}
ChangeIndex index = indexes.getSearchIndex();
return index != null
? index.getSchema().getStoredFields().keySet()
: ImmutableSet.<String> of();
}
boolean isDisabled() {
return getPermittedLimit() <= 0;
}

4
gerrit-server/src/test/java/com/google/gerrit/server/index/IndexRewriterTest.java
View File

@ -25,6 +25,7 @@ import static com.google.gerrit.server.query.Predicate.and;
import static com.google.gerrit.server.query.Predicate.or;
import static org.junit.Assert.assertEquals;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.query.AndPredicate;
import com.google.gerrit.server.query.Predicate;
@ -285,7 +286,8 @@ public class IndexRewriterTest extends GerritBaseTests {
}
private static QueryOptions options(int start, int limit) {
return QueryOptions.create(CONFIG, start, limit);
return QueryOptions.create(CONFIG, start, limit,
ImmutableSet.<String> of());
}
private Set<Change.Status> status(String query) throws QueryParseException {

25
gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
View File

@ -56,6 +56,7 @@ import com.google.gerrit.server.change.ChangeTriplet;
import com.google.gerrit.server.change.PatchSetInserter;
import com.google.gerrit.server.git.BatchUpdate;
import com.google.gerrit.server.git.validators.CommitValidators;
import com.google.gerrit.server.index.ChangeField;
import com.google.gerrit.server.index.IndexCollection;
import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.server.project.ChangeControl;
@ -1263,6 +1264,30 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
cd.messages();
}
@Test
public void prepopulateOnlyRequestedFields() throws Exception {
assume().that(notesMigration.enabled()).isFalse();
TestRepository<Repo> repo = createProject("repo");
Change change = insert(newChange(repo, null, null, null, null));
db = new DisabledReviewDb();
requestContext.setContext(newRequestContext(userId));
// Use QueryProcessor directly instead of API so we get ChangeDatas back.
List<ChangeData> cds = queryProcessor
.setRequestedFields(ImmutableSet.of(
ChangeField.PATCH_SET.getName(),
ChangeField.CHANGE.getName()))
.queryChanges(queryBuilder.parse(change.getId().toString()))
.changes();
assertThat(cds).hasSize(1);
ChangeData cd = cds.get(0);
cd.change();
cd.patchSets();
exception.expect(DisabledReviewDb.Disabled.class);
cd.currentApprovals();
}
protected ChangeInserter newChange(
TestRepository<Repo> repo,

7
gerrit-server/src/test/java/com/google/gerrit/server/query/change/LuceneQueryChangesV14Test.java
View File

@ -78,6 +78,13 @@ public class LuceneQueryChangesV14Test extends LuceneQueryChangesTest {
// Ignore.
}
@Override
@Ignore
@Test
public void prepopulateOnlyRequestedFields() throws Exception {
// Ignore.
}
@Test
public void isReviewed() throws Exception {
clockStepMs = MILLISECONDS.convert(2, MINUTES);

1
gerrit-server/src/test/java/com/google/gerrit/testutil/TestChanges.java
View File

@ -108,6 +108,7 @@ public class TestChanges {
ChangeNotes notes = new ChangeNotes(repoManager, migration, allUsers, c)
.load();
expect(ctl.getNotes()).andStubReturn(notes);
expect(ctl.getId()).andStubReturn(c.getId());
EasyMock.replay(ctl);
return ctl;
}

128
gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/SetReviewersCommand.java
View File

@ -18,9 +18,8 @@ import com.google.gerrit.extensions.api.changes.AddReviewerInput;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.ChangesCollection;
@ -28,14 +27,10 @@ import com.google.gerrit.server.change.DeleteReviewer;
import com.google.gerrit.server.change.PostReviewers;
import com.google.gerrit.server.change.ReviewerResource;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gerrit.sshd.CommandMetaData;
import com.google.gerrit.sshd.SshCommand;
import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.ResultSet;
import com.google.inject.Inject;
import com.google.inject.Provider;
@ -47,7 +42,9 @@ import org.slf4j.LoggerFactory;
import java.io.IOException;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
@CommandMetaData(name = "set-reviewers", description = "Add or remove reviewers on a change")
@ -69,7 +66,7 @@ public class SetReviewersCommand extends SshCommand {
@Argument(index = 0, required = true, multiValued = true, metaVar = "COMMIT", usage = "changes to modify")
void addChange(String token) {
try {
changes.addAll(parseChangeId(token));
addChangeImpl(token);
} catch (UnloggedFailure e) {
throw new IllegalArgumentException(e.getMessage(), e);
} catch (OrmException e) {
@ -80,9 +77,6 @@ public class SetReviewersCommand extends SshCommand {
@Inject
private ReviewDb db;
@Inject
private Provider<InternalChangeQuery> queryProvider;
@Inject
private ReviewerResource.Factory reviewerFactory;
@ -95,25 +89,26 @@ public class SetReviewersCommand extends SshCommand {
@Inject
private Provider<CurrentUser> userProvider;
@Inject
private ChangeControl.GenericFactory changeControlFactory;
@Inject
private ChangesCollection changesCollection;
@Inject
private ChangeUtil changeUtil;
private Set<Account.Id> toRemove = new HashSet<>();
private Set<Change.Id> changes = new HashSet<>();
private Map<Change.Id, ChangeResource> changes = new LinkedHashMap<>();
@Override
protected void run() throws UnloggedFailure {
boolean ok = true;
for (Change.Id changeId : changes) {
for (ChangeResource rsrc : changes.values()) {
try {
ok &= modifyOne(changeId);
ok &= modifyOne(rsrc);
} catch (Exception err) {
ok = false;
log.error("Error updating reviewers on change " + changeId, err);
writeError("fatal", "internal error while updating " + changeId);
log.error("Error updating reviewers on change " + rsrc.getId(), err);
writeError("fatal", "internal error while updating " + rsrc.getId());
}
}
@ -122,8 +117,7 @@ public class SetReviewersCommand extends SshCommand {
}
}
private boolean modifyOne(Change.Id changeId) throws Exception {
ChangeResource changeRsrc = changesCollection.parse(changeId);
private boolean modifyOne(ChangeResource changeRsrc) throws Exception {
boolean ok = true;
// Remove reviewers
@ -168,92 +162,28 @@ public class SetReviewersCommand extends SshCommand {
return ok;
}
private Set<Change.Id> parseChangeId(String idstr)
throws UnloggedFailure, OrmException {
Set<Change.Id> matched = new HashSet<>(4);
boolean isCommit = idstr.matches("^([0-9a-fA-F]{4," + RevId.LEN + "})$");
// By newer style changeKey?
//
boolean changeKeyParses = idstr.matches("^I[0-9a-f]*$");
if (changeKeyParses) {
for (ChangeData cd : queryProvider.get().byKeyPrefix(idstr)) {
matchChange(matched, cd.change());
private void addChangeImpl(String id) throws UnloggedFailure, OrmException {
List<ChangeControl> matched =
changeUtil.findChanges(id, userProvider.get());
List<ChangeControl> toAdd = new ArrayList<>(changes.size());
for (ChangeControl ctl : matched) {
Change c = ctl.getChange();
if (!changes.containsKey(c.getId()) && inProject(c)
&& ctl.isVisible(db)) {
toAdd.add(ctl);
}
}
// By commit?
//
if (isCommit) {
RevId id = new RevId(idstr);
ResultSet<PatchSet> patches;
if (id.isComplete()) {
patches = db.patchSets().byRevision(id);
} else {
patches = db.patchSets().byRevisionRange(id, id.max());
}
for (PatchSet ps : patches) {
matchChange(matched, ps.getId().getParentKey());
}
}
// By older style changeId?
//
boolean changeIdParses = false;
if (idstr.matches("^[1-9][0-9]*$")) {
Change.Id id;
try {
id = Change.Id.parse(idstr);
changeIdParses = true;
} catch (IllegalArgumentException e) {
id = null;
changeIdParses = false;
}
if (changeIdParses) {
matchChange(matched, id);
}
}
if (!changeKeyParses && !isCommit && !changeIdParses) {
throw error("\"" + idstr + "\" is not a valid change");
}
switch (matched.size()) {
switch (toAdd.size()) {
case 0:
throw error("\"" + idstr + "\" no such change");
throw error("\"" + id + "\" no such change");
case 1:
return matched;
ChangeControl ctl = toAdd.get(0);
changes.put(ctl.getId(), changesCollection.parse(ctl));
break;
default:
throw error("\"" + idstr + "\" matches multiple changes");
}
}
private void matchChange(Set<Change.Id> matched, Change.Id changeId) {
if (changeId != null && !matched.contains(changeId)) {
try {
matchChange(matched, db.changes().get(changeId));
} catch (OrmException e) {
log.warn("Error reading change " + changeId, e);
}
}
}
private void matchChange(Set<Change.Id> matched, Change change) {
try {
if (change != null
&& inProject(change)
&& changeControlFactory.controlFor(change,
userProvider.get()).isVisible(db)) {
matched.add(change.getId());
}
} catch (NoSuchChangeException e) {
// Ignore this change.
} catch (OrmException e) {
log.warn("Error reading change " + change.getId(), e);
throw error("\"" + id + "\" matches multiple changes");
}
}