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

Support swagger generation for ktor app #295

Open
jimschubert opened this issue Jan 22, 2018 · 10 comments
Open

Support swagger generation for ktor app #295

jimschubert opened this issue Jan 22, 2018 · 10 comments
Assignees

Comments

@jimschubert
Copy link

jimschubert commented Jan 22, 2018

[question] Code review of my initial ktor generator at swagger-codegen?

Sorry if this isn't the correct place to request community feedback. Please redirect me if there's a better place to ask… I'd usually ask in a community's gitter chat room, but I can't find one for ktor.

I'm a core team member on Swagger Codegen. If you're not familiar with the project, it converts OpenAPI 2.0 (previously known as Swagger 2.0) specifications into different types of projects: clients, servers, documentation, and scripts.

I've just implemented the start of a ktor server generator for swagger codegen. It doesn't support swagger spec generation 100%, but the output and examples would provide a quick stubbed server which can easily be modified.

I was wondering if I could get some feedback on some of the patterns I've used in the generated code? The pull request is at swagger-api/swagger-codegen#7412, and generated output is under the samples/ directory.

There were two questions I have currently:

  1. Is there a way in the locations api to specify query string delimiters to split values into an array? OpenAPI 2.0 supports multi (which I see ktor supports, but I haven't added support for in the linked PR). But OpenAPI 2.0 also supports different delimiters (comma, space, pipe, and tab)… is there a way to automatically split these into an Array[T]?
  2. Because the code is generated at the api endpoint level, and restructuring the swagger specification to group by path isn't entirely trivial (the spec supports different authentication per endpoint), I've had to code a workaround for authentication. Specifically, if I have a GET /pet and a POST /pet where both have authentication, I've created a route registration like this for both:
    route("/pet") {
        put {
            val principal = call.authentication.principal<OAuthAccessTokenResponse>()
            
            if (principal == null) {
                call.respond(HttpStatusCode.Unauthorized)
            } else {
                call.respond(HttpStatusCode.NotImplemented)
            }
        }
    }
    .apply {
        try {
            authentication {
                oauth(client, ApplicationExecutors.asCoroutineDispatcher(), { ApplicationAuthProviders["petstore_auth"] }, {
                    // TODO: define a callback url here.
                    "/"
                })
            }
        } catch(e: io.ktor.application.DuplicateApplicationFeatureException){
            application.environment.log.warn("authentication block for '/pet' is duplicated in code. " +
            "Generated endpoints may need to be merged under a 'route' entry.")
        }
    }

I've wrap the authentication invocation in a try/catch because GET /pet has already registered the Authentication feature. Is there a way to key these separately in the event that our users may have, for example, different API keys for read vs. write APIs?

Thanks!

@orangy
Copy link
Contributor

orangy commented Jan 22, 2018

I will take a look at generated code later, but for authentication we plan to change it like this:

  • install(Authentication) { … } for configuration
  • authenticate { … } builder for introducing scope

Something like this:

authenticate {
   post("foo") {}
   put("bar") {}
   get("secret") {}
}

get("public") {}

Do you think it will work for you?

@orangy orangy self-assigned this Jan 22, 2018
@jimschubert
Copy link
Author

@orangy Thanks, man. I appreciate you checking it out.

It looks like the new authentication api would work, I'd have to see how the configuration is assigned to each endpoint. If the install block has a way to provide different strategies for http verb + path, that would probably work for most use cases. OpenAPI 2.0 spec even allows varying by query string,as long as each endpoint definition has a unique operationId. This is why I was asking about possibly providing a key for the auth strategy.

In the generator templates, I can loop over user provided api definitions as many times as I want, I just didn't see a way to do it with the current api.

@orangy
Copy link
Contributor

orangy commented Jan 22, 2018

authentication key and multiple configurations is a nice idea, but I wonder how often different authentication strategies in a single app is really used? Do you have use cases handy that I can think about?

@jimschubert
Copy link
Author

An example might be a maps application which is publicly read-only with Facebook oauth, but updates and deletes are done via API-KEY to allow vendors to add/delete map locations.

Example:

oauth:

  • GET /poi

apikey:

  • POST /poi
  • DELETE /poi

I can see if I can dig up actual examples from swagger codegen issues.

@orangy
Copy link
Contributor

orangy commented Jan 22, 2018

I see, makes sense. So it should basically look something like this:

install(Authentication) {
   // … default config …, if any
   configure("apikey") { … }
   configure("facebook") { … }
}

authenticate("facebook") {
   get("/poi") { … }
}

authenticate("apikey") {
   get("/poi") { … }
}

@jimschubert
Copy link
Author

Yeah, I think that would work for the generator's need.

The OpenAPI 2.0 (and 3.0) specification allow for logical OR on authentication. (see spec :Schema/security and an example spec document).

In the above use case, for instance, the spec would allow GET to be oauth or apikey. I don't know if that's an immediate concern, though, because an api key would be either passed in the header or query string and these are both accessible if you have oauth authentication block applied. There will be some limitations in how much code can be generated from the spec, and I think that's an acceptable trade-off.

Currently, Swagger Codegen doesn't process OR'd authentication strategies. We have issue swagger-api/swagger-codegen#6644 open to track, but I don't think there's been a fix for server implementations yet.

@orangy
Copy link
Contributor

orangy commented Jan 22, 2018

Just a note, more idiomatic way would be this:

val principal = call.authentication.principal<OAuthAccessTokenResponse>() 
                        ?: return@get call.respond(HttpStatusCode.Unauthorized)

                call.respond(HttpStatusCode.NotImplemented)

But it is also on our list to get parameter/auth/session retrieval with checks to have an easier way for Unauthorized/BadRequest/etc

@orangy orangy changed the title [question] Code review of my initial ktor generator at swagger-codegen? Support swagger generation for ktor app Mar 9, 2018
@orangy
Copy link
Contributor

orangy commented Mar 9, 2018

Update: we've designed some changes to the authentication APIs and believe this should solve the original problem with Swagger code generation. It's work in progress.

@hhariri
Copy link
Contributor

hhariri commented Jul 21, 2020

For further follow-ups on this issue, please see https://youtrack.jetbrains.com/issue/KTOR-807

@oleg-larshin
Copy link

Please check the following ticket on YouTrack for follow-ups to this issue. GitHub issues will be closed in the coming weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants