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

Empty tags cause incorrect deserialization of unwrapped lists #469

Closed
jackson-code1 opened this issue Apr 21, 2021 · 15 comments
Closed

Empty tags cause incorrect deserialization of unwrapped lists #469

jackson-code1 opened this issue Apr 21, 2021 · 15 comments
Milestone

Comments

@jackson-code1
Copy link

jackson-code1 commented Apr 21, 2021

We spotted an issue when trying to deserialize an empty tag followed by a list property.

Beans:

import java.util.ArrayList;
import java.util.List;
import javax.xml.bind.annotation.XmlAccessType;
import javax.xml.bind.annotation.XmlAccessorType;
import javax.xml.bind.annotation.XmlAttribute;
import javax.xml.bind.annotation.XmlElement;
import javax.xml.bind.annotation.XmlRootElement;
import javax.xml.bind.annotation.XmlType;

@XmlAccessorType(XmlAccessType.FIELD)
@XmlType(name = "", propOrder = {"middle"})
@XmlRootElement(name = "OuterBean", namespace = "http://jackson.test.model")

public class OuterBean
{
    @XmlElement(name = "Middle", namespace = "http://jackson.test.model")
    public OuterBean.MiddleBean middle;
    
    @XmlAccessorType(XmlAccessType.FIELD)
    @XmlType(name = "", propOrder = {"inner1", "inner2"})
    
    public static class MiddleBean
    {
        @XmlElement(name = "Inner1", namespace = "http://jackson.test.model")
        public OuterBean.MiddleBean.InnerBean1 inner1;
        @XmlElement(name = "Inner2", namespace = "http://jackson.test.model")
        private List <OuterBean.MiddleBean.InnerBean2> inner2;

        @XmlAccessorType(XmlAccessType.FIELD)
        @XmlType(name = "", propOrder = {})
        public static class InnerBean1
        {
            @XmlAttribute(name = "Str")
            public String str;
            
            @XmlElement(name = "InnerBean1Item", namespace = "http://jackson.test.model")
            public List <OuterBean.MiddleBean.InnerBean1.InnerBean1Item> item;
          
            @XmlAccessorType(XmlAccessType.FIELD)
            @XmlType(name = "", propOrder = {})
            public static class InnerBean1Item
            {
                @XmlAttribute(name = "Id")
                protected String id;
            }
        }
        @XmlAccessorType(XmlAccessType.FIELD)
        @XmlType(name = "", propOrder = {})
        public static class InnerBean2
        {
            @XmlAttribute(name = "Str2")
            public String str2;
        }
    }
}

Test case:

import org.junit.Before;
import org.junit.Test;
import com.fasterxml.jackson.annotation.JsonInclude.Include;
import com.fasterxml.jackson.databind.SerializationFeature;
import com.fasterxml.jackson.dataformat.xml.XmlMapper;
import com.fasterxml.jackson.module.jaxb.JaxbAnnotationIntrospector;

public class EmptyElementsTest
{
    XmlMapper mapper;

    @Before
    public void before()
    {
        mapper = new XmlMapper();
        mapper.setSerializationInclusion(Include.NON_NULL);
        mapper.enable(SerializationFeature.INDENT_OUTPUT);

        mapper.setAnnotationIntrospector(new JaxbAnnotationIntrospector());
    }

    @Test
    public void testEmptyElements() throws Exception
    {

        String xmlInput = "<OuterBean xmlns='http://jackson.test.model'>" +
            "  <Middle>" +
            "    <Inner1/>" +
            "    <Inner2 Str2='aaaa'/>" +
            "  </Middle>" +
            "</OuterBean>";

        OuterBean deserializedBean = mapper.readValue(xmlInput, OuterBean.class);
        String actual = mapper.writeValueAsString(deserializedBean);
        System.out.println(actual);
    }
}

Actual output:

<OuterBean xmlns="http://jackson.test.model">
  <Middle>
    <Inner1/>
  </Middle>
</OuterBean>

Expected output:

<OuterBean xmlns="http://jackson.test.model">
  <Middle>
    <Inner1/>
    <Inner2 Str2="aaaa"/>
  </Middle>
</OuterBean>

The problem seems to be related to deserialization, because deserializedBean has the Inner1 field value (populated with null values) but the Inner2 field does not deserialize correctly. (the deserialized Inner2 field is an empty list)

