Swift5 use json type for any #9206

aymanbagabas · 2021-04-07T16:47:11Z

Use a custom JSON type instead of Any

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.1.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.

aymanbagabas · 2021-04-07T17:14:09Z

@4brunu looks like it's unable to pull one of the dependencies


[ERROR] Failed to execute goal org.apache.maven.plugins:maven-enforcer-plugin:3.0.0-M2:enforce (enforce-maven) on project openapi-generator-project: Execution enforce-maven of goal org.apache.maven.plugins:maven-enforcer-plugin:3.0.0-M2:enforce failed: Plugin org.apache.maven.plugins:maven-enforcer-plugin:3.0.0-M2 or one of its dependencies could not be resolved: Could not transfer artifact org.sonatype.sisu:sisu-guice:jar:noaop:2.1.7 from/to google-maven-central (https://maven-central.storage-download.googleapis.com/maven2/): GET request of: org/sonatype/sisu/sisu-guice/2.1.7/sisu-guice-2.1.7-noaop.jar from google-maven-central failed: Connection reset -> [Help 1]

4brunu · 2021-04-07T17:15:15Z

To reset CI, try to close and open the PR.

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

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

aymanbagabas · 2021-04-08T14:59:55Z

AnyCodable does not conform to Hashable Flight-School/AnyCodable#45 this is stopping me from switching to AnyCodable

aymanbagabas · 2021-04-08T15:09:44Z

Flight-School/AnyCodable#51 PR submitted and awaiting review

4brunu · 2021-04-08T15:17:09Z

You are fast 🙂
By the way I was checking you PR on AnyCodable and both AnyCodable, AnyDecodable and AnyEncodable conform to Equatable.
Maybe also AnyCodable, AnyDecodable and AnyEncodable should conform to Hashable?

aymanbagabas · 2021-04-08T15:28:19Z

You are fast 🙂
By the way I was checking you PR on AnyCodable and both AnyCodable, AnyDecodable and AnyEncodable conform to Equatable.
Maybe also AnyCodable, AnyDecodable and AnyEncodable should conform to Hashable?

I updated the PR to conform all to Hashable

aymanbagabas · 2021-04-09T19:38:55Z

I've reverted the mapping change. Just an FYI, swagger codegen swift5 generator also maps binary to Data https://github.com/swagger-api/swagger-codegen-generators/blob/master/src/main/java/io/swagger/codegen/v3/generators/swift/Swift5Codegen.java#L202

4brunu · 2021-04-11T21:29:32Z

@aymanbagabas why did you revert the map of binary to Data?
That change looked good to me.

4brunu · 2021-04-11T21:38:16Z

Two more thighs:

could you please also add the dependency AnyCodable XcodeGen?
since we are waiting for the PR you opened on the AnyCodable, and when that PR gets accepted and released, it may create a conflict with the hashable extension on this PR, could you change the AnyCodable dependency to be exactly 0.4.0? Just to try to avoid this future problem

aymanbagabas · 2021-04-12T14:20:27Z

@4brunu CI is failing even though I updated the test files to reflect the new mapping. Do you know what's going on?

EDIT: I noticed it passes when I also map file to Data.

4brunu · 2021-04-12T14:35:25Z

I think you need to update the unit tests in those two places

openapi-generator/modules/openapi-generator/src/test/java/org/openapitools/codegen/swift5/Swift5ModelTest.java

Lines 90 to 93 in 53e5986

    
           Assert.assertEquals(property4.dataType, "URL"); 
        
           Assert.assertEquals(property4.name, "binary"); 
        
           Assert.assertNull(property4.defaultValue); 
        
           Assert.assertEquals(property4.baseType, "URL");

openapi-generator/modules/openapi-generator/src/test/java/org/openapitools/codegen/swift5/Swift5ClientCodegenTest.java

Lines 109 to 110 in 53e5986

    
           Assert.assertEquals(op.returnType, "URL"); 
        
           Assert.assertEquals(op.bodyParam.dataType, "URL");

aymanbagabas · 2021-04-12T14:40:30Z

I think you need to update the unit tests in those two places

But I already did https://github.com/OpenAPITools/openapi-generator/pull/9206/files#diff-fa265925df4a882b13f49697c23722899f0400726b9c12ae75d8a88a7151d89b

4brunu · 2021-04-12T15:00:13Z

I don't have the time to dig unto it now, but maybe it's related to this

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

Lines 3210 to 3212 in 53e5986

    
           } else if (ModelUtils.isBinarySchema(p)) { 
        
               property.isBinary = true; 
        
               property.isFile = true; // file = binary in OAS3

EDIT: I tried to comment the line property.isFile = true; but it didn't work, so my hint may be wrong

aymanbagabas · 2021-04-12T15:14:49Z

// file = binary in OAS3

Shouldn't we map file to Data?

4brunu · 2021-04-12T15:44:53Z

No, let's deal with this problem in a different PR, and we need to investigate more on this issue

aymanbagabas · 2021-04-12T16:10:15Z

No, let's deal with this problem in a different PR, and we need to investigate more on this issue

From reading the OAS3 specs, file data type is not a special type as it's used to be in OAS2, instead, it's treated like any other schema type and is mapped to binary or base64.

aymanbagabas · 2021-04-12T16:19:13Z

I don't have the time to dig unto it now, but maybe it's related to this

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

Lines 3210 to 3212 in 53e5986

} else if (ModelUtils.isBinarySchema(p)) {

property.isBinary = true;

property.isFile = true; // file = binary in OAS3

EDIT: I tried to comment the line property.isFile = true; but it didn't work, so my hint may be wrong

There is also this

openapi-generator/modules/openapi-generator/src/main/java/org/openapitools/codegen/utils/ModelUtils.java

Lines 282 to 288 in 5359776

    
           public static boolean isFileSchema(Schema schema) { 
        
               if (schema instanceof FileSchema) { 
        
                   return true; 
        
               } 
        
               // file type in oas2 mapped to binary in oas3 
        
               return isBinarySchema(schema); 
        
           }

So essentially file equals to binary. FileSchema is defined as

    public FileSchema() {
        super("string", "binary");
    }

aymanbagabas · 2021-04-21T18:09:35Z

I've dropped the mapping commit in favor of #9308

4brunu · 2021-04-21T18:15:36Z

Thanks 👍
Looks good to me, let's wait for the CI to finish

4brunu · 2021-04-22T10:08:56Z

@wing328 for me this PR is ready to merge

4brunu · 2021-04-22T10:09:38Z

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

@@ -342,7 +342,15 @@ protected void addAdditionPropertiesToCodeGenModel(CodegenModel codegenModel,
        final Schema additionalProperties = getAdditionalProperties(schema);

        if (additionalProperties != null) {
-            codegenModel.additionalPropertiesType = getSchemaType(additionalProperties);
+            Schema inner = null;
+            if (ModelUtils.isArraySchema(schema)) {


@aymanbagabas could you please explain what this change does? (the code block, not the line itself)

This was getting the wrong inner type for additionalProperties when the schema is either an array or a map.

Thanks for explaining the change 🙂

aymanbagabas force-pushed the swift5-use-json-type-for-any branch from 6c02cd5 to 4a3c0b5 Compare April 7, 2021 16:56

aymanbagabas mentioned this pull request Apr 7, 2021

[swift] Add Swift Vapor client library #8515

Closed

5 tasks

aymanbagabas force-pushed the swift5-use-json-type-for-any branch 3 times, most recently from 0e292af to 6b665eb Compare April 7, 2021 19:18

4brunu reviewed Apr 8, 2021

View reviewed changes

aymanbagabas force-pushed the swift5-use-json-type-for-any branch from 6b665eb to 0eecc43 Compare April 8, 2021 14:48

aymanbagabas force-pushed the swift5-use-json-type-for-any branch from 0eecc43 to 71858ef Compare April 8, 2021 15:19

aymanbagabas force-pushed the swift5-use-json-type-for-any branch 3 times, most recently from 9e19eab to 5a3fe1a Compare April 9, 2021 19:23

aymanbagabas force-pushed the swift5-use-json-type-for-any branch from 5a3fe1a to 679c852 Compare April 12, 2021 14:10

ghost mentioned this pull request Apr 21, 2021

Swift5: Binary / File type generated incorrectly as URL in responses #9308

Closed

aymanbagabas force-pushed the swift5-use-json-type-for-any branch from 679c852 to 1654742 Compare April 21, 2021 18:02

aymanbagabas added 4 commits April 21, 2021 14:03

[swift5] Fix additionalProps inner type

6b6714b

[swift5] Add and use AnyCodable type instead of any

f0f7998

[swift5] Use the original petstore w/fake models for testing default

e4b63fa

[swift5] Update swift-tools-version to 5.1

367f856

aymanbagabas force-pushed the swift5-use-json-type-for-any branch from 1654742 to 68cd30f Compare April 21, 2021 18:05

[swift5] Update samples and docs

75b522a

aymanbagabas force-pushed the swift5-use-json-type-for-any branch from 68cd30f to 75b522a Compare April 21, 2021 18:07

4brunu reviewed Apr 22, 2021

View reviewed changes

wing328 added Client: Swift Issue: Bug labels Apr 26, 2021

wing328 added this to the 5.1.1 milestone Apr 26, 2021

wing328 merged commit f7c3773 into OpenAPITools:master Apr 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Swift5 use json type for any #9206

Swift5 use json type for any #9206

aymanbagabas commented Apr 7, 2021 •

edited

Loading

aymanbagabas commented Apr 7, 2021

4brunu commented Apr 7, 2021

aymanbagabas commented Apr 8, 2021

aymanbagabas commented Apr 8, 2021

4brunu commented Apr 8, 2021

aymanbagabas commented Apr 8, 2021

aymanbagabas commented Apr 9, 2021

4brunu commented Apr 11, 2021

4brunu commented Apr 11, 2021

aymanbagabas commented Apr 12, 2021 •

edited

Loading

4brunu commented Apr 12, 2021

aymanbagabas commented Apr 12, 2021

4brunu commented Apr 12, 2021 •

edited

Loading

aymanbagabas commented Apr 12, 2021

4brunu commented Apr 12, 2021

aymanbagabas commented Apr 12, 2021

aymanbagabas commented Apr 12, 2021 •

edited

Loading

aymanbagabas commented Apr 21, 2021

4brunu commented Apr 21, 2021

4brunu commented Apr 22, 2021

4brunu Apr 22, 2021 •

edited

Loading

aymanbagabas Apr 22, 2021

4brunu Apr 22, 2021

Swift5 use json type for any #9206

Swift5 use json type for any #9206

Conversation

aymanbagabas commented Apr 7, 2021 • edited Loading

PR checklist

aymanbagabas commented Apr 7, 2021

4brunu commented Apr 7, 2021

aymanbagabas commented Apr 8, 2021

aymanbagabas commented Apr 8, 2021

4brunu commented Apr 8, 2021

aymanbagabas commented Apr 8, 2021

aymanbagabas commented Apr 9, 2021

4brunu commented Apr 11, 2021

4brunu commented Apr 11, 2021

aymanbagabas commented Apr 12, 2021 • edited Loading

4brunu commented Apr 12, 2021

aymanbagabas commented Apr 12, 2021

4brunu commented Apr 12, 2021 • edited Loading

aymanbagabas commented Apr 12, 2021

4brunu commented Apr 12, 2021

aymanbagabas commented Apr 12, 2021

aymanbagabas commented Apr 12, 2021 • edited Loading

aymanbagabas commented Apr 21, 2021

4brunu commented Apr 21, 2021

4brunu commented Apr 22, 2021

4brunu Apr 22, 2021 • edited Loading

Choose a reason for hiding this comment

aymanbagabas Apr 22, 2021

Choose a reason for hiding this comment

4brunu Apr 22, 2021

Choose a reason for hiding this comment

aymanbagabas commented Apr 7, 2021 •

edited

Loading

aymanbagabas commented Apr 12, 2021 •

edited

Loading

4brunu commented Apr 12, 2021 •

edited

Loading

aymanbagabas commented Apr 12, 2021 •

edited

Loading

4brunu Apr 22, 2021 •

edited

Loading