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

null-handling ignored #437

Closed
ilgrosso opened this issue Dec 1, 2020 · 22 comments
Closed

null-handling ignored #437

ilgrosso opened this issue Dec 1, 2020 · 22 comments
Labels
will-not-fix Issue for which there is no plan to fix as described or requested

Comments

@ilgrosso
Copy link

ilgrosso commented Dec 1, 2020

With 2.12.1-SNAPSHOT the following code:

        XML_MAPPER = new XmlMapper();
        XML_MAPPER.configOverride(List.class).setSetterInfo(JsonSetter.Value.forValueNulls(Nulls.AS_EMPTY));
        XML_MAPPER.configOverride(Set.class).setSetterInfo(JsonSetter.Value.forValueNulls(Nulls.AS_EMPTY));
        XML_MAPPER.configOverride(Map.class).setSetterInfo(JsonSetter.Value.forValueNulls(Nulls.AS_EMPTY));
        XML_MAPPER.enable(FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL);

        List<LoggerTO> original = new ArrayList<>();

        StringWriter writer = new StringWriter();
        XML_MAPPER.writeValue(writer, original);

        List<LoggerTO> actual = XML_MAPPER.readValue(writer.toString(), new TypeReference<List<LoggerTO>>() {
        });
        assertEquals(original, actual);

fails with message

org.opentest4j.AssertionFailedError: expected: <[]> but was: <null>

I have verified that serialization produces

<ArrayList/>
@ilgrosso
Copy link
Author

@cowtowncoder any update on this?

@cowtowncoder
Copy link
Member

No, I have not had time to look into this further unfortunately.

@cowtowncoder
Copy link
Member

cowtowncoder commented Dec 17, 2020

Ok. Looking at this test case: it is actually working as expected. @JsonSetter unfortunately only covers handling of properties referenced by POJOs, and specifically root values are not considered (they are not referenced by POJO properties). So although you can specify configOverride setting for a type, that only changes handling of properties of that type: for @JsonSetter annotation this is bit more clear in that you can not use it on classes at all, only on accessors (fields, methods, constructor parameters).
This handling is not specific to XML module; same is true for JSON and other formats.

@cowtowncoder cowtowncoder added the will-not-fix Issue for which there is no plan to fix as described or requested label Dec 17, 2020
@ilgrosso
Copy link
Author

@cowtowncoder thanks for your answer.

My problem is that, since we are using Jackson dataformats to handle at the same time JSON, YAML and XML payloads for REST, the same exact test case above works as expected with JSON and YAML in 2.12.1-SNAPSHOT but fails as described with XML.
Naturally, all works instead with 2.11.4.

Could you suggest how to change or extend / override something into XmlMapper (rather than annotating POJOs since this might affect JSON or YAML handling) in order to align its behavior with ObjectMapper and YAMLMapper?

@cowtowncoder
Copy link
Member

But could you just disable FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL? Without that setting, empty element never becomes null in the first place. Neither YAML nor JSON can coerce explicit null at root level.

Other than that I think I would need to understanding something more about use case: combination of settings is bit incosistent.

@ilgrosso
Copy link
Author

@cowtowncoder the following code:

        XML_MAPPER = new XmlMapper();
        XML_MAPPER.configOverride(List.class).setSetterInfo(JsonSetter.Value.forValueNulls(Nulls.AS_EMPTY));
        XML_MAPPER.configOverride(Set.class).setSetterInfo(JsonSetter.Value.forValueNulls(Nulls.AS_EMPTY));
        XML_MAPPER.configOverride(Map.class).setSetterInfo(JsonSetter.Value.forValueNulls(Nulls.AS_EMPTY));

        AnyObjectCR original = new AnyObjectCR();
        original.setName("newPrinter");
        original.setType("PRINTER");
        original.getPlainAttrs().add(new Attr.Builder("location").value("new").build());

        StringWriter writer = new StringWriter();
        objectMapper().writeValue(writer, original);

        AnyObjectCR actual = objectMapper().readValue(writer.toString(), AnyObjectCR.class);
        assertEquals(original, actual);

is throwing the following exception:

com.fasterxml.jackson.databind.exc.MismatchedInputException: 
Cannot deserialize value of type `java.util.ArrayList<org.apache.syncope.common.lib.to.MembershipTO>` from String value (token `JsonToken.VALUE_STRING`)
 at [Source: (StringReader); line: 1, column: 137] (through reference chain: org.apache.syncope.common.lib.request.AnyObjectCR["dynMemberships"])

FYI, the definition of AnyObjectCR

The same test code works fine with ObjectMapper and YAMLMapper - and also with XmlMapper when enabling FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL.

At the moment, unfortunately, I am in the situation where I have 1 XML test failure in case FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL is enabled and several XML test failures when disabled: this is blocking the upgrade from 2.11.4.

@cowtowncoder
Copy link
Member

I would need a reproduction with input, ideally trimmed down to as small an example as possible.
Could generate XML, include that in test. configOverrides should not have any effect wrt that failure.