It is worth mentioning, that, if you remove the from the xmlInput or if Inner1 is a non-empty tag with a valid value then the Inner2 field gets deserialized correctly.

Library versions:
jackson-annotations-2.12.3.jar,
jackson-core-2.12.3.jar,
jackson-databind-2.12.3.jar,
jackson-dataformat-xml-2.12.3.jar,
jackson-module-jaxb-annotations-2.12.3.jar,

@jackson-code1
Copy link
Author

jackson-code1 commented Apr 22, 2021

@cowtowncoder do you believe it is a Jackson issue or have we misconfigured something?
Any possible workaround? :)

@cowtowncoder
Copy link
Member

Sounds like a potential bug but I haven't really had time to dig into this.

Two suggestions:

  1. Start with serialization: construct Java object set, write as XML: this shows the structure Jackson expects
  2. If structure differs, figure out why -- List wrapping may be the difference; Jackson defaults to always-wrapping, JAXB defaults to no wrapping

Other than configuration of wrapping/no-wrapping for List/Collection properties (either global default or per-property), I don't think configuration settings would help.
One problem with XML module is that its validity checking on deserialization is bit limited so if structure it sees does not match, it may not report it but get confused.

@jackson-code1
Copy link
Author

Thanks for the hints!

The serialization produces exactly the same XML as we are providing as the input in the test case, which suggests that the XML structure is correct.

One more thing, if we set the:
FromXmlParser.Feature.EMPTY_ELEMENT_AS_NULL as true then it works correctly but this is not what we want to achieve, because we want empty XML elements to deserialize into empty beans.

@cowtowncoder
Copy link
Member

cowtowncoder commented Apr 24, 2021

@jackson-code1 I don't think you did try using Jackson for serializing actual structure since the way I see, structure does differ: wrapping is used for Inner2. So I spent about half an hour looking into issue that you could have found out by actually doing that.

So the problem IS that wrapping is used for inner2 (as per Jackson defuault settings). but your XML assumes no-wrapping. You need to either add

        @JacksonXmlElementWrapper(useWrapping = false)
        public List<InnerBean2> inner2;

or configure XmlMapper to default to this setting.
With that, mapping seems to work as far as I can see.

And what is happening with input is simple: you get <Inner2> which matches the property, having no elements inside (attribute in such cases may be skipped without exceptions), resulting either empty List or null, depending on settings.

@jackson-code1
Copy link
Author

@cowtowncoder I tried with the annotation you suggested but the test case is still red.
Could you share the test case that worked for you?

With regard to serialization, yes we tried serializing the following input:

        OuterBean b = new OuterBean();
        b.middle = new OuterBean.MiddleBean();
        b.middle.inner1 = new OuterBean.MiddleBean.InnerBean1();
        b.middle.getInner2().add(new OuterBean.MiddleBean.InnerBean2());
        b.middle.getInner2().get(0).str2 = "aaaa";

        String stringVal = mapper.writeValueAsString(b);

which results in:

<OuterBean xmlns="http://jackson.test.model">
  <Middle>
    <Inner1/>
    <Inner2 Str2="aaaa"/>
  </Middle>
</OuterBean>

You mentioned that the attribute of Inner2 might be ignored but we tried it also with a non-empty Inner2 and the behavior was the same.

@cowtowncoder
Copy link
Member

Ok that is strange; I wonder if the issue is related to JAXB annotations. In my case I have a simplified version, using Jackson annotations. I think I'll check in version in git and add a link, easier than embedding.

cowtowncoder added a commit that referenced this issue Apr 27, 2021
@cowtowncoder
Copy link
Member

Oh, one thing: if you are using JaxbAnnotationIntrospector only, that does not support Jackson annotations. That would explain why annotation I mentioned would have no effect. You would need to use something like:

        AnnotationIntrospector intr = XmlAnnotationIntrospector.Pair.instance
            (new JaxbAnnotationIntrospector(TypeFactory.defaultInstance()),
                    new JacksonAnnotationIntrospector());
        mapper.setAnnotationIntrospector(intr);

Alternatively if you want to default to "no wrapper" style, you can avoid annotations and just use:

xmlMapper.setDefaultUseWrapper(false);

This is probably simpler thing to do.

@jackson-code1
Copy link
Author

Thank you for investigating it and for the unit test!

