jackson-databind icon indicating copy to clipboard operation
jackson-databind copied to clipboard

treat creator properties as required by default in 3.0

Open soc opened this issue 2 years ago • 7 comments

Is your feature request related to a problem? Please describe.

With records becoming more prevalent, the two suggested workarounds to make properties required would mean that most of the code in that record would actually deal with making Jackson behave:

public record Person(String firstName, String lastName) // all params optional

// workaround 1
public record Person(
  @JsonProperty(required = true) String firstName,
  @JsonProperty(required = true) String lastName)

// workaround 2
public record Person(String firstName, String lastName) {
  public Person {
    Objects.requireNonNull(firstName);
    Objects.requireNonNull(firstName);
  }
}

Describe the solution you'd like A clear and concise description of what you want to happen

Flip the default, such that all properties not marked with @JsonProperty(required = true) or being Optional are required.

Usage example

public record Person(String firstName, String lastName) // all params required

Additional context

All people I encountered have been unaware of the current default behavior, and considered it unexpected/undesirable when they found out about it.

soc avatar Jun 03 '22 13:06 soc

Documentation reads:

    /**
     * Global default setting (if any) for leniency: if disabled ({link Boolean#TRUE}),
     * "strict" (not lenient): default setting if absence of value is considered "lenient"
     * in Jackson 2.x. Default setting may be overridden by per-type and per-property
     * settings.
     *
     * @since 2.10
     */

Even with best efforts (and having observed the behavior) I cannot parse this sentence – would a PR be accepted to work on the wording, regardless of this issue itself?

soc avatar Jun 03 '22 14:06 soc

@soc the documentation simply means that this is the default value for the 'lenient' setting

yawkat avatar Jun 03 '22 14:06 yawkat

Not sure about "simply" ... that sentence has 4 ifs, 3 pairs of parentheses and 2 nested colons.

Edit: Looks like FAIL_ON_MISSING_CREATOR_PROPERTIES is what I'm looking for in 2.x.

soc avatar Jun 03 '22 14:06 soc

Ok, so, first yes, documentation improvements are always appreciated.

But as to required properties: one challenge is that Records as a type are quite new and were not around when Jackson was originally developed. As such expectations one has with Records can differ a lot from POJO -- and this is partly the reason why Jackson defaults to "you give what you have, we make best use" and generally is quite permissive. The whole support for required properties (and only via Creator methods) is an add-on.

Having said that, there is no strict reason why defaulting and expectations could not differ between the two so said perhaps Record types would default to handling where all values were required. There could be a MapperFeature to control this, separate from POJOs (there could then be 2 MapperFeatures). I am not sure most users would expect or want strict required for POJO cases; although many do. So this seems like a configurable case.

But one big blocker for some of the improvements is that the introspection of properties needs to be rewritten: currently there are cases where Creator properties and others are not properly linked -- this is the root cause of majority of existing Record bugs. And the support for Record types is bit of a hack as well; but fixing that is tied up with property introspection.

I really hope to somehow figure out how to have more time to work on above-mentioned rewrite: it has been high on my todo list for past 2 years, but it does require kind of focus that has been difficult with my current day job (unrelated to Jackson). But after completing 2.14 (for which it won't happen) I do hope to focus on this rewrite, to unblock tons of other improvements.

Having said all that, I am sure documentation part for what exists currently can be improved.

cowtowncoder avatar Jun 06 '22 15:06 cowtowncoder

Looks like FAIL_ON_MISSING_CREATOR_PROPERTIES is what I'm looking for in 2.x.

Scratch that – this option pretty much breaks on optional properties. Neither @JsonProperty(required = false) nor Optional<...> work with it.

The option also makes Jackson demand an explicitly specified property name in @JsonProperty (it worked fine without it before).

I have trouble seeing any scenarios where those particular behaviors would be desirable.

soc avatar Jun 10 '22 12:06 soc

@cowtowncoder

Having said that, there is no strict reason why defaulting and expectations could not differ between the two so said perhaps Record types would default to handling where all values were required.

I think it would be preferable if a future version (e. g. 3.x) would have sane and consistent defaults, regardless of whether a class or record is used. (Same with JsonCreator.Mode currently not being consistent on class vs. record.)

E. g. given

public record Person(String firstName, String lastName, Optional<Integer> age) {}

and

{  firstName: 'Joe' }

I want Jackson to complain about lastName missing, but not about age missing; and that shouldn't change if I replace record with class.

soc avatar Jun 10 '22 12:06 soc

There is plenty of room for improvement. At this point I lack time to do much about implementation; limitation isn't knowing what to improve but having time to do major underlying changes.

cowtowncoder avatar Jun 13 '22 02:06 cowtowncoder