jakartaee / jsonb-api

Jakarta JSON Binding
https://eclipse-ee4j.github.io/jsonb-api/
Other
79 stars 39 forks source link

Cannot use @JsonbCreator with absent fields #121

Closed emattheis closed 2 years ago

emattheis commented 5 years ago

In my experience, it is quite common to have JSON documents whose properties are present/absent depending on the scenario in which they are used. For example, the following two JSON documents might both represent a user:

{
  "firstName": "John",
  "lastName": "Doe"
}
{
  "firstName": "John",
  "middleInitial": "Q",
  "lastName": "Public"
}

I can easily map these documents to a Java class using the classic JavaBean pattern and JSON-B will be happy to deserialize either of the two documents above:

public class User {
    private String firstName;

    private String middleInitial;

    private String lastName;

    public String getFirstName() {
        return firstName;
    }

    public void setFirstName(String firstName) {
        this.firstName = firstName;
    }

    public String getMiddleInitial() {
        return middleInitial;
    }

    public void setMiddleInitial(String middleInitial) {
        this.middleInitial = middleInitial;
    }

    public String getLastName() {
        return lastName;
    }

    public void setLastName(String lastName) {
        this.lastName = lastName;
    }
}

However, if I want to map these documents to a Java class using an immutable pattern leveraging the @JsonbCreator annotation, it is disallowed by the spec:

import javax.json.bind.annotation.JsonbCreator;
import javax.json.bind.annotation.JsonbProperty;

public class User {
    private final String firstName;

    private final String middleInitial;

    private final String lastName;

    @JsonbCreator
    public User(@JsonbProperty("firstName") String firstName,
                @JsonbProperty("middleInitial") String middleInitial,
                @JsonbProperty("lastName") String lastName) {
        this.firstName = firstName;
        this.middleInitial = middleInitial;
        this.lastName = lastName;
    }

    public String getFirstName() {
        return firstName;
    }

    public String getMiddleInitial() {
        return middleInitial;
    }

    public String getLastName() {
        return lastName;
    }
}

Section 4.5 say:

In case a field required for a parameter mapping doesn’t exist in JSON document, JsonbException MUST be thrown.

In my opinion, usage of @JsonbCreator should be compatible with any document that can otherwise be deserialized by the default mapping methods, so this spec requirement is at odds with Section 3.14, which says:

The deserialization operation of a property absent in JSON document MUST not set the value of the field, the setter (if available) MUST not be called, and thus original value of the field MUST be preserved.

rmannibucau commented 5 years ago

+1 agree there is no reason it is not supported - didnt test but should work on johnzon impl by default too

emattheis commented 5 years ago

Sorry, I hit the 'Comment' button before I had finished my entire post 😛

@rmannibucau the problem is that the spec specifically disallows this support, so one cannot write portable code for this pattern.

I do this all the time with Jackson, but I'm interested in moving to the JSON-B spec for portability/consistency.

rmannibucau commented 5 years ago

Yep

Point being most impls support it and this is not backward incompatible - until you were using it to test some fields were absent but this was an antipattern already. So no issue to do it

emattheis commented 5 years ago

Sure, but how to address it in the spec? I'm using Resteasy which brings in Yasson which doesn't support this because it follows the spec precisely. Any implementation that allows this behavior is by definition not spec compliant.

rmannibucau commented 5 years ago

Do a pr to change the wording of the spec ;)

emattheis commented 5 years ago

I would if I knew where the spec source code was. Is there a repo for that somewhere?

Also, I'm curious to know how that clause made it in to the spec in the first place since it wasn't in the public draft I first reviewed. It seems that someone thought it was important and one can certainly imagine ways to preserve the existing behavior while allowing more flexibility.

e.g.

@JsonbCreator(treatAbsentAsNull = true)
rmannibucau commented 5 years ago

