opendev/gerrit
Code Issues Proposed changes

Don't inject AllUsersNameProvider

Browse Source 
This provider provides the name as a singleton based on the config.
Injecting the AllUsersName directly implicitly calls the Provider at
injection time, so injecting the Provider just means unnecessary code
to get() at call sites.

While we're in there, make AllUsersNameProvider an actual @Singleton.

Change-Id: I44a859fa229983df115542f7ad97c57d29396996
This commit is contained in:
Dave Borowitz
2016-02-18 14:48:20 -05:00
parent 4b69634e21
commit bebb1790a2
7 changed files with 30 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -35,7 +35,6 @@ import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.AllUsersNameProvider;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
@@ -122,11 +121,11 @@ public class PatchLineCommentsUtil {
@Inject
PatchLineCommentsUtil(GitRepositoryManager repoManager,
AllUsersNameProvider allUsersProvider,
AllUsersName allUsers,
DraftCommentNotes.Factory draftFactory,
NotesMigration migration) {
this.repoManager = repoManager;
this.allUsers = allUsersProvider.get();
this.allUsers = allUsers;
this.draftFactory = draftFactory;
this.migration = migration;
}

2
gerrit-server/src/main/java/com/google/gerrit/server/config/AllUsersNameProvider.java
View File

@@ -16,9 +16,11 @@ package com.google.gerrit.server.config;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import org.eclipse.jgit.lib.Config;
@Singleton
public class AllUsersNameProvider implements Provider<AllUsersName> {
public static final String DEFAULT = "All-Users";

21
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
View File

@@ -47,7 +47,6 @@ import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.reviewdb.server.ReviewDbUtil;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.AllUsersNameProvider;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.ScanningChangeCacheImpl;
import com.google.gerrit.server.project.NoSuchChangeException;
@@ -121,7 +120,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
private final GitRepositoryManager repoManager;
private final NotesMigration migration;
private final AllUsersNameProvider allUsersProvider;
private final AllUsersName allUsers;
private final Provider<InternalChangeQuery> queryProvider;
private final ProjectCache projectCache;
@@ -129,12 +128,12 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
@Inject
public Factory(GitRepositoryManager repoManager,
NotesMigration migration,
AllUsersNameProvider allUsersProvider,
AllUsersName allUsers,
Provider<InternalChangeQuery> queryProvider,
ProjectCache projectCache) {
this.repoManager = repoManager;
this.migration = migration;
this.allUsersProvider = allUsersProvider;
this.allUsers = allUsers;
this.queryProvider = queryProvider;
this.projectCache = projectCache;
}
@@ -181,7 +180,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
project, changeId, change.getProject());
// TODO: Throw NoSuchChangeException when the change is not found in the
// database
return new ChangeNotes(repoManager, migration, allUsersProvider, project,
return new ChangeNotes(repoManager, migration, allUsers, project,
change).load();
}
@@ -194,12 +193,12 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
* @return change notes
*/
public ChangeNotes createFromIndexedChange(Change change) {
return new ChangeNotes(repoManager, migration, allUsersProvider,
return new ChangeNotes(repoManager, migration, allUsers,
change.getProject(), change);
}
public ChangeNotes createForNew(Change change) throws OrmException {
return new ChangeNotes(repoManager, migration, allUsersProvider,
return new ChangeNotes(repoManager, migration, allUsers,
change.getProject(), change).load();
}
@@ -209,7 +208,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
checkState(!migration.readChanges(), "do not call"
+ " createFromIdOnlyWhenNotedbDisabled when notedb is enabled");
Change change = db.changes().get(changeId);
return new ChangeNotes(repoManager, migration, allUsersProvider,
return new ChangeNotes(repoManager, migration, allUsers,
change.getProject(), change).load();
}
@@ -222,7 +221,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
throws OrmException {
checkState(!migration.readChanges(), "do not call"
+ " createFromChangeWhenNotedbDisabled when notedb is enabled");
return new ChangeNotes(repoManager, migration, allUsersProvider,
return new ChangeNotes(repoManager, migration, allUsers,
change.getProject(), change).load();
}
@@ -275,10 +274,10 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
@VisibleForTesting
public ChangeNotes(GitRepositoryManager repoManager, NotesMigration migration,
AllUsersNameProvider allUsersProvider, Project.NameKey project,
AllUsersName allUsers, Project.NameKey project,
Change change) {
super(repoManager, migration, change != null ? change.getId() : null);
this.allUsers = allUsersProvider.get();
this.allUsers = allUsers;
this.project = project;
this.change = change != null ? new Change(change) : null;
}

5
gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java
View File

@@ -23,7 +23,6 @@ import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.AllUsersNameProvider;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -51,10 +50,10 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
@Inject
public Factory(GitRepositoryManager repoManager,
NotesMigration migration,
AllUsersNameProvider allUsers) {
AllUsersName allUsers) {
this.repoManager = repoManager;
this.migration = migration;
this.draftsProject = allUsers.get();
this.draftsProject = allUsers;
}
public DraftCommentNotes create(Change.Id changeId, Account.Id accountId) {

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

@@ -44,7 +44,6 @@ import com.google.gerrit.server.account.VersionedAccountQueries;
import com.google.gerrit.server.change.ChangeTriplet;
import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.AllUsersNameProvider;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.TrackingFooters;
import com.google.gerrit.server.git.GitRepositoryManager;
@@ -169,7 +168,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
final AccountResolver accountResolver;
final GroupBackend groupBackend;
final AllProjectsName allProjectsName;
final AllUsersNameProvider allUsersName;
final AllUsersName allUsersName;
final PatchListCache patchListCache;
final GitRepositoryManager repoManager;
final ProjectCache projectCache;
@@ -201,7 +200,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
AccountResolver accountResolver,
GroupBackend groupBackend,
AllProjectsName allProjectsName,
AllUsersNameProvider allUsersName,
AllUsersName allUsersName,
PatchListCache patchListCache,
GitRepositoryManager repoManager,
ProjectCache projectCache,
@@ -239,7 +238,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
AccountResolver accountResolver,
GroupBackend groupBackend,
AllProjectsName allProjectsName,
AllUsersNameProvider allUsersName,
AllUsersName allUsersName,
PatchListCache patchListCache,
GitRepositoryManager repoManager,
ProjectCache projectCache,
@@ -860,8 +859,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
@Operator
public Predicate<ChangeData> query(String name) throws QueryParseException {
AllUsersName allUsers = args.allUsersName.get();
try (Repository git = args.repoManager.openRepository(allUsers)) {
try (Repository git = args.repoManager.openRepository(args.allUsersName)) {
VersionedAccountQueries q = VersionedAccountQueries.forUser(self());
q.load(git);
String query = q.getQueryList().getQuery(name);
@@ -870,7 +868,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
}
} catch (RepositoryNotFoundException e) {
throw new QueryParseException("Unknown named query (no " +
allUsers.get() +" repo): " + name, e);
args.allUsersName + " repo): " + name, e);
} catch (IOException | ConfigInvalidException e) {
throw new QueryParseException("Error parsing named query: " + name, e);
}
@@ -886,8 +884,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
@Operator
public Predicate<ChangeData> destination(String name)
throws QueryParseException {
AllUsersName allUsers = args.allUsersName.get();
try (Repository git = args.repoManager.openRepository(allUsers)) {
try (Repository git = args.repoManager.openRepository(args.allUsersName)) {
VersionedAccountDestinations d =
VersionedAccountDestinations.forUser(self());
d.load(git);
@@ -898,7 +895,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
}
} catch (RepositoryNotFoundException e) {
throw new QueryParseException("Unknown named destination (no " +
allUsers.get() +" repo): " + name, e);
args.allUsersName + " repo): " + name, e);
} catch (IOException | ConfigInvalidException e) {
throw new QueryParseException("Error parsing named destination: " + name, e);
}

6
gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
View File

@@ -37,6 +37,7 @@ import com.google.gerrit.server.account.CapabilityControl;
import com.google.gerrit.server.account.FakeRealm;
import com.google.gerrit.server.account.GroupBackend;
import com.google.gerrit.server.account.Realm;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.AllUsersNameProvider;
import com.google.gerrit.server.config.AnonymousCowardName;
import com.google.gerrit.server.config.AnonymousCowardNameProvider;
@@ -97,7 +98,7 @@ public class AbstractChangeNotesTest extends GerritBaseTests {
private Injector injector;
private String systemTimeZone;
@Inject private AllUsersNameProvider allUsers;
@Inject private AllUsersName allUsers;
@Before
public void setUp() throws Exception {
@@ -125,6 +126,7 @@ public class AbstractChangeNotesTest extends GerritBaseTests {
@Override
public void configure() {
install(new GitModule());
bind(AllUsersName.class).toProvider(AllUsersNameProvider.class);
bind(NotesMigration.class).toInstance(MIGRATION);
bind(GitRepositoryManager.class).toInstance(repoManager);
bind(ProjectCache.class).toProvider(Providers.<ProjectCache> of(null));
@@ -151,7 +153,7 @@ public class AbstractChangeNotesTest extends GerritBaseTests {
});
injector.injectMembers(this);
repoManager.createRepository(allUsers.get());
repoManager.createRepository(allUsers);
changeOwner = userFactory.create(co.getId());
otherUser = userFactory.create(ou.getId());
otherUserId = otherUser.getAccountId();

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

@@ -29,7 +29,6 @@ import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.AllUsersNameProvider;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.ChangeDraftUpdate;
import com.google.gerrit.server.notedb.ChangeNotes;
@@ -89,7 +88,7 @@ public class TestChanges {
public static ChangeUpdate newUpdate(Injector injector,
GitRepositoryManager repoManager, NotesMigration migration, Change c,
final AllUsersNameProvider allUsers, final IdentifiedUser user)
final AllUsersName allUsers, final IdentifiedUser user)
throws Exception {
ChangeUpdate update = injector.createChildInjector(new FactoryModule() {
@Override
@@ -97,7 +96,6 @@ public class TestChanges {
factory(ChangeUpdate.Factory.class);
factory(ChangeDraftUpdate.Factory.class);
bind(IdentifiedUser.class).toInstance(user);
bind(AllUsersName.class).toProvider(allUsers);
}
}).getInstance(ChangeUpdate.Factory.class).create(
stubChangeControl(repoManager, migration, c, allUsers, user),
@@ -131,9 +129,9 @@ public class TestChanges {
}
}
public static ChangeControl stubChangeControl(
private static ChangeControl stubChangeControl(
GitRepositoryManager repoManager, NotesMigration migration,
Change c, AllUsersNameProvider allUsers,
Change c, AllUsersName allUsers,
IdentifiedUser user) throws OrmException {
ChangeControl ctl = EasyMock.createMock(ChangeControl.class);
expect(ctl.getChange()).andStubReturn(c);