gemini-code-assist[bot] commented on code in PR #38524:
URL: https://github.com/apache/beam/pull/38524#discussion_r3255389971
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/options/PipelineOptionsFactory.java:
##########
@@ -1797,15 +1797,35 @@ private static JsonDeserializer<Object>
getDeserializerForMethod(Method method)
}
static Object deserializeNode(JsonNode node, Method method) throws
IOException {
- if (node.isNull()) {
+ if (node == null || node.isNull() || node.isMissingNode()) {
return null;
}
+ if (node.isTextual() && node.asText().isEmpty()) {
+ return null;
+ }
+
+ JavaType targetType = MAPPER.constructType(method.getGenericReturnType());
+
+ // Portable pipeline options from protobuf Struct are usually
string-encoded scalars.
+ // convertValue coerces them reliably; TreeTraversingParser +
StdDeserializer can NPE on
+ // Jackson 2.18+ (e.g. LongDeserializer._parseLong when parsing string
values).
+ if (node.isValueNode()) {
+ return MAPPER.convertValue(node, targetType);
+ }
Review Comment:
This block introduces two potential issues:
1. **Empty String Regression**: Returning `null` for all empty textual nodes
(lines 1803-1805) will cause a regression for `String` type options, where `""`
is a valid value distinct from `null`. This check should be restricted to
non-string target types to avoid NPEs in numeric deserializers while preserving
empty string values for string options.
2. **Custom Deserializer Bypass**: The shortcut for value nodes (lines
1812-1814) bypasses custom deserializers defined via `@JsonDeserialize` on the
getter method. Since `MAPPER.convertValue` only knows about the `targetType`
and doesn't account for method-level annotations, any custom logic will be
ignored for value nodes (e.g., a string that should be parsed into a custom
object).
It is safer to move `targetType` initialization up, refine the empty string
check, and remove the value node shortcut, relying instead on the try-catch
block below to handle Jackson 2.18 NPEs while respecting custom deserializers.
```suggestion
JavaType targetType =
MAPPER.constructType(method.getGenericReturnType());
if (node.isTextual() && node.asText().isEmpty() &&
!targetType.isTypeOrSubTypeOf(String.class)) {
return null;
}
// Portable pipeline options from protobuf Struct are usually
string-encoded scalars.
// convertValue coerces them reliably; TreeTraversingParser +
StdDeserializer can NPE on
// Jackson 2.18+ (e.g. LongDeserializer._parseLong when parsing string
values).
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