Hmm not sure where it is in new repo, it was in old one (https://github.com/javaee/jsonb-spec). Personally Id prefer to not have that toggle at all, it does not make sense and adds complexity for almost no gain at all.

emattheis commented 5 years ago

I agree with you, but I'm also not sure why the clause in section 3.14 exists. I would prefer to have the behavior for absent properties be the same for @JsonbCreator and the default deserialization methods, so null or Optional.empty() should be inferred for setters/field assignment as well. I think trying to retain an existing value after a deserialization is a mistake.

To set a default value in the event of an absent property, I would prefer to see the introduction of another annotation that does that explicitly.

e.g.

@JsonbDefaultValue("- valid JSON string here -")

In practice, there is no good way to distinguish between an explicitly null property and a missing property once the JSON has been deserialized.

rmannibucau commented 5 years ago

If needed it can be done in the constructor, if forbidden the constructor validates it anyway (potentially with business value) so at the end there is not a single valid case not enabled and it would align constructor on setters/fields behavior which is way saner IMHO.

emattheis commented 5 years ago

Not sure I follow. As it stands today, if you have the following JSON:

{}

and you deserialize to the following class:

public class Value {
    private String value = "DEFAULT";

    public String getValue() {
        return value;
    }

    public void setValue(String value) {
        this.value = value;
    }
}

then getValue() should return "DEFAULT".

On the other hand, if you use this class:

import javax.json.bind.annotation.JsonbCreator;
import javax.json.bind.annotation.JsonbProperty;

public class Value {
    private final String value;

    @JsonbCreator
    public Value(@JsonbProperty("value") String value) {
        this.value = value;
    }

    public String getValue() {
        return value;
    }
}

then it MUST fail per the spec, but you're saying that johnzon works, in which case getValue() should return null, so the behavior is fundamentally different from the other class.

Trying to write a default in the setter or constructor doesn't work because your code cannot tell if there was an explicit null in the payload or if the property was absent (and, of course, it wouldn't work if you wanted to use field assignment directly for some reason).

That being said, I don't have any compelling reason to WANT a default value, but it seems like someone must given the language in the spec. I'd be happy to see it changed to state that null or Optional.empty() will be inferred for any mapped property missing from the JSON document.

rmannibucau commented 5 years ago

The point is that default or error cases must be handled in an user method anyway so only feature failing is parsing error which is not very valuable here since trivially replaced by error case. Doing the change dont break apps, so null is fine in all cases IMHO.

bravehorsie commented 5 years ago

@emattheis @JsonbCreator(treatAbsentAsNull = true) would prevent errors when property is missing in JSON unintentionally and is actually required. If set to true a constructor could further check a property for null to set the default value.

That being said, I don't have any compelling reason to WANT a default value, but it seems like someone must given the language in the spec. I'd be happy to see it changed to state that null or Optional.empty() will be inferred for any mapped property missing from the JSON document.

In order to set null for all missing properties implementation would have to post process every object after deserialization, because it would not be "called" explicitly to set a property with null when the property doesn't appear in the JSON document. Post processing would further slow down the performance.

rmannibucau commented 5 years ago

It depends a lot the impl and perf will not vary for most of them thanks to the buffering.

emattheis commented 5 years ago

The point is that default or error cases must be handled in an user method anyway so only feature failing is parsing error which is not very valuable here since trivially replaced by error case. Doing the change dont break apps, so null is fine in all cases IMHO.

@rmannibucau I agree, but I still think it's better for @JsonbCreator to behave the same way as setter/field access with respect to absent properties. I think section 3.14.1 is problematic because it describes asymmetrical behavior with regard to serialization and deserialization - i.e. null values result in absent properties when serializing, but absent properties will not necessarily result in null values when deserializing. Also, on a slightly related topic, the spec doesn't seem to mention how null values should be handled in a map.

In order to set null for all missing properties implementation would have to post process every object after deserialization, because it would not be "called" explicitly to set a property with null when the property doesn't appear in the JSON document. Post processing would further slow down the performance.

@bravehorsie couldn't the builder object responsible for constructing a type be pre-initialized with defaults for mapped properties? Then those values would naturally be present for anything the parser didn't explicitly fill in. I haven't dug that far into the Yasson code, but it seems doable with minimal overhead from a design perspective.

emattheis commented 5 years ago

@bravehorsie after looking into the Yasson codebase, I see what you mean. Still, it looks trivial to support null for absent properties in the creator case.

