opendev/gerrit
Code Issues Proposed changes

Account.Id: Return Optional from parse method

Browse Source 
This is better than throwing a NumberFormatException as done before.
Callers can easily forget to handle NumberFormatException. It's better
to return an Optional so that it's obvious that callers must handle the
case where parsing didn't succeed.

Change-Id: I0bade70a31430674c739f7e1af51f71f22027c88
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2018-02-02 15:01:44 +01:00
parent 19d1baaa4f
commit fb42262089
7 changed files with 41 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
gerrit-gwtui/src/main/java/com/google/gerrit/client/Dispatcher.java
View File

@@ -107,6 +107,7 @@ import com.google.gwt.core.client.RunAsyncCallback;
import com.google.gwt.http.client.URL; import com.google.gwt.http.client.URL;
import com.google.gwtexpui.user.client.UserAgent; import com.google.gwtexpui.user.client.UserAgent;
import com.google.gwtorm.client.KeyUtil; import com.google.gwtorm.client.KeyUtil;
import java.util.Optional;
public class Dispatcher { public class Dispatcher {
public static String toPatch( public static String toPatch(
@@ -308,8 +309,9 @@ public class Dispatcher {
private static void dashboard(String token) { private static void dashboard(String token) {
String rest = skip(token); String rest = skip(token);
if (rest.matches("[0-9]+")) { Optional<Account.Id> accountId = Account.Id.tryParse(rest);
Gerrit.display(token, new AccountDashboardScreen(Account.Id.parse(rest))); if (accountId.isPresent()) {
Gerrit.display(token, new AccountDashboardScreen(accountId.get()));
return; return;
} }

9
java/com/google/gerrit/httpd/auth/become/BecomeAnyAccountLoginServlet.java
View File

@@ -229,14 +229,13 @@ class BecomeAnyAccountLoginServlet extends HttpServlet {
} }
private Optional<AuthResult> byAccountId(String idStr) { private Optional<AuthResult> byAccountId(String idStr) {
final Account.Id id; Optional<Account.Id> id = Account.Id.tryParse(idStr);
try { if (!id.isPresent()) {
id = Account.Id.parse(idStr);
} catch (NumberFormatException nfe) {
return Optional.empty(); return Optional.empty();
} }
try { try {
return auth(accounts.get(id)); return auth(accounts.get(id.get()));
} catch (IOException | ConfigInvalidException e) { } catch (IOException | ConfigInvalidException e) {
getServletContext().log("cannot query database", e); getServletContext().log("cannot query database", e);
return Optional.empty(); return Optional.empty();

11
java/com/google/gerrit/reviewdb/client/Account.java
View File

@@ -22,6 +22,7 @@ import com.google.gerrit.extensions.client.DiffPreferencesInfo;
import com.google.gwtorm.client.Column; import com.google.gwtorm.client.Column;
import com.google.gwtorm.client.IntKey; import com.google.gwtorm.client.IntKey;
import java.sql.Timestamp; import java.sql.Timestamp;
import java.util.Optional;
/** /**
* Information about a single user. * Information about a single user.
@@ -94,10 +95,12 @@ public final class Account {
} }
/** Parse an Account.Id out of a string representation. */ /** Parse an Account.Id out of a string representation. */
public static Id parse(String str) { public static Optional<Id> tryParse(String str) {
Id r = new Id(); try {
r.fromString(str); return Optional.of(new Id(Integer.parseInt(str)));
return r; } catch (NumberFormatException e) {
return Optional.empty();
}
} }
public static Id fromRef(String name) { public static Id fromRef(String name) {

20
java/com/google/gerrit/server/account/AccountResolver.java
View File

@@ -98,17 +98,21 @@ public class AccountResolver {
throws OrmException, IOException, ConfigInvalidException { throws OrmException, IOException, ConfigInvalidException {
Matcher m = Pattern.compile("^.* \\(([1-9][0-9]*)\\)$").matcher(nameOrEmail); Matcher m = Pattern.compile("^.* \\(([1-9][0-9]*)\\)$").matcher(nameOrEmail);
if (m.matches()) { if (m.matches()) {
Account.Id id = Account.Id.parse(m.group(1)); Optional<Account.Id> id = Account.Id.tryParse(m.group(1));
return Streams.stream(accounts.get(id)) if (id.isPresent()) {
.map(a -> a.getAccount().getId()) return Streams.stream(accounts.get(id.get()))
.collect(toImmutableSet()); .map(a -> a.getAccount().getId())
.collect(toImmutableSet());
}
} }
if (nameOrEmail.matches("^[1-9][0-9]*$")) { if (nameOrEmail.matches("^[1-9][0-9]*$")) {
Account.Id id = Account.Id.parse(nameOrEmail); Optional<Account.Id> id = Account.Id.tryParse(nameOrEmail);
return Streams.stream(accounts.get(id)) if (id.isPresent()) {
.map(a -> a.getAccount().getId()) return Streams.stream(accounts.get(id.get()))
.collect(toImmutableSet()); .map(a -> a.getAccount().getId())
.collect(toImmutableSet());
}
} }
if (nameOrEmail.matches(Account.USER_NAME_PATTERN)) { if (nameOrEmail.matches(Account.USER_NAME_PATTERN)) {

12
java/com/google/gerrit/server/index/change/ChangeField.java
View File

@@ -28,7 +28,6 @@ import static java.util.stream.Collectors.toSet;
import com.google.common.annotations.VisibleForTesting; import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Enums; import com.google.common.base.Enums;
import com.google.common.base.Optional;
import com.google.common.base.Splitter; import com.google.common.base.Splitter;
import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSet;
@@ -77,6 +76,7 @@ import java.util.HashSet;
import java.util.List; import java.util.List;
import java.util.Locale; import java.util.Locale;
import java.util.Objects; import java.util.Objects;
import java.util.Optional;
import java.util.Set; import java.util.Set;
import java.util.function.Function; import java.util.function.Function;
import java.util.stream.Stream; import java.util.stream.Stream;
@@ -273,15 +273,15 @@ public class ChangeField {
continue; continue;
} }
Optional<ReviewerStateInternal> reviewerState = com.google.common.base.Optional<ReviewerStateInternal> reviewerState =
Enums.getIfPresent(ReviewerStateInternal.class, v.substring(0, i)); Enums.getIfPresent(ReviewerStateInternal.class, v.substring(0, i));
if (!reviewerState.isPresent()) { if (!reviewerState.isPresent()) {
log.error("Failed to parse reviewer state from reviewer field: %s", v); log.error("Failed to parse reviewer state from reviewer field: %s", v);
continue; continue;
} }
Account.Id accountId = Account.Id.parse(v.substring(i + 1, i2)); Optional<Account.Id> accountId = Account.Id.tryParse(v.substring(i + 1, i2));
if (accountId == null) { if (!accountId.isPresent()) {
log.error("Failed to parse account ID from reviewer field: %s", v); log.error("Failed to parse account ID from reviewer field: %s", v);
continue; continue;
} }
@@ -293,7 +293,7 @@ public class ChangeField {
} }
Timestamp timestamp = new Timestamp(l); Timestamp timestamp = new Timestamp(l);
b.put(reviewerState.get(), accountId, timestamp); b.put(reviewerState.get(), accountId.get(), timestamp);
} }
return ReviewerSet.fromTable(b.build()); return ReviewerSet.fromTable(b.build());
} }
@@ -313,7 +313,7 @@ public class ChangeField {
continue; continue;
} }
Optional<ReviewerStateInternal> reviewerState = com.google.common.base.Optional<ReviewerStateInternal> reviewerState =
Enums.getIfPresent(ReviewerStateInternal.class, v.substring(0, i)); Enums.getIfPresent(ReviewerStateInternal.class, v.substring(0, i));
if (!reviewerState.isPresent()) { if (!reviewerState.isPresent()) {
log.error("Failed to parse reviewer state from reviewer by email field: %s", v); log.error("Failed to parse reviewer state from reviewer by email field: %s", v);

9
java/com/google/gerrit/server/mail/SignedTokenEmailTokenVerifier.java
View File

@@ -78,14 +78,7 @@ public class SignedTokenEmailTokenVerifier implements EmailTokenVerifier {
if (!matcher.matches()) { if (!matcher.matches()) {
throw new InvalidTokenException(); throw new InvalidTokenException();
} }
Account.Id id = Account.Id.tryParse(matcher.group(1)).orElseThrow(InvalidTokenException::new);
Account.Id id;
try {
id = Account.Id.parse(matcher.group(1));
} catch (IllegalArgumentException err) {
throw new InvalidTokenException(err);
}
String newEmail = matcher.group(2); String newEmail = matcher.group(2);
return new ParsedToken(id, newEmail); return new ParsedToken(id, newEmail);
} }

8
java/com/google/gerrit/server/notedb/NoteDbChangeState.java
View File

@@ -127,7 +127,13 @@ public class NoteDbChangeState {
List<String> draftParts = s.splitToList(p); List<String> draftParts = s.splitToList(p);
checkArgument( checkArgument(
draftParts.size() == 2, "invalid draft state part for change %s: %s", changeId, p); draftParts.size() == 2, "invalid draft state part for change %s: %s", changeId, p);
draftIds.put(Account.Id.parse(draftParts.get(0)), ObjectId.fromString(draftParts.get(1))); Optional<Account.Id> accountId = Account.Id.tryParse(draftParts.get(0));
checkArgument(
accountId.isPresent(),
"invalid account ID in draft state part for change %s: %s",
changeId,
p);
draftIds.put(accountId.get(), ObjectId.fromString(draftParts.get(1)));
} }
return Optional.of(create(changeMetaId, draftIds)); return Optional.of(create(changeMetaId, draftIds));
} }