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

Deprecate JsonNode.asText(String) (note: reverted in 2.17.1) #4416

Closed
apeteri opened this issue Mar 5, 2024 · 6 comments
Closed

Deprecate JsonNode.asText(String) (note: reverted in 2.17.1) #4416

apeteri opened this issue Mar 5, 2024 · 6 comments
Labels
2.17 Issues planned at earliest for 2.17
Milestone

Comments

@apeteri
Copy link

apeteri commented Mar 5, 2024

NOTE: see #4471 for current state -- this was done for 2.17.0 release but reverted for 2.17.1


Describe the bug

It looks like the fallback value in JsonNode#asText(String) never gets a chance to appear as the 0-arg asText() should never return null, according to its Javadoc:

/**
* Method that will return a valid String representation of
* the container value, if the node is a value node
* (method {@link #isValueNode} returns true),
* otherwise empty String.
*/
public abstract String asText();
/**
* Method similar to {@link #asText()}, except that it will return
* <code>defaultValue</code> in cases where null value would be returned;
* either for missing nodes (trying to access missing property, or element
* at invalid item for array) or explicit nulls.
*/
public String asText(String defaultValue) {
String str = asText();
return (str == null) ? defaultValue : str;
}

This seems to be the case in all implementations of asText() I can see, and has been corroborated in other issues such as #25.

The only exception is if someone calls TextNode(String) directly with null as the input. TextNode#valueOf(String) already converts these cases into null references, so the "almost" in the title is quite strong.

Version Information

2.16.1

Expected behavior

Either replace the null check with an emptyness check (but then "legitimate empty" strings will also use the default value), or deprecate the method? Not quite sure!

@apeteri apeteri added the to-evaluate Issue that has been received but not yet evaluated label Mar 5, 2024
@apeteri apeteri changed the title Default value parameter JsonNode#asText(String) is almost* never used Default value parameter in JsonNode#asText(String) is almost* never used Mar 5, 2024
@apeteri apeteri changed the title Default value parameter in JsonNode#asText(String) is almost* never used Default value in JsonNode#asText(String) is almost* never used Mar 5, 2024
@cowtowncoder
Copy link
Member

Ok explanation makes sense: I'd vote for deprecation. We can do that for 2.17 -- would be happy to get a PR (with Javadoc explaining reasoning).

@cowtowncoder cowtowncoder changed the title Default value in JsonNode#asText(String) is almost* never used Deprecate JsonNode.asText(String) Mar 7, 2024
@cowtowncoder
Copy link
Member

Interestingly enough, this method behaves bit different than advertised: "defaultValue" will be returned for all MissingNodes and NullNodes EVEN THOUGH THEIR asText() does NOT return null... uh.

All the more reason to deprecate it, tho, remove from 3.0.

@cowtowncoder cowtowncoder added 2.17 Issues planned at earliest for 2.17 and removed to-evaluate Issue that has been received but not yet evaluated labels Mar 7, 2024
@cowtowncoder cowtowncoder added this to the 2.17.0 milestone Mar 7, 2024
@apeteri
Copy link
Author

apeteri commented Mar 7, 2024

I didn't get to that one apparently. 😄 Thank you for the fix!

@cowtowncoder
Copy link
Member

@apeteri no problem; not a big deal. Thank you for pointing this out.

@cowtowncoder
Copy link
Member

Looks like this should be reverted, based on feedback and better understanding on logic & updated description.

@cowtowncoder cowtowncoder changed the title Deprecate JsonNode.asText(String) Deprecate JsonNode.asText(String) (note: reverted in 2.17.1) Apr 27, 2024
@cowtowncoder
Copy link
Member

Reverted for 2.17.1 via #4471 .

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

No branches or pull requests

2 participants