I think I'm coming around to @rmannibucau's point of view: relaxing the spec and supporting null for absent properties in creators should be very low impact - at worst you get a null value in a field where you used to get a complete failure to parse. The spec can point out the pitfalls of this behavior in contrast to the default behavior. If someone wants to mix and match creator and property/field access, they can still do so and retain the default behavior for certain fields. Those looking to leverage creators for immutable types will get what they expect.

I'll put together a PR for Yasson and I'm happy to submit a PR for the spec as well if there's a repo for that.

bravehorsie commented 5 years ago

@emattheis

couldn't the builder object responsible for constructing a type be pre-initialized with defaults for mapped properties?

I don't see that solution working. You can't instantiate a type with a @JsonbCreator with a "preset" null values, because you have to call that particular constructor with matching parameters and that would create another instance. Post-processing ready beans (or an input properties for them) seems as only option to me and having to iterate over properties which has not been found in JSON may not be super fast in all cases.

When setting nulls implementations would also have to deal with primitive types pre setting them with default values. Another consideration is that there are POJOs with preset default values not expecting to be reset to null, user logic may throw NPEs for those.

@rmannibucau Do you mean implementations shouldn't worry about performance because of buffering? Does that buffering apply in all runtime cases or only a part of them depending on the environment?

rmannibucau commented 5 years ago

@bravehorsie there is no case where bufferring is not used for parsing except a few ones for config files etc where the perf is not really impacting for this feature, all other runtime cases have buffering mecanism on top of the stream which will make fail early case not that helping on the perf side of things. Having the user impl throwing NPE or other exception is fine and more particular, if validation is done in constructors it is generally more than just null but ranges and other more advanced things the spec will not support until jsonschema is part of jsonp (which is not tomorrow since the spec is still moving). So at the end jsonbcreator is mainly used to be immutable which is a very good thing but also require to support null cases natively otherwise you can't be immutable at all without another layer (DTO on top of the DTO ;)).

bravehorsie commented 5 years ago

@rmannibucau So that means implementation considerations are not worth performance testing an optimization because it has little to no impact?

rmannibucau commented 5 years ago

@bravehorsie I'd rephrase it as "API shouldn't be impacted by implementation details, in particular when perf are not impacted enough for most users". Statistically webservices - http or not - will not be impacted by that at all (cause of the size of the median payload + the fact they will protect themself from a client attacking this way) and local file reading very very rarely impacted - except for big batches where it is fine to have the small latency impacted by dropping this feature.

bravehorsie commented 5 years ago

@rmannibucau Thank you for pointing that out. Perhaps when considering load in the webservices as a whole and network buffering, than jsonb impl optimizations may not be significant. But that may depend. However introducing new features in the API (like setting nulls for all absent properties) should take considerations on what impact it has on implementations IMHO. There are Java <-> JSON alternatives out there (not conformant to JSONB, not using JSONP), which are very well optimized and even in cases when time latency is not significant it may save some of your smartphone battery power.

rmannibucau commented 5 years ago

@bravehorsie hmm have to admit I'm tempted to say that if it is true at that level they will likely use jsonp and stay away from jsonb (for more reasons than just the networking). Also if you think about the usage, the server side is still mainstream for JSON-B I think - even if client one is not negligible - so we should ensure we match server side style programming and the immutable style which is not possible today. So I think null should really be the default as it was in 1.0 I think (or was it already there?) and we can discuss of another issue (feature request) to get a @JsonbCreateor(failIfPartial=true) - with failIfPartial=false by default) in a 1.1 or so. But it will require to have it on the class as well for setter style of mapping.

Wdyt?

bravehorsie commented 5 years ago

@rmannibucau Agree, server side is mainstream, but not necessary it is avoided on smaller devices, for example see here: https://github.com/eclipse-ee4j/yasson/issues/144

1.0 was the only final spec version that was ever released. I don't remember any of the changes regarding null values. Currently spec is forcing error on missing creator values. So in the next version we could get both - null values for missing creator params by default and @JsonbCreateor(failIfPartial=true).

What does not look useful to me is setting null values for all missing properties outside of creator params, which will force awkward implementation details. Hope this suggestion is being dropped.

