Skip to content

[swift] Add Swift Vapor client library#8515

Closed
aymanbagabas wants to merge 6 commits intoOpenAPITools:masterfrom
aymanbagabas:swift5-vapor
Closed

[swift] Add Swift Vapor client library#8515
aymanbagabas wants to merge 6 commits intoOpenAPITools:masterfrom
aymanbagabas:swift5-vapor

Conversation

@aymanbagabas
Copy link
Contributor

@aymanbagabas aymanbagabas commented Jan 23, 2021

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.

@auto-labeler auto-labeler bot added the WIP Work in Progress label Jan 23, 2021
@auto-labeler
Copy link

auto-labeler bot commented Jan 23, 2021

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

@aymanbagabas aymanbagabas force-pushed the swift5-vapor branch 4 times, most recently from ad966c3 to d19159b Compare January 28, 2021 22:57
@aymanbagabas
Copy link
Contributor Author

@4brunu @tanner0101

Hi all,

This PR is not done yet but I would like to have your feedback on this. I still need to do the API docs and testing, the main components are pretty much done. This would resolve #5014 and #4957

@4brunu
Copy link
Contributor

4brunu commented Jan 29, 2021

Hi @aymanbagabas,
First of all, great work, and thanks for taking the time and effort to do this contribution 👍

Since Vapor is a server side framework, and this introduces a lot of changes, I think it makes more sence to separate this into a new generator.
Looking at other generators, for example kotlin, it has AbstractKotlinCodegen.java, which has the shared base code for all kotlin generator, and then KtormSchemaCodegen.java, KotlinClientCodegen.java, KotlinServerCodegen.java, KotlinSpringServerCodegen.java, KotlinVertxServerCodegen.java.
I think we could do the same, since we already have Swift5ClientCodegen.java.

What do you think about this?
Related to Vapor, I never used it, so it's difficult to me to review this implementation.

I will comment on some changed on the code, to try to better understand some of the changes.

Copy link
Contributor

@4brunu 4brunu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, I just left a few comments, to try to understand the changes 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, could you elabore on what's the use case for this default response?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed a way to determine if operation responses contain a default response. The way I'm handling each operation return value is to return an enum based on the response status code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you delete this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

List is not a Swift type. I had a problem while testing DocuSign specs where they define a type List and that got mapped to the wrong value Array which is a primitive type in swift.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate more on this issue please? Perhaps show an example of the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that List is not a Swift nor OAS data type. I was working with DS specs and one of their models is named List, defined as below. I was facing a problem where the generator was naming this model Array instead of List.

