ktorio / ktor

Framework for quickly creating connected applications in Kotlin with minimal effort
https://ktor.io
Apache License 2.0
12.56k stars 1.02k forks source link

Support swagger generation for ktor app #295

Open jimschubert opened 6 years ago

jimschubert commented 6 years ago

[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 commented 6 years ago

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

Something like this:

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

get("public") {}

Do you think it will work for you?

jimschubert commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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:

apikey:

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

orangy commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 4 years ago

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

oleg-larshin commented 3 years ago

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