Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot use @JsonbCreator with absent fields #121

Closed
emattheis opened this issue Feb 14, 2019 · 62 comments · Fixed by #285
Closed

Cannot use @JsonbCreator with absent fields #121

emattheis opened this issue Feb 14, 2019 · 62 comments · Fixed by #285
Labels
enhancement New feature or request
Milestone

Comments

@emattheis
Copy link

emattheis commented Feb 14, 2019

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
Copy link

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

@emattheis
Copy link
Author

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
Copy link

rmannibucau commented Feb 14, 2019

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
Copy link
Author

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
Copy link

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

@emattheis
Copy link
Author

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
Copy link

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
Copy link
Author

emattheis commented Feb 14, 2019

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
Copy link

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
Copy link
Author

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
Copy link

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
Copy link
Contributor

@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
Copy link

rmannibucau commented Feb 15, 2019

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

@emattheis
Copy link
Author

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
Copy link
Author

@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
Copy link
Contributor

@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
Copy link

@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
Copy link
Contributor

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

@rmannibucau
Copy link

@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
Copy link
Contributor

@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
Copy link

rmannibucau commented Feb 18, 2019

@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
Copy link
Contributor

@rmannibucau Agree, server side is mainstream, but not necessary it is avoided on smaller devices, for example see here: eclipse-ee4j/yasson#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
Copy link

@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
Copy link
Contributor

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

@emattheis
Copy link
Author

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
Copy link

misl commented Jun 18, 2019

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
Copy link

Spec defines it actually: setter or equivalent.

@misl
Copy link

misl commented Jun 18, 2019

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
Copy link

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

@Verdent
Copy link
Contributor

Verdent commented Sep 15, 2021

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

  • Allow marking creator parameter as "optional"

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
Copy link

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
Copy link
Contributor

Verdent commented Sep 15, 2021

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
Copy link

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
Copy link

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
Copy link
Contributor

Verdent commented Sep 15, 2021

@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
Copy link

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
Copy link

nmatt commented Sep 15, 2021

@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
Copy link

@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
Copy link

nmatt commented Sep 15, 2021

@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
Copy link

@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
Copy link
Contributor

m0mus commented Sep 16, 2021

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
Copy link

@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).

@nmatt
Copy link

nmatt commented Sep 16, 2021

@rmannibucau: This is not about validation. it's about the case where both the property being absent and and the property being explicitly null in the JSON are valid, but have different semantics (*). This is straightforward with setters, and therefore arguably should be made straightforward with constructores.

(*) That may seem like a poor design choice, but can come up due to API evolution, among other things. For example, version 2 of the API may introduce a new mandatory property, whose value was previously merely implied. To continue supporting existing clients, the previously implied value is used as the default value when the property is absent in the JSON (in the Java class, by initializing the corresponding field with that value). The setter of the new property doesn't allow null because the property is mandatory (= the getter always returns a non-null value). In version 3, a need comes up to make the value represented by the property optional. The setter is therefore relaxed to allow null. JSON sent by version 1 clients (i.e. with the property absent) continues to map to the specific default value, whose semantics is different from the new null value.

@rmannibucau
Copy link

@nmatt hmm, not really. null presence or absence is not something the mapper should cover because otherwise you must also cover bigdecimal as string or number difference etc. All that is impossible without adding Jsonb specific inputs you don't really need. Also note that the setter option already does not work since null can be skipped by the impl (should actually) so it can not be called if null is there. So really this feature is not a feature of the spec as of today (and I think it is fine). At the end it is only about valued validation which as explained is not relevant so we can drop it with the backward compat toggle.

@nmatt
Copy link

nmatt commented Sep 16, 2021

@rmannibucau: The JSON-B spec at https://download.oracle.com/otndocs/jcp/json_b-1-pr-spec/ (linked from https://javaee.github.io/jsonb-spec/download.html) requires that the distinction between absent and null be preserved (section 3.14.1). Has this been changed? Where can I get the current spec? (never mind, found it)

EDIT: This is also still true in the 2.0.0 spec, same section ("Null Java field").

@rmannibucau
Copy link

@nmatt you are right for the setter (for the field it is way more ambiguous since the default can be detected being or not null and then the set be skipped) but it also confirms my points: it is schema validation (attribute presence) whch is not JSON-B about nor a feature we wanted to bundle in 1.0. If it is really desired JsonValue injection support is there and enables all needed validation functionally. Don't get me wrong, I'm not saying validation is not something jakarta should cover but not in jsonb "core" and not just one of the dozen of validations rules jsonschema covers.

@nmatt
Copy link

nmatt commented Sep 16, 2021

@rmannibucau: I disagree that this is about validation. This is about the conceptual data model: Are absent properties and null properties equivalent (same logical value) or not when deserializing JSON? The existing spec says that they are not interchangeable. The existing @JsonbCreator does not support absent values (consistent with the fact that they are not interchangeable). Adding support for absent values, but conflating them with null values, on the one hand brings it closer to the setter capabilities, but on the other hand creates an inconsistency with the setters regarding the data model for deserialization. In my mind it would be better to maintain a consistent model.

@rmannibucau
Copy link

@nmatt

Are absent properties and null properties equivalent (same logical value) or not when deserializing JSON?

This is the point, for JSON yes. I know you can use the presence to do anything different but for JSON (and more widely JSON ecosystem, including the most used, javascript) null or absence is the same.

Once again, I'm not saying you must not be able to detect it is there or not but:

  1. You are able to do
  2. It is about shape validation (this is why i mention jsonschema which is likely the most known solution for that) and not about mapping itself

If you don't agree the issue is quite immediate:

public class Foo { public String bar = "dummy"; }

if you get {"bar":null} and {} you dont have the same model whereas it is functionally equivalent in terms of data (it is not in terms of schema but once again JSON-B is not, as of today, about the schema at all)

@nmatt
Copy link

nmatt commented Sep 16, 2021

@rmannibucau: I don't want to draw this argument out unnecessarily, but to make some final points: It is not about shape validation if both absent and null are valid within the same schema but have different semantics. By "data model" I didn't mean "schema", but the generic meta-model of (arbitrary) JSON data; no schemata involved. My point is you are choosing a different meta-model now for constructors than what JSON-B has for setters. It's also not correct to say that Javascript doesn't distinguish between absent and null: an absent property is undefined in Javascript, not null, and thus {"bar":null}.bar === {}.bar evaluates to false.

@rmannibucau
Copy link

@nmatt you are picky but it does not change the point, in a java object null or absence is the same, nothing to discuss there. In javascript it is the same too until you validate the shape (schema validation) where you check with undefined test or hasOwnProperty method. So it is correct to say the difference between an explicit null and an absence is only about the schema, not the data. Once again, I see your use case and think at some point we must cover it but it is not in the scope of this issue nor in JSON-B scope (yet).

@erik-mattheis-snhu
Copy link

erik-mattheis-snhu commented Sep 16, 2021

Glad to see some interest here even though I've largely moved on and given up on JSON-B 😛

The problem I originally ran into is that the specification and implementation force validation into the picture by requiring restrictive behavior when using a creator that isn't required in the setter approach. With the setter approach, I can tell if the field was present in the JSON based on whether or not setter was called (assuming an implementation is required to call the setter when the value is explicitly null - I'm not going to check that right now). Of course, I can't do that in a creator because all the values are passed at once, so I was seeking flexibility there.

This is undeniably a useful thing that many people need in order to adopt JSON-B.

Arguing about whether it is validation or not is pointless - that is water under the bridge.

The fact that Java has no distinction between null and undefined is also immaterial. JSON certainly supports payloads that make a semantic distinction between properties that are absent and properties that are present with a null value. This is not picky, this is the real world with messy polyglot implementations.

This is also a completely solved problem in other JSON Java libraries, so it remains a shame to see JSON-B held back from more widespread adoption by this continuous debate.

@rmannibucau
Copy link

Just a reminder it is solved in jsonb since v1, jsonvalues can be injected and marshalled as in any other json lib...

@emattheis
Copy link
Author

@rmannibucau I think you understand what I meant.

Nobody in this thread is arguing about the functionality that we know works. The inability to use JSON-B to bind JSON to immutable types in Java is a shame. There is no technical reason this can't be solved.

@Verdent
Copy link
Contributor

Verdent commented Sep 16, 2021

@emattheis @nmatt @rmannibucau @m0mus Thank you guys for your feedback on the issue here :-) I will prepare PR with the proposed changes and I will try to address your arguments there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.