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 for #162 (XML Empty tag to Empty string) #218

Merged
merged 1 commit into from
Jun 21, 2017

Conversation

oistein
Copy link

@oistein oistein commented Dec 22, 2016

Fix for issue #162.

@cowtowncoder cowtowncoder changed the title Fix issue #162 Fix for #162 (XML Empty tag to Empty string) Jan 6, 2017
@oistein
Copy link
Author

oistein commented Jan 23, 2017

Any updates on this?

@Mo-la-machette
Copy link

Hello everyone ! Is this pull request going to be merged soon? thank you

return null;
}

StringBuilder text = new StringBuilder();
Copy link
Member

Choose a reason for hiding this comment

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

Hmmh. Why? This was an optimization for the common case of just single segment, in which case construction of intermediate StringBuilder was avoided. It's probably not a big deal either way, but usually I prefer not fixing things that ain't broke.

Copy link
Member

Choose a reason for hiding this comment

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

Oh. Actually, I see. So, this was to get empty String over null.
I think I'll modify this bit differently, after first merging,.

@cowtowncoder
Copy link
Member

Ok, I am really torn here. The challenge here is this: fix changes low-level behavior, regardless of what target type might be. This means that it could change handling of empty/null cases for any number of other scalar types; here we'd prefer only changing String case (which I think should ALWAYS get to empty String). In addition, distinction between empty XML element and start/end should not have any significance, from XML perspective: logically the two are equivalent.

Still.. I think I'm ok for trying this out for 2.9.0, and see if it works better than before. I may be sorry (I've had a few similar-looking cases that burnt :) )... but we'll see.

@cowtowncoder cowtowncoder merged commit 2648916 into FasterXML:master Jun 21, 2017
@cowtowncoder
Copy link
Member

Ended up implementing #245 and #246 that should help here: so, by default empty element (but NOT start+end with no content) will be exposed as JsonToken.VALUE_NULL. But this may changed via FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL to instead return JsonToken.VALUE_STRING (with text of "").
Enabling this feature is likely most natural way to deal with XML content, and doing that will also help with things like "empty" Objects (instead of null reference).

@mliberty
Copy link

It would be nice to also have a way to make start+end with no content deserialize to null, especially since that was previous behavior.

@cowtowncoder
Copy link
Member

@mliberty these combinations are bit of a mess. But I didn't think start+end did deserialize to null previously?

I am now bit torn as to whether to also add matching feature for START+END; but that should go in 3.0.

Also, there seem to be (at least?) 3 distinct possibilities for both:

  1. Expose as "native" null
  2. Expose as empty String value ""
  3. Exposes as logical START_OBJECT/END_OBJECT

all of which make sense for some use cases.

Although due to wide variety of coercions available, perhaps (3) is not needed: it is bit trickier to support (need to emit 2 tokens; will confuse functionality at databind level).

@cowtowncoder
Copy link
Member

Unfortunately with 2.x there is no way to make all cases work well: it would be easy enough to induce null for start+end, but that would probably break some code that expects empty String.
With "null coercion" many things are possible, but in patch release such functional changes should not be added.

3.0.0 will now implement this, however, as suggested.

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.

None yet

5 participants