Fix handling of null values in string Arrays and Collections #585
Conversation
mbladel
force-pushed
the
string_collections
branch
from
a44132d
to
925d2d8
Compare
| import static org.junit.Assert.assertNull; | ||
|
|
||
| // [dataformat-xml#584] | ||
| public class StringListRoundtripTest { |
There was a problem hiding this comment.
Looks good, but one big(ger) ask: in addition to root values (which are actually harder to support, bit less common to use), would it be possible to also add "as-property" cases where String arrays, Collections, Maps are properties of POJOs? That would we can double-check that empty Strings do not accidentally become nulls, which is my main concern.
| @@ -932,12 +932,11 @@ public String nextTextValue() throws IOException | |||
| switch (token) { | |||
| case XmlTokenStream.XML_END_ELEMENT: | |||
| if (_mayBeLeaf) { | |||
| // NOTE: this is different from nextToken() -- produce "", NOT null | |||
There was a problem hiding this comment.
Ok, this explicit comment makes me nervous: it specifically spells out that when asking from nextTextValue(), empty element should not be coerced into null.
But since all existing unit tests, and new ones too, pass, this may make sense.
One related note: the reason Maps work ok is that there is no specialized access to "String-valued maps" (but there is for String[] and Collection<String>!) to use JsonParser.nextTextValue().
One way to alleviate my concerns is to make sure tests are improved -- I noticed additions which are great.
The main addition I'd like to see is addition of checking of String[] / List<String> valued POJO properties, with null, "". If those retain values as expected I am comfortable with this change.
| _mayBeLeaf = false; | ||
| _currToken = JsonToken.VALUE_STRING; | ||
| _currToken = JsonToken.VALUE_NULL; |
There was a problem hiding this comment.
Could you also add one-line comment referencing
/ / [dataformat-xml#584]: retain null values
|
Since I am pushing 2.15.0-rc1 soon, I will actually merge this & add comment. We shoyld be able to get some validation on safety with rc1, more so than waiting until rc2 or 2.15.0 final. Additional tests would be great but can be another PR. |
Possible fix for #584.
As a note, empty and
nullstring values inMaps were correctly handled even before the change (although requiring the map itself to be wrapped in a POJO). I included a test anyways.