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

Add MapperFeature.SET_PROPERTY_CREATOR_AS_DEFAULT for handling single-arg constructor as property based #1631

Closed
lpandzic opened this issue May 19, 2017 · 3 comments
Milestone

Comments

@lpandzic
Copy link
Contributor

Add a feature flag to change the default behavior of handling single-arg constructor as property based (to eliminate unnecessary @JsonCreator needed otherwise).

This is a continuation of #1498 but it should be done in backward compatible manner (flag should be disabled by default).

cowtowncoder added a commit that referenced this issue Jun 7, 2017
added SET_PROPERTY_CREATOR_AS_DEFAULT MapperFeature
@cowtowncoder cowtowncoder added this to the 2.9.0.pr4 milestone Jun 7, 2017
@cowtowncoder cowtowncoder changed the title Flag for handling single-arg constructor as property based Add MapperFeature.SET_PROPERTY_CREATOR_AS_DEFAULT for handling single-arg constructor as property based Jun 7, 2017
cowtowncoder added a commit that referenced this issue Jun 7, 2017
…lso renamed feature as `MapperFeature.CREATOR_MODE_DEFAULT_PROPERTIES`
@cowtowncoder cowtowncoder reopened this Jun 7, 2017
@cowtowncoder
Copy link
Member

Quick note: changed feature name to MapperFeature.CREATOR_MODE_DEFAULT_PROPERTIES.

However: fundamentally patch as suggested is wrong. It does claim that a constructor or factory method has explicit indicator for use as Creator (equivalent of @JsonCreator); this should not be claimed.
I changed logic so that setting will override case of using plain @JsonCreator (which leaves "mode" as DEFAULT), and this will allow changing handling of single-argument constructor/factory-method.

The problem I have in forcing discovery of creators is that:

  1. It will expose all kinds of constructors, not just public ones, possibly indicating methods that are never meant to be used as Creators
  2. Since all constructors (and possibly factory methods) are exposed, it is possible to have conflicts: worse, it is not possible to resolve those with explicit @JsonCreator (because feature would essentially claim all constructors have one)

and although users would only hit problems if enabling this feature, the feature itself would then be of questionable value.

I am not 100% sure if there is value for feature as per my changes; it does allow slightly reducing boilerplate. Feature could also perhaps be used by parameter names module, or other functionality, even if they need to add complementary functionality.

@lpandzic
Copy link
Contributor Author

lpandzic commented Jun 11, 2017

In my opinion if there's a case where you have to add annotations to support value types, then that's a big usability problem and it breaks Principle of least astonishment.

It will expose all kinds of constructors, not just public ones, possibly indicating methods that are never meant to be used as Creators

Can we limit this feature then to only public constructors?

I would like to see exact scenarios where this feature, as was requested, would cause problems.
None of the JDK classes, for example, have parameter names for parameters of their constructors.

@cowtowncoder
Copy link
Member

@lpandzic The method in AnnotationIntrospector indicates forced inclusion, if it returns non-null value. It can not and should not be filtered base on visibility, as it is used to override use. You can't have your cake and eat it.

As to JDK types not having parameter names: is this really the case? Are JDK types not compiled with this information? I am not sure I would want to rely on that.
But more than JDK types this would affect existing code by existing users: constructors are often defined for various purposes, and which if any should be used as constructor for deserialization should not change unpredictably. I am convinced this is what would happen to many users.

I feel we are going round and round in circles again. I will remove the feature, and not consider any of this for 2.x. With 3.x there is need to rewrite introspection, and compatibility changes are acceptable for major revision. Exact logic will need a lot of work; but fundamentally whole POJO introspection will have to sort of start by considering Creators to use, as this will affect further determination of properties. As part of this phase it should be possible to both configure some aspects and to configure handlers.

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