[Swift5]: encode nil when swift5 optional is empty #10929

jarrodparkes · 2021-11-22T21:27:38Z

Attempts to solve #10926.

4brunu · 2021-11-22T21:52:04Z

Hey @jarrodparkes thanks for creating this PR 👍
I think we should make this configurable via OAS extension x-swift-always-encode or by introducing a flag in the swift generator.
I would also like to hear the opinion of @aymanbagabas, since he introduced this change, he might be able to help review or give some ideas to solve this issue.
Thanks

jarrodparkes · 2021-11-22T22:04:52Z

@4brunu

Sounds good. I wanted to use an extension (like x-swift-always-encode), but was having major difficulty figuring out how to implement. The current approach is just a naive solution that I was able to manage while trying to limit the scope of the change.

4brunu · 2021-11-23T10:09:09Z

The problem is that this is a behaviour change.
Because before the null values were not sent to the server, and this way they are.
An easy solution for this is to create a flag like this one.

openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/Swift5ClientCodegen.java

Lines 250 to 252 in e5159ef

    
           cliOptions.add(new CliOption(CodegenConstants.NON_PUBLIC_API, 
        
                   CodegenConstants.NON_PUBLIC_API_DESC 
        
                           + "(default: false)"));

But let's wait for feedback.

4brunu · 2021-11-23T10:13:29Z

But I'm not sure what's the right behaviour, or if there is a right behaviour.
@aymanbagabas and @wing328 could you please help to define what's the right behaviour, or if there is no right and this should be configurable?
The question here is, if we are sending a post request, and one of the properties of the body, for example the name is null, should the name be sent to the server as null, or should we should ignore the name and only send the id?

class Pet {
    let id: Int
    let name: String?
}

Thanks

aymanbagabas · 2021-11-23T11:44:13Z

@4brunu imo the behavior should be configurable. However, iirc, when this feature was implemented, it basically checked if a property can be nullable in the specs and uses encodeNil accordingly.

EDIT: related commit 0f5e7d1#diff-64f663dd600a47be7f81fb31e4858eeacb9627b668c5f35b7afcc98da25916de encodeIfPresent

4brunu · 2021-11-23T12:30:27Z

I think we can create a flag like EncodeModelNullProperties or something similar to allow this customisation.

4brunu · 2021-11-23T12:30:47Z

@aymanbagabas thanks for your feedback 👍

4brunu · 2021-11-25T17:01:15Z

@jarrodparkes if you need help with this please let me know 🙂

jarrodparkes · 2021-11-26T00:05:21Z

@4brunu yes please, I would love help with this

4brunu · 2021-11-26T01:06:10Z

Here is another PR where I did something similar, add an option and use it in the mustache files, does this help?

#4556

Look at the file Swift4CodeGen.java and the mustaches files for the nonpublic if else's.

jarrodparkes · 2021-11-29T00:15:51Z

@4brunu this will give a good start I'm sure. I think the open question I'd have after a quick cursory glance is that your option is sort of a "global" option versus something I'd want to specify (1) per-model AND (2) as an array of property names

4brunu · 2021-11-29T15:04:37Z

I think it's a bit strange for an API to have different rules for different endpoints, so I think the global rule works.
But this is open to discussion, if someone has a different opinion.

jarrodparkes · 2021-11-29T16:54:19Z

@4brunu that's fair and accurate with the API I'm working with. I'll go with that for now in my implementation.

Later we could readdress the per-model/etc. if that is still a concern

4brunu · 2021-11-29T16:55:28Z

Looks good to me 👍

jarrodparkes · 2021-11-30T16:37:56Z

restarted work on this today 😄 !

jarrodparkes · 2021-11-30T16:38:41Z

@4brunu I think i've got the core of this PR ready to go now — let me know what you think 👍

4brunu · 2021-11-30T16:50:18Z

modules/openapi-generator/src/main/resources/swift5/modelObject.mustache

