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

MissingNode.toString() returns null (4 character token) instead of empty string #2566

Closed
arakelian opened this issue Dec 9, 2019 · 4 comments
Milestone

Comments

@arakelian
Copy link

Recently upgraded from 2.9 to 2.10.1, and several internal unit tests failed. Determine that cause of failure was toString() method was removed from MissingNode.

@arakelian
Copy link
Author

arakelian commented Dec 9, 2019

Just read blog post https://medium.com/@cowtowncoder/jackson-2-10-feature-jsonnode-improvements-18894c3ac3b5, which said: "Although users sometimes used toString() implementation of JsonNode values, this was actually not supported usage by Jackson.".

I'll close ticket, because I assume this change was by design, and I'm supposed to use asText(). My only input is that I wish the major version was bumped with breaking changes like this, Apache style. Thanks for an excellent library.

@arakelian arakelian reopened this Dec 9, 2019
@arakelian
Copy link
Author

I changed my mind on this one, after looking at the code before/after. Open to your thoughts, @cowtowncoder.

Here's the new code in 2.10 (see BaseJsonNode):

   @Override
   public String toString() {
       return InternalNodeMapper.nodeToString(this);
   }

   @Override
   public String toPrettyString() {
       return InternalNodeMapper.nodeToPrettyString(this);
   }

I read this code as saying, "if you want a pretty printed rendering of the Json, use toPrettyString(), or, for non-pretty version, using toString()".

And looking at the toString() method for MissingNode in 2.9, we see:

@Override
public String toString() {
    // toString() should never return null
    return "";
}

As a user of version 2.9 of the library, I think it's reasonable to believe that MissingNode would continue to return an empty string in version 2.x of the library (e.g. 2.10+) on call to toString().

That's also what I got from your comment this ticket (at the bottom): #25. You wrote: "Anyone reading this issue: if you would prefer to see a String value (most likely either "" or "null"), please file a new issue with the request for change. Such could be considered for Jackson 3.0."

Personally, I like the current behavior. I think returning "null" for a MissingNode.toString() is counter-intuitive. A NullNode represents a "null" value. MissingNode represents something entirely different -- it's really just a placeholder, sort of like java.lang.Optional, to avoid null checks everywhere.

Thanks for your consideration.

@cowtowncoder
Copy link
Member

Hmmh. Tricky question, this one. This was not an intentional functional change, although I am unsure how to categorize this since behavior of toString() for JsonNode was never specified as anything that users should rely on, aside from diagnostics output.

My first thought is that the ideal outcome of attempts to serialize MissingNode would perhaps be throwing IllegalArgumentException (or UnsupportedOperationException), since it does not represent any legal value, but absence of one. Sort of like Java 8 Optional.

But given that there is existing behavior that is not completely unreasonable -- output of "" -- that does sound like it should either have been continued as such, or at very least should not have become same as NullNode.

I will have to think about this a bit, but I am leaning towards your suggestion of reverting the change for 2.10.2.

@cowtowncoder
Copy link
Member

Ok so for what it is worth, the way things work in 2.10 (before I change it) is:

  1. Instead of explicit toString(), actual serialization methods are used (serialize())
  2. Since serialization these methods must produce valid JSON (so MissingNode can not simply not output anything without potentially causing exception), MissingNode chose to write VALUE_NULL

Interestingly enough there is actually a test (TestMissingNode) that suggests null is expected, to keep behavior same as if explicitly serializing as json.

@cowtowncoder cowtowncoder changed the title MissingNode.toString returns "null" (4 character string) instead of empty string MissingNode.toString() returns null (4 character token) instead of empty string Dec 11, 2019
@cowtowncoder cowtowncoder added this to the 2.10.2 milestone Dec 11, 2019
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