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

[Fix #381] Fixing null pointer #382

Open
wants to merge 2 commits into
base: develop/1.x
Choose a base branch
from

Conversation

fjtirado
Copy link
Contributor

@fjtirado fjtirado commented Nov 26, 2024

Fix #381

@fjtirado fjtirado force-pushed the Fix_#381 branch 2 times, most recently from 41b6aeb to 6184705 Compare November 26, 2024 19:17
@eiiches
Copy link
Owner

eiiches commented Nov 27, 2024

Thank you for reporting and submitting a patch!

This clearly fixes the issue for += case, but I'm not sure if this patch is enough to fix the NPE for other operators (such as |=) and other path-related functions (setpath, delpaths, etc.). So, I'm wondering if we should fix ObjectFieldPath#mutate() instead. Let me take a look again tomorrow.

@fjtirado
Copy link
Contributor Author

fjtirado commented Nov 27, 2024

Thank you for reporting and submitting a patch!

This clearly fixes the issue for += case, but I'm not sure if this patch is enough to fix the NPE for other operators (such as |=) and other path-related functions (setpath, delpaths, etc.). So, I'm wondering if we should fix ObjectFieldPath#mutate() instead. Let me take a look again tomorrow.

I filed a new commit that change the mutate. To be honest, I was lazy and did not want to search which mutate implementation I have to change. Now that you give me the clue, I have no excuse ;)

if (newval != null)
JsonNode newval = newobj.get(key);
newval = mutation.apply(newval != null ? newval : NullNode.getInstance());
if (newval != null && !newval.isNull())
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSON null (NullNode) is a valid value for object fields, so && !newval.isNull() is unnecessary.

$ jq -c '.foo += null' <<< '{}'
{"foo":null}

Other than that, the patch looks good 👍

Copy link
Contributor Author

@fjtirado fjtirado Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remove the isNull, a lot of test will fail (acutally it took me a while to figure out that). The reason is the same why you add the original null check, to prevent null values to be added to the object (you have some unit test to prevent that)

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

Successfully merging this pull request may close these issues.

NullPointerException when property is not present in context json
2 participants