opendev/gerrit
Code Issues Proposed changes

Only construct ChangeResources via ChangesCollection.parse

Browse Source 
In a notedb world we would like to slurp all information about a
change from the git-based storage as soon as a ChangeResource is
constructed, which will involve extra work in this parse, analogous to
how parse currently validates the change permissions.

Preserve the ability to pass in a particular ChangeControl object,
since various callers have specific requirements like reusing the
control elsewhere, or using a control for a different user.

Change-Id: Ia7c9761ef92bb8f05ae3323210babaa02b0dbe6f
This commit is contained in:
Dave Borowitz 2013-12-16 14:30:28 -08:00
parent 1691f61558
commit fcb204627b
10 changed files with 66 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java
View File

@ -34,7 +34,7 @@ import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.ChangesCollection;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.change.Revisions;
import com.google.gerrit.server.extensions.webui.UiActions;
@ -76,6 +76,7 @@ class PatchSetDetailFactory extends Handler<PatchSetDetail> {
private final ReviewDb db;
private final PatchListCache patchListCache;
private final ChangeControl.Factory changeControlFactory;
private final ChangesCollection changes;
private final Revisions revisions;
private Project.NameKey projectKey;
@ -93,6 +94,7 @@ class PatchSetDetailFactory extends Handler<PatchSetDetail> {
PatchSetDetailFactory(final PatchSetInfoFactory psif, final ReviewDb db,
final PatchListCache patchListCache,
final ChangeControl.Factory changeControlFactory,
final ChangesCollection changes,
final Revisions revisions,
@Assisted("psIdBase") @Nullable final PatchSet.Id psIdBase,
@Assisted("psIdNew") final PatchSet.Id psIdNew,
@ -101,6 +103,7 @@ class PatchSetDetailFactory extends Handler<PatchSetDetail> {
this.db = db;
this.patchListCache = patchListCache;
this.changeControlFactory = changeControlFactory;
this.changes = changes;
this.revisions = revisions;
this.psIdBase = psIdBase;
@ -179,7 +182,7 @@ class PatchSetDetailFactory extends Handler<PatchSetDetail> {
detail.setCommands(Lists.newArrayList(Iterables.transform(
UiActions.sorted(UiActions.plugins(UiActions.from(
revisions,
new RevisionResource(new ChangeResource(control), patchSet),
new RevisionResource(changes.parse(control), patchSet),
Providers.of(user)))),
new Function<UiAction.Description, UiCommandDetail>() {
@Override

27
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ReviewProjectAccess.java
View File

@ -20,6 +20,7 @@ import com.google.gerrit.common.data.AccessSection;
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.common.data.PermissionRule;
import com.google.gerrit.extensions.api.changes.AddReviewerInput;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
@ -33,6 +34,7 @@ import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.GroupBackend;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.ChangesCollection;
import com.google.gerrit.server.change.MergeabilityChecker;
import com.google.gerrit.server.change.PostReviewers;
import com.google.gerrit.server.config.AllProjectsNameProvider;
@ -41,7 +43,6 @@ import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.mail.CreateChangeSender;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.project.SetParent;
@ -78,11 +79,11 @@ public class ReviewProjectAccess extends ProjectAccessHandler<Change.Id> {
private final IdentifiedUser user;
private final PatchSetInfoFactory patchSetInfoFactory;
private final Provider<PostReviewers> reviewersProvider;
private final ChangeControl.GenericFactory changeFactory;
private final MergeabilityChecker mergeabilityChecker;
private final ChangeHooks hooks;
private final CreateChangeSender.Factory createChangeSenderFactory;
private final ProjectCache projectCache;
private final ChangesCollection changes;
@Inject
ReviewProjectAccess(final ProjectControl.Factory projectControlFactory,
@ -90,11 +91,11 @@ public class ReviewProjectAccess extends ProjectAccessHandler<Change.Id> {
MetaDataUpdate.User metaDataUpdateFactory, ReviewDb db,
IdentifiedUser user, PatchSetInfoFactory patchSetInfoFactory,
Provider<PostReviewers> reviewersProvider,
ChangeControl.GenericFactory changeFactory,
MergeabilityChecker mergeabilityChecker, ChangeHooks hooks,
CreateChangeSender.Factory createChangeSenderFactory,
ProjectCache projectCache,
AllProjectsNameProvider allProjects,
ChangesCollection changes,
Provider<SetParent> setParent,
@Assisted("projectName") Project.NameKey projectName,
@ -109,11 +110,11 @@ public class ReviewProjectAccess extends ProjectAccessHandler<Change.Id> {
this.user = user;
this.patchSetInfoFactory = patchSetInfoFactory;
this.reviewersProvider = reviewersProvider;
this.changeFactory = changeFactory;
this.mergeabilityChecker = mergeabilityChecker;
this.hooks = hooks;
this.createChangeSenderFactory = createChangeSenderFactory;
this.projectCache = projectCache;
this.changes = changes;
}
@Override
@ -165,9 +166,15 @@ public class ReviewProjectAccess extends ProjectAccessHandler<Change.Id> {
} catch (Exception err) {
log.error("Cannot send email for new change " + change.getId(), err);
}
addProjectOwnersAsReviewers(change);
ChangeResource rsrc;
try {
rsrc = changes.parse(changeId);
} catch (ResourceNotFoundException e) {
throw new IOException(e);
}
addProjectOwnersAsReviewers(rsrc);
if (parentProjectUpdate) {
addAdministratorsAsReviewers(change);
addAdministratorsAsReviewers(rsrc);
}
return changeId;
}
@ -186,12 +193,10 @@ public class ReviewProjectAccess extends ProjectAccessHandler<Change.Id> {
db.patchSetAncestors().insert(toInsert);
}
private void addProjectOwnersAsReviewers(Change change) {
private void addProjectOwnersAsReviewers(ChangeResource rsrc) {
final String projectOwners =
groupBackend.get(SystemGroupBackend.PROJECT_OWNERS).getName();
try {
ChangeResource rsrc =
new ChangeResource(changeFactory.controlFor(change, user));
AddReviewerInput input = new AddReviewerInput();
input.reviewer = projectOwners;
reviewersProvider.get().apply(rsrc, input);
@ -201,15 +206,13 @@ public class ReviewProjectAccess extends ProjectAccessHandler<Change.Id> {
}
}
private void addAdministratorsAsReviewers(Change change) {
private void addAdministratorsAsReviewers(ChangeResource rsrc) {
List<PermissionRule> adminRules =
projectCache.getAllProjects().getConfig()
.getAccessSection(AccessSection.GLOBAL_CAPABILITIES)
.getPermission(GlobalCapability.ADMINISTRATE_SERVER).getRules();
for (PermissionRule r : adminRules) {
try {
ChangeResource rsrc =
new ChangeResource(changeFactory.controlFor(change, user));
AddReviewerInput input = new AddReviewerInput();
input.reviewer = r.getGroup().getUUID().get();
reviewersProvider.get().apply(rsrc, input);

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

@ -133,11 +133,12 @@ public class ChangeJson {
private final IdentifiedUser.GenericFactory userFactory;
private final ProjectControl.GenericFactory projectControlFactory;
private final PatchSetInfoFactory patchSetInfoFactory;
private final ChangesCollection changes;
private final FileInfoJson fileInfoJson;
private final AccountInfo.Loader.Factory accountLoaderFactory;
private final DynamicMap<DownloadScheme> downloadSchemes;
private final DynamicMap<DownloadCommand> downloadCommands;
private final DynamicMap<RestView<ChangeResource>> changes;
private final DynamicMap<RestView<ChangeResource>> changeViews;
private final Revisions revisions;
private final PatchListCache patchListCache;
@ -156,11 +157,12 @@ public class ChangeJson {
IdentifiedUser.GenericFactory uf,
ProjectControl.GenericFactory pcf,
PatchSetInfoFactory psi,
ChangesCollection changes,
FileInfoJson fileInfoJson,
AccountInfo.Loader.Factory ailf,
DynamicMap<DownloadScheme> downloadSchemes,
DynamicMap<DownloadCommand> downloadCommands,
DynamicMap<RestView<ChangeResource>> changes,
DynamicMap<RestView<ChangeResource>> changeViews,
Revisions revisions,
PatchListCache patchListCache) {
this.db = db;
@ -170,11 +172,12 @@ public class ChangeJson {
this.userFactory = uf;
this.projectControlFactory = pcf;
this.patchSetInfoFactory = psi;
this.changes = changes;
this.fileInfoJson = fileInfoJson;
this.accountLoaderFactory = ailf;
this.downloadSchemes = downloadSchemes;
this.downloadCommands = downloadCommands;
this.changes = changes;
this.changeViews = changeViews;
this.revisions = revisions;
this.patchListCache = patchListCache;
@ -322,8 +325,8 @@ public class ChangeJson {
if (has(CURRENT_ACTIONS) && userProvider.get().isIdentifiedUser()) {
out.actions = Maps.newTreeMap();
for (UiAction.Description d : UiActions.from(
changes,
new ChangeResource(control(cd)),
changeViews,
changes.parse(control(cd)),
userProvider)) {
out.actions.put(d.getId(), new ActionInfo(d));
}
@ -821,7 +824,7 @@ public class ChangeJson {
out.actions = Maps.newTreeMap();
for (UiAction.Description d : UiActions.from(
revisions,
new RevisionResource(new ChangeResource(control(cd)), in),
new RevisionResource(changes.parse(control(cd)), in),
userProvider)) {
out.actions.put(d.getId(), new ActionInfo(d));
}

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

@ -35,7 +35,7 @@ public class ChangeResource implements RestResource, HasETag {
private final ChangeControl control;
public ChangeResource(ChangeControl control) {
ChangeResource(ChangeControl control) {
this.control = control;
}

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

@ -83,6 +83,16 @@ public class ChangesCollection implements
return new ChangeResource(control);
}
public ChangeResource parse(Change.Id id)
throws ResourceNotFoundException, OrmException {
return parse(TopLevelResource.INSTANCE,
IdString.fromUrl(Integer.toString(id.get())));
}
public ChangeResource parse(ChangeControl control) throws OrmException {
return new ChangeResource(control);
}
private List<Change> findChanges(String id)
throws OrmException, ResourceNotFoundException {
// Try legacy id

14
gerrit-server/src/main/java/com/google/gerrit/server/change/MergeabilityChecker.java
View File

@ -37,7 +37,6 @@ import com.google.gerrit.server.git.MetaDataUpdate;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.git.WorkQueue.Executor;
import com.google.gerrit.server.index.ChangeIndexer;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.util.RequestContext;
import com.google.gerrit.server.util.ThreadLocalRequestContext;
import com.google.gwtorm.server.OrmException;
@ -64,8 +63,8 @@ public class MergeabilityChecker implements GitReferenceUpdatedListener {
private final ThreadLocalRequestContext tl;
private final SchemaFactory<ReviewDb> schemaFactory;
private final ChangeControl.GenericFactory changeControlFactory;
private final IdentifiedUser.GenericFactory identifiedUserFactory;
private final ChangesCollection changes;
private final Provider<Mergeable> mergeable;
private final ChangeIndexer indexer;
private final ListeningExecutorService executor;
@ -75,16 +74,15 @@ public class MergeabilityChecker implements GitReferenceUpdatedListener {
@Inject
public MergeabilityChecker(ThreadLocalRequestContext tl,
SchemaFactory<ReviewDb> schemaFactory,
ChangeControl.GenericFactory changeControlFactory,
IdentifiedUser.GenericFactory identifiedUserFactory,
Provider<Mergeable> mergeable, ChangeIndexer indexer,
@MergeabilityChecksExecutor Executor executor,
ChangesCollection changes, Provider<Mergeable> mergeable,
ChangeIndexer indexer, @MergeabilityChecksExecutor Executor executor,
MergeabilityCheckQueue mergeabilityCheckQueue,
MetaDataUpdate.Server metaDataUpdateFactory) {
this.tl = tl;
this.schemaFactory = schemaFactory;
this.changeControlFactory = changeControlFactory;
this.identifiedUserFactory = identifiedUserFactory;
this.changes = changes;
this.mergeable = mergeable;
this.indexer = indexer;
this.executor = MoreExecutors.listeningDecorator(executor);
@ -263,8 +261,8 @@ public class MergeabilityChecker implements GitReferenceUpdatedListener {
PatchSet ps = db.patchSets().get(change.currentPatchSetId());
Mergeable m = mergeable.get();
m.setForce(force);
MergeableInfo info = m.apply(new RevisionResource(new ChangeResource(
changeControlFactory.controlFor(change, context.getCurrentUser())), ps));
MergeableInfo info = m.apply(
new RevisionResource(changes.parse(change.getId()), ps));
return change.isMergeable() != info.mergeable;
} catch (ResourceConflictException e) {
// change is closed

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

@ -73,6 +73,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
private final Provider<ReviewDb> db;
private final ChangesCollection changes;
private final ChangeIndexer indexer;
private final AccountsCollection accounts;
private final EmailReviewComments.Factory email;
@ -87,11 +88,13 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
@Inject
PostReview(Provider<ReviewDb> db,
ChangesCollection changes,
ChangeIndexer indexer,
AccountsCollection accounts,
EmailReviewComments.Factory email,
ChangeHooks hooks) {
this.db = db;
this.changes = changes;
this.indexer = indexer;
this.accounts = accounts;
this.email = email;
@ -193,7 +196,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
ChangeControl target = caller.forUser(accounts.parse(in.onBehalfOf));
return new RevisionResource(new ChangeResource(target), rev.getPatchSet());
return new RevisionResource(changes.parse(target), rev.getPatchSet());
}
private void checkLabels(RevisionResource revision, boolean strict,

7
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
View File

@ -69,7 +69,7 @@ import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.change.ChangeInserter;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.ChangesCollection;
import com.google.gerrit.server.change.MergeabilityChecker;
import com.google.gerrit.server.change.PatchSetInserter;
import com.google.gerrit.server.change.PatchSetInserter.ChangeKind;
@ -269,6 +269,7 @@ public class ReceiveCommits {
private final CommitValidators.Factory commitValidatorsFactory;
private final TagCache tagCache;
private final AccountCache accountCache;
private final ChangesCollection changes;
private final ChangeInserter.Factory changeInserterFactory;
private final WorkQueue workQueue;
private final ListeningExecutorService changeUpdateExector;
@ -329,6 +330,7 @@ public class ReceiveCommits {
final TagCache tagCache,
final AccountCache accountCache,
final ChangeCache changeCache,
final ChangesCollection changes,
final ChangeInserter.Factory changeInserterFactory,
final CommitValidators.Factory commitValidatorsFactory,
@CanonicalWebUrl final String canonicalWebUrl,
@ -364,6 +366,7 @@ public class ReceiveCommits {
this.canonicalWebUrl = canonicalWebUrl;
this.tagCache = tagCache;
this.accountCache = accountCache;
this.changes = changes;
this.changeInserterFactory = changeInserterFactory;
this.commitValidatorsFactory = commitValidatorsFactory;
this.workQueue = workQueue;
@ -1561,7 +1564,7 @@ public class ReceiveCommits {
private void submit(ChangeControl changeCtl, PatchSet ps)
throws OrmException, IOException {
Submit submit = submitProvider.get();
RevisionResource rsrc = new RevisionResource(new ChangeResource(changeCtl), ps);
RevisionResource rsrc = new RevisionResource(changes.parse(changeCtl), ps);
Change c = submit.submit(rsrc, currentUser);
if (c == null) {
addError("Submitting change " + changeCtl.getChange().getChangeId()

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

@ -42,7 +42,7 @@ import com.google.gerrit.server.account.AccountManager;
import com.google.gerrit.server.account.AuthRequest;
import com.google.gerrit.server.change.ChangeInserter;
import com.google.gerrit.server.change.ChangeJson.ChangeInfo;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.ChangesCollection;
import com.google.gerrit.server.change.PostReview;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.project.ChangeControl;
@ -79,6 +79,7 @@ public abstract class AbstractQueryChangesTest {
@Inject protected AccountManager accountManager;
@Inject protected ChangeControl.GenericFactory changeControlFactory;
@Inject protected ChangeInserter.Factory changeFactory;
@Inject protected ChangesCollection changes;
@Inject protected CreateProject.Factory projectFactory;
@Inject protected IdentifiedUser.RequestFactory userFactory;
@Inject protected InMemoryDatabase schemaFactory;
@ -365,7 +366,7 @@ public abstract class AbstractQueryChangesTest {
input.message = "toplevel";
input.labels = ImmutableMap.<String, Short> of("Code-Review", (short) 1);
postReview.apply(new RevisionResource(
new ChangeResource(ctl), ins.getPatchSet()), input);
changes.parse(ctl), ins.getPatchSet()), input);
assertTrue(query("label:Code-Review=-2").isEmpty());
assertTrue(query("label:Code-Review-2").isEmpty());
@ -479,7 +480,7 @@ public abstract class AbstractQueryChangesTest {
ReviewInput input = new ReviewInput();
input.message = "toplevel";
postReview.apply(new RevisionResource(
new ChangeResource(ctl1), ins1.getPatchSet()), input);
changes.parse(ctl1), ins1.getPatchSet()), input);
change1 = db.changes().get(change1.getId());
assertTrue(lastUpdatedMs(change1) > lastUpdatedMs(change2));
@ -512,7 +513,7 @@ public abstract class AbstractQueryChangesTest {
ReviewInput input = new ReviewInput();
input.message = "toplevel";
postReview.apply(new RevisionResource(
new ChangeResource(ctl1), ins1.getPatchSet()), input);
changes.parse(ctl1), ins1.getPatchSet()), input);
change1 = db.changes().get(change1.getId());
assertTrue(lastUpdatedMs(change1) > lastUpdatedMs(change2));
@ -610,7 +611,7 @@ public abstract class AbstractQueryChangesTest {
input.comments = ImmutableMap.<String, List<ReviewInput.Comment>> of(
"Foo.java", ImmutableList.<ReviewInput.Comment> of(comment));
postReview.apply(new RevisionResource(
new ChangeResource(ctl), ins.getPatchSet()), input);
changes.parse(ctl), ins.getPatchSet()), input);
assertTrue(query("comment:foo").isEmpty());
assertResultEquals(change, queryOne("comment:toplevel"));

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

@ -22,6 +22,7 @@ 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.change.ChangeResource;
import com.google.gerrit.server.change.ChangesCollection;
import com.google.gerrit.server.change.DeleteReviewer;
import com.google.gerrit.server.change.PostReviewers;
import com.google.gerrit.server.change.ReviewerResource;
@ -88,6 +89,9 @@ public class SetReviewersCommand extends SshCommand {
@Inject
private ChangeControl.Factory changeControlFactory;
@Inject
private ChangesCollection changesCollection;
private Set<Account.Id> toRemove = new HashSet<Account.Id>();
private Set<Change.Id> changes = new HashSet<Change.Id>();
@ -110,8 +114,7 @@ public class SetReviewersCommand extends SshCommand {
}
private boolean modifyOne(Change.Id changeId) throws Exception {
ChangeResource changeRsrc =
new ChangeResource(changeControlFactory.validateFor(changeId));
ChangeResource changeRsrc = changesCollection.parse(changeId);
boolean ok = true;
// Remove reviewers