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

First element in unwrapped XML array is ignored during deserialization to base class #567

Open
tetradon opened this issue Jan 26, 2023 · 6 comments
Labels
has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue

Comments

@tetradon
Copy link

tetradon commented Jan 26, 2023

During Jackson migration, I discovered an issue that looks like a regression in 2.11.1. In this version, the first element in XML array is ignored when I deserialize to the base class.

@JsonSubTypes({
        @JsonSubTypes.Type(value = Wrapper.class, name = "wrapper")
})
@JsonIgnoreProperties(ignoreUnknown = true)
@JsonInclude(JsonInclude.Include.NON_NULL)
public class Base {
}
@JacksonXmlRootElement(localName = "wrapper")
public class Wrapper extends Base {

    @JacksonXmlProperty(localName = "item")
    @JacksonXmlElementWrapper(useWrapping = false)
    private List<Item> items = new ArrayList<>();

    public Wrapper(List<Item> items) {
        this.items = items;
    }

    public Wrapper() {
    }

    public List<Item> getItems() {
        return items;
    }

    public void setItems(List<Item> items) {
        this.items = items;
    }

    @Override
    public String toString() {
        return "Wrapper{" +
                "items=" + items +
                '}';
    }
}
@JacksonXmlRootElement(localName = "item")
@JsonInclude(JsonInclude.Include.NON_NULL)
@JsonIgnoreProperties(ignoreUnknown = true)
public class Item {

    private String id;

    public Item(String id) {
        this.id = id;
    }

    public Item() {
    }

    public String getId() {
        return id;
    }

    public void setId(String id) {
        this.id = id;
    }

    @Override
    public String toString() {
        return "Item{" +
                "id='" + id + '\'' +
                '}';
    }
}
public class Main {
    public static void main(String[] args) throws JsonProcessingException {
        String xmlString = """
                <?xml version="1.0" encoding="UTF-8"?>
                <wrapper type="wrapper">
                    <item><id>1</id></item>
                    <item><id>2</id></item>
                    <item><id>3</id></item>
                </wrapper>
                """;
        Base base = new XmlMapper().readValue(xmlString, Base.class);
        System.out.println(base);
    }
}

Output:

Version 2.11.0
image

Version 2.11.1+
image

Workarounds:

  1. Deserizalize to the concrete Wrapper class instead of Base:
    new XmlMapper().readValue(xmlString, Wrapper.class);
  2. Add following setter for the Wrapper class
   @JsonSetter
   public void setItems(Item item) {
        this.items.add(item);
    }

Unofortunately workarounds doesn't suit my needs as this is library code base that is used in many other projects :(

Link to the code in GitHub:
https://github.com/tetradon/jackson-xml-deserialization-issue

@cowtowncoder
Copy link
Member

Version 2.11.1 is very old so I would strongly recommend upgrading to a later version, like 2.14.1.
There is a chance the issue may have been resolved as there have been many fixes between these minor versions.
(although to be honest polymorphic deserialization is tricky with XML using Jackson so there may well be edge cases)

@tetradon
Copy link
Author

Version 2.11.1 is very old so I would strongly recommend upgrading to a later version, like 2.14.1. There is a chance the issue may have been resolved as there have been many fixes between these minor versions. (although to be honest polymorphic deserialization is tricky with XML using Jackson so there may well be edge cases)

Yeah, the goal is to migrate to the latest, my point is that the issue was introduced in 2.11.1 and actual for all versions after it, including the latest 2.14.1

@cowtowncoder cowtowncoder added the has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue label Feb 6, 2023
@cowtowncoder
Copy link
Member

Ok, as long as it still fail on 2.14.1 that's all I needed; I wasn't sure from the description.

Other than that couple of suggestions:

  • Never use @JsonIgnoreProperties(ignoreUnknown = true) on tests -- it tends to hide problems
  • It is good idea to try serialize value that is being deserialized: Jackson tries to guarantee round-tripping but cannot always deserialize all kinds of XML (I realize this seemed to work in earlier versions but still)

@tetradon
Copy link
Author

tetradon commented Feb 7, 2023

I've removed @JsonIgnoreProperties(ignoreUnknown = true) and added unit tests to the project with the original case and roundtrip, both are failing

https://github.com/tetradon/jackson-xml-deserialization-issue/blob/main/src/test/java/JacksonXmlTest.java

@cowtowncoder
Copy link
Member

cowtowncoder commented Feb 20, 2023

Ok. So the issue is likely combination of problems due to use of polymorphism (for both wrapper and Lists elements), unwrapped Lists and buffering: legal combination of things, but unfortunately something that is known to be problematic (I think #525 may be due to same root cause -- although there is no reordering needed here; possibly #426 too).

I don't know how this could have worked in earlier versions, nor exactly what is going on here. But at least there is a test that I will try to add under failing tests of this module.

@cowtowncoder
Copy link
Member

Ok, I added a failing test (reproduction) in project so at least there's slightly less set up work for investigation.

But I also realized that while I suspect buffering (and inability for token stream to handle "unwrapped" case when reading from buffer) is related, this does look bit different from other failing cases, as there's "first one missing" symptom.
That is probably related to handling of unwrapped-aspect (sequence of items is not contained in one scope but two for some reason).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue
Projects
None yet
Development

No branches or pull requests

2 participants