It looks like the annotations are the issue - I slightly modified the test case and witch the JAXB annotations (XmlAttribute, XmlElement) the test case fails:

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import java.util.ArrayList;
import java.util.List;
import javax.xml.bind.annotation.XmlAttribute;
import javax.xml.bind.annotation.XmlElement;
import org.junit.Test;
import com.fasterxml.jackson.annotation.JsonPropertyOrder;
import com.fasterxml.jackson.dataformat.xml.XmlMapper;
import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlElementWrapper;
import com.fasterxml.jackson.module.jaxb.JaxbAnnotationIntrospector;

// Trying to reproduce [dataformat-xml#469]
public class ListDeser469Test
{
    static class OuterBean {
        @XmlElement(name = "Middle", namespace = "http://jackson.test.model")
        public MiddleBean middle;
    }

    @JsonPropertyOrder({"inner1", "inner2"})
    static class MiddleBean
    {
        @XmlElement(name = "Inner1", namespace = "http://jackson.test.model")
        public InnerBean1 inner1;

        @JacksonXmlElementWrapper(useWrapping = false)
        @XmlElement(name = "Inner2", namespace = "http://jackson.test.model")
        public List <InnerBean2> inner2;
    }

    static class InnerBean1
    {
        @XmlAttribute(name = "Str")
        public String str;

        @XmlElement(name = "InnerBean1Item", namespace = "http://jackson.test.model")
        public List <InnerBean1Item> item;
    }

    static class InnerBean1Item
    {
        @XmlAttribute(name = "Id")
        public String id;
    }

    static class InnerBean2
    {
        @XmlAttribute(name = "Str2")
        public String str2;

        protected InnerBean2() { }
        public InnerBean2(String s) { str2 = s; }
    }

    private final XmlMapper MAPPER = new XmlMapper();

    @Test
    public void testIssue469() throws Exception
    {
        MAPPER.setAnnotationIntrospector(new JaxbAnnotationIntrospector());

        // First: create POJO value to test round-trip:
        {
            OuterBean source = new OuterBean();
            source.middle = new MiddleBean();
            List<InnerBean2> items = new ArrayList<>();
            items.add(new InnerBean2("foo"));
            source.middle.inner2 = items;

            String xml = MAPPER.writerWithDefaultPrettyPrinter()
                            .writeValueAsString(source);

            OuterBean result = MAPPER.readValue(xml, OuterBean.class);

            MiddleBean mid = result.middle;
            assertNotNull(mid);
            assertNotNull(mid.inner2);
            assertEquals(1, mid.inner2.size());
            assertEquals("foo", mid.inner2.get(0).str2);
        }

        // And then verify from XML String
        String xmlInput = "<OuterBean xmlns='http://jackson.test.model'>\n" +
                        "  <Middle>\n" +
                        "    <Inner1/>\n" +
                        "    <Inner2 Str2='aaaa'/>\n" +
                        "  </Middle>\n" +
                        "</OuterBean>\n";

        OuterBean outer = MAPPER.readValue(xmlInput, OuterBean.class);

        MiddleBean mid = outer.middle;
        assertNotNull(mid);

        assertNotNull(mid.inner2);
        assertEquals(1, mid.inner2.size());
        assertEquals("aaaa", mid.inner2.get(0).str2);
    }
}

@jackson-code1
Copy link
Author

@cowtowncoder so we managed to break your test case by using:

MAPPER.setAnnotationIntrospector(new JacksonXmlAnnotationIntrospector(false));

The false flag is meant to set useWrapper = false

The test case is green only if the following 2 conditions are met:

1. MAPPER.setAnnotationIntrospector(new JacksonXmlAnnotationIntrospector()); // by default useWrapper = true
        @JacksonXmlElementWrapper(useWrapping = false)
        public List<InnerBean2> inner2;

which assumes a selective use pf the useWrapping flag.

The problem with this approach is that we are not sure which properties in our model should use wrapping and which not.

Would you be able to investigate the wrapping issue further or give us some hints how we could mitigate it?

@cowtowncoder
Copy link
Member

Hmmh. This is weird. It seems as if serialization behavior changes as expected, but deserialization still fails.

Your usage is correct and there should be no need to use both annotation and annotation introspector settings.
I will see if I can figure out where the discrepancy comes from.

cowtowncoder added a commit that referenced this issue May 1, 2021
@cowtowncoder
Copy link
Member

cowtowncoder commented May 1, 2021

Finally figured out actual triggering condition: it does not seem to be the annotation introspector, but rather depends entirely on whether InnerBean1 has wrapping or not -- if it does not, then something goes wrong in state handling.
I added a failing test for this. Not sure how much time I have to look into this, but at least there is now a reproduction.

EDIT: also looks like simplified version of this causes fail:

<outer>
  <inner1/>
  <inner2 str2='aaaa'/>
</outer>

but making attribute element passes:

<outer>
  <inner1/>
  <inner2>
    <str2>aaaa</str2>
  </inner2>
</outer>

so there is something odd in handling of unwrapped Lists, wrt attributes.

@cowtowncoder cowtowncoder added this to the 2.12.4 milestone May 3, 2021
@cowtowncoder cowtowncoder changed the title Empty tags cause incorrect deserialization of lists Empty tags cause incorrect deserialization of unwrapped lists May 3, 2021
@jackson-code1
Copy link
Author

jackson-code1 commented May 5, 2021

Thank you very much, @cowtowncoder, for tackling it!

Is there any way how we could use the fix locally before the official release?
I tried to extend XmlBeanDeserializerModifier in order to use a locally updated WrapperHandlingDeserializer but could not override
public ValueDeserializer modifyDeserializer(DeserializationConfig config, BeanDescription beanDesc, ValueDeserializer deser0)
because ValueDeserializer is not yet released or am I missing something?

@cowtowncoder
Copy link
Member

@jackson-code1 Typically you can either build and install (mvn clean install) a new snapshot version locally, or try using Sonatype's snapshot repository (pom.xml has reference to that repo).
I'll make sure to now push 2.10.4-SNAPSHOT there.

I don't know if you could work around it by sub-classing or so -- but your reference to ValueDeserializer suggests you are looking at master branch which would be for 3.0.0-SNAPSHOT. This is not a version you can use.
Instead you'd need a build from 2.12 branch.

@sschuberth
Copy link

I have a hunch that the fix for this broke our XML parsing when upgrading our Kotlin project from 2.12.3 to 2.12.4. We parse a test.csproj file that looks like

<Project Sdk="Microsoft.NET.Sdk">
  <ItemGroup>
    <PackageReference Include="jQuery" Version="3.3.1"/>
    <PackageReference Include="WebGrease" Version="1.5.2"/>
    <PackageReference Include="foobar" Version="1.2.3"/>
  </ItemGroup>

  <PropertyGroup>
    <TargetFramework>test</TargetFramework>
    <RootNamespace>is</RootNamespace>
    <AspNetCoreHostingModel>ignored</AspNetCoreHostingModel>
  </PropertyGroup>

  <ItemGroup>
    <Content Remove="compilerconfig.json" />
    <Content Remove="wwwroot\css\x.css" />
    <Content Remove="wwwroot\css\x.min.css" />
  </ItemGroup>
</Project>

into Kotlin data classes that look like

@JsonIgnoreProperties(ignoreUnknown = true)
private data class ItemGroup(
    @JsonProperty(value = "PackageReference")
    @JacksonXmlElementWrapper(useWrapping = false)
    val packageReference: List<PackageReference>?
)

@JsonIgnoreProperties(ignoreUnknown = true)
private data class PackageReference(
    @JacksonXmlProperty(isAttribute = true, localName = "Include")
    val include: String,
    @JacksonXmlProperty(isAttribute = true, localName = "Version")
    val version: String
)

with the line

NuGetSupport.XML_MAPPER.readValue<List<ItemGroup>>(definitionFile)

This used to work in 2.12.3 and resulted in

1 = {DotNetPackageFileReader$ItemGroup@7250} ItemGroup(packageReference=[PackageReference(include=jQuery, version=3.3.1), PackageReference(include=WebGrease, version=1.5.2), PackageReference(include=foobar, version=1.2.3)])
 packageReference = {ArrayList@7280}  size = 3
  0 = {DotNetPackageFileReader$PackageReference@7282} PackageReference(include=jQuery, version=3.3.1)
  1 = {DotNetPackageFileReader$PackageReference@7283} PackageReference(include=WebGrease, version=1.5.2)
  2 = {DotNetPackageFileReader$PackageReference@7284} PackageReference(include=foobar, version=1.2.3)

but with 2.12.4 we get

1 = {DotNetPackageFileReader$ItemGroup@7251} ItemGroup(packageReference=[])
 packageReference = {ArrayList@7259}  size = 0

with the same code.

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

3 participants