@cowtowncoder cowtowncoder reopened this Dec 19, 2020
@ilgrosso
Copy link
Author

@cowtowncoder thanks for re-opening this issue; I've setup the reproducing sample at https://github.com/ilgrosso/jackson-dataformat-xml-437

The code is using 2.12.1-SNAPSHOT by default. As you can see, while the two test cases provided work fine with JSON and YAML, I did override for XML to show how enabling or disabling FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL makes them fail alternatively.

@cowtowncoder
Copy link
Member

@ilgrosso When I clone the project, run with JDK 11, all tests pass?

@ilgrosso
Copy link
Author

@cowtowncoder yes, the tests are passing on purpose: the cases for XML in fact

https://github.com/ilgrosso/jackson-dataformat-xml-437/blob/main/src/test/java/net/tirasa/test/SerializationTest.java

are set to assert alternate failures.

@cowtowncoder
Copy link
Member

cowtowncoder commented Dec 23, 2020

@ilgrosso I must be slow here: how do I need to modify tests to fail?

@ilgrosso
Copy link
Author

@cowtowncoder sorry I thought to make the problem clearer this way...

Take the first XML case, e.g. emptyAsRoot(): https://github.com/ilgrosso/jackson-dataformat-xml-437/blob/main/src/test/java/net/tirasa/test/XMLTest.java#L42-L48

when FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL is enabled, the actual test code defined in super.emptyAsRoot() is asserted to fail; when the feature is disabled, it is asserted to work.

Same happens with the second test case, e.g. emptyAsMember(): https://github.com/ilgrosso/jackson-dataformat-xml-437/blob/main/src/test/java/net/tirasa/test/XMLTest.java#L52-L57

when FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL is disabled, the actual test code defined in super.emptyAsMember() is asserted to fail; when the feature is enabled, it is asserted to work.

The test cases defined by superclass are run unchanged with JSON and YAML.

@cowtowncoder
Copy link
Member

cowtowncoder commented Dec 27, 2020

I don't really get it: why would a test assert that the wrong thing happens? It would be easier to show expected behavior and then I can see what happens instead.
Instead of asserting that things work in some specific wrong way.

Be that as it may, I think I get the intent now.

@cowtowncoder
Copy link
Member

It looks like you are comparing serializing a List either as root value, or as member; with or without EMPTY_ELEMENT_AS_NULL, and neither setting works for both cases as you'd like it.

Cases are not equivalent, however: in root case, List is empty, but in property case, List has one entry, POJO with null for value.

It seems to me that the issue is with POJO value, and it's String-valued value property. Due to XML not having native concept of null, it is not really possible to denote difference between null and empty String (""), except for use of xsi:nil attribute or something.

I suspect there may be a change in String value handling from 2.11, regarding nullability. But I think is where further isolating test difference would be useful -- to see exactly what different do you see between 2.11 and 2.12.
Setting should not have any effect on serialization, for example.
I am also not sure if the different between empty List and List with 1 "empty" POJO value is intentional.

@ilgrosso
Copy link
Author

ilgrosso commented Dec 28, 2020

@cowtowncoder sorry for the confusion.

I have updated the test project, hopefully to make it simpler and more consistent.
Following your suggestion, I have also renamed emptyAsMember() to nonEmptyListAsMember().

After pulling, if you run it via

$ mvn clean verify

you get the following test failures:

[ERROR] Failures: 
[ERROR]   XMLTest>SerializationTest.emptyListAsRoot:26 expected: <[]> but was: <null>
[ERROR]   XMLTest.nonEmptyListAsMember_disable_EMPTY_ELEMENT_AS_NULL:51->SerializationTest.nonEmptyListAsMember:38 expected: <Root{pojos=[POJO{value=null}]}> but was: <Root{pojos=[POJO{value=}]}>

As you can see from test code, the XmlObjectMapper instance is configured by default with FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL enabled; this setting makes:

  • XMLTest#emptyListAsRoot to fail
  • XMLTest#nonEmptyListAsMember to succeed
  • XMLTest.nonEmptyListAsMember_disable_EMPTY_ELEMENT_AS_NULL, where XMLTest#nonEmptyListAsMember is invoked after disabling FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL, to fail

I hope I was able to make clear how setting FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL enabled or disabled causes either of the test cases to fail.

Finally, I have also added a Maven profile to run the same test suite against Jackson 2.11.4; if you run the project via

$ mvn -Pjackson-2.11 clean verify

all tests are passing.

Thanks again for your support.

@cowtowncoder
Copy link
Member

Thank you for updates. I will see if that helps me figure out what is the difference and whether change is something that needs to be fixed or something to be documented.

@cowtowncoder
Copy link
Member

At this point this test unfortunately only validates that the behavior with 2.12 differs from what your use case (based on 2.11) expects.
This is not enough for me to see where and how specific parts of handling is wrong: change of behavior may or may not be intentional: if not, it is likely bug; if it is, it is possibly bug fix to earlier behavior. 2.12 contains many bug fixes.

For example: when serializing an empty List, then deserializing that back with FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL set returns null: not empty List.
As far as I can see that is what the feature defines it should do. That may be different from behavior of 2.11 but would be considered a bug fix.

@ilgrosso
Copy link
Author

For example: when serializing an empty List, then deserializing that back with FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL set returns null: not empty List.
As far as I can see that is what the feature defines it should do. That may be different from behavior of 2.11 but would be considered a bug fix.

Thanks for your answers; two questions related to this last point:

  1. why FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL is needed with 2.12 in first place? As you can see from test code, this feature is not needed with 2.11 to make the tests pass; when omitted with 2.12, however, the failures are:
[ERROR] Failures: 
[ERROR]   XMLTest>SerializationTest.nonEmptyListAsMember:38 expected: <Root{pojos=[POJO{value=null}]}> but was: <Root{pojos=[POJO{value=}]}>
[ERROR]   XMLTest.nonEmptyListAsMember_disable_EMPTY_ELEMENT_AS_NULL:51->SerializationTest.nonEmptyListAsMember:38 expected: <Root{pojos=[POJO{value=null}]}> but was: <Root{pojos=[POJO{value=}]}>
  1. could you please indicate if there is an easy point where I could, with a local module or something, restore the 2.11 behavior on this point? or avoid that FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL affects the root element?

@cowtowncoder
Copy link
Member

Documentation of EMPTY_ELEMENT_AS_NULL says:

        /**
         * Feature that indicates whether XML Empty elements (ones where there are
         * no separate start and end tags, but just one tag that ends with "/&gt;")
         * are exposed as {@link JsonToken#VALUE_NULL}) or not. If they are not
         * returned as `null` tokens, they will be returned as {@link JsonToken#VALUE_STRING}
         * tokens with textual value of "" (empty String).
         *<p>
         * Default setting was {@code true} (for backwards compatibility from 2.9 to 2.11 (inclusive)
         * but was changed in 2.12 to be {@code false} (see [dataformat-xml#411] for details)
         *
         * @since 2.9
         */

so it changes underlying token stream to infer concept of null as token, something that XML does not have without additional layers (such as XML Schema specification and xsi:nil attribute). It works that way for all XML elements, including root element.
As notes say, it was enabled in 2.11 automatically but with 2.12 is no longer enabled. You did not have to enable it with 2.11 but may do so (makes no different).

But at this point I do not think EMPTY_ELEMENT_AS_NULL is a useful feature and will probably be fully removed from Jackson 3.x.
The reason being that Jackson 2.12 will handle empty content (both <elem /> and <elem></elem>) in a way that is usually expected (POJO with default settings, empty Collection/Map/array; scalars with default value) and supports (optional) use of xsi:nil for true nulls. Trying to guess null-ness from one element variation has not turned out to worked very well in practice.

I still do not see any specific change that is suggested and will now close the issue. A new issue may be filed with specific aspect of handling of 2.12.0 that appears wrong. Difference in behavior between 2.11 and 2.12 is not sufficient.

@ilgrosso
Copy link
Author

ilgrosso commented Dec 31, 2020

Ok, thanks for pointing out the Javadoc for EMPTY_ELEMENT_AS_NULL.

Just to summarize, what I am experiencing now is that, with 2.12.1-SNAPSHOT, the feature enabled and the settings as explained above, this:

<ArrayList/>

is deserialized as null while pojos from this:

<Root><pojos/></Root>

is deserialized as an empty list.

FTR, this:

<ArrayList></ArrayList>

is also deserialized an empty list.

Hence, my current goal is to set things so that an empty collection (possibly, only when root element) is serialized as in the last case rather than in the first one.

@ilgrosso
Copy link
Author

FYI with these settings all is passing now, both with 2.11 and 2.12.1-SNAPSHOT: https://github.com/ilgrosso/jackson-dataformat-xml-437/blob/main/src/test/java/net/tirasa/test/XMLTest.java#L23-L39

@cowtowncoder
Copy link
Member

cowtowncoder commented Jan 1, 2021

For what it is worth, empty element like <ArrayList></ArrayList> (or <ArrayList />) should be deserialized as empty List with 2.12, as long as you do NOT enable FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL. This was not the case with 2.11, regardless of setting as handling of empty elements had problems -- mostly because at low level such elements would be equivalent to JSON empty String value. Support for this case was improved in jackson-databind 2.12, to let XML module work better for this relatively common case.

I hope it is now possible to get things to work in a usable way: unfortunately the full combination of various aspects is not easy to reason about and it can be frustrating to find a good combination. Especially when attempting to make things work the same way against different versions.

I am planning to release 2.12.1 very soon now, for what that is worth; there are a few important fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
will-not-fix Issue for which there is no plan to fix as described or requested
Projects
None yet
Development

No branches or pull requests

2 participants