rmannibucau commented 5 years ago

@bravehorsie hmm, there is no need of any "setNeutralElement/setNull" out of the "factory" method (constructor in general) since the jvm does it for you these days (IIRC it was not that guarantee < java 6 or something like that). BTW any idea when the next version can be released?

bravehorsie commented 5 years ago

@rmannibucau There is a process of passing specifications to Eclipse, an update will probably be possible after that process is complete.

emattheis commented 5 years ago

When setting nulls implementations would also have to deal with primitive types pre setting them with default values.

I had to deal with this for the Yasson PR as well

Another consideration is that there are POJOs with preset default values not expecting to be reset to null, user logic may throw NPEs for those.

Agreed. This is why I'm generally in favor of a more explicit way to specify a default via annotation. The current behavior seems more like a side effect of the parser implementation that made it into the spec.

What does not look useful to me is setting null values for all missing properties outside of creator params, which will force awkward implementation details. Hope this suggestion is being dropped.

I definitely understand the performance concerns better now, and I'm fine with dropping the idea of specifying null as the default across the board. However, I still think the current behavior should be undefined rather than an expected side effect.

That being said, I also think JSON-B should focus on developer productivity and main stream use-cases ahead of performance. There are obvious (and not-so-obvious) performance tradeoffs when making the decision to employ an object-mapping scheme, but developers who do so shouldn't be hamstrung by limitations in the name of performance. They are always free to fall back to JSON-P or other lower-level solutions. A developer writing to JSON-B is making a choice to value portability above optimization.

misl commented 5 years ago

I am trying to migrate from Jackson to JSON-B and I definitely miss the @JsonProperty 'required' attribute. If JSON-B had the same (or similar) attribute then the algorithm calling the @JsonbCreator method could easily the use this attribute to support absent/optional fields.

    @JsonbCreator
    public User(@JsonbProperty("firstName") String firstName,
                @JsonbProperty("middleInitial", required=false) String middleInitial,
                @JsonbProperty("lastName") String lastName) {
        this.firstName = firstName;
        this.middleInitial = middleInitial;
        this.lastName = lastName;
    }

An alternative solution might be to specify the optional fields as Optional<T> in the @JsonbCreator method.

    @JsonbCreator
    public User(@JsonbProperty("firstName") String firstName,
                @JsonbProperty("middleInitial") Optional<String> middleInitial,
                @JsonbProperty("lastName") String lastName) {
        this.firstName = firstName;
        this.middleInitial = middleInitial.isPresent() ? middleInitial.get() : null;
        this.lastName = lastName;
    }

As mentioned before: Section 4.5 of the spec says:

In case a field required for a parameter mapping doesn’t exist in JSON document, JsonbException MUST be thrown.

However the funny thing is that the specification does not mention how to specify a field in case it is NOT required (in other words optional).

rmannibucau commented 5 years ago

Spec defines it actually: setter or equivalent.

misl commented 5 years ago

Hmm, that's a weird definition of required (in my opinion). I can understand that a setter or equivalent is required to be able map a field (from JSON to object), but that does not necessarily make that field mandatory.

rmannibucau commented 5 years ago

It is the opposite, constructor defines how to make it required and setter optional.

misl commented 5 years ago

Do I understand it correctly that @JsonbCreator method is being called first for object instantiation and then additional setters are being invoked? Thinking of it, it sounds like the right thing to do. Unfortunately it breaks my immutable object :-(

The good part is that I now know how to get around my current issues. But still hope the specification will get enhanced to include something like Jackson already has ;-)

rmannibucau commented 5 years ago

Yes, right, optional could be a way then but personally Id prefer null to be supported OOTB s most impl do and let the constructor itself validate the values (null just being one particular value validation between hundreds).

emattheis commented 5 years ago

I think the crux of this issue is about distinguishing between absent properties in a JSON document and null-value properties. In my opinion,Optional<T> is a bad idea, because the meaning of empty is designed to be equivalent to having a value of null, and you wouldn't expect an Optional-typed parameter to ever be null, though the temptation is to do exactly that to indicate absence of the property in the document.

