Kill ChangeControl.Factory

Instead use the GenericFactory. You can't use ChangeControl.Factory
inside of REST API handlers. There are only 3 callers of
ChangeControl.Factory. Its easier to remove this than to
keep it around confusing people that are newer to the code
base.

Change-Id: I12361405475a3fa4c2f855187488fdaea51fd696
This commit is contained in:
Stefan Beller
2015-01-13 17:40:41 -08:00
parent df6a0109f9
commit 53a5ccd4df
6 changed files with 50 additions and 66 deletions

View File

@@ -21,6 +21,7 @@ import com.google.gerrit.reviewdb.client.Patch;
import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.mime.FileTypeRegistry; import com.google.gerrit.server.mime.FileTypeRegistry;
import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ChangeControl;
@@ -73,16 +74,19 @@ public class CatServlet extends HttpServlet {
private final GitRepositoryManager repoManager; private final GitRepositoryManager repoManager;
private final SecureRandom rng; private final SecureRandom rng;
private final FileTypeRegistry registry; private final FileTypeRegistry registry;
private final ChangeControl.Factory changeControl; private final Provider<CurrentUser> userProvider;
private final ChangeControl.GenericFactory changeControl;
@Inject @Inject
CatServlet(final GitRepositoryManager grm, final Provider<ReviewDb> sf, CatServlet(final GitRepositoryManager grm, final Provider<ReviewDb> sf,
final FileTypeRegistry ftr, final ChangeControl.Factory ccf) { final FileTypeRegistry ftr, final ChangeControl.GenericFactory ccf,
final Provider<CurrentUser> usrprv) {
requestDb = sf; requestDb = sf;
repoManager = grm; repoManager = grm;
rng = new SecureRandom(); rng = new SecureRandom();
registry = ftr; registry = ftr;
changeControl = ccf; changeControl = ccf;
userProvider = usrprv;
} }
@Override @Override
@@ -140,7 +144,8 @@ public class CatServlet extends HttpServlet {
final PatchSet patchSet; final PatchSet patchSet;
try { try {
final ReviewDb db = requestDb.get(); final ReviewDb db = requestDb.get();
final ChangeControl control = changeControl.validateFor(changeId); final ChangeControl control = changeControl.validateFor(changeId,
userProvider.get());
project = control.getProject(); project = control.getProject();
patchSet = db.patchSets().get(patchKey.getParentKey()); patchSet = db.patchSets().get(patchKey.getParentKey());

View File

@@ -27,6 +27,7 @@ import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountDiffPreference; import com.google.gerrit.reviewdb.client.AccountDiffPreference;
import com.google.gerrit.reviewdb.client.AccountDiffPreference.Whitespace; import com.google.gerrit.reviewdb.client.AccountDiffPreference.Whitespace;
import com.google.gerrit.reviewdb.client.AccountPatchReview; import com.google.gerrit.reviewdb.client.AccountPatchReview;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Patch; import com.google.gerrit.reviewdb.client.Patch;
import com.google.gerrit.reviewdb.client.PatchLineComment; import com.google.gerrit.reviewdb.client.PatchLineComment;
import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSet;
@@ -50,6 +51,7 @@ import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject; import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.assistedinject.Assisted; import com.google.inject.assistedinject.Assisted;
import com.google.inject.util.Providers; import com.google.inject.util.Providers;
@@ -77,7 +79,8 @@ class PatchSetDetailFactory extends Handler<PatchSetDetail> {
private final PatchSetInfoFactory infoFactory; private final PatchSetInfoFactory infoFactory;
private final ReviewDb db; private final ReviewDb db;
private final PatchListCache patchListCache; private final PatchListCache patchListCache;
private final ChangeControl.Factory changeControlFactory; private final Provider<CurrentUser> userProvider;
private final ChangeControl.GenericFactory changeControlFactory;
private final ChangesCollection changes; private final ChangesCollection changes;
private final Revisions revisions; private final Revisions revisions;
private final PatchLineCommentsUtil plcUtil; private final PatchLineCommentsUtil plcUtil;
@@ -96,7 +99,8 @@ class PatchSetDetailFactory extends Handler<PatchSetDetail> {
@Inject @Inject
PatchSetDetailFactory(final PatchSetInfoFactory psif, final ReviewDb db, PatchSetDetailFactory(final PatchSetInfoFactory psif, final ReviewDb db,
final PatchListCache patchListCache, final PatchListCache patchListCache,
final ChangeControl.Factory changeControlFactory, final Provider<CurrentUser> userProvider,
final ChangeControl.GenericFactory changeControlFactory,
final ChangesCollection changes, final ChangesCollection changes,
final Revisions revisions, final Revisions revisions,
final PatchLineCommentsUtil plcUtil, final PatchLineCommentsUtil plcUtil,
@@ -106,6 +110,7 @@ class PatchSetDetailFactory extends Handler<PatchSetDetail> {
this.infoFactory = psif; this.infoFactory = psif;
this.db = db; this.db = db;
this.patchListCache = patchListCache; this.patchListCache = patchListCache;
this.userProvider = userProvider;
this.changeControlFactory = changeControlFactory; this.changeControlFactory = changeControlFactory;
this.changes = changes; this.changes = changes;
this.revisions = revisions; this.revisions = revisions;
@@ -120,7 +125,8 @@ class PatchSetDetailFactory extends Handler<PatchSetDetail> {
public PatchSetDetail call() throws OrmException, NoSuchEntityException, public PatchSetDetail call() throws OrmException, NoSuchEntityException,
PatchSetInfoNotAvailableException, NoSuchChangeException { PatchSetInfoNotAvailableException, NoSuchChangeException {
if (control == null || patchSet == null) { if (control == null || patchSet == null) {
control = changeControlFactory.validateFor(psIdNew.getParentKey()); control = changeControlFactory.validateFor(psIdNew.getParentKey(),
userProvider.get());
patchSet = db.patchSets().get(psIdNew); patchSet = db.patchSets().get(psIdNew);
if (patchSet == null) { if (patchSet == null) {
throw new NoSuchEntityException(); throw new NoSuchEntityException();

View File

@@ -34,17 +34,20 @@ import com.google.inject.Provider;
class PatchDetailServiceImpl extends BaseServiceImplementation implements class PatchDetailServiceImpl extends BaseServiceImplementation implements
PatchDetailService { PatchDetailService {
private final PatchScriptFactory.Factory patchScriptFactoryFactory; private final PatchScriptFactory.Factory patchScriptFactoryFactory;
private final ChangeControl.Factory changeControlFactory; private final ChangeControl.GenericFactory changeControlFactory;
private final Provider<ReviewDb> dbProvider;
@Inject @Inject
PatchDetailServiceImpl(final Provider<ReviewDb> schema, PatchDetailServiceImpl(final Provider<ReviewDb> schema,
final Provider<CurrentUser> currentUser, final Provider<CurrentUser> currentUser,
final PatchScriptFactory.Factory patchScriptFactoryFactory, final PatchScriptFactory.Factory patchScriptFactoryFactory,
final ChangeControl.Factory changeControlFactory) { final ChangeControl.GenericFactory changeControlFactory,
Provider<ReviewDb> dbProvider) {
super(schema, currentUser); super(schema, currentUser);
this.patchScriptFactoryFactory = patchScriptFactoryFactory; this.patchScriptFactoryFactory = patchScriptFactoryFactory;
this.changeControlFactory = changeControlFactory; this.changeControlFactory = changeControlFactory;
this.dbProvider = dbProvider;
} }
@Override @Override
@@ -59,8 +62,9 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements
new Handler<PatchScript>() { new Handler<PatchScript>() {
@Override @Override
public PatchScript call() throws Exception { public PatchScript call() throws Exception {
Change.Id changeId = patchKey.getParentKey().getParentKey(); ChangeControl control = changeControlFactory.validateFor(
ChangeControl control = changeControlFactory.validateFor(changeId); patchKey.getParentKey().getParentKey(),
getCurrentUser());
return patchScriptFactoryFactory.create( return patchScriptFactoryFactory.create(
control, patchKey.getFileName(), psa, psb, dp).call(); control, patchKey.getFileName(), psa, psb, dp).call();
} }

View File

@@ -34,7 +34,6 @@ public class GerritRequestModule extends FactoryModule {
bind(IdentifiedUser.RequestFactory.class).in(SINGLETON); bind(IdentifiedUser.RequestFactory.class).in(SINGLETON);
bind(PerRequestProjectControlCache.class).in(RequestScoped.class); bind(PerRequestProjectControlCache.class).in(RequestScoped.class);
bind(ChangeControl.Factory.class).in(SINGLETON);
bind(ProjectControl.Factory.class).in(SINGLETON); bind(ProjectControl.Factory.class).in(SINGLETON);
factory(SubmoduleOp.Factory.class); factory(SubmoduleOp.Factory.class);

View File

@@ -53,6 +53,15 @@ public class ChangeControl {
db = d; db = d;
} }
public ChangeControl controlFor(Change.Id changeId, CurrentUser user)
throws NoSuchChangeException, OrmException {
Change change = db.get().changes().get(changeId);
if (change == null) {
throw new NoSuchChangeException(changeId);
}
return controlFor(change, user);
}
public ChangeControl controlFor(Change change, CurrentUser user) public ChangeControl controlFor(Change change, CurrentUser user)
throws NoSuchChangeException { throws NoSuchChangeException {
final Project.NameKey projectKey = change.getProject(); final Project.NameKey projectKey = change.getProject();
@@ -66,6 +75,15 @@ public class ChangeControl {
} }
} }
public ChangeControl validateFor(Change.Id changeId, CurrentUser user)
throws NoSuchChangeException, OrmException {
Change change = db.get().changes().get(changeId);
if (change == null) {
throw new NoSuchChangeException(changeId);
}
return validateFor(change, user);
}
public ChangeControl validateFor(Change change, CurrentUser user) public ChangeControl validateFor(Change change, CurrentUser user)
throws NoSuchChangeException, OrmException { throws NoSuchChangeException, OrmException {
ChangeControl c = controlFor(change, user); ChangeControl c = controlFor(change, user);
@@ -76,59 +94,6 @@ public class ChangeControl {
} }
} }
public static class Factory {
private final ProjectControl.Factory projectControl;
private final Provider<ReviewDb> db;
@Inject
Factory(final ProjectControl.Factory p, final Provider<ReviewDb> d) {
projectControl = p;
db = d;
}
public ChangeControl controlFor(final Change.Id id)
throws NoSuchChangeException {
final Change change;
try {
change = db.get().changes().get(id);
if (change == null) {
throw new NoSuchChangeException(id);
}
} catch (OrmException e) {
throw new NoSuchChangeException(id, e);
}
return controlFor(change);
}
public ChangeControl controlFor(final Change change)
throws NoSuchChangeException {
try {
final Project.NameKey projectKey = change.getProject();
return projectControl.validateFor(projectKey).controlFor(change);
} catch (NoSuchProjectException e) {
throw new NoSuchChangeException(change.getId(), e);
}
}
public ChangeControl validateFor(final Change.Id id)
throws NoSuchChangeException, OrmException {
return validate(controlFor(id), db.get());
}
public ChangeControl validateFor(final Change change)
throws NoSuchChangeException, OrmException {
return validate(controlFor(change), db.get());
}
private static ChangeControl validate(final ChangeControl c, final ReviewDb db)
throws NoSuchChangeException, OrmException{
if (!c.isVisible(db)) {
throw new NoSuchChangeException(c.getChange().getId());
}
return c;
}
}
public interface AssistedFactory { public interface AssistedFactory {
ChangeControl create(RefControl refControl, Change change); ChangeControl create(RefControl refControl, Change change);
ChangeControl create(RefControl refControl, ChangeNotes notes); ChangeControl create(RefControl refControl, ChangeNotes notes);

View File

@@ -29,6 +29,7 @@ import com.google.gerrit.server.change.ReviewerResource;
import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectControl; import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.sshd.CommandMetaData; import com.google.gerrit.sshd.CommandMetaData;
import com.google.gerrit.sshd.SshCommand; import com.google.gerrit.sshd.SshCommand;
import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.OrmException;
@@ -87,7 +88,10 @@ public class SetReviewersCommand extends SshCommand {
private Provider<DeleteReviewer> deleteReviewerProvider; private Provider<DeleteReviewer> deleteReviewerProvider;
@Inject @Inject
private ChangeControl.Factory changeControlFactory; private Provider<CurrentUser> userProvider;
@Inject
private ChangeControl.GenericFactory changeControlFactory;
@Inject @Inject
private ChangesCollection changesCollection; private ChangesCollection changesCollection;
@@ -248,7 +252,8 @@ public class SetReviewersCommand extends SshCommand {
try { try {
if (change != null if (change != null
&& inProject(change) && inProject(change)
&& changeControlFactory.controlFor(change).isVisible(db)) { && changeControlFactory.controlFor(change,
userProvider.get()).isVisible(db)) {
matched.add(change.getId()); matched.add(change.getId());
} }
} catch (NoSuchChangeException e) { } catch (NoSuchChangeException e) {