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

Support for array delegate creator #1010

Merged
merged 6 commits into from Dec 11, 2015
Merged

Conversation

hgwood
Copy link
Contributor

@hgwood hgwood commented Nov 18, 2015

Hello, and thank you for Jackson. :)

Currently, as far as I can tell, Jackson supports 8 creators (methods or constructors annotated with @JsonCreator) on any type at the same time: 1 default (no-arg), 1 using properties (@JsonProperty annotated args), 1 delegate (1 non-primitive arg), 5 primitive (1 primitive arg). This can be seen in the CreatorCollector class. If a type has 2 creators that fall in the same category, only one of them is picked up.

This means you cannot have something like this...

    public class ArrayOrObject {
        private final List<Object> objects;
        private final Object object;

        @JsonCreator public ArrayOrObject(List<Object> objects) {
            this.objects = objects;
            this.object = null;
        }

        @JsonCreator public ArrayOrObject(Object object) {
            this.objects = null;
            this.object = object;
        }
    }

...because both creators have a non-primitive arg and fall in the category of delegate creators. Deserialization will work for either a JSON array or JSON object and fail for the other, when both are required. To my knowledge, supporting such a construct requires a custom deserializer or a deserializer modifier. Another way is to have a creator accept a JsonNode argument. Both of these approach require quite a lot of work.

This matters because it is a valid real-world use case. For example, the items property of a schema in the JSON Schema Draft 4 spec can be either a single schema or an array of schemas. Notice that a single schema has different semantics from a array with a single schema in it, so ACCEPT_SINGLE_VALUE_AS_ARRAY doesn't help.

Tinkering with the code, I fail to see why Jackson could not support an additional creator for arrays. Of course, it increases the chances of ambiguity, but there's probably ways to mitigate that.

Instead of going too far into consequences right now, I've written a quick proof of concept in this PR. It makes the example work. I thought we could discuss from there. Here's how it works: when a delegate creator is detected and added to the CreatorCollector, if the argument of the creator is collection-like, then the creator is stored in a different place. It is then passed like other creators to the StdValueInstanciator, which exposes a new method createUsingArrayDelegate, that BeanDeserializerBase uses in the array case (deserializeFromArray method).

What do you think of this feature? Would it be useful? Do you see any impediments to its implementation? Is the POC going in the right direction?

@hgwood
Copy link
Contributor Author

hgwood commented Nov 18, 2015

It seems IntelliJ and Maven can't agree on tests results. Looking into it.

@hgwood
Copy link
Contributor Author

hgwood commented Nov 18, 2015

All tests back to green. Build on JDK7 is green. I'm assuming the JDK6 one never is so I'm ignoring it.

@cowtowncoder
Copy link
Member

@hgwood Thank you for suggesting this, contributing a patch! I'll have to think about this; I can see benefits, although there is some potential ambiguity. But I do understand why this would be useful, given usage pattern of "single X or Array of X" pattern (which I think is an anti-pattern, when all is said and done, but I seem to be in minority... but it is widely used regardless). And when there is need to support constructs that are common I'll go with the flow. :)

One thing that might be useful would be to bring this up on jackson-dev discussion group (google group). It would be good to maybe get some comments on usability, and maybe find out what potential concerns there are. My initial concern would be backwards compatibility, such that this should not break anything that currently works.

A good thing is that timing-wise this could make it in 2.7.0 as a new feature; I am trying to finalize 2.7.0-rc1 this week.

@cowtowncoder
Copy link
Member

Patch itself looks pretty clean, good job! Couldn't have implemented it better myself.

@hgwood
Copy link
Contributor Author

hgwood commented Nov 25, 2015

Thanks for the feedback. I'll create a post on the google group.

@hgwood
Copy link
Contributor Author

hgwood commented Nov 25, 2015

@cowtowncoder Well, I wrote a message on the google group, hit Publish, and now I don't see my post anywhere. Should I try again?

@hgwood
Copy link
Contributor Author

hgwood commented Nov 25, 2015

@cowtowncoder I published again and this time noticed the post was pending approval. Sorry for the double post.

@cowtowncoder
Copy link
Member

@hgwood for some reason they went to moderation; I ok'd them so you should be good to go. Thanks!

@hgwood hgwood changed the title Experimental support for array delegate creator Support for array delegate creator Nov 25, 2015
@cowtowncoder
Copy link
Member

Ok I think this is good as-is, and I'd like to get it in 2.7.0-rc2.

One practicality now would be getting the contributor license agreement (CLA); a copy can be found at:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

and usually print, fill+sign, scan, email to info fasterxml dot com is the simplest way. We only one copy and then I can merge all contributions.
So if you could get this sent when you get a chance I will go ahead and merge this. Great idea, and hopefully simplifies development for others as well.

@hgwood
Copy link
Contributor Author

hgwood commented Dec 10, 2015

Done.

cowtowncoder added a commit that referenced this pull request Dec 11, 2015
Support for array delegate creator
@cowtowncoder cowtowncoder merged commit 31477e4 into FasterXML:master Dec 11, 2015
@cowtowncoder cowtowncoder added this to the 2.7.0-rc2 milestone Dec 11, 2015
@hgwood
Copy link
Contributor Author

hgwood commented Dec 11, 2015

Thanks @cowtowncoder!

@cowtowncoder
Copy link
Member

Hmmh. Looks like this probably caused #1053. Wish I had thought bit more carefully ramifications, need for retrofitting.

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

Successfully merging this pull request may close these issues.

None yet

2 participants