Deprecate @JsonbProperty.nillable() in favor of @JsonbNillable on fields

[jsonbrobot]

While running a hands-on-lab with people wanting to get acquainted with JSON-B a number of participants attempted to apply the @JsonbNillable in places where the @JsonbProperty with boolean nillable should be used.

While the javadocs on @JsonbNillable point in the right direction for the desired functionality, for some people this is not quite intuitive.

Could this be made easier on new JSON-B users by making the annotation @JsonbNillable applicable to places where @JsonbProperty should now be used, or would there be objections making this not feasible/desirable?

[jsonbrobot]

@bravehorsie Commented
That will introduce problem with ambiguous configuration, for example:
@JsonbProperty(nillable=false)
@JsonbNillable
Strig property;

So which nillable takes precedence here? Please note we can't check if both are set to raise exception in that case because of @JsonbProperty default nillable value for example when declared as @JsonbProperty("propName") it defaults to false.

[jsonbrobot]

• Issue Imported From: https://github.com/javaee/jsonb-spec/issues/52
• Original Issue Raised By:@bvdzwet
• Original Issue Assigned To: Unassigned

[aguibert]

It's too bad there are two separate way to configure nillable with JSON-B annotations. If we could have a do-over, I think it would make more sense to just have @JsonbNillable and not have @JsonbProperty.nillable() at all.

Perhaps we could attempt to move towards re-aligning this in the next version of JSON-B by deprecating the @JsonbProperty.nillable() attribute in favor of @JsonbNillable on the field, and make @JsonbNillable take precedence when used on a field. This would not be a breaking change because JSON-B 1.0 applications were not able to apply @JsonbNillable to a field.

[rmannibucau]

Hmm, it would require users to migrate - to not have deprecated code - without real gain. Maybe just some documentation to adjust on jsonb website since it is not a blocker?

[aguibert]

It would be a very simple migration for users, and the gain is that it clears up user confusion. I have been confused myself about @JsonbProperty.nillable() vs @JsonbNillable before, and I had to fire up an IDE and actually try out the API before it made sense.

[rmannibucau]

To be fair on that topic I didnt see any confusion so wonder if it is a big enough issue to break an API (deprecating leads to rework in several projects by dev practises so is as kmpacting as breaking).
Also once the doc is read it is not ambiguous and cant be so think we can keep it like that.

[aguibert]

I don't agree that some projects moving to use @JsonbIgnore everywhere would be a breaking change. Do you have a scenario in mind of how this could cause an inconvenience for users?

In the event of a conflict (i.e. @JsonbProperty and @JsonbNillable specified on a field), then we would make @JsonbNillable win out because it would be the newer way of specifying nillable.

The full nillabe resolution strategy could be as follows:

1. Look for @JsonbNillable annotation on the field
2. [Deprecated] Look for a @JsonbProperty annotation on the field
3. Look for a @JsonbNillable annotation at the class level

[rmannibucau]

Yes, the policy on some projects is to not use deprecated api - it just makes the build failling so even if not a breaking change by signature, it is lived as one.
It can be ok when it is really dropping a feature or replacing it with something better but here it is strictly equivalent and does not bring anything to the user.
It can even be seen as a regression and the annotations hell java sometimes get (you have 1 line of code and 10 annotations to summarize it).
So the point here is really: is the negative side to deprecate and ask users to move - cause this is what deprecating means - compensated by a new feature? Clearly not here since your 1. and 2. is the exact same user feature.

[m0mus]

I agree with @aguibert. We should deprecate JsonbProperty.nillable, extend usage of @JsonbNillable and clarify that it's the recommended way in the spec.

[aguibert]

@rmannibucau I think the two different ways of doing the same thing should never have made it into the 1.0 spec, and I am hoping we can migrate away from that in the next version of JSON-B.

I still disagree with your argument that this is a breaking change. If projects have configured their projects to fail if compiler warnings are found, then they have decided to opt-into a more brittle build in favor of having a "clean" codebase that follows the most modern practices.

It can be ok when it is really dropping a feature or replacing it with something better but here it is strictly equivalent and does not bring anything to the user.

The end goal would be in a distant future release of JSON-B we would eventually remove the @JsonbProperty.nillable() attribute, but before we can remove it completely it should be deprecated for a while to give users time to migrate.

[rmannibucau]

@aguibert point is that this assumption is wrong:

think the two different ways of doing the same thing should never have made it into the 1.0 spec

The idea was to limit the number of annotation on the fields/properties (like @resource instead of 4-5 annotations). We missed the adapters/(de)serializers though :(.

[aguibert]

In general I agree that limiting the number of annotations is better than having multiple annotations.

However, if there is a case where an annotation can be applied to a class that "inherits" down to all fields, it is better to allow that same annotation to be applied at the smaller scope, rather than the user needing to know that it becomes an attribute of a different annotation when applied at the field/method level. For example, consider JUnit's @Ignore (or now @Disabled) annotation. You can apply the annotation to a class to ignore all tests in the class, or you can apply it to a specific test method.

[rmannibucau]

Strictly speaking it can make sense but it is important to not forget spec already exists and to confirm to users we dont change API at will without any strong reason. IMHO it is not one here since it does not enable any new use case and does not simplify the API (1 method for 1 annotation).

[aguibert]

Closed #52 as a dup of this. At min, we should update javadoc to minimize confusion here.