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

read list value by element java types #3595

Open
wants to merge 10 commits into
base: 2.14
Choose a base branch
from

Conversation

zrlw
Copy link

@zrlw zrlw commented Sep 8, 2022

@yawkat
Copy link
Member

yawkat commented Sep 8, 2022

(note: I will be calling this feature "tuples" because that's what they are)

im not a fan of this approach. imo adding new methods to ObjectMapper for this is not very flexible (needs duplication from other read methods, does not give a way to define such a "tuple" using an annotation). also, adding a new method to JsonDeserializer is weird, i don't really see how subclasses should implement it.

i can think of two approaches to get the same feature without these drawbacks:

  • we could introduce a special JavaType for tuples, with a special JsonDeserializer. this seems like an easy solution, though it means that the new JavaType does not have a match in the java language, which is a bit weird.
  • alternatively, we could adjust ObjectDeserializer to be able to deserialize objects from a non-object json "shape". the user would define a tuple class with the fields of the different types they want, and ObjectDeserializer would parse this tuple class from a json array instead of from a json object.

@yawkat
Copy link
Member

yawkat commented Sep 8, 2022

Ah, I just looked at how the kotlin module does this, and it turns out my second suggestion is already implemented: FasterXML/jackson-module-kotlin#72 – you can annotate a class with an array shape, and it will be deserialized exactly like you want. imo this is already a good solution for the tuple feature, with the added benefit of better type safety.

    public static void main(String[] args) throws JsonProcessingException {
        TestTuple tuple = new TestTuple();
        tuple.integer = 1;
        tuple.bigDecimal = new BigDecimal("2");
        tuple.l = 3L;
        tuple.s = null;

        ObjectMapper objectMapper = new ObjectMapper();
        String json = objectMapper.writeValueAsString(tuple);
        // json == [1,2,3,null]
        objectMapper.readValue(json, TestTuple.class); // returns same tuple
    }

    @JsonFormat(shape = JsonFormat.Shape.ARRAY)
    public static class TestTuple {
        public Integer integer;
        public BigDecimal bigDecimal;
        public long l;
        public String s;
    }

@zrlw
Copy link
Author

zrlw commented Sep 8, 2022

it is weird to define a annotated class that includes the parameter types of each rpc method for generized service adaptation.

@cowtowncoder
Copy link
Member

Hmmh. First of all, thank you for submitting this contribution. I can see why someone might want Tuples.

My initial reaction is that this is too intrusive, adding new method for many deserializers to implement. It also seems like there should be something to denote proper Tuple types.

One suggestion here, related to @yawkat's concern is that ObjectMapper is probably the wrong place for this -- methods should be added (only) in ObjectReader. Type list/array could be added there; and in fact I think it should be possible to keep handling of Tuple fields within ObjectReader and not require new method(s) in JsonDeserializer.

So I think I would be more amenable to such an approach: configuring ObjectReader with List/array of JavaTypes (with possible convenience method of Class[] alternative), and then readValue() taking that into account (if "type List" is defined).

@zrlw
Copy link
Author

zrlw commented Sep 9, 2022

introduce a special JavaType for tuples:
#3597

@zrlw
Copy link
Author

zrlw commented Sep 11, 2022

One suggestion here, related to @yawkat's concern is that ObjectMapper is probably the wrong place for this -- methods should be added (only) in ObjectReader. Type list/array could be added there; and in fact I think it should be possible to keep handling of Tuple fields within ObjectReader and not require new method(s) in JsonDeserializer.

changed. but it seemed not to be elegant as lots of codes was copyed from CollectionDeserializer to ObjectReader just for avoiding to require new method in JsonDeserializer.

maybe #3597 is better.

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

3 participants