...
    "list": {
      "type": "object",
      "properties": {
        "anchorAllowWhiteSpaceInCharacters": {
          "description": "",
          "type": "string"
        },
        "anchorAllowWhiteSpaceInCharactersMetadata": {
          "$ref": "#/definitions/propertyMetadata",
          "description": ""
        },
        "anchorCaseSensitive": {
...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, thanks for this fix and for the extra info 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain this change please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swift uses backticks to escape reserved words. Refer to the note section here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change, that I suggested to open a new PR with, against the 6.0.x branch

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that this breaks objCompatible files, so I changed it to escape objCompatible reserved words with a _ and back-ticks otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain this change please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Character uses quotes when initialized same as strings it needs to be surrounded by quotes " when defined as an enum.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is a bug fix? It's not working in the current release?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit dropped. I was trying something out and forgot to remove this line.

@4brunu
Copy link
Contributor

4brunu commented Jan 29, 2021

Is this already working already? 🙂
Nice work, thanks once again for your contribution, and please don't feel discouraged by all the questions I left, I'm just trying to understand the changes.

Copy link
Contributor Author

@aymanbagabas aymanbagabas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @4brunu for taking the time to review this pr.

While in fact Vapor is a server-side framework, here I'm using Vapor's client/content APIs to generate Vapor compatible APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed a way to determine if operation responses contain a default response. The way I'm handling each operation return value is to return an enum based on the response status code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

List is not a Swift type. I had a problem while testing DocuSign specs where they define a type List and that got mapped to the wrong value Array which is a primitive type in swift.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swift uses backticks to escape reserved words. Refer to the note section here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Character uses quotes when initialized same as strings it needs to be surrounded by quotes " when defined as an enum.

@aymanbagabas
Copy link
Contributor Author

Looking at other generators, for example kotlin, it has AbstractKotlinCodegen.java, which has the shared base code for all kotlin generator, and then KtormSchemaCodegen.java, KotlinClientCodegen.java, KotlinServerCodegen.java, KotlinSpringServerCodegen.java, KotlinVertxServerCodegen.java.
I think we could do the same, since we already have Swift5ClientCodegen.java.

What do you think about this?
Related to Vapor, I never used it, so it's difficult to me to review this implementation.

While it will be cleaner, I don't see that as necessary since all the libraries (including this one) are client-side generators. And the language mapping is the same, the only difference I can see is that Vapor already implements all the boiler plate code needed for generating a client API, i.e. encoding, decoding, and other helpers are all built-in Vapor.

What I would do is separate Vapor templates from other libraries' templates, I think that would be a good middle ground. What do you think?

@aymanbagabas aymanbagabas force-pushed the swift5-vapor branch 4 times, most recently from b9137f7 to 673a0fe Compare February 9, 2021 19:59
@aymanbagabas aymanbagabas changed the title [WIP][swift] Add Swift Vapor library [swift] Add Swift Vapor client library Feb 9, 2021
@aymanbagabas aymanbagabas marked this pull request as ready for review February 9, 2021 20:02
@aymanbagabas aymanbagabas force-pushed the swift5-vapor branch 3 times, most recently from 1886af8 to 2ae9e2b Compare February 9, 2021 20:27
@4brunu
Copy link
Contributor

4brunu commented Feb 10, 2021

While it will be cleaner, I don't see that as necessary since all the libraries (including this one) are client-side generators. And the language mapping is the same, the only difference I can see is that Vapor already implements all the boiler plate code needed for generating a client API, i.e. encoding, decoding, and other helpers are all built-in Vapor.

What I would do is separate Vapor templates from other libraries' templates, I think that would be a good middle ground. What do you think?

From what I understand, this will be used to generate a client project, that will be used on a Vapor server, so that the Vapor server can invoke other API servers, is this correct or did I understand it wrong?

The main blocker that I see in this PR, is that it has a lot of breaking changes.
So I suggest to separate the breaking changes from the new library option Vapor to make it easier to merge.
I will comment all the breaking changes so that we can discuss them.

Copy link
Contributor

@4brunu 4brunu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I commented all the breaking changes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate more on this issue please? Perhaps show an example of the issue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is a bug fix? It's not working in the current release?

@aymanbagabas aymanbagabas force-pushed the swift5-vapor branch 5 times, most recently from 2d56a1a to 5ab962c Compare March 1, 2021 04:12
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aymanbagabas I think the change of AnyType and object to JSON, could be applied to all libraries.
What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be a great idea since JSON is Codable which can be decoded/encoded easily.

@4brunu
Copy link
Contributor

4brunu commented Mar 31, 2021

@aymanbagabas since this PR it quite big now, would you mind isolate some changes here, and send them in a different PR?
Two that come to mind are useSPMFileStructure and the JSON object.
What do you think of this?

I did make useSPMFileStructure into its own PR but not the other one. Man splitting and restructuring git commits is not fun at all.

Thanks, I already reviewed it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aymanbagabas this should only be done when using classes, not structs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codable can be applied to both AFAICT. I added encode(encoder:) and init(decoder:) explicitly to decode null values since you need to use encoderIfPresent() and decoderIfPresent() to capture nulls instead of ignoring the optionals all together.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we can use explicitly encode(encoder:) to structs, but I think we should rely on the implicit implementation, because it's less code to maintain and one less source of bugs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I now, the implicit implementation of encode(encoder:) already deals with the nulls, or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you're right. I meant to encode nullable variables. To do so, you gotta use encode(encoder:) instead of encodeIfPresent(encoder:) for optional values so that it encodes nulls. We really don't need init(decoder:) so I'm just gonna drop that one. Here's a good example of what I'm talking about https://stackoverflow.com/a/47268112/10913628.

These are the possible four cases of encoding. I'm using String as an example.

isNullable isRequired type func
true true String? encode()
true false String? encodeIfPresent()
false true String encode()
false false String? encodeIfPresent()

My question is, does the generator always has isNullable and isRequired available? Because I know that nullable is an OAI 3.0 feature and it doesn't exist in either 2.0 nor 3.1.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since that's a question related to OAI, I don't know the answer.
But I think everything comes down to String? or String.
Looking at the first case, isNullable true, and isRequired true, I'm not sure if the func should be encode() or encodeIfPresent().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to encode null in the generated JSON, we have to use encode() instead of encodeIfPresent(). Note that encodeIfPresent() is the one used for optionals and that's the default implementation of func encode(to:).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@4brunu I will make this change to encode nulls in the encoded JSONs. I'm also gonna start splitting this PR into smaller ones.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your effort in splitting this big PR into small ones, that will make it a lot easier to review each PR.

Some good candidates to a small PR are this one, Any to JSON, reserved word _ or `.

Ideally this PR end's up just having the Vapor changes.

@aymanbagabas
Copy link
Contributor Author

@4brunu @wing328 I've split most of the changes in the following PRs
#9074 #9203 #9204 #9205 #9206
Adding Vapor client is the last one left which depends heavily on these PRs. I'll keep this PR open for discussion.

@4brunu 4brunu mentioned this pull request Apr 8, 2021
5 tasks
@aymanbagabas aymanbagabas force-pushed the swift5-vapor branch 5 times, most recently from 82cd240 to e7da117 Compare May 4, 2021 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Client: Swift WIP Work in Progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments