FasterXML / jackson-dataformat-xml

Extension for Jackson JSON processor that adds support for serializing POJOs as XML (and deserializing from XML) as an alternative to JSON
Apache License 2.0
567 stars 221 forks source link

Required attribute of `@JsonProperty` is ignored when deserializing from XML #538

Open johandeschutterGET opened 2 years ago

johandeschutterGET commented 2 years ago

When JSON is parsed, there is a check on properties that are annotated with JsonProperty with attibute required to true. If the required property is missing, a MismatchedInputException with message "Missing required creator property" is thrown.

When XML is parsed, there is no check on required properties/element. If the required property/element is missing, there is no exception. Adding DeserializationFeature.FAIL_ON_MISSING_CREATOR_PROPERTIES doesn't make any difference.

There is constructor is annotated with JsonCreator and with the required property annotated with JsonProperty.

It worked when jackson-dataformat-xml version 2.11.4 was used, it doesn't work since version 2.12.0

Do I need to use another annotation for a required XML element?

Example class:

@JsonRootName(value = "bar")
public class Bar
{
    @JsonProperty(value = "foo", required = true)
    private int foo;

    public Bar()
    {}

    @JsonCreator
    public Bar(@JsonProperty(value = "foo", required = true) final int foo)
    {
        this.foo = foo;
    }

    public int getFoo()
    {
        return this.foo;
    }

    @Override
    public int hashCode()
    {
        return Objects.hash(this.foo);
    }

    @Override
    public boolean equals(final Object obj)
    {
        if (this == obj)
        {
            return true;
        }
        if (!(obj instanceof Bar))
        {
            return false;
        }
        final Bar other = (Bar) obj;
        return this.foo == other.foo;
    }

    @Override
    public String toString()
    {
        return "Bar: " + Objects.toString(this.foo);
    }
}

Example of unit test on JSON, this unit test throws an exception as expected, because property foo is missing.

    @Test(expected = JsonProcessingException.class)
    public void deserializeJson() throws Exception
    {
        final ObjectMapper jsonMapper = new ObjectMapper();

        final Bar expected = new Bar(123);
        final Bar actual = jsonMapper.readValue("{}", Bar.class); // missing property foo in JSON string
    }

Example of unit test on XML, this unit test does not throw an exception

   @Test(expected = JsonProcessingException.class)
    public void deserializeXml() throws Exception
    {
        final XmlMapper xmlMapper = new XmlMapper();
        xmlMapper.configure(DeserializationFeature.FAIL_ON_MISSING_CREATOR_PROPERTIES, true);

        final Bar expected = new Bar(123);
        final Bar actual = xmlMapper.readValue("<bar></bar>", Bar.class); // missing element foo in XML string

        Assert.fail("Expected com.fasterxml.jackson.databind.exc.MismatchedInputException: Missing required creator property 'foo' (index 0)");
    }
cowtowncoder commented 2 years ago

Hmmh. That should work, unless I am missing something. Perhaps this is because of the oddities of the root element handling, in context of "empty element"; it could be that processing is unintentionally short-cut to go through default constructor or something.

I hope to look into this in near future (but there's a bit of a backlog): thank you for reporting the issue, and especially thank you for providing the test case!

johandeschutterGET commented 2 years ago

The parsing of an empty XML element(1 tag, no start and end tag) is correct, if the feature FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL is enabled. The result is a NULL.

    @Test
    public void deserializeEmptyTag() throws Exception
    {
        final XmlMapper xmlMapper = new XmlMapper();
        xmlMapper.configure(FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL, true);
        final Bar actual = xmlMapper.readValue("<bar/>", Bar.class);
        Assert.assertNull(actual);
    }

The parsing of XML element with value empty string (start and end tag with empty string between), with feature FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL enabled, fails. The result is an object (Bar: 0) created by the default constructor, the required property foo is zero (= default value). There is no MismatchedInputException thrown for the missing foo property.

    @Test
    public void deserializeTagEmptyString() throws Exception
    {
        final XmlMapper xmlMapper = new XmlMapper();
        xmlMapper.configure(FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL, true);
        final Bar actual = xmlMapper.readValue("<bar></bar>", Bar.class);
        Assert.assertNull(actual); // java.lang.AssertionError: expected null, but was:<Bar: 0>
    }

The parsing of XML element with value empty string (start and end tag with empty string between) is correct, if the coercion configuration is defined for type Bar in order to process an empty string input as null.

    @Test
    public void deserializeTagEmptyStringCorrected() throws Exception
    {
        final XmlMapper xmlMapper = new XmlMapper();
        xmlMapper.coercionConfigFor(Bar.class).setCoercion(CoercionInputShape.EmptyString, CoercionAction.AsNull);
        final Bar actual = xmlMapper.readValue("<bar></bar>", Bar.class);
        Assert.assertNull(actual);
    }
cowtowncoder commented 2 years ago

@johandeschutterGET Yes, you are right. Apologies for slow follow up here, I am finally catching up here.

Ok. Looking at this, what happens is that this is essentially coercion from Empty String into "empty" value of POJO. With other formats -- say, JSON -- empty String value would use different logic than Object value and thus there should be no exception. Unfortunately XML is special: due to structure being what it is, empty Element looks either as "Object with no properties" or "empty String".

To further complicate things, however, from databind perspective (format-agnostic), value at this point definitely looks like String value, not Object.

But maybe I can figure out something...

cowtowncoder commented 2 years ago

Also: I think this is specific to Root value as well: it would not be (as) problematic for anything referenced by some other value.

Unfortunately I am not sure of how to tackle this at this point in time -- while it would be possible to detect existence of "property-based" Creator (via ValueInstantiator); and even more, properties involved including requiredness; it's not very easy to backtrack from the point of "empty String" handling back to make things look like Object. Ideally I suspect this should be handled by XML module itself to expose empty element as START_OBJECT / END_OBJECT pair.

Unfortunately I think behavior was actually changed in 2.12 from such a setup to exposing as JSON String to support root-level scalars... So going back to that might be tricky to pull off. Unless we could somehow figure out target for root value is POJO, based on deserializer?

cowtowncoder commented 2 years ago

Thinking about this more: translation is within initialize() of XmlTokenStream, called when FromXmlParser is being constructed. It'd be easy to change shape of tokens here.

The problem is that at this point we do not (yet, sort of) have information on JsonDeserializer being used.

So likely it'd have to be done, instead, from readRootValue() of XmlDeserializationContext which does have this information. It could probably determine that we are using a BeanDeserializer, and if token was JsonToken.VALUE_STRING with empty/blank contents, force re-initialization of FromXmlParser/XmlTokenStream to instead offer START_OBJECT/END_OBJECT sequence. Phew. It could additionally even check if there's ValueInstantiator to see what creators exist. Or maybe that there is no From-String instantiator.

It seems somewhat doable, just a bit complicated and convoluted :)

cowtowncoder commented 2 years ago

As per above note, I think #491 is due to the same root problem. So solving this issue would resolve at least 2 reported issues.

cowtowncoder commented 2 years ago

Ok, I was able to figure out a much simpler approach, essentially reverting some of the changes done in 2.12 for coercion root-element textual content. But without breaking what that change was initially added to fix.

So should work as expected in 2.14.0; hoping to release 2.14.0-rc1 within a week or so.