Fix all ChangeFields to properly handle null values
Many ChangeData methods can return null values, typically indicating
a change is corrupt. Handle these gracefully within ChangeFields by
just not emitting the field values.
Fix a few ChangeData methods to return null that were previously
throwing exceptions, for consistency.
Testing this is hard. We get a little coverage for missing patch sets
by trying to reindex the changes in CheckIT; unfortunately this won't
work for testing a missing change.
Change-Id: Ia9b2722bb07d0025ca45631648ef060ff5b4f117
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index 96b737f..3049a98 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -42,6 +42,7 @@
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.MetaDataUpdate;
import com.google.gerrit.server.git.ProjectConfig;
+import com.google.gerrit.server.index.ChangeIndexer;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.Util;
import com.google.gerrit.testutil.ConfigSuite;
@@ -107,6 +108,9 @@
@Inject
protected GitRepositoryManager repoManager;
+ @Inject
+ protected ChangeIndexer indexer;
+
protected Git git;
protected GerritServer server;
protected TestAccount admin;
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/CheckIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/CheckIT.java
index db45e4c..ab6bc42 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/CheckIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/CheckIT.java
@@ -40,6 +40,7 @@
PushOneCommit.Result r = createChange();
Change c = getChange(r);
db.patchSets().deleteKeys(Collections.singleton(c.currentPatchSetId()));
+ indexer.index(db, c);
List<ProblemInfo> problems = gApi.changes()
.id(r.getChangeId())
@@ -65,6 +66,7 @@
Change c = getChange(r);
c.setStatus(Change.Status.NEW);
db.changes().update(Collections.singleton(c));
+ indexer.index(db, c);
ChangeInfo info = gApi.changes()
.id(r.getChangeId())
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java
index d8fb7db..9f90e37 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/ChangeField.java
@@ -14,9 +14,11 @@
package com.google.gerrit.server.index;
+import static com.google.common.base.MoreObjects.firstNonNull;
+
import com.google.common.base.Function;
-import com.google.common.base.MoreObjects;
import com.google.common.base.Splitter;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
@@ -27,7 +29,6 @@
import com.google.gerrit.reviewdb.client.PatchLineComment;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
-import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.ChangeData.ChangedLines;
import com.google.gerrit.server.query.change.ChangeQueryBuilder;
@@ -37,6 +38,8 @@
import com.google.gwtorm.server.OrmException;
import com.google.protobuf.CodedOutputStream;
+import org.eclipse.jgit.revwalk.FooterLine;
+
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.sql.Timestamp;
@@ -70,7 +73,11 @@
@Override
public String get(ChangeData input, FillArgs args)
throws OrmException {
- return input.change().getKey().get();
+ Change c = input.change();
+ if (c == null) {
+ return null;
+ }
+ return c.getKey().get();
}
};
@@ -81,8 +88,11 @@
@Override
public String get(ChangeData input, FillArgs args)
throws OrmException {
- return ChangeStatusPredicate.canonicalize(
- input.change().getStatus());
+ Change c = input.change();
+ if (c == null) {
+ return null;
+ }
+ return ChangeStatusPredicate.canonicalize(c.getStatus());
}
};
@@ -93,7 +103,11 @@
@Override
public String get(ChangeData input, FillArgs args)
throws OrmException {
- return input.change().getProject().get();
+ Change c = input.change();
+ if (c == null) {
+ return null;
+ }
+ return c.getProject().get();
}
};
@@ -104,7 +118,11 @@
@Override
public String get(ChangeData input, FillArgs args)
throws OrmException {
- return input.change().getProject().get();
+ Change c = input.change();
+ if (c == null) {
+ return null;
+ }
+ return c.getProject().get();
}
};
@@ -115,7 +133,11 @@
@Override
public String get(ChangeData input, FillArgs args)
throws OrmException {
- return input.change().getDest().get();
+ Change c = input.change();
+ if (c == null) {
+ return null;
+ }
+ return c.getDest().get();
}
};
@@ -126,7 +148,11 @@
@Override
public String get(ChangeData input, FillArgs args)
throws OrmException {
- return MoreObjects.firstNonNull(input.change().getTopic(), "");
+ Change c = input.change();
+ if (c == null) {
+ return null;
+ }
+ return firstNonNull(c.getTopic(), "");
}
};
@@ -137,7 +163,11 @@
@Override
public Timestamp get(ChangeData input, FillArgs args)
throws OrmException {
- return input.change().getLastUpdatedOn();
+ Change c = input.change();
+ if (c == null) {
+ return null;
+ }
+ return c.getLastUpdatedOn();
}
};
@@ -149,14 +179,19 @@
@Override
public Iterable<String> get(ChangeData input, FillArgs args)
throws OrmException {
- return input.currentFilePaths();
+ return firstNonNull(input.currentFilePaths(),
+ ImmutableList.<String> of());
}
};
public static Set<String> getFileParts(ChangeData cd) throws OrmException {
+ List<String> paths = cd.currentFilePaths();
+ if (paths == null) {
+ return ImmutableSet.of();
+ }
Splitter s = Splitter.on('/').omitEmptyStrings();
Set<String> r = Sets.newHashSet();
- for (String path : cd.currentFilePaths()) {
+ for (String path : paths) {
for (String part : s.split(path)) {
r.add(part);
}
@@ -201,7 +236,11 @@
@Override
public Integer get(ChangeData input, FillArgs args)
throws OrmException {
- return input.change().getOwner().get();
+ Change c = input.change();
+ if (c == null) {
+ return null;
+ }
+ return c.getOwner().get();
}
};
@@ -212,9 +251,12 @@
@Override
public Iterable<Integer> get(ChangeData input, FillArgs args)
throws OrmException {
+ Change c = input.change();
+ if (c == null) {
+ return null;
+ }
Set<Integer> r = Sets.newHashSet();
- if (!args.allowsDrafts &&
- input.change().getStatus() == Change.Status.DRAFT) {
+ if (!args.allowsDrafts && c.getStatus() == Change.Status.DRAFT) {
return r;
}
for (PatchSetApproval a : input.approvals().values()) {
@@ -249,9 +291,13 @@
public Iterable<String> get(ChangeData input, FillArgs args)
throws OrmException {
try {
- return Sets.newHashSet(args.trackingFooters.extract(
- input.commitFooters()).values());
- } catch (NoSuchChangeException | IOException e) {
+ List<FooterLine> footers = input.commitFooters();
+ if (footers == null) {
+ return null;
+ }
+ return Sets.newHashSet(
+ args.trackingFooters.extract(footers).values());
+ } catch (IOException e) {
throw new OrmException(e);
}
}
@@ -305,7 +351,11 @@
@Override
public byte[] get(ChangeData input, FieldDef.FillArgs args)
throws OrmException {
- return CODEC.encodeToByteArray(input.change());
+ Change c = input.change();
+ if (c == null) {
+ return null;
+ }
+ return CODEC.encodeToByteArray(c);
}
}
@@ -352,7 +402,7 @@
public String get(ChangeData input, FillArgs args) throws OrmException {
try {
return input.commitMessage();
- } catch (NoSuchChangeException | IOException e) {
+ } catch (IOException e) {
throw new OrmException(e);
}
}
@@ -384,7 +434,11 @@
@Override
public String get(ChangeData input, FillArgs args)
throws OrmException {
- return input.isMergeable() ? "1" : null;
+ Boolean m = input.isMergeable();
+ if (m == null) {
+ return null;
+ }
+ return m ? "1" : null;
}
};
@@ -395,7 +449,11 @@
@Override
public String get(ChangeData input, FillArgs args)
throws OrmException {
- return input.isMergeable() ? "1" : "0";
+ Boolean m = input.isMergeable();
+ if (m == null) {
+ return null;
+ }
+ return m ? "1" : "0";
}
};
@@ -406,7 +464,8 @@
@Override
public Integer get(ChangeData input, FillArgs args)
throws OrmException {
- return input.changedLines().insertions;
+ ChangedLines lines = input.changedLines();
+ return lines != null ? lines.insertions : null;
}
};
@@ -417,7 +476,8 @@
@Override
public Integer get(ChangeData input, FillArgs args)
throws OrmException {
- return input.changedLines().deletions;
+ ChangedLines lines = input.changedLines();
+ return lines != null ? lines.deletions : null;
}
};
@@ -428,8 +488,11 @@
@Override
public Integer get(ChangeData input, FillArgs args)
throws OrmException {
- ChangedLines changedLines = input.changedLines();
- return changedLines.insertions + changedLines.deletions;
+ ChangedLines lines = input.changedLines();
+ if (lines == null) {
+ return null;
+ }
+ return lines.insertions + lines.deletions;
}
};
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
index 0f1a7bb..dcb4924 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -473,29 +473,31 @@
currentApprovals = approvals;
}
- public String commitMessage() throws NoSuchChangeException, IOException,
- OrmException {
+ public String commitMessage() throws IOException, OrmException {
if (commitMessage == null) {
- loadCommitData();
+ if (!loadCommitData()) {
+ return null;
+ }
}
return commitMessage;
}
- public List<FooterLine> commitFooters() throws NoSuchChangeException,
- IOException, OrmException {
+ public List<FooterLine> commitFooters() throws IOException, OrmException {
if (commitFooters == null) {
- loadCommitData();
+ if (!loadCommitData()) {
+ return null;
+ }
}
return commitFooters;
}
- private void loadCommitData() throws NoSuchChangeException, OrmException,
+ private boolean loadCommitData() throws OrmException,
RepositoryNotFoundException, IOException, MissingObjectException,
IncorrectObjectTypeException {
PatchSet.Id psId = change().currentPatchSetId();
PatchSet ps = db.patchSets().get(psId);
if (ps == null) {
- throw new NoSuchChangeException(legacyId);
+ return false;
}
String sha1 = ps.getRevision().get();
Repository repo = repoManager.openRepository(change().getProject());
@@ -511,6 +513,7 @@
} finally {
repo.close();
}
+ return true;
}
/**
@@ -587,15 +590,18 @@
this.mergeable = mergeable;
}
- public boolean isMergeable() throws OrmException {
+ public Boolean isMergeable() throws OrmException {
if (mergeable == null) {
Change c = change();
+ if (c == null) {
+ return null;
+ }
if (c.getStatus() == Change.Status.MERGED) {
mergeable = true;
} else {
PatchSet ps = currentPatchSet();
if (ps == null) {
- throw new OrmException("Missing patch set for mergeability check");
+ return null;
}
Repository repo = null;
try {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java
index 8f33c01..52846ed 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java
@@ -24,7 +24,6 @@
import com.google.gerrit.server.data.QueryStatsAttribute;
import com.google.gerrit.server.events.EventFactory;
import com.google.gerrit.server.project.ChangeControl;
-import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.SubmitRuleEvaluator;
import com.google.gerrit.server.query.QueryParseException;
import com.google.gson.Gson;
@@ -266,11 +265,6 @@
ErrorMessage m = new ErrorMessage();
m.message = e.getMessage();
show(m);
- } catch (NoSuchChangeException e) {
- log.error("Missing change: " + e.getMessage(), e);
- ErrorMessage m = new ErrorMessage();
- m.message = "missing change " + e.getMessage();
- show(m);
}
} finally {
try {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/TrackingIdPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/TrackingIdPredicate.java
index bdf32fa..4f2a8d7 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/TrackingIdPredicate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/TrackingIdPredicate.java
@@ -18,13 +18,14 @@
import com.google.gerrit.server.config.TrackingFooters;
import com.google.gerrit.server.index.ChangeField;
import com.google.gerrit.server.index.IndexPredicate;
-import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gwtorm.server.OrmException;
+import org.eclipse.jgit.revwalk.FooterLine;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.io.IOException;
+import java.util.List;
class TrackingIdPredicate extends IndexPredicate<ChangeData> {
private static final Logger log = LoggerFactory.getLogger(TrackingIdPredicate.class);
@@ -41,9 +42,10 @@
Change c = object.change();
if (c != null) {
try {
- return trackingFooters.extract(object.commitFooters())
- .values().contains(getValue());
- } catch (NoSuchChangeException | IOException e) {
+ List<FooterLine> footers = object.commitFooters();
+ return footers != null && trackingFooters.extract(
+ object.commitFooters()).values().contains(getValue());
+ } catch (IOException e) {
log.warn("Cannot extract footers from " + c.getChangeId(), e);
}
}
