opendev/gerrit
Code Issues Proposed changes

Require getting ChangeJson through a factory

Browse Source 
The ChangeJson object is not thread safe.  It internally has a mutable
options array and tracks information about accounts in a local cache.

RestViews keep reusing this object in @Singleton views in a way that
is not thread safe.

Change the API to require obtaining the ChangeJson from a Factory
after passing in the Set<ListChangeOptions> the caller wants to use.

Change-Id: I2167b909fd52c10565fc232aefb2f1d27c65c8de
This commit is contained in:
Shawn Pearce
2015-07-17 20:50:04 -07:00
parent 2efa6030fd
commit ba24004f70
15 changed files with 87 additions and 75 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -81,7 +81,7 @@ class ChangeApiImpl implements ChangeApi {
private final GetTopic getTopic;
private final PutTopic putTopic;
private final PostReviewers postReviewers;
private final Provider<ChangeJson> changeJson;
private final ChangeJson.Factory changeJson;
private final PostHashtags postHashtags;
private final GetHashtags getHashtags;
private final ListChangeComments listComments;
@@ -102,7 +102,7 @@ class ChangeApiImpl implements ChangeApi {
GetTopic getTopic,
PutTopic putTopic,
PostReviewers postReviewers,
Provider<ChangeJson> changeJson,
ChangeJson.Factory changeJson,
PostHashtags postHashtags,
GetHashtags getHashtags,
ListChangeComments listComments,
@@ -276,7 +276,7 @@ class ChangeApiImpl implements ChangeApi {
if (u.isIdentifiedUser()) {
((IdentifiedUser) u).clearStarredChanges();
}
return changeJson.get().addOptions(s).format(change);
return changeJson.create(s).format(change);
} catch (OrmException e) {
throw new RestApiException("Cannot retrieve change", e);
}

7
gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java
View File

@@ -53,7 +53,7 @@ public class Abandon implements RestModifyView<ChangeResource, AbandonInput>,
private final ChangeHooks hooks;
private final AbandonedSender.Factory abandonedSenderFactory;
private final Provider<ReviewDb> dbProvider;
private final ChangeJson json;
private final ChangeJson.Factory json;
private final ChangeIndexer indexer;
private final ChangeUpdate.Factory updateFactory;
private final ChangeMessagesUtil cmUtil;
@@ -62,7 +62,7 @@ public class Abandon implements RestModifyView<ChangeResource, AbandonInput>,
Abandon(ChangeHooks hooks,
AbandonedSender.Factory abandonedSenderFactory,
Provider<ReviewDb> dbProvider,
ChangeJson json,
ChangeJson.Factory json,
ChangeIndexer indexer,
ChangeUpdate.Factory updateFactory,
ChangeMessagesUtil cmUtil) {
@@ -90,8 +90,7 @@ public class Abandon implements RestModifyView<ChangeResource, AbandonInput>,
throw new ResourceConflictException("draft changes cannot be abandoned");
}
change = abandon(control, input.message, caller.getAccount());
ChangeInfo result = json.format(change);
return result;
return json.create(ChangeJson.NO_OPTIONS).format(change);
}
public Change abandon(ChangeControl control,

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

@@ -98,8 +98,9 @@ import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.ChangeData.ChangedLines;
import com.google.gerrit.server.query.change.QueryResult;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
@@ -122,6 +123,12 @@ import java.util.TreeMap;
public class ChangeJson {
private static final Logger log = LoggerFactory.getLogger(ChangeJson.class);
public static final Set<ListChangesOption> NO_OPTIONS =
Collections.emptySet();
public interface Factory {
ChangeJson create(Set<ListChangesOption> options);
}
private final Provider<ReviewDb> db;
private final LabelNormalizer labelNormalizer;
@@ -145,7 +152,7 @@ public class ChangeJson {
private AccountLoader accountLoader;
private FixInput fix;
@Inject
@AssistedInject
ChangeJson(
Provider<ReviewDb> db,
LabelNormalizer ln,
@@ -163,7 +170,8 @@ public class ChangeJson {
WebLinks webLinks,
ChangeMessagesUtil cmUtil,
Provider<ConsistencyChecker> checkerProvider,
ActionJson actionJson) {
ActionJson actionJson,
@Assisted Set<ListChangesOption> options) {
this.db = db;
this.labelNormalizer = ln;
this.userProvider = user;
@@ -181,17 +189,9 @@ public class ChangeJson {
this.cmUtil = cmUtil;
this.checkerProvider = checkerProvider;
this.actionJson = actionJson;
options = EnumSet.noneOf(ListChangesOption.class);
}
public ChangeJson addOption(ListChangesOption o) {
options.add(o);
return this;
}
public ChangeJson addOptions(Collection<ListChangesOption> o) {
options.addAll(o);
return this;
this.options = options.isEmpty()
? EnumSet.noneOf(ListChangesOption.class)
: EnumSet.copyOf(options);
}
public ChangeJson fix(FixInput fix) {

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

@@ -25,19 +25,20 @@ import com.google.gerrit.server.project.ChangeControl;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import java.util.EnumSet;
public class Check implements RestReadView<ChangeResource>,
RestModifyView<ChangeResource, FixInput> {
private final ChangeJson json;
private final ChangeJson.Factory jsonFactory;
@Inject
Check(ChangeJson json) {
this.json = json;
json.addOption(ListChangesOption.CHECK);
Check(ChangeJson.Factory json) {
this.jsonFactory = json;
}
@Override
public Response<ChangeInfo> apply(ChangeResource rsrc) throws OrmException {
return Response.withMustRevalidate(json.format(rsrc));
return Response.withMustRevalidate(newChangeJson().format(rsrc));
}
@Override
@@ -49,6 +50,10 @@ public class Check implements RestReadView<ChangeResource>,
&& !ctl.getCurrentUser().getCapabilities().canMaintainServer()) {
throw new AuthException("Cannot fix change");
}
return Response.withMustRevalidate(json.fix(input).format(rsrc));
return Response.withMustRevalidate(newChangeJson().fix(input).format(rsrc));
}
private ChangeJson newChangeJson() {
return jsonFactory.create(EnumSet.of(ListChangesOption.CHECK));
}
}

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

@@ -43,12 +43,12 @@ public class CherryPick implements RestModifyView<RevisionResource, CherryPickIn
UiAction<RevisionResource> {
private final Provider<ReviewDb> dbProvider;
private final CherryPickChange cherryPickChange;
private final ChangeJson json;
private final ChangeJson.Factory json;
@Inject
CherryPick(Provider<ReviewDb> dbProvider,
CherryPickChange cherryPickChange,
ChangeJson json) {
ChangeJson.Factory json) {
this.dbProvider = dbProvider;
this.cherryPickChange = cherryPickChange;
this.json = json;
@@ -84,7 +84,7 @@ public class CherryPick implements RestModifyView<RevisionResource, CherryPickIn
cherryPickChange.cherryPick(revision.getChange(),
revision.getPatchSet(), input.message, refName,
refControl);
return json.format(cherryPickedChangeId);
return json.create(ChangeJson.NO_OPTIONS).format(cherryPickedChangeId);
} catch (InvalidChangeOperationException e) {
throw new BadRequestException(e.getMessage());
} catch (MergeException e) {

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

@@ -85,7 +85,7 @@ public class CreateChange implements
private final ProjectsCollection projectsCollection;
private final CommitValidators.Factory commitValidatorsFactory;
private final ChangeInserter.Factory changeInserterFactory;
private final ChangeJson json;
private final ChangeJson.Factory jsonFactory;
private final ChangeUtil changeUtil;
private final boolean allowDrafts;
@@ -97,7 +97,7 @@ public class CreateChange implements
ProjectsCollection projectsCollection,
CommitValidators.Factory commitValidatorsFactory,
ChangeInserter.Factory changeInserterFactory,
ChangeJson json,
ChangeJson.Factory json,
ChangeUtil changeUtil,
@GerritServerConfig Config config) {
this.db = db;
@@ -107,7 +107,7 @@ public class CreateChange implements
this.projectsCollection = projectsCollection;
this.commitValidatorsFactory = commitValidatorsFactory;
this.changeInserterFactory = changeInserterFactory;
this.json = json;
this.jsonFactory = json;
this.changeUtil = changeUtil;
this.allowDrafts = config.getBoolean("change", "allowDrafts", true);
}
@@ -219,6 +219,7 @@ public class CreateChange implements
ins.setGroups(groups);
ins.insert();
ChangeJson json = jsonFactory.create(ChangeJson.NO_OPTIONS);
return Response.created(json.format(change.getId()));
}
}

16
gerrit-server/src/main/java/com/google/gerrit/server/change/GetChange.java
View File

@@ -23,30 +23,34 @@ import com.google.inject.Inject;
import org.kohsuke.args4j.Option;
import java.util.EnumSet;
public class GetChange implements RestReadView<ChangeResource> {
private final ChangeJson json;
private final ChangeJson.Factory json;
private final EnumSet<ListChangesOption> options =
EnumSet.noneOf(ListChangesOption.class);
@Option(name = "-o", usage = "Output options")
void addOption(ListChangesOption o) {
json.addOption(o);
options.add(o);
}
@Option(name = "-O", usage = "Output option flags, in hex")
void setOptionFlagsHex(String hex) {
json.addOptions(ListChangesOption.fromBits(Integer.parseInt(hex, 16)));
options.addAll(ListChangesOption.fromBits(Integer.parseInt(hex, 16)));
}
@Inject
GetChange(ChangeJson json) {
GetChange(ChangeJson.Factory json) {
this.json = json;
}
@Override
public Response<ChangeInfo> apply(ChangeResource rsrc) throws OrmException {
return Response.withMustRevalidate(json.format(rsrc));
return Response.withMustRevalidate(json.create(options).format(rsrc));
}
Response<ChangeInfo> apply(RevisionResource rsrc) throws OrmException {
return Response.withMustRevalidate(json.format(rsrc));
return Response.withMustRevalidate(json.create(options).format(rsrc));
}
}

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

@@ -33,14 +33,13 @@ import java.util.concurrent.TimeUnit;
public class GetCommit implements RestReadView<RevisionResource> {
private final GitRepositoryManager repoManager;
private final ChangeJson json;
private final ChangeJson.Factory json;
@Option(name = "--links", usage = "Add weblinks")
private boolean addLinks;
@Inject
GetCommit(GitRepositoryManager repoManager,
ChangeJson json) {
GetCommit(GitRepositoryManager repoManager, ChangeJson.Factory json) {
this.repoManager = repoManager;
this.json = json;
}
@@ -53,8 +52,9 @@ public class GetCommit implements RestReadView<RevisionResource> {
String rev = rsrc.getPatchSet().getRevision().get();
RevCommit commit = rw.parseCommit(ObjectId.fromString(rev));
rw.parseBody(commit);
Response<CommitInfo> r = Response.ok(
json.toCommit(rsrc.getControl(), rw, commit, addLinks));
CommitInfo info = json.create(ChangeJson.NO_OPTIONS)
.toCommit(rsrc.getControl(), rw, commit, addLinks);
Response<CommitInfo> r = Response.ok(info);
if (rsrc.isCacheable()) {
r.caching(CacheControl.PRIVATE(7, TimeUnit.DAYS));
}

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

@@ -47,28 +47,29 @@ import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.io.IOException;
import java.util.EnumSet;
@Singleton
public class Rebase implements RestModifyView<RevisionResource, RebaseInput>,
UiAction<RevisionResource> {
private static final Logger log = LoggerFactory.getLogger(Rebase.class);
private static final EnumSet<ListChangesOption> OPTIONS = EnumSet.of(
ListChangesOption.CURRENT_REVISION,
ListChangesOption.CURRENT_COMMIT);
private final GitRepositoryManager repoManager;
private final Provider<RebaseChange> rebaseChange;
private final ChangeJson json;
private final ChangeJson.Factory json;
private final Provider<ReviewDb> dbProvider;
@Inject
public Rebase(GitRepositoryManager repoManager,
Provider<RebaseChange> rebaseChange,
ChangeJson json,
ChangeJson.Factory json,
Provider<ReviewDb> dbProvider) {
this.repoManager = repoManager;
this.rebaseChange = rebaseChange;
this.json = json
.addOption(ListChangesOption.CURRENT_REVISION)
.addOption(ListChangesOption.CURRENT_COMMIT);
this.json = json;
this.dbProvider = dbProvider;
}
@@ -97,7 +98,7 @@ public class Rebase implements RestModifyView<RevisionResource, RebaseInput>,
throw new ResourceNotFoundException(change.getId().toString());
}
return json.format(change.getId());
return json.create(OPTIONS).format(change.getId());
}
private String findBaseRev(RevWalk rw, RevisionResource rsrc,

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

@@ -53,7 +53,7 @@ public class Restore implements RestModifyView<ChangeResource, RestoreInput>,
private final ChangeHooks hooks;
private final RestoredSender.Factory restoredSenderFactory;
private final Provider<ReviewDb> dbProvider;
private final ChangeJson json;
private final ChangeJson.Factory json;
private final ChangeIndexer indexer;
private final ChangeMessagesUtil cmUtil;
private final ChangeUpdate.Factory updateFactory;
@@ -62,7 +62,7 @@ public class Restore implements RestModifyView<ChangeResource, RestoreInput>,
Restore(ChangeHooks hooks,
RestoredSender.Factory restoredSenderFactory,
Provider<ReviewDb> dbProvider,
ChangeJson json,
ChangeJson.Factory json,
ChangeIndexer indexer,
ChangeMessagesUtil cmUtil,
ChangeUpdate.Factory updateFactory) {
@@ -136,8 +136,7 @@ public class Restore implements RestModifyView<ChangeResource, RestoreInput>,
db.patchSets().get(change.currentPatchSetId()),
Strings.emptyToNull(input.message),
dbProvider.get());
ChangeInfo result = json.format(change);
return result;
return json.create(ChangeJson.NO_OPTIONS).format(change);
}
@Override

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

@@ -44,12 +44,12 @@ import java.io.IOException;
@Singleton
public class Revert implements RestModifyView<ChangeResource, RevertInput>,
UiAction<ChangeResource> {
private final ChangeJson json;
private final ChangeJson.Factory json;
private final ChangeUtil changeUtil;
private final PersonIdent myIdent;
@Inject
Revert(ChangeJson json,
Revert(ChangeJson.Factory json,
ChangeUtil changeUtil,
@GerritPersonIdent PersonIdent myIdent) {
this.json = json;
@@ -69,18 +69,19 @@ public class Revert implements RestModifyView<ChangeResource, RevertInput>,
throw new ResourceConflictException("change is " + status(change));
}
Change.Id revertedChangeId;
try {
Change.Id revertedChangeId =
changeUtil.revert(control, change.currentPatchSetId(),
Strings.emptyToNull(input.message),
new PersonIdent(myIdent, TimeUtil.nowTs()), new NoSshInfo());
return json.format(revertedChangeId);
revertedChangeId = changeUtil.revert(control,
change.currentPatchSetId(),
Strings.emptyToNull(input.message),
new PersonIdent(myIdent, TimeUtil.nowTs()),
new NoSshInfo());
} catch (InvalidChangeOperationException e) {
throw new BadRequestException(e.getMessage());
} catch (NoSuchChangeException e) {
throw new ResourceNotFoundException(e.getMessage());
}
return json.create(ChangeJson.NO_OPTIONS).format(revertedChangeId);
}
@Override

7
gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
View File

@@ -472,12 +472,12 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
RestModifyView<ChangeResource, SubmitInput> {
private final Provider<ReviewDb> dbProvider;
private final Submit submit;
private final ChangeJson json;
private final ChangeJson.Factory json;
@Inject
CurrentRevision(Provider<ReviewDb> dbProvider,
Submit submit,
ChangeJson json) {
ChangeJson.Factory json) {
this.dbProvider = dbProvider;
this.submit = submit;
this.json = json;
@@ -495,8 +495,9 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
} else if (!rsrc.getControl().isPatchVisible(ps, dbProvider.get())) {
throw new AuthException("current revision not accessible");
}
Output out = submit.apply(new RevisionResource(rsrc, ps), input);
return json.format(out.change);
return json.create(ChangeJson.NO_OPTIONS).format(out.change);
}
}
}

11
gerrit-server/src/main/java/com/google/gerrit/server/change/SubmittedTogether.java
View File

@@ -40,13 +40,12 @@ public class SubmittedTogether implements RestReadView<ChangeResource> {
private static final Logger log = LoggerFactory.getLogger(
SubmittedTogether.class);
private final ChangeJson json;
private final ChangeJson.Factory json;
private final Provider<ReviewDb> dbProvider;
private final MergeSuperSet mergeSuperSet;
@Inject
SubmittedTogether(ChangeJson json,
SubmittedTogether(ChangeJson.Factory json,
Provider<ReviewDb> dbProvider,
MergeSuperSet mergeSuperSet) {
this.json = json;
@@ -61,12 +60,12 @@ public class SubmittedTogether implements RestReadView<ChangeResource> {
try {
ChangeSet cs = mergeSuperSet.completeChangeSet(dbProvider.get(),
ChangeSet.create(resource.getChange()));
json.addOptions(EnumSet.of(
return json.create(EnumSet.of(
ListChangesOption.CURRENT_REVISION,
ListChangesOption.CURRENT_COMMIT,
ListChangesOption.DETAILED_LABELS,
ListChangesOption.LABELS));
return json.format(cs.ids());
ListChangesOption.LABELS))
.format(cs.ids());
} catch (OrmException | IOException e) {
log.error("Error on getting a ChangeSet", e);
throw e;

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

@@ -69,6 +69,7 @@ import com.google.gerrit.server.auth.AuthBackend;
import com.google.gerrit.server.auth.UniversalAuthBackend;
import com.google.gerrit.server.avatar.AvatarProvider;
import com.google.gerrit.server.cache.CacheRemovalListener;
import com.google.gerrit.server.change.ChangeJson;
import com.google.gerrit.server.change.ChangeKindCacheImpl;
import com.google.gerrit.server.change.MergeabilityCacheImpl;
import com.google.gerrit.server.events.EventFactory;
@@ -186,6 +187,7 @@ public class GerritGlobalModule extends FactoryModule {
factory(AddReviewerSender.Factory.class);
factory(CapabilityControl.Factory.class);
factory(ChangeData.Factory.class);
factory(ChangeJson.Factory.class);
factory(CreateChangeSender.Factory.class);
factory(GroupDetailFactory.Factory.class);
factory(GroupInfoCacheFactory.Factory.class);

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

@@ -38,7 +38,7 @@ import java.util.regex.Matcher;
import java.util.regex.Pattern;
public class QueryChanges implements RestReadView<TopLevelResource> {
private final ChangeJson json;
private final ChangeJson.Factory json;
private final ChangeQueryBuilder qb;
private final QueryProcessor imp;
private final Provider<CurrentUser> user;
@@ -68,7 +68,7 @@ public class QueryChanges implements RestReadView<TopLevelResource> {
}
@Inject
QueryChanges(ChangeJson json,
QueryChanges(ChangeJson.Factory json,
ChangeQueryBuilder qb,
QueryProcessor qp,
Provider<CurrentUser> user) {
@@ -141,7 +141,7 @@ public class QueryChanges implements RestReadView<TopLevelResource> {
QueryParseException {
int cnt = queries.size();
List<QueryResult> results = imp.queryChanges(qb.parse(queries));
List<List<ChangeInfo>> res = json.addOptions(options)
List<List<ChangeInfo>> res = json.create(options)
.formatQueryResults(results);
for (int n = 0; n < cnt; n++) {
List<ChangeInfo> info = res.get(n);