[FEATURE] Jacksonized would use NonNull to set JsonProperty(required = true)

[almson]

Describe the feature
Related to #2673, Jackson uses @JsonProperty(required=true) to document required properties. @Builder uses @NonNull (according to #1043, although it's not documented). It would be nice if the Jackson metadata was generated from the corresponding Builder metadata. (Actually, a @Builder.Required annotation that reuses @Builder.Default machinery might look nicer and be more appropriate.)

Describe the target audience
This feature benefits those who produce documentation and schemas of their Jackson classes. For example, I use https://github.com/mbknor/mbknor-jackson-jsonSchema It is really awesome because it works with https://github.com/json-editor/json-editor to automatically generate web forms.

Example
Here is the desired syntax:

@Jacksonized @SuperBuilder
static class Config
{
    @NonNull
    final Foo value;
}

Here is code that I currently write:

@Jacksonized @SuperBuilder
static class Config
{
    @NonNull
    @JsonProperty(required = true) 
    final Foo value;
}

Granted, it's not a big savings, but it seems natural.

[rzwitserloot]

This one is a ton more straightforward. Lombok already has a system to recognize nonnull annotations specifically, so there's no real question as to how to identify them (there are like 10 different libraries with nonnulls that mean this). One slight issue is the confusing ones. For example, JPA has @NotNull annotation and bizarrely this has zip squat to do with java null references, that is a hint for JPA that, if the POJO is used by JPA to generate a CREATE TABLE SQL statement, to add a NOT NULL constraint to the generated SQL, and the POJO itself should probably just totally allow null there, because JPA POJOs are allowed (and in fact, must neccessarily) be capable of holding values that cannot yet be inserted in the DB (for example, an auto-gen ID key isn't set if the object isn't saved yet).

So that's one small source of annoyance: What if you annotate a field with a JPA @NotNull or some other annotation whose terminology smacks of 'not null' but which doesn't actually mean "No instance of this field is ever supposed to hold null at any time"? Would that lead to confusion?

But, that problem is rampant through all of lombok and is unsolvable, it's not our fault (example: If we generate a setter for a @JpaNotNull field, we do not nullcheck the input and freely allow .setX(null) for it. Which is confusing, but better than the alternatives.

Is 'required = true' an ancient addition to @JsonProperty, or, better yet, has been there since initial release? It'd be a bit of a shame if those working with older jackson versions end up with compilation errors because we generate an annoparam that doesn't exist in their older version.

[almson]

require=true has been around since Jackson 2.0.

However, @NonNull might be problematic for several reasons:

