Change.Id: Introduce tryParse method
The existing parse method uses checkArgument to check that the given
string represents a valid Change.Id. When this fails it results in an
IllegalStateException which callers must handle.
Introduce a new tryParse method which instead returns an Optional
which is empty if the string is not a valid Change.Id.
Refactor existing callers to use it, and deprecate the existing one.
Change-Id: Ibb9b27b5556f0e5abb0287be62640bf895e94fe1
diff --git a/java/com/google/gerrit/entities/Change.java b/java/com/google/gerrit/entities/Change.java
index 739bd38..04e97dc 100644
--- a/java/com/google/gerrit/entities/Change.java
+++ b/java/com/google/gerrit/entities/Change.java
@@ -26,6 +26,7 @@
import java.security.SecureRandom;
import java.sql.Timestamp;
import java.util.Arrays;
+import java.util.Optional;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
@@ -116,13 +117,30 @@
@AutoValue
public abstract static class Id {
- /** Parse a Change.Id out of a string representation. */
+ /**
+ * Parse a Change.Id out of a string representation.
+ *
+ * @deprecated use {@link #tryParse(String)} instead.
+ */
+ @Deprecated
public static Id parse(String str) {
Integer id = Ints.tryParse(str);
checkArgument(id != null, "invalid change ID: %s", str);
return Change.id(id);
}
+ /**
+ * Parse a Change.Id out of a string representation.
+ *
+ * @param str the string to parse
+ * @return Optional containing the Change.Id, or {@code Optional.empty()} if str does not
+ * represent a valid Change.Id.
+ */
+ public static Optional<Id> tryParse(String str) {
+ Integer id = Ints.tryParse(str);
+ return id != null ? Optional.of(Change.id(id)) : Optional.empty();
+ }
+
public static Id fromRef(String ref) {
if (RefNames.isRefsEdit(ref)) {
return fromEditRefPart(ref);
diff --git a/java/com/google/gerrit/httpd/NumericChangeIdRedirectServlet.java b/java/com/google/gerrit/httpd/NumericChangeIdRedirectServlet.java
index 89ad878..77c5381 100644
--- a/java/com/google/gerrit/httpd/NumericChangeIdRedirectServlet.java
+++ b/java/com/google/gerrit/httpd/NumericChangeIdRedirectServlet.java
@@ -24,6 +24,7 @@
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.io.IOException;
+import java.util.Optional;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
@@ -46,17 +47,15 @@
if (idString.endsWith("/")) {
idString = idString.substring(0, idString.length() - 1);
}
- Change.Id id;
- try {
- id = Change.Id.parse(idString);
- } catch (IllegalArgumentException e) {
+ Optional<Change.Id> id = Change.Id.tryParse(idString);
+ if (!id.isPresent()) {
rsp.sendError(HttpServletResponse.SC_NOT_FOUND);
return;
}
ChangeResource changeResource;
try {
- changeResource = changesCollection.parse(id);
+ changeResource = changesCollection.parse(id.get());
} catch (ResourceConflictException | ResourceNotFoundException e) {
rsp.sendError(HttpServletResponse.SC_NOT_FOUND);
return;
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index 61b90f1..47f5ad6 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -1394,7 +1394,11 @@
private List<Change> parseChange(String value) throws QueryParseException {
if (PAT_LEGACY_ID.matcher(value).matches()) {
- return asChanges(args.queryProvider.get().byLegacyChangeId(Change.Id.parse(value)));
+ Optional<Change.Id> id = Change.Id.tryParse(value);
+ if (!id.isPresent()) {
+ throw error("Invalid change id " + value);
+ }
+ return asChanges(args.queryProvider.get().byLegacyChangeId(id.get()));
} else if (PAT_CHANGE_ID.matcher(value).matches()) {
List<Change> changes = asChanges(args.queryProvider.get().byKeyPrefix(parseChangeId(value)));
if (changes.isEmpty()) {