Skip to content

Conversation

@mcliedtke
Copy link
Contributor

Better support for Ion type annotations.

Re-implemented an IonAnnotationTypeResolverBuilder which allows the use for standard Jackson type annotations. This also removes the need for a custom module.

See JsonTypeInfoAnnotationsTest for example usage and configurations.

Re-implemented an IonAnnotationTypeResolverBuilder which allows the use for standard Jackson type annotations. This also removes the need for a custom module.

See JsonTypeInfoAnnotationsTest for example usage and configurations.
@cowtowncoder
Copy link
Member

Quick question: would this make sense to be automatically registered by IonObjectMapper as well?
I assume users would probably prefer it, considering wider support for polymorphic types.

Second question: I'd be happy to merge this, but need to make sure we have CLA that covers the contribution. We have one corporate one that might apply; but if not, we'd need CLA (either personal or CCLA):

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

and usual way is to print, fill & sign, scan, send to info at fasterxml dot com.

@mcliedtke
Copy link
Contributor Author

Maybe, but I am not sure I understand the impact of what the registration would do. Is the suggestion to have something like this.setDefaultTyping(<typeResolver>); in the IonObjectMapper constructor? What would it mean for types that have no @JsonTypeInfo?

I believe the corporate one should suffice (?) as I am porting changes made internally out to this project (not my own changes developed outside of the company). What situations would the existing corporate one not cover that the CLA would (or vice versa)

@cowtowncoder
Copy link
Member

Ah wrt CLA we are good. I forgot this was handled earlier (github doesn't make it super easy to keep some aspect connected, and I try to respect privacy of contributors). So existing cla is fine, I just forgot it had been updated.

On type resolver... yeah. I may need to think about this a bit. I definitely would NOT want to enable default typing as baseline, as it is potential security hole. At the same time would be nice not to require users to explicitly register handlers.

@mcliedtke
Copy link
Contributor Author

Just checking back and seeing if you had any insight on he resolver bit? I feel like I don't know enough to suggest a path forward but curious if you had any thoughts.

@cowtowncoder
Copy link
Member

Ok. Looking at this again, I think there are multiple things I am not happy about, much from perspective of how difficult this aspect is to override.

So, from Jackson API/usage, there are 2 quite distinct mechanisms:

  1. Default typing (no annotations, applies to categories of classes, configured directly via ObjectMapper)
  2. Per-type/property annotation polymorphism via @JsonTypeInfo -- routed through AnnotationIntrospector.

It would be great if the two were more integrated, and there was good override mechanism.

As to proposed solution, I just think it'd be great not to require user to annotation resolver builder, since that should be provided by IonObjectMapper. I think it is perfectly reasonable to require use of that mapper, and not care about other cases. If mapper is needed, it can instantiate module, or embed it, but it's a detail user need not worry about.

Due to dualism, I think that use of Ion-specific AnnotationIntrospector still makes sense for (2).
But I think that if possible, default typing should use this new type resolver builder.

Does this make more sense? I have been (and am) bit time-crunched and lacking forcus with Jackson components, so apologies for slow response rate and high latency.
Feel free to ping me when necessary. I would like this to be resolved for 2.9.2, to have first-class Ion support.

@mcliedtke
Copy link
Contributor Author

Sorry about this late reply. I was out for a while between getting sick and vacation. I am open to your suggestion of allowing this to be more or less default behavior of the IonObjectMapper, it seems to make a lot of sense for this to be easier to enable by default. I like the idea of this being set in a module that can be registered by the mapper.

I have tried some experimenting but I don't think I quite understand how to enable this. I am not seeing the methods in a module to enable this kind of behavior. I also tried playing around in the Mapper constructor itself to change the default typing (this.setDefaultTyping(...)) but I get different behavior than just annotating classes with the IonAnnotationTypeResolverBuilder.

Any suggestions on how this should be implemented?

@cowtowncoder
Copy link
Member

@mcliedtke I don't know to be honest. Another challenge we'll have is that 3.0 will change the way modules interact with mapper quite a bit, if (and probably when) I'll make ObjectMapper itself immutable, constructed using builder-based approach. This will require that modules interact with setup context bit differently... probably not a big deal over all, but may need to sort of implement solution twice.
So you may want to prototype with 2.9 branch as that is stable, whereas master (3.0.0-SNAPSHOT) is right now bit of a moving target.

@cowtowncoder
Copy link
Member

I think this is obsolete, closing -- if not, may be reopened.

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