Deserializing fails when using builder classes with `Iterable` Collection setters

[bpasson]

When using builder classes with method signatures like Builder orderLines(Iterable<? extends OrderLine> lines) for collections, deserialization fails with the following.

com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot construct instance of `com.example.OrderLine$Builder` (although at least one Creator exists): no String-argument constructor/factory method to deserialize from String value ('1')
 at [Source: (BufferedInputStream); line: 5, column: 14] (through reference chain: com.example.Order$Builder["line"]->java.util.ArrayList[0])

A reproducer can be found at https://github.com/bpasson/jackson-dataformat-xml-issue-646

This concerns jackson-dataformat-xml version 2.17.0.

Detailed Example

Consider the following objects:

@JsonDeserialize(builder = Order.Builder.class)
@JsonIgnoreProperties(ignoreUnknown = true)
@JacksonXmlRootElement(localName="order")
public class Order {
    private final Long id;
    private final List<OrderLine> lines;

    private Order(Long id, List<OrderLine> lines) {
        this.id = id;
        this.lines = lines;
    }

    @JsonProperty("id")
    public Long getId() {
        return id;
    }

    @JsonProperty("line")
    List<OrderLine> getOrderLines() {
        return lines;
    }

    public Builder builder() {
        return new Builder();
    }

    public static class Builder {
        private Long id;
        private List<OrderLine> lines = new ArrayList<>();

        @JsonProperty("id")
        public Builder id(Long id) {
            this.id = id;
            return this;
        }

        @JsonProperty("line")
        public Builder lines(Iterable<? extends OrderLine> lines) { // fails
        //public Builder lines(Collection<? extends OrderLine> lines) { // works
            this.lines.clear();
            for (OrderLine line : lines) {
                this.lines.add(line);
            }
            return this;
        }

        public Order build() {
            return new Order(id, lines);
        }
    }
}

@JsonDeserialize(builder = OrderLine.Builder.class)
@JsonIgnoreProperties(ignoreUnknown = true)
@JacksonXmlRootElement(localName = "line")
public class OrderLine {

    private final Long id;
    private final String description;

    @JsonProperty("id")
    public Long getId() {
        return id;
    }

    @JsonProperty("description")
    String description() {
        return description;
    }

    private OrderLine(Long id, String description) {
        this.id = id;
        this.description = description;
    }

    public Builder builder() {
        return new Builder();
    }

    public static class Builder {
        private Long id;
        private String description;

        @JsonProperty("id")
        public Builder id(Long id) {
            this.id = id;
            return this;
        }

        @JsonProperty("description")
        public Builder description(String description) {
            this.description = description;
            return this;
        }

        public OrderLine build() {
            return new OrderLine(id, description);
        }
    }
}

When deserializing the following XMl document:

<?xml version="1.0" encoding="UTF-8" ?>
<order>
    <id>1</id>
    <line>
        <id>1</id>
        <description>order line 1</description>
    </line>
    <line>
        <id>2</id>
        <description>order line 2</description>
    </line>
</order>

Deserializing fails with:

com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot construct instance of `com.example.OrderLine$Builder` (although at least one Creator exists): no String-argument constructor/factory method to deserialize from String value ('1')
 at [Source: (BufferedInputStream); line: 5, column: 14] (through reference chain: com.example.Order$Builder["line"]->java.util.ArrayList[0])

This is caused by the following builder method:

@JsonProperty("line")
public Builder lines(Iterable<? extends OrderLine> lines) { // fails
//public Builder lines(Collection<? extends OrderLine> lines) { // works
    this.lines.clear();
    for (OrderLine line : lines) {
        this.lines.add(line);
    }
    return this;
}

If you use Iterable<? extends OrderLine> as method parameter it fails. If you use Collection<? extends OrderLine>, see commented line above, it works. I expected both to work as is the case for Json binding.

[pjfanning]

@bpasson we don't really have many active maintainers of this repo.

I would suggest that this method is a good one to look at if anyone has an interest in trying to fix this.

https://github.com/FasterXML/jackson-dataformat-xml/blob/85905b5eb44a878524a3d25653973bcbac151d1a/src/main/java/com/fasterxml/jackson/dataformat/xml/util/TypeUtil.java#L11

[cowtowncoder]

First things first: never use this line in Jackson problem reproductions:

@JsonIgnoreProperties(ignoreUnknown = true)

as that is known to hide issues left and right and wasting everyone's time. (not saying it does so here but in the past has been big time sink).

Second: @pjfanning is probably right that specific handling XML backend needs is related to that method.

Third: one thing that can help with troubleshooting is to contribute failing (minimal) unit test (under src/test/java/../failing) as a PR -- that saves time in multiple ways, including ensuring that the issue reported is fully understood.

[bpasson]

@cowtowncoder I added a PR to branch 2.18 which contains the failing test. I did some debugging and found the following piece of code in the BasicDeserializerFactory located at BasicDeserializerFactory.java#L2115

if (rawType == CLASS_ITERABLE) {
    // [databind#199]: Can and should 'upgrade' to a Collection type:
    TypeFactory tf = ctxt.getTypeFactory();
    JavaType[] tps = tf.findTypeParameters(type, CLASS_ITERABLE);
    JavaType elemType = (tps == null || tps.length != 1) ? TypeFactory.unknownType() : tps[0];
    CollectionType ct = tf.constructCollectionType(Collection.class, elemType);
    // Should we re-introspect beanDesc? For now let's not...
    return createCollectionDeserializer(ctxt, ct, beanDesc);
}

It upgrades Iterable from a simple type to a collection type, but the type in TypeUtil in the method below it is still seen as a simple type.

https://github.com/FasterXML/jackson-dataformat-xml/blob/85905b5eb44a878524a3d25653973bcbac151d1a/src/main/java/com/fasterxml/jackson/dataformat/xml/util/TypeUtil.java#L11

It this related to the comment // Should we re-introspect beanDesc? For now let's not...?

[cowtowncoder]

Ok, so no, that comment should not be relevant for this purpose (or problem).

But the call to TypeUtil happens before call to BasicDeserializerFactory, I think; and even if not, type "upgrade" only affects local processing. Or put another way: two are not directly linked, TypeUtil call is for adding external handling outside of deserializer to deal with possible wrapping. Still, isIterationType() should cover JavaType here so maybe this is not the problem.

[bpasson]

@cowtowncoder I did a test and Iterable is marked as a simple type and isIterationType() returns false.

If I extend the if clause in TypeUtil to also check the raw class for Iterable the failing test succeeds. This points to a deeper issue where Iterable is classified wrong.

What do you think? If it is classified as an IterationType it should work, although I have no idea what the impact is.

It might even make the explicit upgrading in BasicDeserializerFactory obsolete, but you know more about why it is there than I.

[cowtowncoder]

@bpasson I think my concern was with the fact that Iterable is more generic and less specific in some sense than Iterator and as such addition of IterationType did not include it on purpose. Original issue:

https://github.com/FasterXML/jackson-databind/issues/3950

does not mention this so I guess it could also be an oversight. Especially since it is not about something else implementing Iterable but specific type, see:

https://github.com/FasterXML/jackson-databind/blob/2.18/src/main/java/com/fasterxml/jackson/databind/type/TypeFactory.java#L1635

so... it could be something to improve. I will file an issue for that.

Having said that, I think addition in TypeUtil for specific logic wrt Iterable would probably make most sense here; even if logic in databind gets updated this change would still work.

[bpasson]

@cowtowncoder That sounds sensible. I will update the PR to have TypeUtil explicitly check for Iterable to fix the issue.

The PR now is targeted at 2.18 branch. Is that ok or should I target it at main?

[pjfanning]

There is no main branch and master branch is for 3.0. 2.18 will be released before 3.0.

[cowtowncoder]

@bpasson Like @pjfanning mentioned, we'll go with 2.x.

But this looks quite safe change so I would accept it against 2.17 as well (for 2.17.1); that will be released before 2.18.0 (as we only just started 2.18 branch, it'll be multiple months).

[bpasson]

@cowtowncoder I opened the PR to 2.17. It contains the fix and I moved the original failing, now working, testcase to the lists test package.

[cowtowncoder]

Fix merged in for 2.17.1.