opendev/gerrit
Code Issues Proposed changes

Merge changes I4c27c33e,I9d18a039

Browse Source 
* changes:
  ChangeControl: Pass ReviewDb to all factory methods
  SiteIndexer: Don't mark ready if more than 10% of changes failed
This commit is contained in:
Dave Borowitz
2016-02-09 15:02:23 +00:00
committed by Gerrit Code Review
parent e4ed9a282d 8ea575a35b
commit 2676ba980f
22 changed files with 76 additions and 60 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/change/ConsistencyCheckerIT.java
View File

@@ -610,7 +610,7 @@ public class ConsistencyCheckerIT extends AbstractDaemonTest {
indexer.index(db, c);
ChangeUpdate u = changeUpdateFactory.create(
changeControlFactory.controlFor(c, userFactory.create(adminId)));
changeControlFactory.controlFor(db, c, userFactory.create(adminId)));
u.setBranch(c.getDest().get());
u.setChangeId(c.getKey().get());
u.commit();

2
gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/CatServlet.java
View File

@@ -122,7 +122,7 @@ public class CatServlet extends HttpServlet {
String revision;
try {
final ReviewDb db = requestDb.get();
final ChangeControl control = changeControl.validateFor(changeId,
final ChangeControl control = changeControl.validateFor(db, changeId,
userProvider.get());
if (patchKey.getParentKey().get() == 0) {
// change edit

4
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/BaseServiceImplementation.java
View File

@@ -48,6 +48,10 @@ public class BaseServiceImplementation {
return currentUser.get();
}
protected ReviewDb getDb() {
return schema.get();
}
/**
* Executes {@code action.run} with an active ReviewDb connection.
* <p>

2
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java
View File

@@ -129,7 +129,7 @@ class PatchSetDetailFactory extends Handler<PatchSetDetail> {
Optional<ChangeEdit> edit = null;
ChangeNotes notes;
if (control == null || patchSet == null) {
control = changeControlFactory.validateFor(psIdNew.getParentKey(),
control = changeControlFactory.validateFor(db, psIdNew.getParentKey(),
userProvider.get());
notes = control.getNotes();
if (psIdNew.get() == 0) {

2
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java
View File

@@ -59,7 +59,7 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements
@Override
public PatchScript call() throws Exception {
ChangeControl control = changeControlFactory.validateFor(
patchKey.getParentKey().getParentKey(),
getDb(), patchKey.getParentKey().getParentKey(),
getUser());
return patchScriptFactoryFactory.create(
control, patchKey.getFileName(), psa, psb, dp).call();

2
gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java
View File

@@ -985,7 +985,7 @@ public class ChangeHookRunner implements ChangeHooks, EventDispatcher,
return false;
}
ProjectControl pc = pe.controlFor(user);
return pc.controlFor(change).isVisible(db);
return pc.controlFor(db, change).isVisible(db);
}
private boolean isVisibleTo(Branch.NameKey branchName, CurrentUser user) {

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

@@ -15,11 +15,9 @@
package com.google.gerrit.server.change;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.config.ChangeCleanupConfig;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.query.QueryParseException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.ChangeQueryBuilder;
@@ -42,7 +40,6 @@ public class AbandonUtil {
private final IdentifiedUser.GenericFactory identifiedUserFactory;
private final QueryProcessor queryProcessor;
private final ChangeQueryBuilder queryBuilder;
private final ChangeControl.GenericFactory changeControlFactory;
private final Abandon abandon;
@Inject
@@ -51,13 +48,11 @@ public class AbandonUtil {
IdentifiedUser.GenericFactory identifiedUserFactory,
QueryProcessor queryProcessor,
ChangeQueryBuilder queryBuilder,
ChangeControl.GenericFactory changeControlFactory,
Abandon abandon) {
this.cfg = cfg;
this.identifiedUserFactory = identifiedUserFactory;
this.queryProcessor = queryProcessor;
this.queryBuilder = queryBuilder;
this.changeControlFactory = changeControlFactory;
this.abandon = abandon;
}
@@ -95,10 +90,8 @@ public class AbandonUtil {
}
}
private ChangeControl changeControl(ChangeData cd)
throws NoSuchChangeException, OrmException {
Change c = cd.change();
return changeControlFactory.controlFor(c,
identifiedUserFactory.create(c.getOwner()));
private ChangeControl changeControl(ChangeData cd) throws OrmException {
return cd.changeControl(
identifiedUserFactory.create(cd.change().getOwner()));
}
}

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

@@ -279,7 +279,8 @@ public class CherryPickChange {
.append(cherryPickCommit.getId().getName());
changeMessage.setMessage(sb.toString());
ChangeControl ctl = refControl.getProjectControl().controlFor(change);
ChangeControl ctl = refControl.getProjectControl()
.controlFor(db.get(), change);
ChangeUpdate update = updateFactory.create(ctl, TimeUtil.nowTs());
changeMessagesUtil.addChangeMessage(db.get(), update, changeMessage);
update.commit();

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

@@ -531,7 +531,7 @@ public class ConsistencyChecker {
});
ChangeUpdate changeUpdate =
changeUpdateFactory.create(
changeControlFactory.controlFor(change, user.get()));
changeControlFactory.controlFor(db.get(), change, user.get()));
changeUpdate.fixStatus(Change.Status.MERGED);
changeUpdate.commit();
indexer.index(db.get(), change);

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

@@ -244,7 +244,7 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
ReviewDb db = dbProvider.get();
for (PatchSet.Id psId : cs.patchIds()) {
ChangeControl changeControl = changeControlFactory
.controlFor(project, psId.getParentKey(), identifiedUser);
.controlFor(db, project, psId.getParentKey(), identifiedUser);
ChangeData c = changeDataFactory.create(db, changeControl);
if (!changeControl.isVisible(db)) {

3
gerrit-server/src/main/java/com/google/gerrit/server/edit/ChangeEditUtil.java
View File

@@ -119,7 +119,8 @@ public class ChangeEditUtil {
public Optional<ChangeEdit> byChange(Change change)
throws AuthException, IOException, OrmException {
try {
return byChange(changeControlFactory.controlFor(change, user.get()));
return byChange(
changeControlFactory.controlFor(db.get(), change, user.get()));
} catch (NoSuchChangeException e) {
throw new IOException(e);
}

12
gerrit-server/src/main/java/com/google/gerrit/server/git/LabelNormalizer.java
View File

@@ -29,11 +29,13 @@ import com.google.gerrit.common.data.PermissionRange;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.util.Collection;
@@ -72,12 +74,16 @@ public class LabelNormalizer {
}
}
private final Provider<ReviewDb> db;
private final ChangeControl.GenericFactory changeFactory;
private final IdentifiedUser.GenericFactory userFactory;
@Inject
LabelNormalizer(ChangeControl.GenericFactory changeFactory,
LabelNormalizer(
Provider<ReviewDb> db,
ChangeControl.GenericFactory changeFactory,
IdentifiedUser.GenericFactory userFactory) {
this.db = db;
this.changeFactory = changeFactory;
this.userFactory = userFactory;
}
@@ -94,9 +100,9 @@ public class LabelNormalizer {
*/
public Result normalize(Change change, Collection<PatchSetApproval> approvals)
throws NoSuchChangeException, OrmException {
IdentifiedUser user = userFactory.create(change.getOwner());
return normalize(
changeFactory.controlFor(change, userFactory.create(change.getOwner())),
approvals);
changeFactory.controlFor(db.get(), change, user), approvals);
}
/**

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

@@ -899,7 +899,7 @@ public class MergeOp implements AutoCloseable {
db.changes().beginTransaction(change.getId());
//TODO(dborowitz): support InternalUser in ChangeUpdate
ChangeControl control = changeControlFactory.controlFor(change,
ChangeControl control = changeControlFactory.controlFor(db, change,
identifiedUserFactory.create(change.getOwner()));
// TODO(dborowitz): Convert to BatchUpdate.
ChangeUpdate update = changeUpdateFactory.create(control);

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

@@ -1824,7 +1824,7 @@ public class ReceiveCommits {
change = ins.getChange();
if (magicBranch != null && magicBranch.submit) {
submit(projectControl.controlFor(change), ins.getPatchSet());
submit(projectControl.controlFor(state.db, change), ins.getPatchSet());
}
}
}
@@ -2018,7 +2018,7 @@ public class ReceiveCommits {
return false;
}
changeCtl = projectControl.controlFor(change);
changeCtl = projectControl.controlFor(db, change);
if (!changeCtl.canAddPatchSet(db)) {
String locked = ".";
if (changeCtl.isPatchSetLocked(db)) {
@@ -2725,7 +2725,7 @@ public class ReceiveCommits {
Change change =
notesFactory.create(db, project.getNameKey(), cid).getChange();
ChangeControl ctl = projectControl.controlFor(change);
ChangeControl ctl = projectControl.controlFor(db, change);
PatchSet ps = psUtil.get(db, ctl.getNotes(), psi);
if (change == null || ps == null) {
log.warn(project.getName() + " " + psi + " is missing");

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

@@ -178,7 +178,7 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook {
try {
final Set<Change.Id> visibleChanges = new HashSet<>();
for (Change change : changeCache.get(project.getNameKey())) {
if (projectCtl.controlFor(change).isVisible(reviewDb)) {
if (projectCtl.controlFor(reviewDb, change).isVisible(reviewDb)) {
visibleChanges.add(change.getId());
}
}

11
gerrit-server/src/main/java/com/google/gerrit/server/index/SiteIndexer.java
View File

@@ -230,6 +230,17 @@ public class SiteIndexer {
log.error("Error in batch indexer", e);
ok.set(false);
}
// If too many changes failed, maybe there was a bug in the indexer. Don't
// trust the results. This is not an exact percentage since we bump the same
// failure counter if a project can't be read, but close enough.
int nFailed = failedTask.getCount();
int nTotal = nFailed + doneTask.getCount();
double pctFailed = ((double) nFailed) / nTotal * 100;
if (pctFailed > 10) {
log.error("Failed {}/{} changes ({}%); not marking new index as ready",
nFailed, nTotal, Math.round(pctFailed));
ok.set(false);
}
return new Result(sw, ok.get(), doneTask.getCount(), failedTask.getCount());
}

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

@@ -370,7 +370,7 @@ public abstract class ChangeEmail extends NotificationEmail {
protected boolean isVisibleTo(final Account.Id to) throws OrmException {
return projectState == null
|| projectState.controlFor(args.identifiedUserFactory.create(to))
.controlFor(change).isVisible(args.db.get());
.controlFor(args.db.get(), change).isVisible(args.db.get());
}
/** Find all users who are authors of any part of this change. */

4
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilder.java
View File

@@ -163,7 +163,7 @@ public class ChangeRebuilder {
writeToBatch(batch, update, changeRepo);
IdentifiedUser user = userFactory.create(dbProvider, e.who);
update = updateFactory.create(
controlFactory.controlFor(change, user), e.when);
controlFactory.controlFor(db, change, user), e.when);
update.setPatchSetId(e.psId);
if (batch == null) {
batch = update.openUpdateInBatch(bru);
@@ -191,7 +191,7 @@ public class ChangeRebuilder {
for (PatchLineCommentEvent e : draftCommentEvents.get(author)) {
if (draftUpdate == null) {
draftUpdate = draftUpdateFactory.create(
controlFactory.controlFor(change, user), e.when);
controlFactory.controlFor(db, change, user), e.when);
draftUpdate.setPatchSetId(e.psId);
batchForDrafts = draftUpdate.openUpdateInBatch(bruAllUsers);
}

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

@@ -35,7 +35,7 @@ import com.google.gerrit.server.notedb.ChangeNotes;
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.Singleton;
import java.io.IOException;
import java.util.Collection;
@@ -44,34 +44,34 @@ import java.util.List;
/** Access control management for a user accessing a single change. */
public class ChangeControl {
@Singleton
public static class GenericFactory {
private final ProjectControl.GenericFactory projectControl;
private final Provider<ReviewDb> db;
private final ChangeNotes.Factory notesFactory;
private final ChangeFinder changeFinder;
@Inject
GenericFactory(
ProjectControl.GenericFactory p,
Provider<ReviewDb> d,
ChangeNotes.Factory n,
ChangeFinder f) {
projectControl = p;
db = d;
notesFactory = n;
changeFinder = f;
}
public ChangeControl controlFor(Project.NameKey project, Change.Id changeId,
CurrentUser user) throws NoSuchChangeException, OrmException {
return controlFor(notesFactory.create(db.get(), project, changeId), user);
public ChangeControl controlFor(ReviewDb db, Project.NameKey project,
Change.Id changeId, CurrentUser user)
throws NoSuchChangeException, OrmException {
return controlFor(notesFactory.create(db, project, changeId), user);
}
public ChangeControl controlFor(Change change, CurrentUser user)
throws NoSuchChangeException, OrmException {
public ChangeControl controlFor(ReviewDb db, Change change,
CurrentUser user) throws NoSuchChangeException, OrmException {
final Project.NameKey projectKey = change.getProject();
try {
return projectControl.controlFor(projectKey, user).controlFor(change);
return projectControl.controlFor(projectKey, user)
.controlFor(db, change);
} catch (NoSuchProjectException e) {
throw new NoSuchChangeException(change.getId(), e);
} catch (IOException e) {
@@ -90,41 +90,39 @@ public class ChangeControl {
}
}
public ChangeControl validateFor(Change.Id changeId, CurrentUser user)
throws NoSuchChangeException, OrmException {
public ChangeControl validateFor(ReviewDb db, Change.Id changeId,
CurrentUser user) throws NoSuchChangeException, OrmException {
ChangeControl ctl = changeFinder.findOne(changeId, user);
return validateFor(ctl.getChange(), user);
return validateFor(db, ctl.getChange(), user);
}
public ChangeControl validateFor(Change change, CurrentUser user)
throws NoSuchChangeException, OrmException {
ChangeControl c = controlFor(change, user);
if (!c.isVisible(db.get())) {
public ChangeControl validateFor(ReviewDb db, Change change,
CurrentUser user) throws NoSuchChangeException, OrmException {
ChangeControl c = controlFor(db, change, user);
if (!c.isVisible(db)) {
throw new NoSuchChangeException(c.getId());
}
return c;
}
}
@Singleton
public static class Factory {
private final ReviewDb db;
private final ChangeData.Factory changeDataFactory;
private final ChangeNotes.Factory notesFactory;
private final ApprovalsUtil approvalsUtil;
@Inject
Factory(ReviewDb db,
ChangeData.Factory changeDataFactory,
Factory(ChangeData.Factory changeDataFactory,
ChangeNotes.Factory notesFactory,
ApprovalsUtil approvalsUtil) {
this.db = db;
this.changeDataFactory = changeDataFactory;
this.notesFactory = notesFactory;
this.approvalsUtil = approvalsUtil;
}
ChangeControl create(RefControl refControl, Project.NameKey project,
Change.Id changeId) throws OrmException {
ChangeControl create(RefControl refControl, ReviewDb db, Project.NameKey
project, Change.Id changeId) throws OrmException {
return create(refControl,
notesFactory.create(db, project, changeId));
}

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

@@ -194,8 +194,9 @@ public class ProjectControl {
return r;
}
public ChangeControl controlFor(Change change) throws OrmException {
return changeControlFactory.create(controlForRef(change.getDest()),
public ChangeControl controlFor(ReviewDb db, Change change)
throws OrmException {
return changeControlFactory.create(controlForRef(change.getDest()), db,
change.getProject(), change.getId());
}

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

@@ -561,8 +561,8 @@ public class ChangeData {
if (changeControl == null) {
Change c = change();
try {
changeControl =
changeControlFactory.controlFor(c, userFactory.create(c.getOwner()));
changeControl = changeControlFactory.controlFor(
db, c, userFactory.create(c.getOwner()));
} catch (NoSuchChangeException e) {
throw new OrmException(e);
}
@@ -577,10 +577,10 @@ public class ChangeData {
}
try {
if (change != null) {
changeControl = changeControlFactory.controlFor(change, user);
changeControl = changeControlFactory.controlFor(db, change, user);
} else {
changeControl =
changeControlFactory.controlFor(project, legacyId, user);
changeControlFactory.controlFor(db, project, legacyId, user);
}
} catch (NoSuchChangeException e) {
throw new OrmException(e);

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

@@ -114,7 +114,8 @@ class EqualsLabelPredicate extends IndexPredicate<ChangeData> {
//
IdentifiedUser reviewer = userFactory.create(dbProvider, approver);
try {
ChangeControl cc = ccFactory.controlFor(change, reviewer);
ChangeControl cc =
ccFactory.controlFor(dbProvider.get(), change, reviewer);
if (!cc.isVisible(dbProvider.get())) {
// The user can't see the change anymore.
//