An Optional-like construct could certainly be used to support the distinction between absent and null, but I don't think we should abuse Optional<T> for this. Maybe a specific interface like:

interface JsonPropertyValue<T> {
    boolean isPresent();
    Optional<T> getValue() throws NoSuchElementException;
}
rmannibucau commented 5 years ago

Jsonb is about java where absent or null is the same so not sure it is worth it, null can likely skip the "set" phase for perfs in several cases anyway. Also null is still missing for the bean so same outcome functionally so dont think we need yet another wrapper of null.

emattheis commented 5 years ago

@rmannibucau I agree, just wanted to point out that Optional<T> doesn't really solve anything. I don't personally see why anyone would want the current spec behavior of throwing an exception over simply inferring null.

derekm commented 5 years ago

@ivangreene & I had to rip Yasson out of a Jersey- & WebResourceFactory-based client and convert to Johnzon in order to get over the hump of this issue combined with issue https://github.com/eclipse-ee4j/yasson/issues/283.

We require the use of @JsonbCreator to return our maps to their type-safe representation (instead of the disastrous stringly-typed representation that JSON-B forces on us in violation of the map's generic parameters & existing adapters), but then @JsonbCreator prevents us from allowing these maps to be optional.

No offense, but integrating these specs is an absolute mess.

nmatt commented 3 years ago

What's the status on this? Over a year has passed, and unfortunately the 2.0 spec still contains the same wording: "In case a field required for a parameter mapping doesn’t exist in JSON document, JsonbException MUST be thrown."

hendrikebbers commented 3 years ago

oh, sadly this was not targeted in the 2.0 released. And I fall into that pitfall today :( Month to late for JakartaEE 9.... But from my point of view this is a very important topic for the next release. Having support for records in the JSON binding would be great but this would only allow creating over constructor. Would like to help here. @m0mus is that already targeted?

Verdent commented 3 years ago

Hi, I think this needs to be addressed soon since we need the change to be in 2.1 . This is my proposal:

Absent creator parameters

Overview

Sample data

{
    "parameterOne": "parameter one value",
    "parameterThree": "parameter three value"
}

First example

If parameter is marked as JsonbNillable, it would be treated as optional and null value would be provided if the value is missing in the handled json.

public class SampleClass {

    @JsonbCreator
    public static Pojo creator(@JsonbProperty("parameterOne") String parameterOne,
                               @JsonbProperty("parameterTwo") @JsonbNillable String parameterTwo, // null
                               @JsonbProperty("parameterThree") String parameterThree){
        //Do some magic here
    }

}

Second example

It is not permitted to place @JsonbNillable annotation on the primitive type parameter. Instead, use @JsonbDefaultValue annotation which will mark this parameter as an optional and will provide the default value which should be used since null value is not passable to the primitive types.

If @JsonbNullable is present an exception should be thrown. @JsonbDefaultValue can be also used to the non-primitive type of String or any primitive type wrapping class. On every other places an exception should be thrown.

public class SampleClass {

    @JsonbCreator
    public static Pojo creator(@JsonbProperty("parameterOne") String parameterOne,
                               @JsonbProperty("parameterTwo") @JsonbDefaultValue("1") int parameterTwo, //1
                               @JsonbProperty("parameterThree") String parameterThree) {
        //Do some magic here
    }

}

Third example

If parameter is Optional type, it would be treated as optional and empty Optional value would be provided if the value is missing in the handled json. If @JsonbNillable is present, it will be ignored since Optional marked this parameter as optional.

public class SampleClass {

    @JsonbCreator
    public static Pojo creator(@JsonbProperty("parameterOne") String parameterOne,
                               @JsonbProperty("parameterTwo") Optional<String> parameterTwo, //Optional.empty
                               @JsonbProperty("parameterThree") String parameterThree) {
        //Do some magic here
    }

}

Fourth example

The last option is to set all the parameters to be optional via @JsonbCreator option. This will automatically mark all the parameters to be an optional. This way it is not required to add @JsonbNillable annotation to every single parameter.

public class SampleClass {

    @JsonbCreator(optionalParameters = true)
    public static Pojo creator(@JsonbProperty("parameterOne") String parameterOne,
                               @JsonbProperty("parameterTwo") String parameterTwo, // null
                               @JsonbProperty("parameterThree") String parameterThree) {
        //Do some magic here
    }

}

Fifth example

{
    "parameterOne": "parameter one value",
    "parameterTwo": null,
    "parameterThree": "parameter three value"
}

If parameter value is explicitly set to the null and not just missing, the null value should be honored.

public class SampleClass {

    @JsonbCreator
    public static Pojo creator(@JsonbProperty("parameterOne") String parameterOne,
                               @JsonbProperty("parameterTwo") @JsonbDefaultValue("1") Integer parameterTwo, //null
                               @JsonbProperty("parameterThree") String parameterThree) {
        //Do some magic here
    }

}

On the other hand if the type of the parameter is primitive type, an exception should be thrown, since it is not possible pass null to the primitive type.

public class SampleClass {

    @JsonbCreator
    public static Pojo creator(@JsonbProperty("parameterOne") String parameterOne,
                               @JsonbProperty("parameterTwo") @JsonbDefaultValue("1") int parameterTwo, //an exception
                               @JsonbProperty("parameterThree") String parameterThree) {
        //Do some magic here
    }

}

Conclusion

This covers basically all the possible variants. What do you think? Do you guys have any suggestion what to improve or change in general?

rmannibucau commented 3 years ago

Think we should do 4th option without optionalParameters additional entry. Long story short - and as explained - there is no valid case to fail based on a null value (corollar is you should also support < N or > N, length check etc) for creator method/constructor. Since, as of today, no creator is valid supporting null values, all creator will work if we enable to inject null values except the ones relying on this validations - which likely means they don't use the EE integration of JSON-B but a custom reader/writer one for ex. For such a cases - none ;) - I would add a backward compatible flag (jsonb.creator.enableNullValues=false) but I would make the spec making it true by default.

No real API change nor new custom annotation to lear nor more verbosity required for the mainstream case so sounds like the best compromise to me.

Verdent commented 3 years ago

I have done this only because I don't want to make backwards incompatible change and to release a major version because of that. I do agree, that validating parameters as it is now, is kind of redundant and not needed.

Long story short - and as explained - there is no valid case to fail based on a null value (corollar is you should also support < N or > N, length check etc) for creator method/constructor.

I have to say, I have no idea what you are trying to say here :-)

For such a cases - none ;) - I would add a backward compatible flag (jsonb.creator.enableNullValues=false) but I would make the spec making it true by default.

