Optional creator parameters

[Verdent]

Fixes #121

I have decided to drop the new @JsonbDefaultValue annotation and focus on the basics first.

[rmannibucau]

Hi @Verdent , what about just enabling missing parameters and defaulting for primitives only? Seems it will be backward compatible functionally and avoids any new config.

[Verdent]

This solution is backwards compatible also. Where do you see the problem? I am not sure why do you think, that new config option is a bad thing.
I have dropped the defaulting of the primitives in this version. It will not be supported. If one wants to use primitive type, wrapping type should be used instead. We can always add the defaulting for primitives later.

[rmannibucau]

This solution is backwards compatible also.

Agree.

Where do you see the problem? I am not sure why do you think, that new config option is a bad thing.

Any toggle is a bad thing and has impact on downside libraries. A simple example is that most of the times the requirement of the value being passed is disabled in impl and consumers like schema generators consider it true by default, with such a toggle instead of aligning on all instantiations, it will require to be integrated in downside libs to be usable properly. Without the toggle we enable users for a long requested feature, don't really break anyone since the feature is broken as of today and we don't require libs/integrators to support a new toggle, so only goods IMHO to just change the doc/expectations and not the code.

We can always add the defaulting for primitives later.

Ok for me.

[ljamen]

Small changes - looks good!

[m0mus]

LGTM

[rmannibucau]

Any hope we drop @JsonbRequired and drop any misplaced validation from JSON-B to align it in jakarta platform and reduce the validation burden in JSON-B itself? Would also enable to use Records properly without any hack or new specificity which is a big pro instead of polluting the spec with new concepts IMHO.

[Verdent]

I don't see any misplaced validation to be fair.

I am not sure what do you see as a problem between the records and this PR. Why are you still so much against adding anything to the spec?

[rmannibucau]

I don't see any misplaced validation to be fair.

once again, requirement of a field is a validation, there is no validation in JSON-B except this one so if we go the validation way (@JsonbRequired) then we must also support all JSON-Schema validations - yes still a draft and not what we want since it is not the right jakarta spec level.

This is why I strongly think we should just relax the error done in 1.0 with a toggle for existing applications which could maybe rely on that - none in practise? ;) - and be it.

Also note that current PR breaks 1.0/2.0 by changing the default and just adds the required validation API so I just request to not add the annotation and not let JSON-B validated anything.
Users wanting to validate in a creator the presence of a field can do a requireNonNull or anything else more meaningful in terms of exception mapping in all context, including JAX-RS with an ExceptionMapper.

For records you can end up defining a constructor if you fix previous point (backward compat breakeage) whereas I don't want in most cases. If you keep it, it is ok for them and it is the same for records and classes.

[Verdent]

once again, requirement of a field is a validation, there is no validation in JSON-B except this one so if we go the validation way (@JsonbRequired) then we must also support all JSON-Schema validations - yes still a draft and not what we want since it is not the right jakarta spec level.

No we don't need to support anything else. This is no special validation. Just a requirement for it to be present in the JSON document in some way. Nothing else. That's the same as it is right now and we are not supporting any extended validation either.

Any kind of validation is up to the user. We are not doing any.

Also note that current PR breaks 1.0/2.0 by changing the default.

And that's the reason why our current SNAPSHOT is 3.0.0. This PR will be introducing backwards breaking change.

[Verdent]

I am not sure how you would expect this change to be done... I used to have the switch on the config level to keep backwards compatibility, but you were against it. I removed that and you against it again? How do you expect this to be done?

[rmannibucau]

I used to have the switch on the config level to keep backwards compatibility, but you were against it.

yes, it is not at config level but JVM level to keep existing app working. you need to change the config? you recompile so it does not work - and once again, for the 1 or 2 apps using that feature it is really about making it working with a global flag, not a jsonbconfig.withCompat() kind of thing IMHO. So more a jsonb.enableNullableCreatorParameter read in a system property by the impl. I'm also happy if we don't specify it and let it implementation specific while we just enable creator to receive null parameters.

This is no special validation. Just a requirement for it to be present in the JSON document in some way.

This is called a validation, no need to play with words. Look, being a number is a requirement on the type, right? Being between 1 and 10 is a requirement on the range of the value, right? It is all the same really and the impl must take care of the content instead of just binding it when possible, so at the end it is validation and out of mapping scope.
Maybe another way to see it is that it is equivalent to a @NotNull of bean validation ;).

Any kind of validation is up to the user. We are not doing any.

