opendev/gerrit
Code Issues Proposed changes

Fix reading changes from index for older schema versions

Browse Source 
Prior to index v27, there is no project field in the index, so we
can't count on the field being present when we need to create a
ChangeData from it. Detect this special case and call a new (well,
reborn) ChangeData.Factory method with an intentionally ugly name.
This sets the project to null initially, and triggers a lazy read from
Changes when looking up the project. In addition to the ugly name,
check that notedb is not enabled; this is a safe assumption, since the
whole point of v27 was to make it possible to read from notedb without
reading from the Changes table.

Change-Id: Id425b1576bcbe40dee5e77d73d0388705031878f
This commit is contained in:
Dave Borowitz
2016-02-10 12:42:09 -05:00
parent 59c765652e
commit 72825f0f69
4 changed files with 105 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -381,7 +381,7 @@ public class ChangeJson {
// If any problems were fixed, the ChangeData needs to be reloaded.
for (ProblemInfo p : out.problems) {
if (p.status == ProblemInfo.Status.FIXED) {
cd = changeDataFactory.create(cd.db(), cd.getProject(), cd.getId());
cd = changeDataFactory.create(cd.db(), cd.project(), cd.getId());
break;
}
}

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

@@ -15,6 +15,7 @@
package com.google.gerrit.server.notedb;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.server.notedb.ChangeNoteUtil.GERRIT_PLACEHOLDER_HOST;
import com.google.common.annotations.VisibleForTesting;
@@ -134,6 +135,16 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
return new ChangeNotes(repoManager, migration, allUsersProvider,
change.getProject(), change).load();
}
// TODO(dborowitz): Remove when deleting index schemas <27.
public ChangeNotes createFromIdOnlyWhenNotedbDisabled(
ReviewDb db, Change.Id changeId) throws OrmException {
checkState(!migration.readChanges(), "do not call"
+ " createFromIdOnlyWhenNotedbDisabled when notedb is enabled");
Change change = db.changes().get(changeId);
return new ChangeNotes(repoManager, migration, allUsersProvider,
change.getProject(), change).load();
}
}
private final Project.NameKey project;

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

@@ -14,6 +14,7 @@
package com.google.gerrit.server.query.change;
import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.server.ApprovalsUtil.sortApprovals;
import com.google.auto.value.AutoValue;
@@ -275,6 +276,9 @@ public class ChangeData {
ChangeData create(ReviewDb db, Project.NameKey project, Change.Id id);
ChangeData create(ReviewDb db, Change c);
ChangeData create(ReviewDb db, ChangeControl c);
// TODO(dborowitz): Remove when deleting index schemas <27.
ChangeData createOnlyWhenNotedbDisabled(ReviewDb db, Change.Id id);
}
/**
@@ -309,8 +313,8 @@ public class ChangeData {
private final NotesMigration notesMigration;
private final MergeabilityCache mergeabilityCache;
private final Change.Id legacyId;
private final Project.NameKey project;
private ChangeDataSource returnedBySource;
private Project.NameKey project;
private Change change;
private ChangeNotes notes;
private String commitMessage;
@@ -444,6 +448,43 @@ public class ChangeData {
project = notes.getProjectName();
}
@AssistedInject
private ChangeData(
GitRepositoryManager repoManager,
ChangeControl.GenericFactory changeControlFactory,
IdentifiedUser.GenericFactory userFactory,
ProjectCache projectCache,
MergeUtil.Factory mergeUtilFactory,
ChangeNotes.Factory notesFactory,
ApprovalsUtil approvalsUtil,
ChangeMessagesUtil cmUtil,
PatchLineCommentsUtil plcUtil,
PatchSetUtil psUtil,
PatchListCache patchListCache,
NotesMigration notesMigration,
MergeabilityCache mergeabilityCache,
@Assisted ReviewDb db,
@Assisted Change.Id id) {
checkState(!notesMigration.readChanges(),
"do not call createOnlyWhenNotedbDisabled when notedb is enabled");
this.db = db;
this.repoManager = repoManager;
this.changeControlFactory = changeControlFactory;
this.userFactory = userFactory;
this.projectCache = projectCache;
this.mergeUtilFactory = mergeUtilFactory;
this.notesFactory = notesFactory;
this.approvalsUtil = approvalsUtil;
this.cmUtil = cmUtil;
this.plcUtil = plcUtil;
this.psUtil = psUtil;
this.patchListCache = patchListCache;
this.notesMigration = notesMigration;
this.mergeabilityCache = mergeabilityCache;
this.legacyId = id;
this.project = null;
}
public ReviewDb db() {
return db;
}
@@ -545,7 +586,12 @@ public class ChangeData {
return legacyId;
}
public Project.NameKey getProject() {
public Project.NameKey project() throws OrmException {
if (project == null) {
checkState(!notesMigration.readChanges(), "should not have created "
+ " ChangeData without a project when notedb is enabled");
project = change().getProject();
}
return project;
}
@@ -580,7 +626,7 @@ public class ChangeData {
changeControl = changeControlFactory.controlFor(db, change, user);
} else {
changeControl =
changeControlFactory.controlFor(db, project, legacyId, user);
changeControlFactory.controlFor(db, project(), legacyId, user);
}
} catch (NoSuchChangeException e) {
throw new OrmException(e);
@@ -605,7 +651,11 @@ public class ChangeData {
}
public Change reloadChange() throws OrmException {
notes = notesFactory.create(db, project, legacyId);
if (project == null) {
notes = notesFactory.createFromIdOnlyWhenNotedbDisabled(db, legacyId);
} else {
notes = notesFactory.create(db, project, legacyId);
}
change = notes.getChange();
if (change == null) {
throw new OrmException("Unable to load change " + legacyId);
@@ -615,7 +665,7 @@ public class ChangeData {
public ChangeNotes notes() throws OrmException {
if (notes == null) {
notes = notesFactory.create(db, project, legacyId);
notes = notesFactory.create(db, project(), legacyId);
}
return notes;
}
@@ -698,7 +748,7 @@ public class ChangeData {
return false;
}
String sha1 = ps.getRevision().get();
try (Repository repo = repoManager.openRepository(project);
try (Repository repo = repoManager.openRepository(project());
RevWalk walk = new RevWalk(repo)) {
RevCommit c = walk.parseCommit(ObjectId.fromString(sha1));
commitMessage = c.getFullMessage();
@@ -821,7 +871,7 @@ public class ChangeData {
if (ps == null || !changeControl().isPatchVisible(ps, db)) {
return null;
}
try (Repository repo = repoManager.openRepository(project)) {
try (Repository repo = repoManager.openRepository(project())) {
Ref ref = repo.getRefDatabase().exactRef(c.getDest().get());
SubmitTypeRecord str = submitTypeRecord();
if (!str.isOk()) {
@@ -830,7 +880,7 @@ public class ChangeData {
return false;
}
String mergeStrategy = mergeUtilFactory
.create(projectCache.get(project))
.create(projectCache.get(project()))
.mergeStrategyName();
mergeable = mergeabilityCache.get(
ObjectId.fromString(ps.getRevision().get()),
@@ -851,7 +901,7 @@ public class ChangeData {
}
editsByUser = new HashSet<>();
Change.Id id = change.getId();
try (Repository repo = repoManager.openRepository(project)) {
try (Repository repo = repoManager.openRepository(project())) {
for (String ref
: repo.getRefDatabase().getRefs(RefNames.REFS_USERS).keySet()) {
if (Change.Id.fromEditRefPart(ref).equals(id)) {