• Jackson says a required property is considered set even if it's set to null (not that this makes much sense to me).
• It's a bad fit because required is conceptually mutually exclusive to @Builder.Default but it shouldn't be forbidden to combine @NonNull and @Builder.Default.
• It would be weird to apply @NonNull to a primitive type, even though it would need to be. (They're treated the same way as reference types in Jackson. They take on their implicit default values if not set, and are not necessarily required.)

So this would look better with a @Builder.Required annotation.

[rzwitserloot]

... sigh. Nothing is ever easy with persistence and mapping, is it? I'm pulling the accepted tag for now. The problem with @Builder.Required is that having this become a 'builder intrinsic', where builder will refuse to build() unless you set that field, but .setX(null) is fine, feels not useful enough. And if it's just for the purposes of adding that annoparam to @JsonProperty, it needs to be in lombok.extern, but it also wants to an inner type of @Builder.

[janrieke]

Just a s remark: Since Jackson 2.6, required is not purely for documentation, but actually changes the behavior of Jackson. We have to be careful when automatically generating this.

[almson]

@janrieke What it does lines up with what I've outlined, which is that {"X": null} or {"X": 0} is ok but {} will throw an error on parsing. Also it currently only affects @JsonCreator constructors not setters which is what Builder uses..

[rzwitserloot]

And with that, this feature is now rather related to #2669 I think. And that one is going nowhere about as fast as this is :(

[almson]

Alternate, even better, proposal:

Mark @JsonProperty(required=true)anything that is not marked as @Builder.Default. If it doesn't have a default, it's documented as being required. If you want it documented as being default null, explicitly set default null. Simple and crystal-clear.

This doesn't add new annotations, doesn't add semantically conflicting checks like @NonNull, but is also compatible with @NonNull.

[rzwitserloot]

This doesn't add new annotations, doesn't add semantically conflicting checks like @nonnull, but is also compatible with @nonnull.

But it is backwards incompatible. Code you write today without builder.defaults and with @Jacksonized produces @JsonProperty today, and would produce @JsonProperty(required = true) tomorrow. We'd have to lean on 'well, jacksonized is still kinda experimental' in order to handwave that one away.

I think it is okay to do that, but will wait for some more input. Our experimental support of checkerframework's @Called annotations work similarly (no default? Then they are required), so that's at least internally consistent.

[almson]

I'm having trouble finding more info about Lombok+CheckerFramework (eg the @Called annotation). Can you point me to some?

[rzwitserloot]

@almson said:

I'm having trouble finding more info about Lombok+CheckerFramework (eg the @called annotation). Can you point me to some?

Yeah, I'm waiting for the docs too. The basics were communicated to is in private email from Michael Ernst (one of the checkerframework.org leads), we were under the impression the annotations we talked about would be added soon, but they aren't really there.

I'll try to explain the basics of it, but first, I need to explain this extremely exotic java feature.

This is legal java:

class Foobar {
    void hello(Foobar this) {}

This java, if compiled, would compile to the exact same byte code as:

class Foobar {
    void hello() {}

in all ways; e.g. trying to use reflection to get a handle on hello would require calling Foobar.class.getMethod("hello");, not Foobar.class.getMethod("hello", Foobar.this); you can't call it with hello(someFoobarInstance), and if you call .getParameters() on the j.l.r.Method instance, you get an empty array back.

Seems pointless, but, there is a point to it all: It lets you annotate the receiver. That's a fancypants trick that so far no library I ever heard of actually uses, except this checkerframework idea. The idea being that you could write, say, this:

class Foobar {
    public void init() {}
    public void close(@Called("init") Foobar this) {}
}

This is a typetag concept: It is saying that the close() method doesn't exist on all expressions of type Foobar. It only exists on those expressions of type Foobar upon which init() has been called at least once, therefore:

new Foobar().close();

is semantically speaking as illegal as new Foobar().ladieda(); would be - that method (close) does not exist on that type. The point of checkerframework is static analysis, and that's exactly how this would go: checkerframework would generate a warn flag on new Foobar().close() to state that you've violated the 'expanded typing system' that checkerframework operates on, because java itself can't do this. I call this 'type tagging': You've got the type Foobar, tagged with a property that interacts only with pluggable stuff. In this case, checkerframework is the plugin, and the tag is 'static analysis proves that init() has been called on this expression'.

You can ask lombok to generate this stuff to tackle builders and mandatory params. This way, if you write, say: Person.builder().name("Joe").build();, that you get an at-write-time error flagged on this with the message: "You must invoke birthDate() and studentId() on this builder before calling build()" - an oft requested feature, often requested in a boneheaded fashion (the so-called 'cascading builder' pattern. Which we think is a mistake).

[janrieke]

Mark @JsonProperty(required=true) anything that is not marked as @Builder.Default. If it doesn't have a default, it's documented as being required. If you want it documented as being default null, explicitly set default null. Simple and crystal-clear.

But, unfortunately, not in accordance with the Jackson semantics. Jackson does not do any validation by default, only if you request it explicitly. And no validation is what most people expect. Changing that will definitely be surprising and will cause too much harm.

[almson]

@janrieke Jackson doesn't do any validation on setters marked required.

[almson]

@rzwitserloot That's very interesting. I've heard of Checker Framework. It's intriguing. Essentially huge chunks of Java syntax were added just to support it. (Like... explicit receivers!) Yet I've never seen it used and it seemed just a research project. Glad to see it slowly going somewhere.

In the distant future when I write my Python-killing Java ML library, I plan to use it to annotate tensor shapes.

In the meantime, I've wanted to annotate https://github.com/almson/almson-regex which is a Regex builder library. The library ignores OOP for this task and passes around Strings (which makes it so much lighter and easier to use). At the same time, it could benefit from checks. There are 4 types in a regex: charclasses, expressions, quantifier expressions, and replacement expressions, and it would make the library easier to use if the compiler prevented them from getting mixed up.

Do you know if CheckerFramework makes it easy to create such a basic type hierarchy? I think there's many problems where a full-blown OOP approach is too heavy-handed, but some sort of light, auxiliary, easy-to-ignore type tagging is just right.

As for required Builder methods: Yeah, I found some epic threads about that:

• https://stackoverflow.com/questions/29885428/required-arguments-with-a-lombok-builder
• Mark fields as required for Builder #1043

Really, I don't see a huge problem in doing the check at runtime (maybe I've been doing too much Python). As they say--better late than never. But compile-time would be great too.

I'm a bit surprised at the @Called checker, though. It's perfect for builders, but if you're passing an object around there's no way the compiler can be sure if init was called before close. You'd need additional rules, similar to final. Sounds extremely hairy. I would also like to see this problem addressed in a general way using a generic type tag mechanism rather than with custom annotations. Something like

class FooBuilder
{
    @TypeTag("age-is-set", pass: "T")
    public FooBuilder setAge (@TypeTag(capture: "T") FooBuilder this, int age);

    public Foo build (@TypeTag({"age-is-set", "name-is-set", ...}) FooBuilder this);
}

[janrieke]

Jackson doesn't do any validation on setters marked required.

That's true for the moment. However, @JsonProperty(required=true) has already changed its semantics in the past. At first, it was purely for documentation purposes, and from 2.6 on it triggers a validation for @JsonCreators. The Jackson maintainers already stated that it is their wish to extend this behavior to other instantiation means.
As soon as Jackson changes its behavior, Lombok will start getting bug reports on this (and that will be correct, it will be a bug then).
Thus, I strongly object to this "mark-as-required-by-default" approach.

[almson]

I asked the Jackson team if/when/how they plan to handle this breaking change. FasterXML/jackson-databind#2618 (comment)

[almson]

Jackson team answered, saying that if implemented there would be an opt-in mechanism. Also, nobody is working to implement it.

[kun-zhou]

Perhaps a good behavior is to assume NonNull and only drop the constraint if a field is annotated as Nullable.