Well, in 1.0 no because presence validation is done and this is the only thing to relax in 2.1.

And that's the reason why our current SNAPSHOT is 3.0.0. This PR will be introducing backwards breaking change.

Well there are two things on that:

1. We must always provide a way to run an existing app - not under dev - so we need the global toggle as mentionned (otherwise we defeat jakarta and why it is better than some concurrent), even for breaking changes which should be pretty rare.
2. The needed change does not require @JsonbRequired at all and this is what I request to drop.

[Verdent]

Maybe another way to see it is that it is equivalent to a @NotNull of bean validation ;).

Not true. I was thinking of using @NotNull, but significant difference is, that we are not validating if the property has been null in the JSON document or any actual object/value and therefore it is not the same. This @JsonbRequired adds requirement for the parameter with the given name to be present in the document, but does not care about the value assigned to it.

Even if we call it validation, it doesn't change a thing. Since the current state is exactly like that and no extended validation is happening. There is no reason why we should introduce that.

We are having @JsonbNillable also and we are also not claiming that since it is also the validation we should cover the validating if it should be present in the JSON only in cases where the value is between 1 and 10 etc. That's the same logic and does not make sense.

The needed change does not require @JsonbRequired at all and this is what I request to drop.

Well, from my point of view it is desirable to have a way, how to mark some parameters/creators as optional/required even when the global setting requires the different.

[rmannibucau]

Not true. I was thinking of using @NotNull, but significant difference is, that we are not validating if the property has been null in the JSON document or any actual object/value and therefore it is not the same. This @JsonbRequired adds requirement for the parameter with the given name to be present in the document, but does not care about the value assigned to it.

Hmm, sorry @Verdent but it reads as "not true, you are right" from my side.

We are having @JsonbNillable also and we are also not claiming that since it is also the validation we should cover the validating if it should be present in the JSON only in cases where the value is between 1 and 10 etc. That's the same logic and does not make sense.

Does not make much sense for me because it is on serialization side, here we are talking about deserialization so nillable applies as a filter, not a validation in terms of design, no failure.

Well, from my point of view it is desirable to have a way, how to mark some parameters/creators as optional/required even when the global setting requires the different.

Ok, let's step back another time and explain you again the way I see it cause I don't disagree with the fact user should be able to handle this requirement but there are a few point we diverge in terms of design so let's explicit them one more time:

1. As you mentionned such validation applies at parser/reader (I consider them 1-1 from JSON-B perspective) level so any validation belongs to JSON-P level - since JSON-B is built on top of it.
2. Another important point is that once you disabled validation by default it is trivial for an user to do the validation in the creator (if v == null throw) and most of the time the validation will be a bit more (if v == null && v > 10 throw for ex), this is why it is quite pointless in practise to handle any validation, even the presence.
3. When you don't care about the presence semantic but the actual payload presence - what you described which is a subset of validation - you do JSON-Schema (or equivalent) validation at a higher level (servlet filter, configuration loading etc) and then map the validated payload to a POJO/record. Note that in terms of implementation, both the streaming and in memory validations are fine (in particular since now JSON-B implementations support JsonObject -> POJO mapping even if not yet fully specified but I guess this one is less controversy)

So overall, I can see the "it is easy to do so let's do" point but in terms of consistency, design and user need, it does not reach much its goal and you let the end user with the same exact need so let's just enable them - disable the default requirement of < 3.0 - and don't add any noise and incomplete feature.

[m0mus]

@JsonbRequired is added for a case when the parameters must be present. It cannot be considered validation.
Records cannot be supported because we must stick to Java 11 code level. It's the platform requirement.

I am merging this PR and closing the issue.

[rmannibucau]

@m0mus -1, both statements don't make sense, let me explain why

@JsonbRequired is added for a case when the parameters must be present. It cannot be considered validation.

Presence is a validation, no way to workaround it and it belongs to JSON-P - if desired - or application code as explained.
So really, @JsonbRequired is broken by construction until you support all JSON-Schema features which is likely not happening for good reasons so please revert.

Records cannot be supported because we must stick to Java 11 code level. It's the platform requirement.

Jakarta (JavaEE) always supported java > now because it is what users expect.
Any design decision must be taken in consistency with the known Java features of the coming version when close enough - and an already released version is close enough ;). Jakarta is going Java17 anyway so records will be there so any API must be record friendly - one of the reasons we made @JsonbXXX with target=parameter in this release.