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

Consider removing "optimization" of handling Class<?> from TypeFactory.fromParamType #3443

Closed
kostislav opened this issue Apr 4, 2022 · 5 comments
Milestone

Comments

@kostislav
Copy link
Contributor

I wanted to create a custom deserializer for Class<? extends Something> that would check the upper bound and throw an exception if it did not match. Easy enough, I thought, just a simple contextual deserializer.
But, the type kept coming in as a SimpleType without any generic arguments.

After some digging, I found that it is because of a special-case in TypeFactory.fromParamType that strips the generic arguments from Enum, Comparable and Class. While I agree that for the first two the type arguments aren't really useful, they sometimes would be for Class.

Please consider whether these "performance reasons" are worth the unintuitive (and possibly undocumented?) behavior.

Currently, the only way is to subclass TypeFactory to skip the special handling (but subclassing TypeFactory from another package is pretty painful thanks to its "mutant factory methods").

@kostislav kostislav added the to-evaluate Issue that has been received but not yet evaluated label Apr 4, 2022
@cowtowncoder
Copy link
Member

Hmmmh. I would be open to removing that specific case since compared to others it is less likely to actually improve performance (Comparable and Enum are commonly resolved and for some (possibly wasteful but still) use cases have measurable negative impact).

If you change this & run unit tests does the unit test suite still pass? I'd suspect it does.

Also: if you want, a PR against 2.14 would be well received (assuming tests pass as per above).
Although this seems like possibly low-risk change I have learned to be suspicious of "safe" changes that do not fix a bug and thus prefer this going in 2.14 to avoid any chance of 2.13 regression.

@cowtowncoder cowtowncoder added need-test-case To work on issue, a reproduction (ideally unit test) needed and removed to-evaluate Issue that has been received but not yet evaluated labels Apr 4, 2022
@cowtowncoder
Copy link
Member

(labeling as "need-test-case" just as a reminder that changed behavior should be tested to avoid future regression etc -- not that I wouldn't believe behavior currently is as described).

@kostislav
Copy link
Contributor Author

Great, thanks for the heads up! I'll run the change through the test suite and try to create a PR if no problems pop up.

@kostislav
Copy link
Contributor Author

I created a PR, but I think I messed up somehow, sorry for that (first time doing a PR here on github) - it's at #3445

@cowtowncoder cowtowncoder added 2.14 and removed need-test-case To work on issue, a reproduction (ideally unit test) needed labels Apr 21, 2022
@cowtowncoder cowtowncoder modified the milestones: 2/, 2.14.0 Apr 21, 2022
@cowtowncoder
Copy link
Member

Fixed, to be included in 2.14.0.

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

No branches or pull requests

2 participants