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

[swift5] Add property x-null-encodable extension for full control over encoding value/nil or nothing #11141

Merged

Conversation

jarrodparkes
Copy link
Contributor

Attempts to solve #11033

This PR adds a new property vendor extension x-null-encodable for use with the swift5 generator. If set to true, then the generated properties will use the NullEncodable<Wrapped> type:

public enum NullEncodable<Wrapped> {
    case encodeNothing
    case encodeNull
    case encodeValue(Wrapped)
}

This type gives the client full control of whether to encode a value/null -OR- encode nothing.

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

Core Team Members

@jgavris
@ehyche
@Edubits
@jaz-ah
@4brunu

@jarrodparkes jarrodparkes changed the title Issue 11033/swift5 null encodable [swift5] Add property x-null-encodable extension for full control over encoding value/nil or nothing Dec 16, 2021
@4brunu
Copy link
Contributor

4brunu commented Dec 16, 2021

@jarrodparkes one question, this only applies to properties that can be nil or applies to all properties?
I left some comments, that are just to start a discussing, to understand what should be the right behavior

@jarrodparkes
Copy link
Contributor Author

@4brunu

this only applies to properties that can be nil or applies to all properties?

This behavior applies to properties earmarked as x-null-encodable in the spec. Properties with this extension are encoded into the new NullEncodable type which gives clients the flexibility to encode a value, encode null, or encode nothing for the property. This is different than our current behaviors...

  1. A property that is nullable and required will always be encoded regardless of its value
  2. A property that is nullable will only be encoded when the value is non-nil

@4brunu
Copy link
Contributor

4brunu commented Dec 16, 2021

So this will basically override/ignore the nullable and required, right?

@jarrodparkes
Copy link
Contributor Author

@4brunu yeh, more or less. the spec example doesn't make that clear because x-null-encodable is used alongside nullable and required. but, to your point, x-null-encodable essentially overrides them

@jarrodparkes
Copy link
Contributor Author

@4brunu I was able to restore the default values 👍 let me know what you think

@4brunu
Copy link
Contributor

4brunu commented Dec 17, 2021

@jarrodparkes overall this looks good to me, I just left some small comments on some improvements/questions

@4brunu
Copy link
Contributor

4brunu commented Dec 17, 2021

CI is failing, but it looks like it's not related to this PR.
Could you please close and reopen this PR please, to restart the CI?

@jarrodparkes
Copy link
Contributor Author

@4brunu sure thing

@4brunu
Copy link
Contributor

4brunu commented Dec 17, 2021

That's weird the CI failed again with a different error this time, that is still not related to this PR (I think)

@jarrodparkes
Copy link
Contributor Author

jarrodparkes commented Dec 20, 2021

That's weird the CI failed again with a different error this time, that is still not related to this PR (I think)

@4brunu yeh, seems like that might be an issue across a few different PRs.

@wing328 still curious, if you might have some insights on the following...

TL;DR - In postProcessModels we inject a custom property vendorExtension called x-null-encodable-default-value. However, x-null-encodable-default-value doesn't appear to be present/usable in the model_doc.mustache template. Looking at DefaultGenerator.java it seems that the model documentation is resolved AFTER postProcessModels, so perhaps we aren't using something correctly in the mustache template? Any help would be greatly appreciated.

@jarrodparkes
Copy link
Contributor Author

@4brunu let me know if there anything else you need to see here. AFAIK the failed check isn't related to this PR

@wing328
Copy link
Member

wing328 commented Dec 29, 2021

Restarted the bitrise job. Let's see how that goes.

@wing328
Copy link
Member

wing328 commented Dec 29, 2021

@4brunu
Copy link
Contributor

4brunu commented Dec 29, 2021

Looks good to me 👍

@jarrodparkes
Copy link
Contributor Author

anything holding this up @4brunu @wing328 ?

@wing328 wing328 merged commit 9c3cba9 into OpenAPITools:master Jan 2, 2022
@jarrodparkes jarrodparkes deleted the ISSUE-11033/swift5-null-encodable branch January 3, 2022 16:01
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.

3 participants