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

@JsonAnySetter on field annotation for Map #1249

Merged
merged 1 commit into from
Jun 2, 2016

Conversation

LokeshN
Copy link
Contributor

@LokeshN LokeshN commented May 25, 2016

Supporting #1047 @JsonAnySetter annotation on java.util.Map field.

The build might fail for this PR since the @JsonAnySetter annotation is not enabled for the field in
jackson-annotations.
If the code changes are fine, then I will send a separate PR for @JsonAnySetter changes.

@cowtowncoder Let me know if the code changes are fine.

AnnotatedField field = ((AnnotatedField) _setter);
// check whether the annotated field is an instance of Map, if
// not throw Error
if (!Map.class.isAssignableFrom(field.getRawType())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should fail earlier, on construction (or perhaps introspection), no point in waiting until attempt is made to set it.

@cowtowncoder
Copy link
Member

Yes, I think this is good at high level, so please go ahead with PR for jackson-annotations as well!

I will add some notes on smaller tweaks (and one bigger question) for changes here -- thank you for this contribution too, very useful!

// we know it's a 2-arg method, second arg is the value
type = ((AnnotatedMethod) setter).getParameterType(1);
} else if (setter instanceof AnnotatedField) {
// get the type from the content type of the map object
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be good place to verify that we got a Map valued property; type should then be MapType as well (and guarantee there is content type to use).

@LokeshN LokeshN force-pushed the jsonanysetter-map branch from f377459 to 91867e5 Compare May 25, 2016 06:54
@LokeshN
Copy link
Contributor Author

LokeshN commented May 25, 2016

@cowtowncoder thanks for the review. I have made the corresponding changes and also submitted a new PR for jackson-annotations.

@LokeshN LokeshN force-pushed the jsonanysetter-map branch from 91867e5 to a301960 Compare May 26, 2016 03:41
Supporting @JsonAnySetter annotation on java.util.Map
@LokeshN LokeshN force-pushed the jsonanysetter-map branch from a301960 to 71f3907 Compare May 26, 2016 03:49
@LokeshN
Copy link
Contributor Author

LokeshN commented May 28, 2016

@cowtowncoder I have made the changes accordingly and build is also passing now.
Let me know if it is fine.

@cowtowncoder
Copy link
Member

@LokeshN Thank you again! I hope to review this again as soon as possible, and it should make it in 2.8.0 RC1 (i.e. won't release it without going over this PR).

@cowtowncoder cowtowncoder added this to the 2.8.0 milestone Jun 2, 2016
@cowtowncoder cowtowncoder merged commit 3f5dd6e into FasterXML:master Jun 2, 2016
@cowtowncoder
Copy link
Member

Excellent! Merged, will be in 2.8.0(.rc1)

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.

2 participants