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

Warn about/disallow @JsonCreator on enum constructors? #1272

Closed
Stephan202 opened this issue Jun 19, 2016 · 3 comments
Closed

Warn about/disallow @JsonCreator on enum constructors? #1272

Stephan202 opened this issue Jun 19, 2016 · 3 comments

Comments

@Stephan202
Copy link
Contributor

It's currently valid to add the @JsonCreator annotation to an enum constructor, but that doesn't "do" anything. This might confuse users (it did confuse me when I found an occurrence in our code):

  • If an enum has a static method @JsonCreator then that method will be called upon each deserialization, while by definition an enum's constructor is invoked only once for each enum value.
  • Multi-param static method @JsonCreators yield an IllegalArgumentException (see also @JsonCreator not working on a factory with multiple arguments for a enum type #929), but @JsonCreator-annotated enum constructors with multiple parameters don't.
  • In combination with a @JsonValue-annotated method there may even be a mismatch between the number and types of arguments accepted by the constructor (this is the scenario I found in our code): it's unclear then what Jackson will do. (Until one realizes that the constructor's @JsonCreator annotation is bogus. That took me a while to figure out and is the reason I'm filing this issue.)
@cowtowncoder
Copy link
Member

This is a valid concern, as there is no valid use case for @JsonCreator on enum constructor.
There is the bigger question of how aggressively to flag annotations that can technically be placed in a location (that is, there's no way to limit this annotation from being added without preventing it on all constructors), but make no sense for a subset there. I am mostly worried about frameworks possibly adding such annotations, since throwing exception is disruptive; sometimes silent ignoral is more robust choice.

But if feasible I think it would be a good idea in this particular case to signal an error. There's no real mechanism to warn about things in Jackson (logging is avoided on purpose), so this would mean an exception.
The practical question is whether it'd be easy to do this.

Given that I plan on rewriting the whole creator introspection system for 2.9 (if possible) or later minor versions, I think this would naturally fit in with that work, unless there's a simple enough change to make that work with current code.

@cowtowncoder
Copy link
Member

Quick note: #929 is now actually supported, so at least multi-argument factory methods are ok.

@cowtowncoder
Copy link
Member

Since @JsonCreators are supported, to some degree, not sure what to do here. I am sure there are ways to improve handling, error-detection, but would need more concrete plan. Closing; may be re-filed with proposed validation logic consider currently valid usafge.

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