[BUG][Java][Spring] openapiNullable - JsonNullable usage is based on nullable property instead of required property #14765
Comments
…ixes OpenAPITools#14765 JsonNullable was implemented incorrectly, checking for nullable property instead of required property as originally defined by OpenAPI Spec
|
Just stumbled around an additional mustache template |
|
I disagree with the assessment that
If a field is both
If a field is So I think |
|
@daberni in general there are 4 possible scenarion for
So based on all these cases only for point 2 we can use |
|
@welshm Interesting approach, I didn't see it this way so far but I understand the idea. Probably this would be a somehow working approach, the problem is, that it is not implemented this way right now. Currently, usage of If this would be combined with But additionally, which is quite confusing, the beanValidation.mustache depends on So that this would work as you described, one would have to combine this together with One problem which currently also arises, probably because of the combination of the above incomplete implementations, that Spring Model Binding Validation doesn't work properly either. Also after generating the code it is no longer clear which combination of flags resulted in the current code. So as a developer I would have to write different validation and mapping logic depending on required/nullable attributes, instead of coupling my mapping logic to the present types directly, which would be way more straight forward. I see that optimizing away the one case For better understanding, It would be more verbose to couple |
|
I think if the
I think that is likely a matter of perspective - but I agree it could definitely be more clear.
That's more of a discussion on whether the generated code should be done to allow determining violations of the API contract (explicitly sending a In my opinion, One way to resolve this would be to introduce a flag in the generator that could produce |
|
Usage of The issue is, that Regarding validation, because it is implemented incomplete right now it is not possible to validate correctly at the moment. Having a property which is I don't see why The PR was just to highlight a radical change on how I would see the correct implementation of The question is, how do we want to specify the behaviour.
This would be independent from the
I would definitely vote for 3) from our side (obviously :D) because 1) and 2) wouldn't matter in that case. But I would also see a clear definition and implementation for 1) and 2). Maybe utilizing the existing So keeping
|
|
@borsch Am I correct that the behavior you describe, if validation is also considered, should be:
** I do not know the validation behavior of |
|
@Djerem- To perform correctly all validations you need to use
Obviously the That said for retrocompatibility this strict validation for required fields should probably be optional with a parameter to enable it. |
|
From my point of view, you/we should basically think again about the See https://github.com/OpenAPITools/jackson-databind-nullable. Currently i habe no other good RESTful usecase for |
|
I think if it's valid for PATCH it's valid for POST as well. Some projects just use POST instead of PATCH - even it it's not "correct" |
|
I absolutely agree that it is primarily relevant in a PATCH request where one can define, it is important to know if the value was passed or not. But you can't decide on this based on the HTTP method but you have to define something different for that, and for me this is the If it is "not required" this implies that it is "optional". And in an optional manner I must be able to check if it was present or not. If it is "not optional", it would be okay to me that the Spring implementation then uses the default value of the property if it is not present. And then an additional validation like "not null"/"nullable" would kick in. But this way you could clearly differentiate between these two configurations |
|
As a sidenote: we have already adopted the mustache templates in this manner and are very happy with this approach. It would be interesting if someone else would have different requirements to this. It would be nice if we wouldn't have to maintain these templates again all the time (reintegrate with upstream changes) and in my opinion others may would benefit from this approach too. I think the main aspect would be, when one doesn't define the "required-list" completely, every property would be JsonNullable and break a lot of stuff immediatelly. It is a bit cumbersome that everything has to be specified again in the required list, but imho that's the more correct way anyway. I don't know if there would be any simpler solution to this. |
Yeah you are right, that why I said in good RESTful Api(s) ;) @daberni I think you need the nullable + required field, if you would like to have a general solution for every operation. If you say it's only for PATCH (maybe POST) then you have other solutions. See discussion here @welshm have a good conclusion How I said in our use cases, I would like to have everywhere |
|
We are already using the suggested approach for every operation, not only PATCH. What is the benefit of using Or would you couple |
Thats not correct. Also with Collections Optional make no sense, because you can use a empty List for that. But a JsonNullable with Collection can make sense if you use it for patch, because it has a different semantic. Look into the JSON Merge Patch specification. So how the name is calling its for PATCH :)... Easy and quick solution: use it just for PATCH (maybe POST) operation. e.g. @welshm correct? But how I said for a GET/DELETE/PUT JsonNullable make not so much sense :) |
|
@daberni more clear? |
|
Persisting in 7.0.0 beta |
|
I would like to offer another perspective. To me it makes sense for the outermost |
|
I don't understand how |
|
When merging one object into another (using |
|
I don’t get what you would like to say. Please make an example with code. I don’t see how you can achieve a tristate semantic. But also a little bit strange that we should solve a issue with extra big dependencies after a mapping step. |
|
@gnuemacscoin When the property is null in the source object, it means that it should be set to null. When it is not present, than it should be not modified. That is why JsonNullable is used. To provide isPresent() function and distinguish the third state. |
|
Yes and the third state (reset state) make just sense for PATCH ( maybe sometimes for POST not sure). |
public class Opt<T> {
public @Nullable T value;
}
public class Target {
public @Nullable String prop;
public @Nonnull String nonnullProp = ".*";
}
public class Source {
public @Nullable Opt<String> prop;
public @Nullable String nonnullProp;
}
@Mapper(nullValuePropertyMappingStrategy = NullValuePropertyMappingStrategy.IGNORE)
public interface OptMapper {
default <T> @Nullable T from(@Nonnull Opt<T> opt) {
return opt.value;
}
void patch(@MappingTarget @Nonnull Target target, Source source);
}produces the following public class OptMapperImpl implements OptMapper {
@Override
public void patch(Target target, Source source) {
if ( source == null ) {
return;
}
if ( source.nonnullProp != null ) {
target.nonnullProp = source.nonnullProp;
}
if ( source.prop != null ) {
target.prop = from( source.prop );
}
}
} |
|
@gnuemacscoin now I just see two states. Where is the reset state? And again I don’t like to have a mapping step from dto to dto. |
|
@MelleD As I see it, the if(source.prop != null) is the same logic as JsonNullable.isPresent but expressed with null value instead of method call. The from() method extracts value from Opt only if the Opt object is present and can be null. In fact, the Opt class is actually JsonNullable equivalent here. |
That's what I at first thought the role of JsonNullable was, but JsonNullable wraps both |
In my example |
|
I need a closer look, but I don't recognize the reset case when it is set to null. But in the end, you create another wrapper class like jsonnullable. Instead of jackson, mapstruct does the translation. The semantic is a little different and reminds me more of a bad practice from optional wrapper. Even with an optional you can get a tristate with null, as I said, bad practice. The sense of JsonNullable and optional is that you no longer want to have null states but rather work with objects.
That’s perfectly the sense of JsonNullable and Optionals not to handle nulls. For this reason I see no advantage in the suggestion here with mapstruct. We can do the same things with Optional/JsonNullable when we handle required and nullable flags. PS: I have a jsonnullable mapper with mapstruct to map the status into the domain objects. It's perfect for that. And that’s a perfect mapping use case. What is the issue with the proposal? |
|
Anyway here's a @Mapper(nullValuePropertyMappingStrategy = NullValuePropertyMappingStrategy.IGNORE)
public interface OptMapper {
@Condition
default <T> boolean isPresent(@Nonnull JsonNullable<T> nullable) {
return nullable.isPresent();
}
default <T> @Nullable T from(@Nonnull JsonNullable<T> nullable) {
return nullable.get();
}
default <T> @Nonnull JsonNullable<T> toJsonNullableFrom(@Nullable T value) {
return JsonNullable.of(value);
}
void patch(@MappingTarget SomeEntity entity, SomeDto dto);
SomeDto from(SomeEntity entity);
}public class OptMapperImpl implements OptMapper {
@Override
public void patch(SomeEntity entity, SomeDto dto) {
if ( dto == null ) {
return;
}
if ( dto.notNullable != null ) {
entity.notNullable = dto.notNullable;
}
if ( isPresent( dto.nullable ) ) {
entity.nullable = from( dto.nullable );
}
}
@Override
public SomeDto from(SomeEntity entity) {
if ( entity == null ) {
return null;
}
SomeDto someDto = new SomeDto();
someDto.notNullable = entity.notNullable;
someDto.nullable = toJsonNullableFrom( entity.nullable );
return someDto;
}
} |
|
I would do it different, but ok that’s one valid mapper why not. It works! But I don‘t get your point for this discussion. This discussion here is more to avoid null and therefore use Optionals. And where you need a tristate use JsonNullable |
|
@MelleD I disagree with your proposal for
because it's even less orthogonally designed than what we have now. If I were designing this, the most composable thing I can think of is
where public class Present<T> {
public T value;
}or equivalent. Or maybe using vavr's
Now that I know about MapStruct's |
|
I completely disagree. In this case I cannot agree to abuse a wrapper class like Optional as null. For me it's not a question of Mapstruct or not. We have the concepts of Optional its plain Java so there is no need of other types. We already have JsonNullable and they fit perfectly with the open api spec without any additional dependencies. I see no really benefit for this, because I want just 1 (special) case for null. This is the reset state in a PATCH. I want to avoid
Also I see no sense for a required, nullable properties. What is the use case for this? How i said my goal is to avoid
Here every use case fits and you can decide what you need. I have no need to solve every use case with 1 wrapper class this not my goal. |
|
You see what you want to see, you brush away edge cases you see no need for, you eschew composability, you're fine with making invalid states representable, and for some reason you're really pushing for this |
Aha, which state is invalid?
Better than invent new own wrapper classes. Here is a flag https://openapi-generator.tech/docs/generators/spring/ |
|
So I made an attempt at implementing
except I named it |
|
Just a small question, I could not get the JsonNullable generated for a simple project. I just created a sample repository to show the issue. https://github.com/robertop87/opengen Basically the yaml definition has a non required 'tag' property defined like this: But the generated java code is:
And expected is:
am I doing something wrong? |
|
@robertop87 I think that's not related to this issue here. You also opened an issue and it should be handled there... |
|
While I'm here, I'd like to get back to the actual discussion after it drifted off a bit recently... Let me jump back a year as I want to focus on a bug which has been mentioned, but never resolved... On Feb 20 2023 @welshm and @borsch enlightend us about the different scenarios for JsonNullable which I absolutely agree with. @welshm even mentioned
@daberni replied, that...
and @welshm replied:
and @daberni replied:
Then discussion drifted off... Long story short: I think @daberni is still right with his statement - even with version 7.3.0 this is not generated as @welshm stated and it is still based exclusively(!) on the nullable property. This is my example which I just ran with v7.3.0: Config: Schema: JsonNullableAndRequiredTestDto:
type: object
properties:
nullableTrueRequiredTrueExpectString:
type: string
nullable: true
description: |
A field which MUST be included in the JSON and whose value MAY be null.
-> Ergo, a POST/PATCH request can set the field to null in the entity/database.
-> Expected code generation type: String
nullableTrueRequiredFalseExpectJsonNullable:
type: string
nullable: true
description: |
A field which MAY be included in the JSON and whose value MAY be null.
-> Ergo, a POST/PATCH request can set the field to null in the entity/database.
-> Omitting the property in the JSON should neither touch nor clear the target DB field!
-> Sending the property in the JSON with a (value) should update (String) or clear (null) the target DB field!
-> Expected code generation type: JsonNullable<String>
nullableFalseRequiredTrueExpectString:
type: string
description: |
A field which MUST be included in the JSON and whose value MUST NOT be null.
-> Ergo, a POST/PATCH request is not able to null the field in the entity/database.
-> Expected code generation type: String
nullableFalseRequiredFalseExpectString:
type: string
description: |
A field which MAY be included in the JSON and whose value MUST NOT be null.
-> Ergo, a POST/PATCH request is not able to null the field in the entity/database.
-> Omitting the property in the JSON should neither touch nor clear the target DB field!
-> Sending the property in the JSON should update the target DB field!
-> Expected code generation type: String
required:
- nullableTrueRequiredTrueExpectString
- nullableFalseRequiredTrueExpectStringwhich results in the following Java code: public class JsonNullableAndRequiredTestDto {
private JsonNullable<String> nullableTrueRequiredTrueExpectString = JsonNullable.<String>undefined();
private JsonNullable<String> nullableTrueRequiredFalseExpectJsonNullable = JsonNullable.<String>undefined();
private String nullableFalseRequiredTrueExpectString;
private String nullableFalseRequiredFalseExpectString;
...
}For So it looks like v7.3.0 does not adhere to the description of JsonNullable generation @welshm and @borsch gave a year ago. Does anyone have a mustache workaround for this? Or does this need a proper bugfix in the code generator? Addendum: public class JsonNullableAndRequiredTestDto {
private JsonNullable<String> nullableTrueRequiredTrueExpectString = JsonNullable.<String>undefined();
private JsonNullable<String> nullableTrueRequiredFalseExpectJsonNullable = JsonNullable.<String>undefined();
private String nullableFalseRequiredTrueExpectString;
private Optional<String> nullableFalseRequiredFalseExpectString = Optional.empty();
...
}but this doesn't solve the above issue and doesn't make things better in my eyes... It's just making your models worse if you previously used |
|
A technically correct answer (there are also other opinions) has been given a long time ago.
The question, however, is what is your need? What is your use case? What does it mean semantically? So far no one has been able to describe a use case for this case. And yes off course you can customize and override the existing template: here is the template: By the way, I think that not only JsonNullable is a problem but also beanvalidation because it sets a @NotNull for required fields |
|
I can say that openapi beginners have quite a hard time to understand the correct semantics of required and nullable. And, based on actual generator implementations, tend to bend the actual api definitions to product the source code they desire.
This would be usable for partial patch. Attribute missing means "do not touch the current value and I am not stating what the current value is". For POST, PUT, I would expect a default value so there can be clearing substitute for property not available in the payload. |
Correct thats the reason why there is the JsonNullable. If you don't want to use a Container class which handles this then you can disable it. And now you have arguments why it is like it is currently ;) and not to change it. You can do it right now your case no change necessary
But there is in open api no semantic for that.
That's true but another issue ;) |
Our use case would be to generate consistent types for our TypeScript client and Java/Spring backend and ensure that certain parameters must be passed. In TypeScript/JavaScript the distinction of nullable and required is represented with While the TypeScript types are generated in exactly the way I expect, the Java/Spring types are actually generated in an incompatible way and just seem plainly wrong to me, no matter if there is a use case or not (which there is as described above). In fact, it is not even working properly. When I send |
|
The problem I'm describing might be in part due to #11969, but even when I set Edit: Ah well, it has been mentioned before:
|
|
But you're talking about symptoms with different technologies and maybe problems there. I'am talking more about good API design and if the request really make sense. From a purely technical perspective, I don't understand what the difference is between setting a field in the JSON to I've never had to distinguish between them unless you use JSON Merge Patch.
What is stopping you? |
|
@MelleD How do you apply default value other then omitting the property and having the default used? Passing property: null means "the value is null" not "the value is default". This is other use case than merge patch. And please, don't tell me that properties with default cannot be null. |
I'm not saying anything, I haven't had the case yet. Of course we can now conjure up a case artificially. But no one was able to describe a real use case. Just to make it clear, this doesn't concern me at all, I'm just interested in how you design APIs and why.
But anyway see technical no issue here and also no need for a required attribute @jspetrak solved or not? |
|
Hey curious of any progress on the issue? example: PutRequest:
required:
- id
- riskVerified
properties:
id:
type: integer
riskVerified:
type: boolean
nullable: trueI spotted that if I have a PUT request with a Read all the issues threads around.
Thanks |
Bug Report Checklist
Description
In Java CodeGeneration
openapiNullableis used to determine if the special typeJsonNullableshould be used or not.The description of this type from the original documentation is as follows:
So, in a request body (especially in PATCH requests) it is utilized to signalize, if a specifc field was specified in the body or not. This can be used to just ignore any change to the property and keep the old value or to apply a new value to it.
BUT. The current implementation is using the
nullableproperty of a field instead of therequiredproperty. So nullable should actually tell about if a field can be null or not IF it is specified.nullabletells nothing about if a field must be present or can be omitted. This is whatrequiredis for.requireddeclares, if a field as to be present or not, independent of if it may be nullable or not.The fix would be to swap
nullableandrequiredfor the usage ofJsonNullable.openapi-generator version
6.4.0, but goes back until 5.6.0 minimum (probably since
openapiNullablewas implemented first).OpenAPI declaration file content or url
Existing /3_0/petstore-with-nullable-required.yaml can be used as an example
Taking the following Pet model from the referency specification:
The Pet class would be generated like:
Although, because only
nameandphotoUrlsare declared asrequired, all other properties should be wrapped inJsonNullableandnameshould be initialized withnullas it is declared asnullableadditionally.Generation Details
Steps to reproduce
I already created a pull request: #14766
Related issues/PRs
Didn't find any existing issue to this (which is quite suprising).
Suggest a fix
I already created a pull request: #14766
I am totally aware that this is a huge breaking change so I would like to discuss this issue openly. Maybe we need some kind of flag for the new behaviour. Any critics to this behaviour are welcome.
The text was updated successfully, but these errors were encountered: