opendev/gerrit
Code Issues Proposed changes

Merge "Remove ChangeControl from ChangeData"

Browse Source
This commit is contained in:
Patrick Hiesel
2017-09-25 12:28:47 +00:00
committed by Gerrit Code Review
parent cd06ef043a 72caf8f0f7
commit ab7b0a8f0c
20 changed files with 180 additions and 123 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -119,6 +119,7 @@ import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.permissions.LabelPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.RemoveReviewerControl;
@@ -219,6 +220,7 @@ public class ChangeJson {
private final ApprovalsUtil approvalsUtil;
private final RemoveReviewerControl removeReviewerControl;
private final TrackingFooters trackingFooters;
private final ChangeControl.GenericFactory changeControlFactory;
private boolean lazyLoad = true;
private AccountLoader accountLoader;
private FixInput fix;
@@ -251,6 +253,7 @@ public class ChangeJson {
ApprovalsUtil approvalsUtil,
RemoveReviewerControl removeReviewerControl,
TrackingFooters trackingFooters,
ChangeControl.GenericFactory changeControlFactory,
@Assisted Iterable<ListChangesOption> options) {
this.db = db;
this.userProvider = user;
@@ -276,6 +279,7 @@ public class ChangeJson {
this.indexes = indexes;
this.approvalsUtil = approvalsUtil;
this.removeReviewerControl = removeReviewerControl;
this.changeControlFactory = changeControlFactory;
this.options = Sets.immutableEnumSet(options);
this.trackingFooters = trackingFooters;
}
@@ -345,7 +349,7 @@ public class ChangeJson {
}
public ChangeInfo format(RevisionResource rsrc) throws OrmException {
ChangeData cd = changeDataFactory.create(db.get(), rsrc.getChangeResource());
ChangeData cd = changeDataFactory.create(db.get(), rsrc.getNotes());
return format(cd, Optional.of(rsrc.getPatchSet().getId()), true);
}
@@ -494,7 +498,11 @@ public class ChangeJson {
}
}
PermissionBackend.ForChange perm = permissionBackend.user(user).database(db).change(cd);
PermissionBackend.WithUser withUser = permissionBackend.user(user).database(db);
PermissionBackend.ForChange perm =
lazyLoad
? withUser.change(cd)
: withUser.indexedChange(cd, notesFactory.createFromIndexedChange(cd.change()));
Change in = cd.change();
out.project = in.getProject().get();
out.branch = in.getDest().getShortName();
@@ -588,15 +596,20 @@ public class ChangeJson {
} else {
src = null;
}
ChangeControl ctl = null;
if (needMessages || needRevisions) {
ctl = changeControlFactory.controlFor(db.get(), cd.change(), userProvider.get());
}
if (needMessages) {
out.messages = messages(cd, src);
out.messages = messages(ctl, cd, src);
}
finish(out);
// This block must come after the ChangeInfo is mostly populated, since
// it will be passed to ActionVisitors as-is.
if (needRevisions) {
out.revisions = revisions(cd, src, limitToPsId, out);
out.revisions = revisions(ctl, cd, src, limitToPsId, out);
if (out.revisions != null) {
for (Map.Entry<String, RevisionInfo> entry : out.revisions.entrySet()) {
if (entry.getValue().isCurrent) {
@@ -653,11 +666,11 @@ public class ChangeJson {
return result;
}
private boolean submittable(ChangeData cd) {
private boolean submittable(ChangeData cd) throws OrmException {
return SubmitRecord.findOkRecord(cd.submitRecords(SUBMIT_RULE_OPTIONS_STRICT)).isPresent();
}
private List<SubmitRecord> submitRecords(ChangeData cd) {
private List<SubmitRecord> submitRecords(ChangeData cd) throws OrmException {
return cd.submitRecords(SUBMIT_RULE_OPTIONS_LENIENT);
}
@@ -709,7 +722,7 @@ public class ChangeJson {
}
private Map<String, LabelWithStatus> initLabels(
ChangeData cd, LabelTypes labelTypes, boolean standard) {
ChangeData cd, LabelTypes labelTypes, boolean standard) throws OrmException {
Map<String, LabelWithStatus> labels = new TreeMap<>(labelTypes.nameComparator());
for (SubmitRecord rec : submitRecords(cd)) {
if (rec.labels == null) {
@@ -1091,8 +1104,8 @@ public class ChangeJson {
return result;
}
private Collection<ChangeMessageInfo> messages(ChangeData cd, Map<PatchSet.Id, PatchSet> map)
throws OrmException {
private Collection<ChangeMessageInfo> messages(
ChangeControl ctl, ChangeData cd, Map<PatchSet.Id, PatchSet> map) throws OrmException {
List<ChangeMessage> messages = cmUtil.byChange(db.get(), cd.notes());
if (messages.isEmpty()) {
return Collections.emptyList();
@@ -1102,7 +1115,7 @@ public class ChangeJson {
for (ChangeMessage message : messages) {
PatchSet.Id patchNum = message.getPatchSetId();
PatchSet ps = patchNum != null ? map.get(patchNum) : null;
if (patchNum == null || cd.changeControl().isPatchVisible(ps, db.get())) {
if (patchNum == null || ctl.isPatchVisible(ps, db.get())) {
ChangeMessageInfo cmi = new ChangeMessageInfo();
cmi.id = message.getKey().get();
cmi.author = accountLoader.get(message.getAuthor());
@@ -1217,6 +1230,7 @@ public class ChangeJson {
}
private Map<String, RevisionInfo> revisions(
ChangeControl ctl,
ChangeData cd,
Map<PatchSet.Id, PatchSet> map,
Optional<PatchSet.Id> limitToPsId,
@@ -1235,7 +1249,7 @@ public class ChangeJson {
} else {
want = id.equals(cd.change().currentPatchSetId());
}
if (want && cd.changeControl().isPatchVisible(in, db.get())) {
if (want && ctl.isPatchVisible(in, db.get())) {
res.put(in.getRevision().get(), toRevisionInfo(cd, in, repo, rw, false, changeInfo));
}
}
@@ -1392,6 +1406,7 @@ public class ChangeJson {
private Map<String, FetchInfo> makeFetchMap(ChangeData cd, PatchSet in) throws OrmException {
Map<String, FetchInfo> r = new LinkedHashMap<>();
ChangeControl ctl = changeControlFactory.controlFor(db.get(), cd.change(), anonymous);
for (DynamicMap.Entry<DownloadScheme> e : downloadSchemes) {
String schemeName = e.getExportName();
DownloadScheme scheme = e.getProvider().get();
@@ -1400,8 +1415,7 @@ public class ChangeJson {
continue;
}
if (!scheme.isAuthSupported()
&& !cd.changeControl().forUser(anonymous).isPatchVisible(in, db.get())) {
if (!scheme.isAuthSupported() && !ctl.isPatchVisible(in, db.get())) {
continue;
}

8
gerrit-server/src/main/java/com/google/gerrit/server/change/GetRelated.java
View File

@@ -27,6 +27,7 @@ import com.google.gerrit.server.CommonConverters;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.change.RelatedChangesSorter.PatchSetData;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gwtorm.server.OrmException;
@@ -63,13 +64,14 @@ public class GetRelated implements RestReadView<RevisionResource> {
@Override
public RelatedInfo apply(RevisionResource rsrc)
throws RepositoryNotFoundException, IOException, OrmException {
throws RepositoryNotFoundException, IOException, OrmException, NoSuchProjectException {
RelatedInfo relatedInfo = new RelatedInfo();
relatedInfo.changes = getRelated(rsrc);
return relatedInfo;
}
private List<ChangeAndCommit> getRelated(RevisionResource rsrc) throws OrmException, IOException {
private List<ChangeAndCommit> getRelated(RevisionResource rsrc)
throws OrmException, IOException, NoSuchProjectException {
Set<String> groups = getAllGroups(rsrc.getNotes());
if (groups.isEmpty()) {
return Collections.emptyList();
@@ -93,7 +95,7 @@ public class GetRelated implements RestReadView<RevisionResource> {
reloadChangeIfStale(cds, basePs);
for (PatchSetData d : sorter.sort(cds, basePs)) {
for (PatchSetData d : sorter.sort(cds, basePs, rsrc.getUser())) {
PatchSet ps = d.patchSet();
RevCommit commit;
if (isEdit && ps.getId().equals(basePs.getId())) {

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

@@ -26,6 +26,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.reviewdb.server.ReviewDbUtil;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.git.BranchOrderSection;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.MergeUtil;
@@ -108,8 +109,8 @@ public class Mergeable implements RestReadView<RevisionResource> {
return result;
}
ChangeData cd = changeDataFactory.create(db.get(), resource.getChangeResource());
result.submitType = getSubmitType(cd, ps);
ChangeData cd = changeDataFactory.create(db.get(), resource.getNotes());
result.submitType = getSubmitType(resource.getUser(), cd, ps);
try (Repository git = gitManager.openRepository(change.getProject())) {
ObjectId commit = toId(ps);
@@ -141,9 +142,10 @@ public class Mergeable implements RestReadView<RevisionResource> {
return result;
}
private SubmitType getSubmitType(ChangeData cd, PatchSet patchSet) throws OrmException {
private SubmitType getSubmitType(CurrentUser user, ChangeData cd, PatchSet patchSet)
throws OrmException {
SubmitTypeRecord rec =
submitRuleEvaluatorFactory.create(cd).setPatchSet(patchSet).getSubmitType();
submitRuleEvaluatorFactory.create(user, cd).setPatchSet(patchSet).getSubmitType();
if (rec.status != SubmitTypeRecord.Status.OK) {
throw new OrmException("Submit type rule failed: " + rec);
}

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

@@ -570,7 +570,7 @@ public class PostReview
}
private Set<String> getAffectedFilePaths(RevisionResource revision) throws OrmException {
ChangeData changeData = changeDataFactory.create(db.get(), revision.getChangeResource());
ChangeData changeData = changeDataFactory.create(db.get(), revision.getNotes());
return new HashSet<>(changeData.filePaths(revision.getPatchSet()));
}
@@ -1107,7 +1107,7 @@ public class PostReview
if (ctx.getAccountId().equals(ctx.getChange().getOwner())) {
return true;
}
ChangeData cd = changeDataFactory.create(db.get(), ctx);
ChangeData cd = changeDataFactory.create(db.get(), ctx.getNotes());
ReviewerSet reviewers = cd.reviewers();
if (reviewers.byState(REVIEWER).contains(ctx.getAccountId())) {
return true;

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

@@ -446,7 +446,7 @@ public class PostReviewers
result.ccs = Lists.newArrayListWithCapacity(opResult.addedCCs().size());
for (Account.Id accountId : opResult.addedCCs()) {
IdentifiedUser u = identifiedUserFactory.create(accountId);
result.ccs.add(json.format(new ReviewerInfo(accountId.get()), perm.user(u), cd));
result.ccs.add(json.format(caller, new ReviewerInfo(accountId.get()), perm.user(u), cd));
}
accountLoaderFactory.create(true).fill(result.ccs);
for (Address a : reviewersByEmail) {
@@ -459,6 +459,7 @@ public class PostReviewers
IdentifiedUser u = identifiedUserFactory.create(psa.getAccountId());
result.reviewers.add(
json.format(
caller,
new ReviewerInfo(psa.getAccountId().get()),
perm.user(u),
cd,

13
gerrit-server/src/main/java/com/google/gerrit/server/change/RelatedChangesSorter.java
View File

@@ -27,7 +27,9 @@ import com.google.common.collect.MultimapBuilder;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
@@ -53,20 +55,23 @@ import org.eclipse.jgit.revwalk.RevWalk;
@Singleton
class RelatedChangesSorter {
private final GitRepositoryManager repoManager;
private final ProjectControl.GenericFactory projectControlFactory;
@Inject
RelatedChangesSorter(GitRepositoryManager repoManager) {
RelatedChangesSorter(
GitRepositoryManager repoManager, ProjectControl.GenericFactory projectControlFactory) {
this.repoManager = repoManager;
this.projectControlFactory = projectControlFactory;
}
public List<PatchSetData> sort(List<ChangeData> in, PatchSet startPs)
throws OrmException, IOException {
public List<PatchSetData> sort(List<ChangeData> in, PatchSet startPs, CurrentUser user)
throws OrmException, IOException, NoSuchProjectException {
checkArgument(!in.isEmpty(), "Input may not be empty");
// Map of all patch sets, keyed by commit SHA-1.
Map<String, PatchSetData> byId = collectById(in);
PatchSetData start = byId.get(startPs.getRevision().get());
checkArgument(start != null, "%s not found in %s", startPs, in);
ProjectControl ctl = start.data().changeControl().getProjectControl();
ProjectControl ctl = projectControlFactory.controlFor(start.data().project(), user);
// Map of patch set -> immediate parent.
ListMultimap<PatchSetData, PatchSetData> parents =

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

@@ -27,6 +27,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.AccountLoader;
import com.google.gerrit.server.permissions.LabelPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
@@ -77,6 +78,7 @@ public class ReviewerJson {
}
ReviewerInfo info =
format(
rsrc.getChangeResource().getUser(),
new ReviewerInfo(rsrc.getReviewerUser().getAccountId().get()),
permissionBackend.user(rsrc.getReviewerUser()).database(db).change(cd),
cd);
@@ -92,10 +94,12 @@ public class ReviewerJson {
return format(ImmutableList.<ReviewerResource>of(rsrc));
}
public ReviewerInfo format(ReviewerInfo out, PermissionBackend.ForChange perm, ChangeData cd)
public ReviewerInfo format(
CurrentUser user, ReviewerInfo out, PermissionBackend.ForChange perm, ChangeData cd)
throws OrmException, PermissionBackendException {
PatchSet.Id psId = cd.change().currentPatchSetId();
return format(
user,
out,
perm,
cd,
@@ -104,6 +108,7 @@ public class ReviewerJson {
}
public ReviewerInfo format(
CurrentUser user,
ReviewerInfo out,
PermissionBackend.ForChange perm,
ChangeData cd,
@@ -125,7 +130,7 @@ public class ReviewerJson {
if (ps != null) {
for (SubmitRecord rec :
submitRuleEvaluatorFactory
.create(cd)
.create(user, cd)
.setFastEvalLabels(true)
.setAllowDraft(true)
.evaluate()) {

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

@@ -322,12 +322,7 @@ public class Submit
}
ReviewDb db = dbProvider.get();
ChangeData cd;
try {
cd = changeDataFactory.create(db, resource.getChangeResource());
} catch (NoSuchChangeException e) {
return null; // submit not visible
}
ChangeData cd = changeDataFactory.create(db, resource.getNotes());
try {
MergeOp.checkSubmitRule(cd, false);
} catch (ResourceConflictException e) {

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

@@ -71,7 +71,7 @@ public class TestSubmitRule implements RestModifyView<RevisionResource, TestSubm
input.filters = MoreObjects.firstNonNull(input.filters, filters);
SubmitRuleEvaluator evaluator =
submitRuleEvaluatorFactory.create(
changeDataFactory.create(db.get(), rsrc.getChangeResource()));
rsrc.getUser(), changeDataFactory.create(db.get(), rsrc.getNotes()));
List<SubmitRecord> records =
evaluator

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

@@ -65,7 +65,7 @@ public class TestSubmitType implements RestModifyView<RevisionResource, TestSubm
input.filters = MoreObjects.firstNonNull(input.filters, filters);
SubmitRuleEvaluator evaluator =
submitRuleEvaluatorFactory.create(
changeDataFactory.create(db.get(), rsrc.getChangeResource()));
rsrc.getUser(), changeDataFactory.create(db.get(), rsrc.getNotes()));
SubmitTypeRecord rec =
evaluator

10
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
View File

@@ -327,7 +327,8 @@ public class MergeOp implements AutoCloseable {
return allowClosed ? SUBMIT_RULE_OPTIONS_ALLOW_CLOSED : SUBMIT_RULE_OPTIONS;
}
private static List<SubmitRecord> getSubmitRecords(ChangeData cd, boolean allowClosed) {
private static List<SubmitRecord> getSubmitRecords(ChangeData cd, boolean allowClosed)
throws OrmException {
return cd.submitRecords(submitRuleOptions(allowClosed));
}
@@ -391,7 +392,7 @@ public class MergeOp implements AutoCloseable {
commitStatus.maybeFailVerbose();
}
private void bypassSubmitRules(ChangeSet cs, boolean allowClosed) {
private void bypassSubmitRules(ChangeSet cs, boolean allowClosed) throws OrmException {
checkArgument(
!cs.furtherHiddenChanges(), "cannot bypass submit rules for topic with hidden change");
for (ChangeData cd : cs.changes()) {
@@ -705,15 +706,16 @@ public class MergeOp implements AutoCloseable {
Change.Id changeId = cd.getId();
ChangeNotes notes;
Change chg;
SubmitType st;
try {
notes = cd.notes();
chg = cd.change();
st = getSubmitType(cd);
} catch (OrmException e) {
commitStatus.logProblem(changeId, e);
continue;
}
SubmitType st = getSubmitType(cd);
if (st == null) {
commitStatus.logProblem(changeId, "No submit type for change");
continue;
@@ -828,7 +830,7 @@ public class MergeOp implements AutoCloseable {
}
}
private SubmitType getSubmitType(ChangeData cd) {
private SubmitType getSubmitType(ChangeData cd) throws OrmException {
SubmitTypeRecord str = cd.submitTypeRecord();
return str.isOk() ? str.type : null;
}

6
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeSuperSet.java
View File

@@ -160,7 +160,7 @@ public class MergeSuperSet {
}
}
private SubmitType submitType(ChangeData cd, PatchSet ps, boolean visible)
private SubmitType submitType(CurrentUser user, ChangeData cd, PatchSet ps, boolean visible)
throws OrmException, IOException {
// Submit type prolog rules mean that the submit type can depend on the
// submitting user and the content of the change.
@@ -179,7 +179,7 @@ public class MergeSuperSet {
SubmitTypeRecord str =
ps == cd.currentPatchSet()
? cd.submitTypeRecord()
: submitRuleEvaluatorFactory.create(cd).setPatchSet(ps).getSubmitType();
: submitRuleEvaluatorFactory.create(user, cd).setPatchSet(ps).getSubmitType();
if (!str.isOk()) {
logErrorAndThrow("Failed to get submit type for " + cd.getId() + ": " + str.errorMessage);
}
@@ -258,7 +258,7 @@ public class MergeSuperSet {
}
}
if (submitType(cd, ps, visiblePatchSet) == SubmitType.CHERRY_PICK) {
if (submitType(user, cd, ps, visiblePatchSet) == SubmitType.CHERRY_PICK) {
if (visible) {
visibleChanges.add(cd);
} else {

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

@@ -299,7 +299,7 @@ public class ReplaceOp implements BatchUpdateOp {
recipients.add(
getRecipientsFromFooters(ctx.getDb(), accountResolver, draft, commit.getFooterLines()));
recipients.remove(ctx.getAccountId());
ChangeData cd = changeDataFactory.create(ctx.getDb(), ctx);
ChangeData cd = changeDataFactory.create(ctx.getDb(), ctx.getNotes());
MailRecipients oldRecipients = getRecipientsFromReviewers(cd.reviewers());
Iterable<PatchSetApproval> newApprovals =
approvalsUtil.addApprovalsForNewPatchSet(

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

@@ -658,7 +658,8 @@ public class ChangeField {
return Lists.transform(records, r -> GSON.toJson(new StoredSubmitRecord(r)).getBytes(UTF_8));
}
private static Iterable<byte[]> storedSubmitRecords(ChangeData cd, SubmitRuleOptions opts) {
private static Iterable<byte[]> storedSubmitRecords(ChangeData cd, SubmitRuleOptions opts)
throws OrmException {
return storedSubmitRecords(cd.submitRecords(opts));
}

2
gerrit-server/src/main/java/com/google/gerrit/server/mail/send/MergedSender.java
View File

@@ -72,7 +72,7 @@ public class MergedSender extends ReplyToChangeSender {
args.approvalsUtil.byPatchSet(
args.db.get(),
changeData.notes(),
changeData.changeControl().getUser(),
args.identifiedUserFactory.create(changeData.change().getOwner()),
patchSet.getId(),
null,
null)) {

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

@@ -397,7 +397,7 @@ public class ChangeControl {
}
private ChangeData changeData(ReviewDb db, @Nullable ChangeData cd) {
return cd != null ? cd : changeDataFactory.create(db, this);
return cd != null ? cd : changeDataFactory.create(db, getNotes());
}
public boolean isDraftVisible(ReviewDb db, ChangeData cd) throws OrmException {
@@ -442,7 +442,7 @@ public class ChangeControl {
if (cd == null) {
ReviewDb reviewDb = db();
checkState(reviewDb != null, "need ReviewDb");
cd = changeDataFactory.create(reviewDb, ChangeControl.this);
cd = changeDataFactory.create(reviewDb, getNotes());
}
return cd;
}

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

@@ -32,6 +32,7 @@ import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import com.google.inject.util.Providers;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
@@ -517,7 +518,10 @@ public class RefControl {
@Override
public ForChange change(ChangeData cd) {
try {
return cd.changeControl().forUser(getUser()).asForChange(cd, db);
// TODO(hiesel) Force callers to call database() and use db instead of cd.db()
return getProjectControl()
.controlFor(cd.db(), cd.change())
.asForChange(cd, Providers.of(cd.db()));
} catch (OrmException e) {
return FailedPermissionBackend.change("unavailable", e);
}

81
gerrit-server/src/main/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java
View File

@@ -24,6 +24,7 @@ import com.google.gerrit.extensions.client.SubmitType;
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.server.ReviewDb;
import com.google.gerrit.rules.PrologEnvironment;
import com.google.gerrit.rules.StoredValues;
import com.google.gerrit.server.CurrentUser;
@@ -33,6 +34,7 @@ import com.google.gerrit.server.account.Emails;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.assistedinject.Assisted;
import com.googlecode.prolog_cafe.exceptions.CompileException;
import com.googlecode.prolog_cafe.exceptions.ReductionLimitException;
@@ -43,6 +45,7 @@ import com.googlecode.prolog_cafe.lang.StructureTerm;
import com.googlecode.prolog_cafe.lang.SymbolTerm;
import com.googlecode.prolog_cafe.lang.Term;
import com.googlecode.prolog_cafe.lang.VariableTerm;
import java.io.IOException;
import java.io.StringReader;
import java.util.ArrayList;
import java.util.Collections;
@@ -87,29 +90,45 @@ public class SubmitRuleEvaluator {
}
public interface Factory {
SubmitRuleEvaluator create(ChangeData cd);
SubmitRuleEvaluator create(CurrentUser user, ChangeData cd);
}
private final AccountCache accountCache;
private final Accounts accounts;
private final Emails emails;
private final ProjectCache projectCache;
private final Provider<ReviewDb> dbProvider;
private final ChangeControl.GenericFactory changeControlFactory;
private final ChangeData cd;
private SubmitRuleOptions.Builder optsBuilder = SubmitRuleOptions.defaults();
private SubmitRuleOptions opts;
private Change change;
private CurrentUser user;
private PatchSet patchSet;
private boolean logErrors = true;
private long reductionsConsumed;
private ChangeControl control;
private ProjectState projectState;
private Term submitRule;
@Inject
SubmitRuleEvaluator(
AccountCache accountCache, Accounts accounts, Emails emails, @Assisted ChangeData cd) {
AccountCache accountCache,
Accounts accounts,
Emails emails,
ProjectCache projectCache,
Provider<ReviewDb> dbProvider,
ChangeControl.GenericFactory changeControlFactory,
@Assisted CurrentUser user,
@Assisted ChangeData cd) {
this.accountCache = accountCache;
this.accounts = accounts;
this.emails = emails;
this.projectCache = projectCache;
this.dbProvider = dbProvider;
this.changeControlFactory = changeControlFactory;
this.user = user;
this.cd = cd;
}
@@ -225,19 +244,18 @@ public class SubmitRuleEvaluator {
public List<SubmitRecord> evaluate() {
initOptions();
try {
initChange();
init();
} catch (OrmException e) {
return ruleError("Error looking up change " + cd.getId(), e);
}
Change c = control.getChange();
if (!opts.allowClosed() && c.getStatus().isClosed()) {
if (!opts.allowClosed() && change.getStatus().isClosed()) {
SubmitRecord rec = new SubmitRecord();
rec.status = SubmitRecord.Status.CLOSED;
return Collections.singletonList(rec);
}
if (!opts.allowDraft()) {
if (c.getStatus() == Change.Status.DRAFT || patchSet.isDraft()) {
if (change.getStatus() == Change.Status.DRAFT || patchSet.isDraft()) {
return cannotSubmitDraft();
}
}
@@ -250,7 +268,7 @@ public class SubmitRuleEvaluator {
"can_submit",
"locate_submit_filter",
"filter_submit_results",
control.getUser());
user);
} catch (RuleEvalException e) {
return ruleError(e.getMessage(), e);
}
@@ -271,7 +289,9 @@ public class SubmitRuleEvaluator {
private List<SubmitRecord> cannotSubmitDraft() {
try {
if (!control.isDraftVisible(cd.db(), cd)) {
if (!changeControlFactory
.controlFor(dbProvider.get(), change, user)
.isDraftVisible(cd.db(), cd)) {
return createRuleError("Patch set " + patchSet.getId() + " not found");
}
if (patchSet.isDraft()) {
@@ -279,8 +299,7 @@ public class SubmitRuleEvaluator {
}
return createRuleError("Cannot submit draft changes");
} catch (OrmException err) {
PatchSet.Id psId =
patchSet != null ? patchSet.getId() : control.getChange().currentPatchSetId();
PatchSet.Id psId = patchSet != null ? patchSet.getId() : patchSet.getId();
String msg = "Cannot check visibility of patch set " + psId;
log.error(msg, err);
return createRuleError(msg);
@@ -414,17 +433,22 @@ public class SubmitRuleEvaluator {
public SubmitTypeRecord getSubmitType() {
initOptions();
try {
initChange();
init();
} catch (OrmException e) {
return typeError("Error looking up change " + cd.getId(), e);
}
try {
if (control.getChange().getStatus() == Change.Status.DRAFT
&& !control.isDraftVisible(cd.db(), cd)) {
if (change.getStatus() == Change.Status.DRAFT
&& !changeControlFactory
.controlFor(dbProvider.get(), change, user)
.isDraftVisible(cd.db(), cd)) {
return SubmitTypeRecord.error("Patch set " + patchSet.getId() + " not found");
}
if (patchSet.isDraft() && !control.isDraftVisible(cd.db(), cd)) {
if (patchSet.isDraft()
&& !changeControlFactory
.controlFor(dbProvider.get(), change, user)
.isDraftVisible(cd.db(), cd)) {
return SubmitTypeRecord.error("Patch set " + patchSet.getId() + " not found");
}
} catch (OrmException err) {
@@ -561,7 +585,6 @@ public class SubmitRuleEvaluator {
}
private PrologEnvironment getPrologEnvironment(CurrentUser user) throws RuleEvalException {
ProjectState projectState = control.getProjectControl().getProjectState();
PrologEnvironment env;
try {
if (opts.rule() == null) {
@@ -571,12 +594,10 @@ public class SubmitRuleEvaluator {
}
} catch (CompileException err) {
String msg;
if (opts.rule() == null && control.getProjectControl().isOwner()) {
if (opts.rule() == null) {
msg = String.format("Cannot load rules.pl for %s: %s", getProjectName(), err.getMessage());
} else if (opts.rule() != null) {
msg = err.getMessage();
} else {
msg = String.format("Cannot load rules.pl for %s", getProjectName());
msg = err.getMessage();
}
throw new RuleEvalException(msg, err);
}
@@ -598,7 +619,6 @@ public class SubmitRuleEvaluator {
String filterRuleLocatorName,
String filterRuleWrapperName)
throws RuleEvalException {
ProjectState projectState = control.getProjectControl().getProjectState();
PrologEnvironment childEnv = env;
for (ProjectState parentState : projectState.parents()) {
PrologEnvironment parentEnv;
@@ -684,9 +704,20 @@ public class SubmitRuleEvaluator {
}
}
private void initChange() throws OrmException {
if (control == null) {
control = cd.changeControl();
private void init() throws OrmException {
if (change == null) {
change = cd.change();
if (change == null) {
throw new OrmException("No change found");
}
}
if (projectState == null) {
try {
projectState = projectCache.checkedGet(change.getProject());
} catch (IOException e) {
throw new OrmException("Can't load project state", e);
}
}
if (patchSet == null) {
@@ -698,6 +729,6 @@ public class SubmitRuleEvaluator {
}
private String getProjectName() {
return control.getProjectControl().getProjectState().getName();
return projectState.getName();
}
}

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

@@ -58,7 +58,6 @@ import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.ReviewerStatusUpdate;
import com.google.gerrit.server.StarredChangesUtil;
import com.google.gerrit.server.StarredChangesUtil.StarRef;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.GetPureRevert;
import com.google.gerrit.server.change.MergeabilityCache;
import com.google.gerrit.server.config.AllUsersName;
@@ -76,7 +75,6 @@ import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.project.SubmitRuleEvaluator;
import com.google.gerrit.server.project.SubmitRuleOptions;
import com.google.gerrit.server.update.ChangeContext;
import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.ResultSet;
import com.google.inject.Inject;
@@ -282,44 +280,23 @@ public class ChangeData {
public static class Factory {
private final AssistedFactory assistedFactory;
private final ChangeControl.GenericFactory changeControlFactory;
@Inject
Factory(AssistedFactory assistedFactory, ChangeControl.GenericFactory changeControlFactory) {
Factory(AssistedFactory assistedFactory) {
this.assistedFactory = assistedFactory;
this.changeControlFactory = changeControlFactory;
}
public ChangeData create(ReviewDb db, Project.NameKey project, Change.Id id) {
return assistedFactory.create(db, project, id, null, null, null);
return assistedFactory.create(db, project, id, null, null);
}
public ChangeData create(ReviewDb db, Change change) {
return assistedFactory.create(db, change.getProject(), change.getId(), change, null, null);
return assistedFactory.create(db, change.getProject(), change.getId(), change, null);
}
public ChangeData create(ReviewDb db, ChangeNotes notes) {
return assistedFactory.create(
db, notes.getChange().getProject(), notes.getChangeId(), notes.getChange(), notes, null);
}
public ChangeData create(ReviewDb db, ChangeControl control) {
return assistedFactory.create(
db,
control.getChange().getProject(),
control.getId(),
control.getChange(),
control.getNotes(),
control);
}
// TODO(hiesel): Remove these after ChangeControl is removed from ChangeData
public ChangeData create(ReviewDb db, ChangeResource rsrc) throws NoSuchChangeException {
return create(db, changeControlFactory.controlFor(rsrc.getNotes(), rsrc.getUser()));
}
public ChangeData create(ReviewDb db, ChangeContext ctx) throws NoSuchChangeException {
return create(db, changeControlFactory.controlFor(ctx.getNotes(), ctx.getUser()));
db, notes.getChange().getProject(), notes.getChangeId(), notes.getChange(), notes);
}
}
@@ -329,8 +306,7 @@ public class ChangeData {
Project.NameKey project,
Change.Id id,
@Nullable Change change,
@Nullable ChangeNotes notes,
@Nullable ChangeControl control);
@Nullable ChangeNotes notes);
}
/**
@@ -347,7 +323,7 @@ public class ChangeData {
ChangeData cd =
new ChangeData(
null, null, null, null, null, null, null, null, null, null, null, null, null, null,
null, null, null, null, null, project, id, null, null, null);
null, null, null, null, null, project, id, null, null);
cd.currentPatchSet = new PatchSet(new PatchSet.Id(id, currentPatchSetId));
return cd;
}
@@ -356,7 +332,6 @@ public class ChangeData {
private @Nullable final StarredChangesUtil starredChangesUtil;
private final AllUsersName allUsersName;
private final ApprovalsUtil approvalsUtil;
private final ChangeControl.GenericFactory changeControlFactory;
private final ChangeMessagesUtil cmUtil;
private final ChangeNotes.Factory notesFactory;
private final CommentsUtil commentsUtil;
@@ -371,6 +346,7 @@ public class ChangeData {
private final TrackingFooters trackingFooters;
private final GetPureRevert pureRevert;
private final SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory;
private final ChangeControl.GenericFactory changeControlFactory;
// Required assisted injected fields.
private final ReviewDb db;
@@ -426,7 +402,6 @@ public class ChangeData {
@Nullable StarredChangesUtil starredChangesUtil,
ApprovalsUtil approvalsUtil,
AllUsersName allUsersName,
ChangeControl.GenericFactory changeControlFactory,
ChangeMessagesUtil cmUtil,
ChangeNotes.Factory notesFactory,
CommentsUtil commentsUtil,
@@ -441,15 +416,14 @@ public class ChangeData {
TrackingFooters trackingFooters,
GetPureRevert pureRevert,
SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory,
ChangeControl.GenericFactory changeControlFactory,
@Assisted ReviewDb db,
@Assisted Project.NameKey project,
@Assisted Change.Id id,
@Assisted @Nullable Change change,
@Assisted @Nullable ChangeNotes notes,
@Assisted @Nullable ChangeControl control) {
@Assisted @Nullable ChangeNotes notes) {
this.approvalsUtil = approvalsUtil;
this.allUsersName = allUsersName;
this.changeControlFactory = changeControlFactory;
this.cmUtil = cmUtil;
this.notesFactory = notesFactory;
this.commentsUtil = commentsUtil;
@@ -465,6 +439,7 @@ public class ChangeData {
this.trackingFooters = trackingFooters;
this.pureRevert = pureRevert;
this.submitRuleEvaluatorFactory = submitRuleEvaluatorFactory;
this.changeControlFactory = changeControlFactory;
// May be null in tests when created via createForTest above, in which case lazy-loading will
// intentionally fail with NPE. Still not marked @Nullable in the constructor, to force callers
@@ -476,7 +451,6 @@ public class ChangeData {
this.change = change;
this.notes = notes;
this.changeControl = control;
}
public ChangeData setLazyLoad(boolean load) {
@@ -601,7 +575,8 @@ public class ChangeData {
return visibleTo == user;
}
public ChangeControl changeControl() throws OrmException {
private ChangeControl changeControl() throws OrmException {
// TODO(hiesel): Remove this method.
if (changeControl == null) {
Change c = change();
try {
@@ -648,8 +623,7 @@ public class ChangeData {
} catch (IOException e) {
throw new OrmException("project state not available", e);
}
labelTypes =
state.getLabelTypes(changeControl().getChange().getDest(), changeControl().getUser());
labelTypes = state.getLabelTypes(change().getDest(), userFactory.create(change().getOwner()));
}
return labelTypes;
}
@@ -693,7 +667,12 @@ public class ChangeData {
currentApprovals =
ImmutableList.copyOf(
approvalsUtil.byPatchSet(
db, notes(), changeControl().getUser(), c.currentPatchSetId(), null, null));
db,
notes(),
userFactory.create(c.getOwner()),
c.currentPatchSetId(),
null,
null));
} catch (OrmException e) {
if (e.getCause() instanceof NoSuchChangeException) {
currentApprovals = Collections.emptyList();
@@ -965,13 +944,17 @@ public class ChangeData {
return messages;
}
public List<SubmitRecord> submitRecords(SubmitRuleOptions options) {
public List<SubmitRecord> submitRecords(SubmitRuleOptions options) throws OrmException {
List<SubmitRecord> records = submitRecords.get(options);
if (records == null) {
if (!lazyLoad) {
return Collections.emptyList();
}
records = submitRuleEvaluatorFactory.create(this).setOptions(options).evaluate();
records =
submitRuleEvaluatorFactory
.create(userFactory.create(change().getOwner()), this)
.setOptions(options)
.evaluate();
submitRecords.put(options, records);
}
return records;
@@ -986,9 +969,12 @@ public class ChangeData {
submitRecords.put(options, records);
}
public SubmitTypeRecord submitTypeRecord() {
public SubmitTypeRecord submitTypeRecord() throws OrmException {
if (submitTypeRecord == null) {
submitTypeRecord = submitRuleEvaluatorFactory.create(this).getSubmitType();
submitTypeRecord =
submitRuleEvaluatorFactory
.create(userFactory.create(change().getOwner()), this)
.getSubmitType();
}
return submitTypeRecord;
}

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

@@ -31,6 +31,7 @@ import com.google.gerrit.server.data.PatchSetAttribute;
import com.google.gerrit.server.data.QueryStatsAttribute;
import com.google.gerrit.server.events.EventFactory;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.SubmitRuleEvaluator;
import com.google.gson.Gson;
import com.google.gwtorm.server.OrmException;
@@ -78,6 +79,7 @@ public class OutputStreamQuery {
private final EventFactory eventFactory;
private final TrackingFooters trackingFooters;
private final CurrentUser user;
private final ChangeControl.GenericFactory changeControlFactory;
private final SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory;
private OutputFormat outputFormat = OutputFormat.TEXT;
@@ -103,6 +105,7 @@ public class OutputStreamQuery {
EventFactory eventFactory,
TrackingFooters trackingFooters,
CurrentUser user,
ChangeControl.GenericFactory changeControlFactory,
SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory) {
this.db = db;
this.repoManager = repoManager;
@@ -111,6 +114,7 @@ public class OutputStreamQuery {
this.eventFactory = eventFactory;
this.trackingFooters = trackingFooters;
this.user = user;
this.changeControlFactory = changeControlFactory;
this.submitRuleEvaluatorFactory = submitRuleEvaluatorFactory;
}
@@ -250,7 +254,11 @@ public class OutputStreamQuery {
if (includeSubmitRecords) {
eventFactory.addSubmitRecords(
c,
submitRuleEvaluatorFactory.create(d).setAllowClosed(true).setAllowDraft(true).evaluate());
submitRuleEvaluatorFactory
.create(user, d)
.setAllowClosed(true)
.setAllowDraft(true)
.evaluate());
}
if (includeCommitMessage) {
@@ -270,12 +278,13 @@ public class OutputStreamQuery {
}
}
ChangeControl ctl = changeControlFactory.controlFor(db, d.change(), user);
if (includePatchSets) {
eventFactory.addPatchSets(
db,
rw,
c,
d.changeControl().getVisiblePatchSets(d.patchSets(), db),
ctl.getVisiblePatchSets(d.patchSets(), db),
includeApprovals ? d.approvals().asMap() : null,
includeFiles,
d.change(),
@@ -284,7 +293,7 @@ public class OutputStreamQuery {
if (includeCurrentPatchSet) {
PatchSet current = d.currentPatchSet();
if (current != null && d.changeControl().forUser(user).isPatchVisible(current, d.db())) {
if (current != null && ctl.isPatchVisible(current, d.db())) {
c.currentPatchSet = eventFactory.asPatchSetAttribute(db, rw, d.change(), current);
eventFactory.addApprovals(c.currentPatchSet, d.currentApprovals(), labelTypes);
@@ -304,7 +313,7 @@ public class OutputStreamQuery {
db,
rw,
c,
d.changeControl().getVisiblePatchSets(d.patchSets(), db),
ctl.getVisiblePatchSets(d.patchSets(), db),
includeApprovals ? d.approvals().asMap() : null,
includeFiles,
d.change(),