opendev/gerrit
Code Issues Proposed changes

ChangeFinder: Return ChangeNotes instead of ChangeControl

Browse Source 
Change-Id: Idaeeb7d82cdd314bdc446216d4eee13b7bef5004
This commit is contained in:
Patrick Hiesel
2017-09-11 11:36:42 +02:00
parent 064d356c77
commit 57783a398f
8 changed files with 90 additions and 126 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
View File

@@ -253,6 +253,7 @@ public abstract class AbstractDaemonTest {
@Inject private InProcessProtocol inProcessProtocol;
@Inject private Provider<AnonymousUser> anonymousUser;
@Inject private SchemaFactory<ReviewDb> reviewDbProvider;
@Inject private ChangeControl.GenericFactory changeControlFactory;
private List<Repository> toClose;
@@ -1116,9 +1117,10 @@ public abstract class AbstractDaemonTest {
}
protected ChangeResource parseChangeResource(String changeId) throws Exception {
List<ChangeControl> ctls = changeFinder.find(changeId, atrScope.get().getUser());
assertThat(ctls).hasSize(1);
return changeResourceFactory.create(ctls.get(0));
List<ChangeNotes> notes = changeFinder.find(changeId);
assertThat(notes).hasSize(1);
return changeResourceFactory.create(
changeControlFactory.controlFor(notes.get(0), atrScope.get().getUser()));
}
protected String createGroup(String name) throws Exception {

5
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
View File

@@ -116,7 +116,6 @@ import com.google.gerrit.server.config.AnonymousCowardNameProvider;
import com.google.gerrit.server.git.ChangeMessageModifier;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.Util;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
@@ -3304,9 +3303,7 @@ public class ChangeIT extends AbstractDaemonTest {
}
private ChangeResource parseResource(PushOneCommit.Result r) throws Exception {
List<ChangeControl> ctls = changeFinder.find(r.getChangeId(), atrScope.get().getUser());
assertThat(ctls).hasSize(1);
return changeResourceFactory.create(ctls.get(0));
return parseChangeResource(r.getChangeId());
}
private Optional<ReviewerState> getReviewerState(String changeId, Account.Id accountId)

73
gerrit-server/src/main/java/com/google/gerrit/server/ChangeFinder.java
View File

@@ -25,7 +25,7 @@ import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.change.ChangeTriplet;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
@@ -59,7 +59,7 @@ public class ChangeFinder {
private final Cache<Change.Id, String> changeIdProjectCache;
private final Provider<InternalChangeQuery> queryProvider;
private final Provider<ReviewDb> reviewDb;
private final ChangeControl.GenericFactory changeControlFactory;
private final ChangeNotes.Factory changeNotesFactory;
@Inject
ChangeFinder(
@@ -67,24 +67,22 @@ public class ChangeFinder {
@Named(CACHE_NAME) Cache<Change.Id, String> changeIdProjectCache,
Provider<InternalChangeQuery> queryProvider,
Provider<ReviewDb> reviewDb,
ChangeControl.GenericFactory changeControlFactory) {
ChangeNotes.Factory changeNotesFactory) {
this.indexConfig = indexConfig;
this.changeIdProjectCache = changeIdProjectCache;
this.queryProvider = queryProvider;
this.reviewDb = reviewDb;
this.changeControlFactory = changeControlFactory;
this.changeNotesFactory = changeNotesFactory;
}
/**
* Find changes matching the given identifier.
*
* @param id change identifier, either a numeric ID, a Change-Id, or project~branch~id triplet.
* @param user user to wrap in controls.
* @return possibly-empty list of controls for all matching changes, corresponding to the given
* user; may or may not be visible.
* @return possibly-empty list of notes for all matching changes; may or may not be visible.
* @throws OrmException if an error occurred querying the database.
*/
public List<ChangeControl> find(String id, CurrentUser user) throws OrmException {
public List<ChangeNotes> find(String id) throws OrmException {
if (id.isEmpty()) {
return Collections.emptyList();
}
@@ -95,7 +93,7 @@ public class ChangeFinder {
// Try project~numericChangeId
Integer n = Ints.tryParse(id.substring(z + 1));
if (n != null) {
return fromProjectNumber(user, id.substring(0, z), n.intValue());
return fromProjectNumber(id.substring(0, z), n.intValue());
}
}
@@ -103,7 +101,7 @@ public class ChangeFinder {
// Try numeric changeId
Integer n = Ints.tryParse(id);
if (n != null) {
return find(new Change.Id(n), user);
return find(new Change.Id(n));
}
}
@@ -115,33 +113,22 @@ public class ChangeFinder {
Optional<ChangeTriplet> triplet = ChangeTriplet.parse(id, y, z);
if (triplet.isPresent()) {
ChangeTriplet t = triplet.get();
return asChangeControls(query.byBranchKey(t.branch(), t.id()), user);
return asChangeNotes(query.byBranchKey(t.branch(), t.id()));
}
}
// Try isolated Ihash... format ("Change-Id: Ihash").
return asChangeControls(query.byKeyPrefix(id), user);
return asChangeNotes(query.byKeyPrefix(id));
}
private List<ChangeControl> fromProjectNumber(CurrentUser user, String project, int changeNumber)
private List<ChangeNotes> fromProjectNumber(String project, int changeNumber)
throws OrmException {
Change.Id cId = new Change.Id(changeNumber);
try {
return ImmutableList.of(
changeControlFactory.controlFor(
reviewDb.get(), Project.NameKey.parse(project), cId, user));
changeNotesFactory.createChecked(reviewDb.get(), Project.NameKey.parse(project), cId));
} catch (NoSuchChangeException e) {
return Collections.emptyList();
} catch (IllegalArgumentException e) {
String changeNotFound = String.format("change %s not found in ReviewDb", cId);
String projectNotFound =
String.format(
"passed project %s when creating ChangeNotes for %s, but actual project is",
project, cId);
if (e.getMessage().equals(changeNotFound) || e.getMessage().startsWith(projectNotFound)) {
return Collections.emptyList();
}
throw e;
} catch (OrmException e) {
// Distinguish between a RepositoryNotFoundException (project argument invalid) and
// other OrmExceptions (failure in the persistence layer).
@@ -152,18 +139,18 @@ public class ChangeFinder {
}
}
public ChangeControl findOne(Change.Id id, CurrentUser user) throws OrmException {
List<ChangeControl> ctls = find(id, user);
if (ctls.size() != 1) {
public ChangeNotes findOne(Change.Id id) throws OrmException {
List<ChangeNotes> notes = find(id);
if (notes.size() != 1) {
throw new NoSuchChangeException(id);
}
return ctls.get(0);
return notes.get(0);
}
public List<ChangeControl> find(Change.Id id, CurrentUser user) throws OrmException {
public List<ChangeNotes> find(Change.Id id) throws OrmException {
String project = changeIdProjectCache.getIfPresent(id);
if (project != null) {
return fromProjectNumber(user, project, id.get());
return fromProjectNumber(project, id.get());
}
// Use the index to search for changes, but don't return any stored fields,
@@ -173,17 +160,16 @@ public class ChangeFinder {
if (r.size() == 1) {
changeIdProjectCache.put(id, r.get(0).project().get());
}
return asChangeControls(r, user);
return asChangeNotes(r);
}
private List<ChangeControl> asChangeControls(List<ChangeData> cds, CurrentUser user)
throws OrmException {
List<ChangeControl> ctls = new ArrayList<>(cds.size());
private List<ChangeNotes> asChangeNotes(List<ChangeData> cds) throws OrmException {
List<ChangeNotes> notes = new ArrayList<>(cds.size());
if (!indexConfig.separateChangeSubIndexes()) {
for (ChangeData cd : cds) {
checkedAdd(cd, ctls, user);
notes.add(cd.notes());
}
return ctls;
return notes;
}
// If an index implementation uses separate non-atomic subindexes, it's possible to temporarily
@@ -195,18 +181,9 @@ public class ChangeFinder {
Set<Change.Id> seen = Sets.newHashSetWithExpectedSize(cds.size());
for (ChangeData cd : cds) {
if (seen.add(cd.getId())) {
checkedAdd(cd, ctls, user);
notes.add(cd.notes());
}
}
return ctls;
}
private static void checkedAdd(ChangeData cd, List<ChangeControl> ctls, CurrentUser user)
throws OrmException {
try {
ctls.add(cd.changeControl(user));
} catch (NoSuchChangeException e) {
// Ignore
}
return notes;
}
}

48
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangesCollection.java
View File

@@ -26,10 +26,12 @@ import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ChangeFinder;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.ChangePermission;
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.query.change.QueryChanges;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -48,6 +50,7 @@ public class ChangesCollection
private final CreateChange createChange;
private final ChangeResource.Factory changeResourceFactory;
private final PermissionBackend permissionBackend;
private final ChangeControl.GenericFactory changeControlFactory;
@Inject
ChangesCollection(
@@ -58,7 +61,8 @@ public class ChangesCollection
ChangeFinder changeFinder,
CreateChange createChange,
ChangeResource.Factory changeResourceFactory,
PermissionBackend permissionBackend) {
PermissionBackend permissionBackend,
ChangeControl.GenericFactory changeControlFactory) {
this.db = db;
this.user = user;
this.queryFactory = queryFactory;
@@ -67,6 +71,7 @@ public class ChangesCollection
this.createChange = createChange;
this.changeResourceFactory = changeResourceFactory;
this.permissionBackend = permissionBackend;
this.changeControlFactory = changeControlFactory;
}
@Override
@@ -82,34 +87,34 @@ public class ChangesCollection
@Override
public ChangeResource parse(TopLevelResource root, IdString id)
throws ResourceNotFoundException, OrmException, PermissionBackendException {
List<ChangeControl> ctls = changeFinder.find(id.encoded(), user.get());
if (ctls.isEmpty()) {
List<ChangeNotes> notes = changeFinder.find(id.encoded());
if (notes.isEmpty()) {
throw new ResourceNotFoundException(id);
} else if (ctls.size() != 1) {
} else if (notes.size() != 1) {
throw new ResourceNotFoundException("Multiple changes found for " + id);
}
ChangeControl ctl = ctls.get(0);
if (!canRead(ctl)) {
ChangeNotes change = notes.get(0);
if (!canRead(change)) {
throw new ResourceNotFoundException(id);
}
return changeResourceFactory.create(ctl);
return changeResourceFactory.create(controlFor(change));
}
public ChangeResource parse(Change.Id id)
throws ResourceNotFoundException, OrmException, PermissionBackendException {
List<ChangeControl> ctls = changeFinder.find(id, user.get());
if (ctls.isEmpty()) {
List<ChangeNotes> notes = changeFinder.find(id);
if (notes.isEmpty()) {
throw new ResourceNotFoundException(toIdString(id));
} else if (ctls.size() != 1) {
} else if (notes.size() != 1) {
throw new ResourceNotFoundException("Multiple changes found for " + id);
}
ChangeControl ctl = ctls.get(0);
if (!canRead(ctl)) {
ChangeNotes change = notes.get(0);
if (!canRead(change)) {
throw new ResourceNotFoundException(toIdString(id));
}
return changeResourceFactory.create(ctl);
return changeResourceFactory.create(controlFor(change));
}
private static IdString toIdString(Change.Id id) {
@@ -126,11 +131,16 @@ public class ChangesCollection
return createChange;
}
private boolean canRead(ChangeControl ctl) throws PermissionBackendException {
return permissionBackend
.user(user)
.change(ctl.getNotes())
.database(db)
.test(ChangePermission.READ);
private boolean canRead(ChangeNotes notes) throws PermissionBackendException {
return permissionBackend.user(user).change(notes).database(db).test(ChangePermission.READ);
}
private ChangeControl controlFor(ChangeNotes notes) throws ResourceNotFoundException {
try {
return changeControlFactory.controlFor(notes, user.get());
} catch (NoSuchChangeException e) {
throw new ResourceNotFoundException(
String.format("Change %s not found", notes.getChangeId()));
}
}
}

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

@@ -52,11 +52,11 @@ import com.google.gerrit.server.config.AnonymousCowardName;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.CommitsCollection;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.ProjectControl;
@@ -194,19 +194,15 @@ public class CreateChange
ObjectId parentCommit;
List<String> groups;
if (input.baseChange != null) {
List<ChangeControl> ctls = changeFinder.find(input.baseChange, rsrc.getUser());
if (ctls.size() != 1) {
List<ChangeNotes> notes = changeFinder.find(input.baseChange);
if (notes.size() != 1) {
throw new UnprocessableEntityException("Base change not found: " + input.baseChange);
}
ChangeControl ctl = Iterables.getOnlyElement(ctls);
if (!permissionBackend
.user(user)
.change(ctl.getNotes())
.database(db)
.test(ChangePermission.READ)) {
ChangeNotes change = Iterables.getOnlyElement(notes);
if (!permissionBackend.user(user).change(change).database(db).test(ChangePermission.READ)) {
throw new UnprocessableEntityException("Base change not found: " + input.baseChange);
}
PatchSet ps = psUtil.current(db.get(), ctl.getNotes());
PatchSet ps = psUtil.current(db.get(), change);
parentCommit = ObjectId.fromString(ps.getRevision().get());
groups = ps.getGroups();
} else {

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

@@ -172,7 +172,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
// Prepopulate the change exists with proper noteDbState field.
change = newNoteDbOnlyChange(project, changeId);
} else {
checkArgument(change != null, "change %s not found in ReviewDb", changeId);
checkNotNull(change, "change %s not found in ReviewDb", changeId);
checkArgument(
change.getProject().equals(project),
"passed project %s when creating ChangeNotes for %s, but actual project is %s",

53
gerrit-sshd/src/main/java/com/google/gerrit/sshd/ChangeArgumentParser.java
View File

@@ -14,9 +14,8 @@
package com.google.gerrit.sshd;
import static java.util.stream.Collectors.toList;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -30,7 +29,6 @@ import com.google.gerrit.server.permissions.GlobalPermission;
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.ProjectControl;
import com.google.gerrit.sshd.BaseCommand.UnloggedFailure;
import com.google.gwtorm.server.OrmException;
@@ -39,7 +37,6 @@ import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.Optional;
public class ChangeArgumentParser {
private final CurrentUser currentUser;
@@ -85,9 +82,8 @@ public class ChangeArgumentParser {
ProjectControl projectControl,
boolean useIndex)
throws UnloggedFailure, OrmException, PermissionBackendException {
List<ChangeControl> matched =
useIndex ? changeFinder.find(id, currentUser) : changeFromNotesFactory(id, currentUser);
List<ChangeControl> toAdd = new ArrayList<>(changes.size());
List<ChangeNotes> matched = useIndex ? changeFinder.find(id) : changeFromNotesFactory(id);
List<ChangeNotes> toAdd = new ArrayList<>(changes.size());
boolean canMaintainServer;
try {
permissionBackend.user(currentUser).check(GlobalPermission.MAINTAIN_SERVER);
@@ -95,16 +91,16 @@ public class ChangeArgumentParser {
} catch (AuthException | PermissionBackendException e) {
canMaintainServer = false;
}
for (ChangeControl ctl : matched) {
if (!changes.containsKey(ctl.getId())
&& inProject(projectControl, ctl.getProject())
for (ChangeNotes notes : matched) {
if (!changes.containsKey(notes.getChangeId())
&& inProject(projectControl, notes.getProjectName())
&& (canMaintainServer
|| permissionBackend
.user(currentUser)
.change(ctl.getNotes())
.change(notes)
.database(db)
.test(ChangePermission.READ))) {
toAdd.add(ctl);
toAdd.add(notes);
}
}
@@ -113,19 +109,18 @@ public class ChangeArgumentParser {
} else if (toAdd.size() > 1) {
throw new UnloggedFailure(1, "\"" + id + "\" matches multiple changes");
}
ChangeControl ctl = toAdd.get(0);
changes.put(ctl.getId(), changesCollection.parse(ctl));
Change.Id cId = toAdd.get(0).getChangeId();
ChangeResource changeResource;
try {
changeResource = changesCollection.parse(cId);
} catch (ResourceNotFoundException e) {
throw new UnloggedFailure(1, "\"" + id + "\" no such change");
}
changes.put(cId, changeResource);
}
private List<ChangeControl> changeFromNotesFactory(String id, CurrentUser currentUser)
throws OrmException, UnloggedFailure {
return changeNotesFactory
.create(db, parseId(id))
.stream()
.map(changeNote -> controlForChange(changeNote, currentUser))
.filter(changeControl -> changeControl.isPresent())
.map(changeControl -> changeControl.get())
.collect(toList());
private List<ChangeNotes> changeFromNotesFactory(String id) throws OrmException, UnloggedFailure {
return changeNotesFactory.create(db, parseId(id));
}
private List<Change.Id> parseId(String id) throws UnloggedFailure {
@@ -136,17 +131,9 @@ public class ChangeArgumentParser {
}
}
private Optional<ChangeControl> controlForChange(ChangeNotes change, CurrentUser user) {
try {
return Optional.of(changeControlFactory.controlFor(change, user));
} catch (NoSuchChangeException e) {
return Optional.empty();
}
}
private boolean inProject(ProjectControl projectControl, Project project) {
private boolean inProject(ProjectControl projectControl, Project.NameKey project) {
if (projectControl != null) {
return projectControl.getProject().getNameKey().equals(project.getNameKey());
return projectControl.getProject().getNameKey().equals(project);
}
// No --project option, so they want every project.

11
gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/PatchSetParser.java
View File

@@ -21,10 +21,8 @@ import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ChangeFinder;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.query.change.ChangeData;
@@ -44,7 +42,6 @@ public class PatchSetParser {
private final ChangeNotes.Factory notesFactory;
private final PatchSetUtil psUtil;
private final ChangeFinder changeFinder;
private final Provider<CurrentUser> self;
@Inject
PatchSetParser(
@@ -52,14 +49,12 @@ public class PatchSetParser {
Provider<InternalChangeQuery> queryProvider,
ChangeNotes.Factory notesFactory,
PatchSetUtil psUtil,
ChangeFinder changeFinder,
Provider<CurrentUser> self) {
ChangeFinder changeFinder) {
this.db = db;
this.queryProvider = queryProvider;
this.notesFactory = notesFactory;
this.psUtil = psUtil;
this.changeFinder = changeFinder;
this.self = self;
}
public PatchSet parsePatchSet(String token, ProjectControl projectControl, String branch)
@@ -141,8 +136,8 @@ public class PatchSetParser {
return notesFactory.create(db.get(), projectControl.getProject().getNameKey(), changeId);
}
try {
ChangeControl ctl = changeFinder.findOne(changeId, self.get());
return notesFactory.create(db.get(), ctl.getProject().getNameKey(), changeId);
ChangeNotes notes = changeFinder.findOne(changeId);
return notesFactory.create(db.get(), notes.getProjectName(), changeId);
} catch (NoSuchChangeException e) {
throw error("\"" + changeId + "\" no such change");
}