Skip to content

Commit

Permalink
Merge changes I1316adf6,I4031a3f8,I6073fb02,I9566dcc2,I4d335c61,I4656…
Browse files Browse the repository at this point in the history 
…925c,I96fe842b

* changes:
  Assume labels are correct in ListChanges
  Cache IdentifiedUser across PrologEnvironments
  Use Providers.of() instead of hand-rolled Provider
  Load patch set approvals in parallel
  Lookup user full names on the fly only once during change query
  Bulk load change and patch set data, reuse approvals
  Pass PatchSet not PatchSet.Id to canSubmit()
  • Loading branch information
@spearce
spearce authored and gerrit code review committed May 3, 2012
2 parents daf6bd9 + d0df169 commit b512830
Show file tree
Hide file tree
Showing 12 changed files with 235 additions and 150 deletions.
2 changes: 1 addition & 1 deletion gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java
View file
Expand Up @@ -134,7 +134,7 @@ public ChangeDetail call() throws OrmException, NoSuchEntityException,
detail.setCanEdit(control.getRefControl().canWrite());

if (detail.getChange().getStatus().isOpen()) {
List<SubmitRecord> submitRecords = control.canSubmit(db, patch.getId());
List<SubmitRecord> submitRecords = control.canSubmit(db, patch);
for (SubmitRecord rec : submitRecords) {
if (rec.labels != null) {
for (SubmitRecord.Label lbl : rec.labels) {
Expand Down
5 changes: 3 additions & 2 deletions .../src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java
View file
Expand Up @@ -83,7 +83,8 @@ public PatchSetPublishDetail call() throws OrmException,
final Change.Id changeId = patchSetId.getParentKey();
final ChangeControl control = changeControlFactory.validateFor(changeId);
change = control.getChange();
patchSetInfo = infoFactory.get(db, patchSetId);
PatchSet patchSet = db.patchSets().get(patchSetId);
patchSetInfo = infoFactory.get(change, patchSet);
drafts = db.patchComments().draftByPatchSetAuthor(patchSetId, user.getAccountId()).toList();

aic.want(change.getOwner());
Expand Down Expand Up @@ -119,7 +120,7 @@ public PatchSetPublishDetail call() throws OrmException,
.toList();

boolean couldSubmit = false;
List<SubmitRecord> submitRecords = control.canSubmit(db, patchSetId);
List<SubmitRecord> submitRecords = control.canSubmit(db, patchSet);
for (SubmitRecord rec : submitRecords) {
if (rec.status == SubmitRecord.Status.OK) {
couldSubmit = true;
Expand Down
34 changes: 30 additions & 4 deletions gerrit-server/src/main/java/com/google/gerrit/rules/StoredValues.java
View file
Expand Up @@ -16,19 +16,24 @@

import static com.google.gerrit.rules.StoredValue.create;

import com.google.common.collect.Maps;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountDiffPreference.Whitespace;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetInfo;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.AccountDiffPreference.Whitespace;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.AnonymousUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.patch.PatchList;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.patch.PatchListKey;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.query.change.ChangeData;

import com.googlecode.prolog_cafe.lang.Prolog;
import com.googlecode.prolog_cafe.lang.SystemException;
Expand All @@ -37,21 +42,25 @@
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;

import java.util.Map;

public final class StoredValues {
public static final StoredValue<ReviewDb> REVIEW_DB = create(ReviewDb.class);
public static final StoredValue<Change> CHANGE = create(Change.class);
public static final StoredValue<PatchSet.Id> PATCH_SET_ID = create(PatchSet.Id.class);
public static final StoredValue<ChangeData> CHANGE_DATA = create(ChangeData.class);
public static final StoredValue<PatchSet> PATCH_SET = create(PatchSet.class);
public static final StoredValue<ChangeControl> CHANGE_CONTROL = create(ChangeControl.class);

public static final StoredValue<PatchSetInfo> PATCH_SET_INFO = new StoredValue<PatchSetInfo>() {
@Override
public PatchSetInfo createValue(Prolog engine) {
PatchSet.Id patchSetId = StoredValues.PATCH_SET_ID.get(engine);
Change change = StoredValues.CHANGE.get(engine);
PatchSet ps = StoredValues.PATCH_SET.get(engine);
PrologEnvironment env = (PrologEnvironment) engine.control;
PatchSetInfoFactory patchInfoFactory =
env.getInjector().getInstance(PatchSetInfoFactory.class);
try {
return patchInfoFactory.get(REVIEW_DB.get(engine), patchSetId);
return patchInfoFactory.get(change, ps);
} catch (PatchSetInfoNotAvailableException e) {
throw new SystemException(e.getMessage());
}
Expand Down Expand Up @@ -102,6 +111,23 @@ public void run() {
}
};

public static final StoredValue<AnonymousUser> ANONYMOUS_USER =
new StoredValue<AnonymousUser>() {
@Override
protected AnonymousUser createValue(Prolog engine) {
PrologEnvironment env = (PrologEnvironment) engine.control;
return env.getInjector().getInstance(AnonymousUser.class);
}
};

public static final StoredValue<Map<Account.Id, IdentifiedUser>> USERS =
new StoredValue<Map<Account.Id, IdentifiedUser>>() {
@Override
protected Map<Account.Id, IdentifiedUser> createValue(Prolog engine) {
return Maps.newHashMap();
}
};

private StoredValues() {
}
}
2 changes: 1 addition & 1 deletion gerrit-server/src/main/java/com/google/gerrit/server/changedetail/Submit.java
View file
Expand Up @@ -80,7 +80,7 @@ public ReviewResult call() throws IllegalStateException,
throw new NoSuchChangeException(changeId);
}

List<SubmitRecord> submitResult = control.canSubmit(db, patchSetId);
List<SubmitRecord> submitResult = control.canSubmit(db, patch);
if (submitResult.isEmpty()) {
throw new IllegalStateException(
"ChangeControl.canSubmit returned empty list");
Expand Down
30 changes: 19 additions & 11 deletions gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchSetInfoFactory.java
View file
Expand Up @@ -18,7 +18,6 @@
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetInfo;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.client.UserIdentity;
import com.google.gerrit.reviewdb.server.ReviewDb;
Expand All @@ -29,6 +28,7 @@
import com.google.inject.Singleton;

import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Repository;
Expand Down Expand Up @@ -68,31 +68,39 @@ public PatchSetInfo get(RevCommit src, PatchSet.Id psi) {
}

public PatchSetInfo get(ReviewDb db, PatchSet.Id patchSetId)
throws PatchSetInfoNotAvailableException {
Repository repo = null;
throws PatchSetInfoNotAvailableException {
try {
final PatchSet patchSet = db.patchSets().get(patchSetId);
final Change change = db.changes().get(patchSet.getId().getParentKey());
final Project.NameKey projectKey = change.getProject();
repo = repoManager.openRepository(projectKey);
return get(change, patchSet);
} catch (OrmException e) {
throw new PatchSetInfoNotAvailableException(e);
}
}

public PatchSetInfo get(Change change, PatchSet patchSet)
throws PatchSetInfoNotAvailableException {
Repository repo;
try {
repo = repoManager.openRepository(change.getProject());
} catch (RepositoryNotFoundException e) {
throw new PatchSetInfoNotAvailableException(e);
}
try {
final RevWalk rw = new RevWalk(repo);
try {
final RevCommit src =
rw.parseCommit(ObjectId.fromString(patchSet.getRevision().get()));
PatchSetInfo info = get(src, patchSetId);
PatchSetInfo info = get(src, patchSet.getId());
info.setParents(toParentInfos(src.getParents(), rw));
return info;
} finally {
rw.release();
}
} catch (OrmException e) {
throw new PatchSetInfoNotAvailableException(e);
} catch (IOException e) {
throw new PatchSetInfoNotAvailableException(e);
} finally {
if (repo != null) {
repo.close();
}
repo.close();
}
}

Expand Down
72 changes: 46 additions & 26 deletions gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
View file
Expand Up @@ -26,10 +26,11 @@
import com.google.gerrit.rules.StoredValues;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import com.google.gwtorm.server.ResultSet;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.util.Providers;

import com.googlecode.prolog_cafe.compiler.CompileException;
import com.googlecode.prolog_cafe.lang.IntegerTerm;
Expand All @@ -49,6 +50,8 @@
import java.util.List;
import java.util.Set;

import javax.annotation.Nullable;


/** Access control management for a user accessing a single change. */
public class ChangeControl {
Expand Down Expand Up @@ -161,7 +164,7 @@ public Change getChange() {

/** Can this user see this change? */
public boolean isVisible(ReviewDb db) throws OrmException {
if (change.getStatus() == Change.Status.DRAFT && !isDraftVisible(db)) {
if (change.getStatus() == Change.Status.DRAFT && !isDraftVisible(db, null)) {
return false;
}
return isRefVisible();
Expand All @@ -174,7 +177,7 @@ public boolean isRefVisible() {

/** Can this user see the given patchset? */
public boolean isPatchVisible(PatchSet ps, ReviewDb db) throws OrmException {
if (ps.isDraft() && !isDraftVisible(db)) {
if (ps.isDraft() && !isDraftVisible(db, null)) {
return false;
}
return isVisible(db);
Expand Down Expand Up @@ -235,10 +238,20 @@ public boolean isOwner() {

/** Is this user a reviewer for the change? */
public boolean isReviewer(ReviewDb db) throws OrmException {
return isReviewer(db, null);
}

/** Is this user a reviewer for the change? */
public boolean isReviewer(ReviewDb db, @Nullable ChangeData cd)
throws OrmException {
if (getCurrentUser() instanceof IdentifiedUser) {
final IdentifiedUser user = (IdentifiedUser) getCurrentUser();
ResultSet<PatchSetApproval> results =
db.patchSetApprovals().byChange(change.getId());
Iterable<PatchSetApproval> results;
if (cd != null) {
results = cd.currentApprovals(Providers.of(db));
} else {
results = db.patchSetApprovals().byChange(change.getId());
}
for (PatchSetApproval approval : results) {
if (user.getAccountId().equals(approval.getAccountId())) {
return true;
Expand Down Expand Up @@ -278,34 +291,39 @@ public boolean canRemoveReviewer(PatchSetApproval approval) {
return false;
}

public List<SubmitRecord> canSubmit(ReviewDb db, PatchSet.Id patchSetId) {
public List<SubmitRecord> canSubmit(ReviewDb db, PatchSet patchSet) {
return canSubmit(db, patchSet, null, false);
}

public List<SubmitRecord> canSubmit(ReviewDb db, PatchSet patchSet,
@Nullable ChangeData cd, boolean fastEvalLabels) {
if (change.getStatus().isClosed()) {
SubmitRecord rec = new SubmitRecord();
rec.status = SubmitRecord.Status.CLOSED;
return Collections.singletonList(rec);
}

if (!patchSetId.equals(change.currentPatchSetId())) {
return ruleError("Patch set " + patchSetId + " is not current");
if (!patchSet.getId().equals(change.currentPatchSetId())) {
return ruleError("Patch set " + patchSet.getPatchSetId() + " is not current");
}

try {
if (change.getStatus() == Change.Status.DRAFT){
if (!isVisible(db)) {
return ruleError("Patch set " + patchSetId + " not found");
if (change.getStatus() == Change.Status.DRAFT) {
if (!isDraftVisible(db, cd)) {
return ruleError("Patch set " + patchSet.getPatchSetId() + " not found");
} else {
return ruleError("Cannot submit draft changes");
}
}
if (isDraftPatchSet(patchSetId, db)) {
if (!isVisible(db)) {
return ruleError("Patch set " + patchSetId + " not found");
if (patchSet.isDraft()) {
if (!isDraftVisible(db, cd)) {
return ruleError("Patch set " + patchSet.getPatchSetId() + " not found");
} else {
return ruleError("Cannot submit draft patch sets");
}
}
} catch (OrmException err) {
return logRuleError("Cannot read patch set " + patchSetId, err);
return logRuleError("Cannot read patch set " + patchSet.getId(), err);
}

List<Term> results = new ArrayList<Term>();
Expand All @@ -323,7 +341,8 @@ public List<SubmitRecord> canSubmit(ReviewDb db, PatchSet.Id patchSetId) {
try {
env.set(StoredValues.REVIEW_DB, db);
env.set(StoredValues.CHANGE, change);
env.set(StoredValues.PATCH_SET_ID, patchSetId);
env.set(StoredValues.CHANGE_DATA, cd);
env.set(StoredValues.PATCH_SET, patchSet);
env.set(StoredValues.CHANGE_CONTROL, this);

submitRule = env.once(
Expand All @@ -334,6 +353,10 @@ public List<SubmitRecord> canSubmit(ReviewDb db, PatchSet.Id patchSetId) {
+ getProject().getName());
}

if (fastEvalLabels) {
env.once("gerrit", "assume_range_from_label");
}

try {
for (Term[] template : env.all(
"gerrit", "can_submit",
Expand Down Expand Up @@ -372,6 +395,10 @@ public List<SubmitRecord> canSubmit(ReviewDb db, PatchSet.Id patchSetId) {
parentEnv.once("gerrit", "locate_submit_filter", new VariableTerm());
if (filterRule != null) {
try {
if (fastEvalLabels) {
env.once("gerrit", "assume_range_from_label");
}

Term resultsTerm = toListTerm(results);
results.clear();
Term[] template = parentEnv.once(
Expand Down Expand Up @@ -515,16 +542,9 @@ private void appliedBy(SubmitRecord.Label label, Term status) {
}
}

private boolean isDraftVisible(ReviewDb db) throws OrmException {
return isOwner() || isReviewer(db);
}

private boolean isDraftPatchSet(PatchSet.Id id, ReviewDb db) throws OrmException {
PatchSet ps = db.patchSets().get(id);
if (ps == null) {
throw new OrmException("Patch set " + id + " not found");
}
return ps.isDraft();
private boolean isDraftVisible(ReviewDb db, ChangeData cd)
throws OrmException {
return isOwner() || isReviewer(db, cd);
}

private static boolean isUser(Term who) {
Expand Down

0 comments on commit b512830

Please sign in to comment.