@@ -63,7 +63,21 @@
    {{#nonPublicApi}}internal{{/nonPublicApi}}{{^nonPublicApi}}public{{/nonPublicApi}} func encode(to encoder: Encoder) throws {
        var container = encoder.container(keyedBy: CodingKeys.self)
        {{#allVars}}
+        {{#encodeModelNullProperties}}
+        {{#required}}
+        try container.encode{{#isNullable}}IfPresent{{/isNullable}}({{{name}}}, forKey: .{{{name}}})


@jarrodparkes if something isNullable shouldn't we call container.encodeNil?

Maybe this should be

{{#required}} {{^isNullable}} try container.encode({{{name}}}, forKey: .{{{name}}}) {{/isNullable}} {{#isNullable}} if let {{{name}}} = {{{name}}} { try container.encode({{{name}}}, forKey: .{{{name}}}) } else { try container.encodeNil(forKey: .{{{name}}}) } {{/isNullable}} {{/required}}

What do you think? This is more a question, than an answer, so I would like to have your feedback on this 🙂

@4brunu Your code sample looks right to me, but the problems I seemed to be encountering were weird compound states with required and isNullable. In fact, these two booleans have confused me (and other devs on my team) quite a bit since they seem to refer to the same thing depending on context.

What I was using as my "source of truth" was regenerate the sample and make sure all optionals are wrapped with the if/let construct OR encodeIfPresent. If the resulting code ever tried to encode an optional, then it wouldn't even compile.

The code I currently seemed to cover these cases well, but completely open for improvement/change. I can try your suggestion and see if the resulting code looks good.

required means if is must be present, either the value or null.
isNullable defines if the type can be null.
https://stackoverflow.com/questions/45575493/what-does-required-in-openapi-really-mean

This makes me rethink the entire PR.

Instead of an addition, this should be a bug fix, and we don't need the new flag encodeModelNullProperties.

I think this should be the implementation for all the cases.

{{#required}} {{^isNullable}} try container.encode({{{name}}}, forKey: .{{{name}}}) {{/isNullable}} {{#isNullable}} if let {{{name}}} = {{{name}}} { try container.encode({{{name}}}, forKey: .{{{name}}}) } else { try container.encodeNil(forKey: .{{{name}}}) } {{/isNullable}} {{/required}} {{^required}} try container.encode{{#isNullable}}IfPresent{{/isNullable}}({{{name}}}, forKey: .{{{name}}}) {{/required}}

What do you think?

But if it's required, it should always be encoded, even if it's null.
If it's not required, it should be encoded only if it's present, or am I wrong?
So I think the current implementation is correct, or am I mistaken?

I think we are both saying the same thing and the implementation is wrong.

But if it's required, it should always be encoded, even if it's null.
If it's not required, it should be encoded only if it's present, or am I wrong?
So I think the current implementation is correct, or am I mistaken?

translated

But if it is {{#required}}, then it should always be encoded, even if it's null. ==> encode
If it's {{^required}}, then it should be encoded only if it's present ==> encodeIfPresent

We are saying the same thing 🙂
That's the current implementation on master branch

try container.encode{{^required}}IfPresent{{/required}}({{{name}}}, forKey: .{{{name}}})

I think I'm going crazy :face_palm: you're right. I think I was probably just using the combination of required and x-nullable wrong this whole time

It's normal, it's confusing the required vs nullable thing.

modules/openapi-generator/src/main/resources/swift5/modelObject.mustache

jarrodparkes · 2021-11-30T19:07:24Z

TL;DR - I was missing required and x-nullable (isNullable). To explicitly encode nil for a property, include it as required and specify it as x-nullable: true

jarrodparkes closed this Nov 30, 2021

jarrodparkes force-pushed the feat-10926/swift-5-encode-nil branch from 272c34c to c13067d Compare November 30, 2021 15:44

add swift5 option to explicitly encode null for models

0f4fc3b

jarrodparkes reopened this Nov 30, 2021

4brunu reviewed Nov 30, 2021

View reviewed changes

modules/openapi-generator/src/main/resources/swift5/modelObject.mustache Show resolved Hide resolved

jarrodparkes mentioned this pull request Nov 30, 2021

[Swift5] Generate models where optional properties (x-swift-always-encode) can be flagged to explicitly encode nil when empty #10926

Closed

jarrodparkes closed this Nov 30, 2021

jarrodparkes deleted the feat-10926/swift-5-encode-nil branch December 1, 2021 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Swift5]: encode nil when swift5 optional is empty #10929

[Swift5]: encode nil when swift5 optional is empty #10929

jarrodparkes commented Nov 22, 2021

4brunu commented Nov 22, 2021

jarrodparkes commented Nov 22, 2021

4brunu commented Nov 23, 2021 •

edited

Loading

4brunu commented Nov 23, 2021

aymanbagabas commented Nov 23, 2021 •

edited

Loading

4brunu commented Nov 23, 2021

4brunu commented Nov 23, 2021

4brunu commented Nov 25, 2021

jarrodparkes commented Nov 26, 2021

4brunu commented Nov 26, 2021 •

edited

Loading

jarrodparkes commented Nov 29, 2021

4brunu commented Nov 29, 2021

jarrodparkes commented Nov 29, 2021

4brunu commented Nov 29, 2021

jarrodparkes commented Nov 30, 2021

jarrodparkes commented Nov 30, 2021

4brunu Nov 30, 2021 •

edited

Loading

4brunu Nov 30, 2021

4brunu Nov 30, 2021

jarrodparkes Nov 30, 2021

4brunu Nov 30, 2021

4brunu Nov 30, 2021

jarrodparkes Nov 30, 2021

4brunu Nov 30, 2021 •

edited

Loading

jarrodparkes Nov 30, 2021

4brunu Nov 30, 2021

jarrodparkes commented Nov 30, 2021

[Swift5]: encode nil when swift5 optional is empty #10929

[Swift5]: encode nil when swift5 optional is empty #10929

Conversation

jarrodparkes commented Nov 22, 2021

4brunu commented Nov 22, 2021

jarrodparkes commented Nov 22, 2021

4brunu commented Nov 23, 2021 • edited Loading

4brunu commented Nov 23, 2021

aymanbagabas commented Nov 23, 2021 • edited Loading

4brunu commented Nov 23, 2021

4brunu commented Nov 23, 2021

4brunu commented Nov 25, 2021

jarrodparkes commented Nov 26, 2021

4brunu commented Nov 26, 2021 • edited Loading

jarrodparkes commented Nov 29, 2021

4brunu commented Nov 29, 2021

jarrodparkes commented Nov 29, 2021

4brunu commented Nov 29, 2021

jarrodparkes commented Nov 30, 2021

jarrodparkes commented Nov 30, 2021

4brunu Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

4brunu Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jarrodparkes commented Nov 30, 2021

4brunu commented Nov 23, 2021 •

edited

Loading

aymanbagabas commented Nov 23, 2021 •

edited

Loading

4brunu commented Nov 26, 2021 •

edited

Loading

4brunu Nov 30, 2021 •

edited

Loading

4brunu Nov 30, 2021 •

edited

Loading