ctubbsii commented on code in PR #5465:
URL: https://github.com/apache/accumulo/pull/5465#discussion_r2036310072
##########
core/src/main/java/org/apache/accumulo/core/data/LoadPlan.java:
##########
@@ -325,15 +329,47 @@ public String toJson() {
return gson.toJson(new JsonAll(destinations));
}
+ private static final Set<String> EXPECTED_FIELDS = Arrays
+
.stream(JsonAll.class.getDeclaredFields()).map(Field::getName).collect(Collectors.toSet());
+ private static final Set<String> EXPECTED_DEST_FIELDS =
+
Arrays.stream(JsonDestination.class.getDeclaredFields()).map(Field::getName)
+ .collect(Collectors.toSet());
+
/**
* Deserializes json to a load plan.
*
* @param json produced by {@link #toJson()}
+ * @throws IllegalArgumentException when illegal json is given or legal json
that does not follow
+ * the load plan schema is given. The minimal acceptable json is
+ * {@code {"destinations":[]}}.
*/
public static LoadPlan fromJson(String json) {
- var dests = gson.fromJson(json, JsonAll.class).destinations.stream()
-
.map(JsonDestination::toDestination).collect(Collectors.toUnmodifiableList());
- return new LoadPlan(dests);
+ try {
+ // https://github.com/google/gson/issues/188 Gson does not support
failing when extra fields
+ // are present. This is custom code to check for extra fields.
+ var jsonObj = gson.fromJson(json, JsonObject.class);
+ Preconditions.checkArgument(jsonObj != null, "Empty json is not
accepted.");
+ Preconditions.checkArgument(jsonObj.keySet().equals(EXPECTED_FIELDS),
+ "Expected fields %s and saw %s", EXPECTED_FIELDS, jsonObj.keySet());
+ Preconditions.checkArgument(jsonObj.get("destinations") instanceof
JsonArray,
+ "Expected value of destinations field to be array");
+ var destinations = jsonObj.getAsJsonArray("destinations");
+ destinations.forEach(dest -> {
+ var keySet = dest.getAsJsonObject().keySet();
+ Preconditions.checkArgument(keySet.equals(EXPECTED_DEST_FIELDS),
+ "Expected fields %s and saw %s", EXPECTED_DEST_FIELDS, keySet);
+ });
+
+ var dests = gson.fromJson(json, JsonAll.class).destinations.stream()
+
.map(JsonDestination::toDestination).collect(Collectors.toUnmodifiableList());
+ return new LoadPlan(dests);
+ } catch (IllegalArgumentException e) {
Review Comment:
I think this one is also okay to just let through.
```suggestion
} catch (NullPointerException | IllegalArgumentException e) {
```
##########
core/src/main/java/org/apache/accumulo/core/data/LoadPlan.java:
##########
@@ -325,15 +329,47 @@ public String toJson() {
return gson.toJson(new JsonAll(destinations));
}
+ private static final Set<String> EXPECTED_FIELDS = Arrays
+
.stream(JsonAll.class.getDeclaredFields()).map(Field::getName).collect(Collectors.toSet());
+ private static final Set<String> EXPECTED_DEST_FIELDS =
+
Arrays.stream(JsonDestination.class.getDeclaredFields()).map(Field::getName)
+ .collect(Collectors.toSet());
+
/**
* Deserializes json to a load plan.
*
* @param json produced by {@link #toJson()}
+ * @throws IllegalArgumentException when illegal json is given or legal json
that does not follow
+ * the load plan schema is given. The minimal acceptable json is
+ * {@code {"destinations":[]}}.
*/
public static LoadPlan fromJson(String json) {
- var dests = gson.fromJson(json, JsonAll.class).destinations.stream()
-
.map(JsonDestination::toDestination).collect(Collectors.toUnmodifiableList());
- return new LoadPlan(dests);
+ try {
+ // https://github.com/google/gson/issues/188 Gson does not support
failing when extra fields
+ // are present. This is custom code to check for extra fields.
+ var jsonObj = gson.fromJson(json, JsonObject.class);
+ Preconditions.checkArgument(jsonObj != null, "Empty json is not
accepted.");
Review Comment:
Objects.requireNonNull would probably be a more standard way to check
nullness. I think that it's okay to treat null args with NPE rather than IAE. I
don't think we need to specifically throw an IAE in these cases. If we throw
NPE instead, nobody's going to confused by the fact that it's not an IAE.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
