Skip to content

[dart-dio-next] Improve handling nullable objects#10118

Merged
wing328 merged 17 commits intoOpenAPITools:masterfrom
Bdaya-Dev:dart-dio-next/use-nullable-json-objects
Aug 18, 2021
Merged

[dart-dio-next] Improve handling nullable objects#10118
wing328 merged 17 commits intoOpenAPITools:masterfrom
Bdaya-Dev:dart-dio-next/use-nullable-json-objects

Conversation

@ahmednfwela
Copy link
Contributor

@ahmednfwela ahmednfwela commented Aug 9, 2021

Currently nullable types need to be handled better at serialization level, for example:
deserializing BuiltMap<String,JsonObject> when one of the values is null will throw an error at run time, since JsonObject is not a nullable type

these changes are not perfect, especially changing these

typeMapping.put("object", "JsonObject?");
typeMapping.put("AnyType", "JsonObject?");

imports.put("JsonObject?", "package:built_value/json_object.dart");

but I am not sure what's the best approach for putting the null operator (?) after a type ?

also this PR makes use of the FullType.nullable named constructor for defining serializers

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh bin/configs/dart*
    ./bin/utils/export_docs_generators.sh
    
  • File the PR against the correct branch: master, 5.3.x, 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

CC: @jaumard @josh-burton @amondnet @sbu-WBT @kuhnroyal @agilob

@kuhnroyal
Copy link
Contributor

Not everything is nullable, we need to default to non-nullable.
This requires built_value >= 8.1.0

Do you have a sample spec for collections with nullable content?

@kuhnroyal
Copy link
Contributor

I created #10122 for built_value update

@kuhnroyal
Copy link
Contributor

Looking at Kotlin and C# implementations, none of them support null in collections. There also doesn't seem to be a representation for this in the specification.

Maybe a ref with nullable: true not sure.
I don't think we should merge this atm.

@ahmednfwela
Copy link
Contributor Author

ahmednfwela commented Aug 10, 2021

@ahmednfwela
Copy link
Contributor Author

The only problem I am facing is:
how to use JsonObject for non-nullable objects, and JsonObject? for nullable objects

@kuhnroyal
Copy link
Contributor

I see, I didn't know that items can have nullable property.
You can try accessing this via CodegenProperty.items.isNullable or CodegenProperty.mostInnerItems.isNullable in the template. I don't think you need to change anything in the Java code. We definitely don't want JsonObject? in the type or import mapping.

@ahmednfwela
Copy link
Contributor Author

@kuhnroyal what do you think about this change? I even removed the java code for the serializer type and wrote it in mustache using recursive patterns

@ahmednfwela
Copy link
Contributor Author

also @wing328 can this be included in 5.2.1 ?

@kuhnroyal
Copy link
Contributor

I like it but there is still a couple things open. I don't think this should go into 5.2.1

@ahmednfwela
Copy link
Contributor Author

@kuhnroyal is there anything else I can fix on my end?

Copy link
Contributor

@kuhnroyal kuhnroyal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really like what you did here! 2 small changes requested

Copy link
Contributor

@kuhnroyal kuhnroyal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work, that will also make it easier to fix a couple problems around missing serializers. Thanks!

@kuhnroyal
Copy link
Contributor

@agilob Do you have anything else here?

Copy link
Contributor

@agilob agilob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kuhnroyal
Copy link
Contributor

@wing328 This can be merged

@wing328 wing328 merged commit 63562dc into OpenAPITools:master Aug 18, 2021
@ahmednfwela ahmednfwela deleted the dart-dio-next/use-nullable-json-objects branch August 18, 2021 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants