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

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

Merged

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

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