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

[crystal][client] Make optional properties nillable in models #10723

Merged
merged 12 commits into from
Jan 29, 2022

Conversation

cyangle
Copy link
Contributor

@cyangle cyangle commented Oct 28, 2021

Closes #10721

Mark model properties as nillable when applicable, and also handle property serialization correctly.

@wing328 The model initializers are already using nillable types for parameters, this PR allows setting legitimate nil values to model properties

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/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master (5.3.0), 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.

Only REQUIRED and NOT NULLABLE variables can NOT have type Nil
All OPTIONAL and NULLABLE-REQUIRED variables have type Nil
Only NULLABLE-REQUIRED variables should emit keys with null values when they are serialized, json example: property name : String? = nil; the json representation for this property is {"name": null}
For all OPTIONAL variables having Nil values, their variable keys would be skipped during serialization. The json representation for OPTIONAL property name : String? = nil;  would be: {}
For any required property, assigning nil value to it will result in compilation error
The datatype simply can not hold value nil, so there's no need to check it
@cyangle
Copy link
Contributor Author

cyangle commented Oct 28, 2021

@wing328 I have updated this PR to remove nullable checks.

I'm not sure whether I should target it to the 6.0.x branch.

@cyangle cyangle changed the title [crystal][client] Closes #10721 Make model properties nillable [crystal][client] Make model properties nillable Oct 29, 2021
Crystal lang doesn't have method equal?
We should use method same? instead of ruby's equal? method

Reference: https://crystal-lang.org/api/master/Reference.html#same?(other:Reference):Bool-instance-method
Setting Pet optional properties to nil values is allowed by add_pet api endpoint
Test model initializations
Test compilation error when model is initialized without required properties
@cyangle
Copy link
Contributor Author

cyangle commented Oct 30, 2021

@wing328 I've added quite a few integration tests to cover the code changes and also found and fixed a comparison bug while I was writing those tests.

Without this PR, users of the generated crystal clients would have to set all properties of models with none null values.

Crystal lang usually doesn't have any official SDK support from vendors. I'm hoping OpenAPI-Generator could be a feasible option for generating SDKs.

Could you please take a look at this PR and provide feedback so that I can improve it and get it merged?

Thanks.

@cyangle cyangle changed the title [crystal][client] Make model properties nillable [crystal][client] Make optional properties nillable in models Oct 30, 2021
@wing328 wing328 added this to the 5.4.0 milestone Jan 3, 2022
@wing328
Copy link
Member

wing328 commented Jan 29, 2022

Tested locally and the result is good.

I'm not sure whether I should target it to the 6.0.x branch.

Let's consider this a bug fix. We'll then see if people need the old (current) behaviour somehow.

@wing328 wing328 merged commit 3f0f92f into OpenAPITools:master Jan 29, 2022
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.

[BUG][crystal][client] Optional properties should have nillable types in models
2 participants