Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ArrayNode.addAll() adds raw null values which cause NPE on deepCopy() and toString() #2442

Closed
heshamMassoud opened this issue Sep 5, 2019 · 4 comments
Milestone

Comments

@heshamMassoud
Copy link
Contributor

heshamMassoud commented Sep 5, 2019

Version: 2.9.9

When I create an ArrayNode and use ArrayNode#addAll to add elements containing a raw null element as follows:

final List<ObjectNode> nodes = Arrays.asList(null, JsonNodeFactory.instance.objectNode());
final ArrayNode arrayNode = JsonNodeFactory.instance.arrayNode();
arrayNode.addAll(nodes);

Calling ArrayNode#deepCopy and ArrayNode#toString on such arrayNode will throw NullPointerExceptions.

However, if we add the nodes using ArrayNode#add as follows:

final ArrayNode arrayNode = JsonNodeFactory.instance.arrayNode();
arrayNode.add((JsonNode) null);
arrayNode.add(JsonNodeFactory.instance.objectNode());

Calling ArrayNode#deepCopy and ArrayNode#toString on such arrayNode will NOT throw any NullPointerExceptions.

This is because ArrayNode#add converts any raw null to a NullNode object:

public ArrayNode add(JsonNode value)
{
if (value == null) { // let's not store 'raw' nulls but nodes
value = nullNode();
}
_add(value);
return this;
}

But ArrayNode#addAll does not. But It should, otherwise how does one deepCopy an arrayNode that contains null raw elements?

@cowtowncoder
Copy link
Member

First of all: thank you for reporting the issue! Yes, that looks like a problem, although surprisingly one that has not been reported before. I agree that raw null is not to be added; and to me it seems there are 2 possibilities: either automatically "upgrade" null as is done in a few other places, or throw an exception to indicate invalid value.

It seems reasonable to expect former, so that nulls would be automatically coerced, given that this happens elsewhere.

I'll think about this bit more, and will get fixed for 2.10 to be included in 2.10.0.

heshamMassoud added a commit to heshamMassoud/jackson-databind that referenced this issue Sep 5, 2019
heshamMassoud added a commit to heshamMassoud/jackson-databind that referenced this issue Sep 5, 2019
@heshamMassoud
Copy link
Contributor Author

Hi @cowtowncoder, thanks for the quick reply. I also agree with you about the former being the more suitable solution to be consistent with the rest of the methods.

I attempted to provide a fix for it here: #2443

@cowtowncoder cowtowncoder added this to the 2.10.0 milestone Sep 6, 2019
@cowtowncoder cowtowncoder changed the title ArrayNode#addAll adds raw null values which cause NullPointerException on ArrayNode#deepCopy and ArrayNode#toString ArrayNode.addAll() adds raw null values which cause NPE on deepCopy() and toString() Sep 6, 2019
cowtowncoder added a commit that referenced this issue Sep 6, 2019
cowtowncoder added a commit that referenced this issue Sep 6, 2019
@heshamMassoud
Copy link
Contributor Author

any expected date when 2.10.0 will be released?

@cowtowncoder
Copy link
Member

@heshamMassoud Hoping to release before end of September, ideally within 2 weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants