+1 to what Dmitry said
This is exactly my point I was giving before.
I would be fine with introducing config property with the global flag. I have no issues with that. It was even proposed before, but I would rather avoid defining system property to do so on the spec
level. That’s something implementation can define just fine. Config property is OK to have on the spec level IMO.
@JsonbReqired annotation has been introduced because of the reasons as Dmitry stated. To provide some extended granularity of the feature which is already in the spec.
> I understand the granularity point but fact is nobody wants it required from the feedback we got so it is a feature nobody wants like that as we seen before so let's just not add it cause
the impl is trivial, it always blows up at our faces later when we do.
Just because of the feedback to the issue where we were discussing loosening the “required” restriction, was that users want it to be relaxed, does not mean, there are no others who want this behaviour.
OR even would use the more granular way of using it (@JsonbReqired). I would be cautious with statements “nobody wants it required”
Od: jsonb-dev <jsonb-dev-bounces@xxxxxxxxxxx>
za uživatele Romain Manni-Bucau
Odesláno: středa 17. listopadu 2021 11:00
Komu: Dmitry Kornilov <dmitry.kornilov@xxxxxxxxxx>
Kopie: jsonb developer discussions <jsonb-dev@xxxxxxxxxxx>; Jakarta specification discussions <jakarta.ee-spec@xxxxxxxxxxx>
Předmět: Re: [jsonb-dev] [jakarta.ee-spec] [External] : @JsonbRequired for JSONB and JSON Schema
I couldn’t find anywhere in the spec that environment variables or system properties can be used to configure your JSONB instance. We have only Config.withXXX
methods. Introducing another method to setup @JsonbCreator parameters optionality is possible, but users will need to make changes in their code to use it.
No no, think you misinterpreted. It is a common way to configure specs - at provider level (our builder). So it is a common practise in jakarta to do it, in particular for such concerns.
Technically, the config switch is the same as putting @JsonbReqired annotation on all your methods annotated with @JsonbCreator. Code change is required anyway.
The difference is in granularity. The global switch is brutally changing behavior for all @JsonbCreators and @JsonbRequired allows changing optionality of parameters on the method level or even on parameters level. We choose the @JsonbRequired way simply because
it’s more flexible than the global switch. On the other hand, these two solutions can co-exist. I am fine to add the global switch to the current. It will make it even more flexible. I am not fine with changing the default behavior without giving users the
ability to roll back.
1. (syst prop): vi && rerun
2. (@xxx): dev, compile test, preprod,..., prod and rerun
cost = 1 vs cost = will not upgrade or too much
I understand the granularity point but fact is nobody wants it required from the feedback we got so it is a feature nobody wants like that as we seen before so let's just not add it cause the impl is trivial, it always blows up at our faces
later when we do.
-- Dmitry
> How will you deal with the use case when you actually want @JsonbParameters parameters to be present?
@JsonbCreator public static MyType of(String requiredValue) { requireNonNull(requiredValue); }
This is not quite the same because it will throw exception when a property is explicitly null and not just when
it is absent.
Ok so replace String by JsonValue and you get the exact same, overall the idea was to say it can be done in user
code or technical layer around it.
Right, but that’s only workable for trivial cases and is precisely what programmers looking for this feature want
to avoid. In the very common case of nested JSON objects, for example, I certainly don’t want to handle converting from JsonValue to POJO in my creator method.
I think we should be careful to avoid saying, “You can do this in code.” when we really mean, “This isn’t something
JSON-B should do.”
Ok got it. Think both statements are actually right but I'd like to use that to step back one sec also and see
how off JSON-B we went - think somebody mentionned it already.
Do you have an example of that use case which is not validation and can't be done binding null in an actual model?
No, I fully support the use of null in the actual model for creators. I think that is what everyone is after.
@JsonbCreator public static Customer of(Address add, String name) {/*...*/}
What would be the usage of getting {"name":"..."} vs {"name":"...", "add":null} which is not validation?
I can see the use case of "if Jsonvalue.NULL then set to null, else ignore" - kind of PATCH impl - but this can't
be mapped to Java without using JsonValue anyway.
The PATCH payload use case is the only one I’ve ever encountered where I wanted to distinguish between absent and null, and there’s no great way to map that to a type-safe POJO.
If you really want to handle that I guess you need a deserializer or parser/reader wrapper - just for the theory
of doing it. Agree it is less complicated but, once again, I agree being able to validate a payload is an important feature, but here we are nowhere close to it with @JsonbRequired, this is why I request to drop it and either not do any validation as planned
or do it but do it properly and not half baked.
+1
In my experience, validating JSON on the wire is rarely useful in situations where I want to immediately map it to a POJO. I would rather use bean validation after the fact. If
I’m validating JSON against a schema, it’s because I need it to remain in JSON form for a non-Java consumer downstream.
> Again, we cannot release a feature with uncovered use cases.
If we’re really concerned about this breaking existing code, then we should introduce a way to opt into the new
behavior instead of forcing programmers to opt in to preexisting behavior.
e.g. - @JsonbCreator(allowAbsentProperties = true)
Think we all agreed the default should change, only question is global switch or not cause all other options require
code change which means the try { fromJson() } catch () can move to something else anyway with code change, even without any other change in JSON-B.
If it's agreed that the default should change, then introducing a config to restore the previous behavior makes
a lot more sense to me.
Introducing @JsonbRequired allows us to cover all use cases the convenient way. Global switch is an acceptable
solution for implementations but not for specifications.
I don’t think that you answered my question. How
will you deal with the use case when you actually want @JsonbParameters parameters to be present?
Again, we cannot release a feature with uncovered use cases.
-- Dmitry
@Dmitry Kornilov this is not true, you assumed
that @JsonbRequired solves a backward compatbility issue but solving it can only be done by a global toggle as explained before. @JsonbRequired is a new feature requiring code change (so nothing backward compat).
Now the question is: is this feature a JSON-B feature or was it a side effect of not being able to call the creator.
Since validation is not in the scope as you explained, it is clearly a side effect: impl must call a factory with N parameters but only have n < N params so we made it failing.
So no regression not having @JsonbRequired and we can work on validation in a later spec version if desired or
just let it this way which is what users requested AFAIK.
Before accepting criticism, I would want to make sure that you folks fully understand the use case.
We are making a backwards incompatible change by not requiring a pretense of @JsonbCreator parameters in your JSON. How will you deal with the use case when you actually want these parameters to be present? We must deal with it because otherwise there is a
space for uncovered use cases.
-- Dmitry
Ok so since there is an agreement on relaxing the spec, let's make it in main branch and since there is a disagreement
on @JsonbRequired let's just drop it and not brute force it to be there without being able to justify it nor integrate it in jakarta ecosystem (yet).
A better and more scalable approach for stream is to do it at parser level (sorry this one is not public) but long
story short it is decorating the parser to add validation to each events. On a spec level I would expect something like:
final var validator = new Validator();
final var validator = Json.createSchemaFactory(Map.of()).createShemaValidator(myJsonObjectJsonSchema);
final var parserFactory = Json.createParserFactory(Map.of(JsonParser.VALIDATOR, validator));
// indeed by "transitivity"
final var readerFactory = Json.createReaderFactory(Map.of(JsonParser.VALIDATOR, validator));
A nicer option would be to enable to pass a map to the parser itself to not have to share it globally and have
multiple instances of factories (current JSON-P limitation, it is the same for prettification for ex).
Currently it is more done this way since this API is not there:
final var myValidatedParser = new ValidatingParser(parserFactory.createParser(...), myJsonObjectJsonSchema);
But this is an implementation detail, the key is that in terms of design there is no real choice to do the validation
at JSON-P level - and more precisely parser to enable more advanced validation on huge payloads).
If you want to talk about validation (like ensuring an attribute has a value in the payload for ex) we need to
ensure:
1. It covers the common needs and not just the most trivial to check,
2. It is integrated to the ecosystem (failure handler, compatibility with ajv for ex, ..... JAXB is a good example
there),
3. It is done properly at the right level, ie JSON-P and JSON-B just wires the needed config (don't forget JSON-B
is written to accept both JsonReader or JsonParser usages by constructions and to be a mapper API and not an implementation which would defeat the spec target by design - enables to not be vendor locked in, otherwise no point using a spec ;))
4. We can use it as a base for other specs (like MP openapi for ex instead of duplicating all the API, in particular
if MP joins jakarta at some point)
I guess it is a big topic we can try to address for next big release - or just do an extension/appendix to propose
it in the spec - but it is likely too much for next immediate one and if we do it in a hurry it would be wrong again and deserve us so let's just drop it for now, no?
The core issue is the mismatch between JSON and Java as it pertains to detecting the presence or absence of a property.
Using traditional JavaBean mechanics, it is possible to differentiate between the absence of a property in the JSON payload (setter is never called) versus a property with a null value (setter is called with a null argument). For creator methods/constructors,
there is no direct way to distinguish between an absent value and a null value and the spec (unfortunately) mandates that an exception is thrown for missing fields.
Relaxing the spec to permit the deserialization of JSON documents with absent fields using creator methods/constructors
and addressing backwards compatibility is the sole issue at hand. Unfortunately, the current spec behavior blurs the line between this issue and validation because any code that relies on creators throwing an exception when a field is absent is implicitly
relying on JSON-B to perform validation.
Personally, I’m in favor of simply relaxing the spec to permit the deserialization of absent fields as null values
in creators and making it clear that it is not possible to differentiate between null and absent properties. I believe that is what users of creators expect, and anyone who needs to differentiate the two cases can use setters.
I agree that @JsonbRequired isn’t a great idea here. It makes the expectation of validation explicit where it was
previously implied. It would be interesting to survey usage of JSON-B to see if anyone really wants this. As Romain points out, it’s not a great solution for backwards-compatibility because it requires changing code anyway. A better solution (in my opinion)
would be to allow programmer to specify the value to use when a field is absent (e.g. @JsonbDefault(“value”) or something similar). This would match the specified behavior for setters (setter must not be called so default value is preserved) and keeps JSON-B
focused on the mechanics of deserialization instead of straying into the realm of validation.
Also agreed but don't think we need any solution for now other than relax the specification that mandates an exception.
> If you want to restore previous behavior for existing app you must have a *system property* - or env var
Sure, feel free to have it on impl level. That’s exactly something what belongs there.
> This is a wrong solution by construction and it does not reach its functional goal.
Its goal is not to create any extended validations, so I think it meets its goal just fine. And because of that
I do not believe anything like Bean Validation integration (or even schema) should be included. This is there just able to turn “the old behaviour” on the specific parameter/creator.
> Most (all actually) users needing validations need more than that so can't use that and use something else ….
I don’t think so. This is more than enough. If anyone wants to do the value validation, sure, feel free to add
it into the creator itself. I do not see how this change of ours would make creator “unusable” in the most cases. As mentioned, several times, it is there to provide backwards compatible functionality, without the need of defining it globally.
> JSON-P needs a validation (at parser level likely)
I have to say, I am not sure, how this would improve user experience or even how it solves the problem we solved
with the @JsonbRequired. Do you have any example which would illustrate the solution and how user would use it with JSONB?
I don’t think we are getting here into the area of bean validation. This is not about validation of the content
in any way.
The PR introduced backwards incompatible change to the creators, since the parameters are no longer required by
default, some other mechanism for specifying required parameters needed to be introduced. @JsonbRequired annotation has been added mainly to provide users ability to specify which parameters are expected to be present in the JSON upon object creation. It does
not care if the value has been null or value. It is simple requirement of some property to be present. If the required property is not present -> fail.
This is a wrong solution by construction and it does not reach its functional goal. If you want to restore previous
behavior for existing app you must have a *system property* - or env var - cause otherwise it requires a new compilation and code change which means you don't need this annotation in such cases anyway.
I do not agree it is messy. The way I see it, is the same behaviour as we used to have, and no extended bean validation
was required either. Therefore, I do not think doing any bean validation integration or even schema creation make sense.
There it is also part of the story only.
Most (all actually) users needing validations need more than that so can't use that and use something else like
a jsonschema based validator or a custom one - often in interceptors or filters - for the most common cases I saw.
So having a built-in validation nobody uses - or intend to use this way - is likely wrong and in terms of consistency
with the ecosystem will make the user life harder.
JSON-P needs a validation (at parser level likely), so once it is there JSON-B will just wire it providing a JsonProvider
in JsonbConfig or something like that and voilà.
This is the level JSON-B must integrate with the ecosystem.
Currently the validation is super rude - no way to recover - and so incomplete that it is disabled in most applications.
Also, if we can split in 2 the topics:
1. relax the instantiation - everybody agreed on that and consider current spec as a blocker
2. required handling or not: there, there is no agreement so maybe just postpone it instead of forcing the merge?
I’d like to follow up on some discussions around the @JsonbRequired introduced.
On the feature itself, I clearly see some benefits to have validation happening before the bean is constructed and
fields populated with the JSON payload. Good feature.
On the other hand, I don’t think @JsonbRequired is a good way to achieve it.
Basically I see 2 options for validation
-
payload validation
-
bean validation
@JsonbRequired in the bean to do payload validation does not seem accurate to me. It will set us for failure in the
future.
If we want to address bean validation, we should consider having some integration of Bean Validation using Bean Validation
annotations. Of course, this means validation will happen after the bean is constructed, but it also is way richer than “is the field required or not”. Users will be able to reuse constraints and implement business logic validation.
I agree that validating the payload before the object gets constructed is very interesting. But then, we should use
JSON Schema for it. I am ok to create JSON Schema equivalent annotations, at least for the stable subset and the most interesting rules. From there, we can generate the JSON Schema, and feed the JSON Parser with it so the parser can effectively validate the
payload before with a valid JSON Schema (same way an XML parser would get the XSD to validate the payload in JAXB).
Having @JsonbRequired seems to be in the middle and a hack for a very simple use case. I would not introduce it now
and give us more time to discuss and think about the payload validation a bit more.
Again the design for @JsonbCreator and the need for the feature is awesome for immutable objects. The way to solve
it with @JsonbRequired is messy.
Moreover, if we look at MicroProfile and OpenAPI, we are also half way because OpenAPI has @Schema, for instance
@Schema(type = SchemaType.OBJECT, implementation = InventoryList.class)
private final String hostname;
From there, we generate an openapi document with all constraints (not a JSON Schema but pretty close).
Let’s say we need business validation or cross fields validations, we’ll use Bean Validation.
Let’s say we want OpenAPI documentation, we’ll use OpenAPI @Schema
And now let’s say we want JSON Payload validations, we’ll use @JsonbRequired?
Can we maybe consider the schema validation as a whole?
We could use the same set of annotations to generate the OpenAPI documentation and to generate the JSON Schema for
the JSON Parser.
I am happy to create an issue to revert @JsonbRequired if there is a consensus to wait.
_______________________________________________
jsonb-dev mailing list
jsonb-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jsonb-dev
_______________________________________________
jsonb-dev mailing list
jsonb-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jsonb-dev
_______________________________________________
jsonb-dev mailing list
jsonb-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jsonb-dev
_______________________________________________
jsonb-dev mailing list
jsonb-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jsonb-dev
_______________________________________________
jsonb-dev mailing list
jsonb-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jsonb-dev
_______________________________________________
jsonb-dev mailing list
jsonb-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jsonb-dev
|