opendev/gerrit
Code Issues Proposed changes

Eliminate potential NPEs with ChangeData.getChange()

Browse Source 
Callers were supposed to be calling hasChange() to determine whether
getChange() would return null, but none were. Remove this error-prone
pair of methods and just use change(), which includes lazy loading.
There are a few places where we need to catch an additional
OrmException but these are easy enough to fix.

Change-Id: I23335e362715f59e2c093ffec88427739ff0cffc
This commit is contained in:
Dave Borowitz 2014-01-03 09:03:24 -08:00
parent 784a5b30a0
commit 86d070f83e
10 changed files with 35 additions and 52 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
gerrit-lucene/src/main/java/com/google/gerrit/lucene/LuceneChangeIndex.java
View File

@ -234,7 +234,7 @@ public class LuceneChangeIndex implements ChangeIndex {
Term id = QueryBuilder.idTerm(cd);
Document doc = toDocument(cd);
try {
if (cd.getChange().getStatus().isOpen()) {
if (cd.change().getStatus().isOpen()) {
Futures.allAsList(
closedIndex.delete(id),
openIndex.insert(doc)).get();
@ -243,9 +243,7 @@ public class LuceneChangeIndex implements ChangeIndex {
openIndex.delete(id),
closedIndex.insert(doc)).get();
}
} catch (ExecutionException e) {
throw new IOException(e);
} catch (InterruptedException e) {
} catch (OrmException | ExecutionException | InterruptedException e) {
throw new IOException(e);
}
}
@ -256,7 +254,7 @@ public class LuceneChangeIndex implements ChangeIndex {
Term id = QueryBuilder.idTerm(cd);
Document doc = toDocument(cd);
try {
if (cd.getChange().getStatus().isOpen()) {
if (cd.change().getStatus().isOpen()) {
Futures.allAsList(
closedIndex.delete(id),
openIndex.replace(id, doc)).get();
@ -265,9 +263,7 @@ public class LuceneChangeIndex implements ChangeIndex {
openIndex.delete(id),
closedIndex.replace(id, doc)).get();
}
} catch (ExecutionException e) {
throw new IOException(e);
} catch (InterruptedException e) {
} catch (OrmException | ExecutionException | InterruptedException e) {
throw new IOException(e);
}
}
@ -280,9 +276,7 @@ public class LuceneChangeIndex implements ChangeIndex {
Futures.allAsList(
openIndex.delete(id),
closedIndex.delete(id)).get();
} catch (ExecutionException e) {
throw new IOException(e);
} catch (InterruptedException e) {
} catch (ExecutionException | InterruptedException e) {
throw new IOException(e);
}
}

19
gerrit-server/src/main/java/com/google/gerrit/rules/StoredValues.java
View File

@ -35,11 +35,11 @@ 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.google.gwtorm.server.OrmException;
import com.googlecode.prolog_cafe.lang.Prolog;
import com.googlecode.prolog_cafe.lang.SystemException;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
@ -52,10 +52,19 @@ public final class StoredValues {
public static final StoredValue<PatchSet> PATCH_SET = create(PatchSet.class);
public static final StoredValue<ChangeControl> CHANGE_CONTROL = create(ChangeControl.class);
public static Change getChange(Prolog engine) throws SystemException {
ChangeData cd = CHANGE_DATA.get(engine);
try {
return cd.change();
} catch (OrmException e) {
throw new SystemException("Cannot load change " + cd.getId());
}
}
public static final StoredValue<PatchSetInfo> PATCH_SET_INFO = new StoredValue<PatchSetInfo>() {
@Override
public PatchSetInfo createValue(Prolog engine) {
Change change = StoredValues.CHANGE_DATA.get(engine).getChange();
Change change = getChange(engine);
PatchSet ps = StoredValues.PATCH_SET.get(engine);
PrologEnvironment env = (PrologEnvironment) engine.control;
PatchSetInfoFactory patchInfoFactory =
@ -74,7 +83,7 @@ public final class StoredValues {
PrologEnvironment env = (PrologEnvironment) engine.control;
PatchSetInfo psInfo = StoredValues.PATCH_SET_INFO.get(engine);
PatchListCache plCache = env.getArgs().getPatchListCache();
Change change = StoredValues.CHANGE_DATA.get(engine).getChange();
Change change = getChange(engine);
Project.NameKey projectKey = change.getProject();
ObjectId a = null;
ObjectId b = ObjectId.fromString(psInfo.getRevId());
@ -95,13 +104,11 @@ public final class StoredValues {
public Repository createValue(Prolog engine) {
PrologEnvironment env = (PrologEnvironment) engine.control;
GitRepositoryManager gitMgr = env.getArgs().getGitRepositoryManager();
Change change = StoredValues.CHANGE_DATA.get(engine).getChange();
Change change = getChange(engine);
Project.NameKey projectKey = change.getProject();
final Repository repo;
try {
repo = gitMgr.openRepository(projectKey);
} catch (RepositoryNotFoundException e) {
throw new SystemException(e.getMessage());
} catch (IOException e) {
throw new SystemException(e.getMessage());
}

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

@ -384,7 +384,7 @@ public class ChangeJson {
}
LabelTypes labelTypes = ctl.getLabelTypes();
if (cd.getChange().getStatus().isOpen()) {
if (cd.change().getStatus().isOpen()) {
return labelsForOpenChange(cd, labelTypes, standard, detailed);
} else {
return labelsForClosedChange(cd, labelTypes, standard, detailed);

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

@ -295,14 +295,6 @@ public class ChangeData {
return legacyId;
}
public Change getChange() {
return change;
}
public boolean hasChange() {
return change != null;
}
boolean fastIsVisibleTo(CurrentUser user) {
return visibleTo == user;
}
@ -323,10 +315,6 @@ public class ChangeData {
return change;
}
void setChange(Change c) {
change = c;
}
public PatchSet currentPatchSet() throws OrmException {
if (currentPatchSet == null) {
Change c = change();
@ -385,7 +373,7 @@ public class ChangeData {
IncorrectObjectTypeException {
PatchSet.Id psId = change().currentPatchSetId();
String sha1 = db.patchSets().get(psId).getRevision().get();
Repository repo = repoManager.openRepository(change.getProject());
Repository repo = repoManager.openRepository(change().getProject());
try {
RevWalk walk = new RevWalk(repo);
try {

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

@ -308,8 +308,8 @@ public class QueryProcessor {
}
LabelTypes labelTypes = cc.getLabelTypes();
c = eventFactory.asChangeAttribute(d.getChange());
eventFactory.extend(c, d.getChange());
c = eventFactory.asChangeAttribute(d.change());
eventFactory.extend(c, d.change());
if (!trackingFooters.isEmpty()) {
eventFactory.addTrackingIds(c,
@ -317,11 +317,11 @@ public class QueryProcessor {
}
if (includeAllReviewers) {
eventFactory.addAllReviewers(c, d.getChange());
eventFactory.addAllReviewers(c, d.change());
}
if (includeSubmitRecords) {
PatchSet.Id psId = d.getChange().currentPatchSetId();
PatchSet.Id psId = d.change().currentPatchSetId();
PatchSet patchSet = db.get().patchSets().get(psId);
List<SubmitRecord> submitResult = cc.canSubmit( //
db.get(), patchSet, null, false, true, true);
@ -368,7 +368,7 @@ public class QueryProcessor {
}
if (includeDependencies) {
eventFactory.addDependencies(c, d.getChange());
eventFactory.addDependencies(c, d.change());
}
show(c);

4
gerrit-server/src/main/java/gerrit/PRED_change_branch_1.java
View File

@ -15,7 +15,6 @@
package gerrit;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.rules.StoredValues;
import com.googlecode.prolog_cafe.lang.Operation;
@ -36,8 +35,7 @@ public class PRED_change_branch_1 extends Predicate.P1 {
engine.setB0();
Term a1 = arg1.dereference();
Change change = StoredValues.CHANGE_DATA.get(engine).getChange();
Branch.NameKey name = change.getDest();
Branch.NameKey name = StoredValues.getChange(engine).getDest();
if (!a1.unify(SymbolTerm.create(name.get()), engine.trail)) {
return engine.fail();

4
gerrit-server/src/main/java/gerrit/PRED_change_owner_1.java
View File

@ -15,7 +15,6 @@
package gerrit;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.rules.StoredValues;
import com.googlecode.prolog_cafe.lang.IntegerTerm;
@ -40,8 +39,7 @@ public class PRED_change_owner_1 extends Predicate.P1 {
engine.setB0();
Term a1 = arg1.dereference();
Change change = StoredValues.CHANGE_DATA.get(engine).getChange();
Account.Id ownerId = change.getOwner();
Account.Id ownerId = StoredValues.getChange(engine).getOwner();
if (!a1.unify(new StructureTerm(user, new IntegerTerm(ownerId.get())), engine.trail)) {
return engine.fail();

4
gerrit-server/src/main/java/gerrit/PRED_change_project_1.java
View File

@ -14,7 +14,6 @@
package gerrit;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.rules.StoredValues;
@ -36,8 +35,7 @@ public class PRED_change_project_1 extends Predicate.P1 {
engine.setB0();
Term a1 = arg1.dereference();
Change change = StoredValues.CHANGE_DATA.get(engine).getChange();
Project.NameKey name = change.getProject();
Project.NameKey name = StoredValues.getChange(engine).getProject();
if (!a1.unify(SymbolTerm.create(name.get()), engine.trail)) {
return engine.fail();

2
gerrit-server/src/main/java/gerrit/PRED_change_topic_1.java
View File

@ -36,7 +36,7 @@ public class PRED_change_topic_1 extends Predicate.P1 {
Term a1 = arg1.dereference();
Term topicTerm = Prolog.Nil;
Change change = StoredValues.CHANGE_DATA.get(engine).getChange();
Change change = StoredValues.getChange(engine);
String topic = change.getTopic();
if (topic != null) {
topicTerm = SymbolTerm.create(topic);

12
gerrit-solr/src/main/java/com/google/gerrit/solr/SolrChangeIndex.java
View File

@ -148,14 +148,14 @@ class SolrChangeIndex implements ChangeIndex, LifecycleListener {
String id = cd.getId().toString();
SolrInputDocument doc = toDocument(cd);
try {
if (cd.getChange().getStatus().isOpen()) {
if (cd.change().getStatus().isOpen()) {
closedIndex.deleteById(id);
openIndex.add(doc);
} else {
openIndex.deleteById(id);
closedIndex.add(doc);
}
} catch (SolrServerException e) {
} catch (OrmException | SolrServerException e) {
throw new IOException(e);
}
commit(openIndex);
@ -167,14 +167,14 @@ class SolrChangeIndex implements ChangeIndex, LifecycleListener {
String id = cd.getId().toString();
SolrInputDocument doc = toDocument(cd);
try {
if (cd.getChange().getStatus().isOpen()) {
if (cd.change().getStatus().isOpen()) {
closedIndex.deleteById(id);
openIndex.add(doc);
} else {
openIndex.deleteById(id);
closedIndex.add(doc);
}
} catch (SolrServerException e) {
} catch (OrmException | SolrServerException e) {
throw new IOException(e);
}
commit(openIndex);
@ -185,14 +185,14 @@ class SolrChangeIndex implements ChangeIndex, LifecycleListener {
public void delete(ChangeData cd) throws IOException {
String id = cd.getId().toString();
try {
if (cd.getChange().getStatus().isOpen()) {
if (cd.change().getStatus().isOpen()) {
openIndex.deleteById(id);
commit(openIndex);
} else {
closedIndex.deleteById(id);
commit(closedIndex);
}
} catch (SolrServerException e) {
} catch (OrmException | SolrServerException e) {
throw new IOException(e);
}
}