I guess I am not having a problem with the option, but it is global for the Jsonb instance. What if you don't want to have validation enabled/disabled on all of the places? Lets say because of some legacy purposes?

No real API change nor new custom annotation to lear nor more verbosity required for the mainstream case so sounds like the best compromise to me.

The new annotation @JsonbDefaultValue would be introduced for String, primitive and its wrapping types. Simply because of what value to pass to the int parameter if it is missing? If it is object... sure null, but in terms of primitive type?

rmannibucau commented 3 years ago

I have to say, I have no idea what you are trying to say here :-)

Current validation only support the presence, it is a very particular case of validation which should never have hit the spec or we would have needed a full jsonschema validation support (with a stable spec so it is still not an option but explaning why validating null is not correct at spec level).

What if you don't want to have validation enabled/disabled on all of the places? Lets say because of some legacy purposes?

If you use that case today it means you already wrap JSON-B in a custom fashion so a global toggle is sufficient. I would deprecated that feature in 2.1 and just keep the global toggle since it is a feature which shouldn't be. Adding an annotation toggle means it is a real feature whereas I strongly think it is never properly used.

The new annotation @JsonbDefaultValue would be introduced for String, primitive and its wrapping types.

I fully get the idea but not that this is already built in in the spec - int example but hope you get the idea:

public MyConstructor(JsonValue myInt) {
this.myInt = myInt == null ? 1 : switch (myInt.getValueType()) {
  case NUMBER -> JsonNumber.class.cast(myInt).intValue();
  default -> throw new IllegalArgumentException("....");
};

If you would like - once again I'm 100% for not having it, you need to do @JsonbDefaultValue.Int(value), @JsonbDefaultValue.String(value), @JsonbDefaultValue.Long(value) etc to be properly typed and likely handle @JsonbDefaultValue.Object("{...}") to be consistent. In all cases it lead to a complex API for something we shouldn't have IMHO so think the global toggle is sufficient, I'm even fine if you want this one to be read from system properties to enable to not rebuild an application at all - will cover all existing cases and avoid a full set of API for a broken case :).

sclassen commented 3 years ago

Regarding the fifth example: Sometimes you are consuming a json which is not under your control. So I would like to se a flag to give the default value precedence over the explicit null.

But I can also live without the @JsonDefaultValue. It is very easy to implement this in a getter.

Verdent commented 3 years ago

@sclassen That's actually a pretty good point :-) I will think about it a bit, how it could be ensured, that null value precedence will not be taken into account, if it is needed.

rmannibucau commented 3 years ago

give the default value precedence over the explicit null.

Isnt it another feature (+ as of now default value is not a thing ;))? If explicit it must stay or you corrupt user input without willing it. That said, as mentionned, it goes the "it is easy technically" path and forgets the "it is easy to use and learn" one so I would request one more time to just revise creator constraint and add a backward compat toggle only which is more than sufficient.

nmatt commented 3 years ago

@rmannibucau: There is an argument to be made that one shouldn't need an extra public constructor taking JsonValues that has to make its own type checks. Since setter properties can easily distinguish between null and absent, so should "creator" constructors, arguably. I don't know how often the situation arises, but it would be unfortunate having to pick the lesser evil between keeping the mutable class or having to add convoluted extra constructors.

Regarding @JsonbDefaultValue, maybe the following would work: Its value would have type String; if the corresponding constructor parameter has type String or Character (or a corresponding JsonbAdapter applies), then the value is used as-is; otherwise the value is interpreted as a JSON literal. Whether the value is correctly-typed can in principle be verified at compile type by annotation processing. Having different annotation types would not, by itself, automatically type-check the values against the constructor parameters either.

In any case, if @JsonbDefaultValue is added, then it should also be applicable to setters.

rmannibucau commented 3 years ago

@nmatt i disagree, if you want a missing primitive you can use a wrapper type or null is invalid. Now if you want a default value you must use a deserializer - or, once again you should also support it for objects, arrays which is obviously not desired like that. So relaly think it is just a technical driven thinking rather than feature one. Lastly, back to the original topic it is overkilled and unrelated.

nmatt commented 3 years ago

@rmannibucau: The scenario I'm worried about is that you have existing JSON-B classes with setters and where the distinction between absent and null is relevant. Now you have an uphill battle to convince the team that it is a good idea to convert it to an immutable class, if that means having to add a deserializer or other non-obvious code, whereas the setter version was very straightforward.

rmannibucau commented 3 years ago

@nmat this case is handled by the toggle (i proposed to even support a system prop to not rebuild apps at all). If you dont enable it you simple requireNonNull/do the check in the creator and done. So we are fully covered and no real battle IMHO. The side note being that if people rely on that for validations, it is a broken pattern since validation is wrong, they should switch to a functional validation anyway - this part is out of the spec but the spec never supported defensive programming nor input shape validation so was never a feature users could rely on.

m0mus commented 3 years ago

Let me put me 2 cents here:

  1. Ideally we want to enable optional parameters without breaking the spec backwards compatibility. It means that changing the default of mandatory parameters is not an option. Extra parameter optionalParams in @JsonbCreator, additional property in Configuration or both is the way to go.
  2. Giving users an ability to set defaults for missing fields is a nice feature. It's applicable to primitive types too which solves the case of primitive types parameters. Potentially we can reuse this annotation on setters to set a default value if property is missing in JSON, but it's out of scope here.

In general I like it as it's proposed. I think we should proceed to a PR. Thanks @Verdent !

rmannibucau commented 3 years ago

@m0mus for 1 we have a solution without new explicit param which defeats this issue IMHO. For 2 I think we leave the spec scope and as mentionned it creates a lot of points to discuss which sounds a lot for the original issue (+ the fact it is not need by design thanks wrapper types).