Kitura / Kitura-OpenAPI

OpenAPI support for Kitura
Apache License 2.0
37 stars 16 forks source link

Ability to Add Descriptors to Endpoints #4

Open neurosaurus opened 6 years ago

neurosaurus commented 6 years ago

This issue is more of an enhancement. It would be useful to have the option to add additional information to each of the endpoints, like a description of the endpoint, any implementation notes, and maybe even ability to include the API owner's contact information.

For example:

GET /v1/todo
Description: List all todo items. Payload will contain all of your todo items.
Implementation Notes: Example payload { }.

I believe this provides the analogous functionality in Spring: http://docs.swagger.io/swagger-core/v1.5.0/apidocs/io/swagger/annotations/ApiOperation.html

ianpartridge commented 6 years ago

A way to achieve part of this would be to change the definitions of the Codable routing functions, for example https://github.com/IBM-Swift/Kitura/blob/master/Sources/Kitura/CodableRouter.swift#L49

Currently these are (just one example):

public func get<O: Codable>(_ route: String, handler: @escaping CodableArrayClosure<O>) { }

but perhaps it could be

public func get<O: Codable>(_ route: String, description: String? = nil, handler: @escaping CodableArrayClosure<O>)

so users could optionally provide a description of their route. The Swagger generation code could read the description and include it in the generated document, if present.

This does seem a bit "bolt-on" though, as Swagger allows users to provide descriptions for many elements of their APIs, e.g. responses, parameters, requestBodies etc. This wouldn't cover any of those.

CC: @seabaylea @nhardman @djones6

nacho4d commented 6 years ago

Xcode read comments, parse them and show them nicely in the documentation pane. I wonder if there is a way to read comments and inject them into swagger. ...

ianpartridge commented 6 years ago

Sadly not, @nacho4d, otherwise this would be ideal.

nacho4d commented 6 years ago

I haven't tried this but apparently there is .swiftdoc file generated when doing swift -emit-module foo.swift which contains documentation. This is where I got the idea from. https://github.com/realm/jazzy/issues/32

I guess this would require an extra step ... Maybe in a future this will be nice to have :)

ianpartridge commented 6 years ago

AFAIK the .swiftdoc format is unstable and doesn't have an API does it?

marwey commented 6 years ago

@ianpartridge I like your above suggestion. No idea if it can be implemented or not. But if yes, what about if we would go one step further and replace the String type with a richer custom type like e.g OpenAPISpec? Code would look like this then:

public func get<O: Codable>(_ route: String, description: OpenAPISpec? = nil, handler: @escaping CodableArrayClosure<O>)

This would allow us to add a lot more meta data to the route and generate much better OpenAPI documentation?

ianpartridge commented 6 years ago

Yes we could use a more flexible datatype than String, and that would allow us to support some of the other optional fields in the OpenAPI Operation object: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.1.md#operationObject

There is a philosophical question here about how closely we should tie the Kitura API to the OpenAPI spec's notion of what a REST API is.

marwey commented 6 years ago

Yes, good point. The fact that the description conforms to OpenAPI operation object could be abstracted away by making OpenAPISpec conform to e.g. an APISpec protocol and use the protocol type in the routing API instead of the concrete type.

ianpartridge commented 6 years ago

Yes those kinds of things would be possible.

I think it's important to be guided by people's usecases too. Are they interested in precisely codifying their REST APIs in Kitura code, and treating the generated Swagger doc as authoritative? Or is the generated Swagger doc more useful during API development and would be edited by other tools in the Swagger ecosystem once people are ready to "bake" the API?

marwey commented 6 years ago

Yes, looking at use cases is important too. Not sure if there will be a clear winner so. I can see (larger?) organisations following a top-down approach before they hand over the spec to development. I can also see a bottom-approach where one would like to understand the API of a newly developed prototype. So I guess giving people various options supporting different use cases is key.

From our own experience I can say that it is really hard to keep the spec in sync with the implementation and vice versa if they are both developed independently. So I would definitely would like to see the source code and spec description to be as tightly integrated as possible and generate the "truth" from the code.

For us three use cases are important:

seabaylea commented 6 years ago

We added the KituraOpenAPI component with the goal of deriving as much of the spec information as possible from the route handler itself as that acts as a "single source of truth" - there is no scope for the generated OpenAPI spec and the way the application handles the route to become out of sync.

We also knew that there's a set of metadata that can't be derived , particularly things like Summary and Description.

The Java approach for this in Spring and Microprofile is to add annotations to the handler, eg:

@GET
@Path("/findByStatus")
@Operation(summary = "Finds Pets by status",
           description = "Multiple status values can be provided with comma separated strings")
public Response findPetsByStatus(...) { ... }

which isn't an approach that we can take, but we can create some kind of descriptor struct (as per @marwey) as associate that with the route.

We could then do the association when the route is registered, eg:

router.get("/findByStatus", handler: findPetsByStatus, description:  findPetsByStatusInfo)

The challenge with this is that there's no real association between the handler and the description, other than setting a best practice for developers of declaring them adjacent to each other, eg:

let findPetsByStatusInfo = HandlerDescription(
    summary: "Finds Pets by status",
    description: "Multiple status values can be provided with comma separated strings")

func findPetsByStatus(query: Status, respondWith([Pets]?, RequestError?) -> Void) -> Void {
}

It would be good if there was a way to more tightly couple the two, but I'm not sure there's one available to us.

marwey commented 6 years ago

Great example, @seabaylea. Helps to see how this would feel if implemented.

One way to bring description and handler closer together would be to define the handler and operation as its own attribute and reference this in the router and description API:

let findByStatusOperation = "/findByStatus"
let findPetsByStatusHandler = findPetsByStatus

let findPetsByStatusInfo = HandlerDescription(
    summary: "Finds Pets by status",
    description: "Multiple status values can be provided with comma separated strings",
    operationId: findByStatusOperation,
    handler: findPetsByStatus
    )

router.get(findByStatusOperation, handler: findPetsByStatusHandler, description:  findPetsByStatusInfo)

Now, it is a bit clearer for a developer how things are connected. But the approach also creates a lot of noise and doesn't add any additional information for us to generate the API spec. We could have picked up operation id and description from the router as well.

So for this example I would be totally fine with the approach outlined by @seabaylea:

router.get("/findByStatus", handler: findPetsByStatus, description:  findPetsByStatusInfo) 

Above, the definition of the get route makes it quite clear how things are connected. For the data we are not able to derive from the route (like description, summary, tags,...) that are purely informational I would be totally fine specifying those as part of a 'HandlerDescription' structure since that information is only described once and therefore no "single source of truth" conflict. Might overlapp a bit with the Swift API documentation of that route - but it really serves a different purpose and audience.

However, a better linkage between handler and description would be beneficial for meta data that is not purely informational. In this context I've checked the 'Operation Object' definition in the OpenAPI spec again and assigned the spec attributes to the place where they could be picked up from for the generator:

Handler description structure

Route definition

Route implementation

Problematic from my point of view is the "route implementation" category since that information can not be derived from the route and will have a "single source of truth" conflict when defined in the description object since this overlapps with the route implementation. One way of linking e.g. the implementation of responses to the handler description could be to define a response collection in the handler description structure and have the reponse register itself with that structure:

From example above:

let findPetsByStatusInfo = HandlerDescription(
    summary: "Finds Pets by status",
    description: "Multiple status values can be provided with comma separated strings")

func findPetsByStatus(query: Status, respondWith([Pets]?, RequestError?) -> Void) -> Void {
}

Adding a response:

let badGatewayResponse = OpenAPIResponse(httpStatus: .badGateway,
                    description: "Bad parameters in request.", 
                    specDescription: findPetsByStatusInfo)
return badGatewayResponse

The 'OpenAPIResponse' initializer would then add itself to a reponse collection by calling

findPetsByStatusInfo.add(response: self)
seabaylea commented 6 years ago

I think an elegant solution would be to create an APIEndpoint (or similar) class/struct that contains both the metadata and the handler, that is then registered with the router, eg:

class findPetsByStatusAPI: APIEndpoint {
    summary: "Finds Pets by status",
    description: "Multiple status values can be provided with comma separated strings"),
    handler: findPetsByStatus

    func findPetsByStatus(query: Status, respondWith([Pets]?, RequestError?) -> Void) -> Void {
    }
}

router.get("/findByStatus", api: findPetsByStatusAPI)

however I suspect that we'll struggle to deal with the type erasure of the handler signature when we're putting it into the class. Its certainly work experimenting with though.

ianpartridge commented 6 years ago

That would increase the API surface area on Router even more though - it's already quite hard for newcomers to grok with the "raw" APIs, Codable routing APIs and the recent type-safe middleware routing APIs meaning we have >200 methods on Router 😨.

seabaylea commented 6 years ago

If we manage to get generic varargs that will drop considerably though ;-)

marwey commented 6 years ago

I personally like it when a system offers "incremental" capabilities. So I can start easy and use the API in a 'simple' way. When things are getting more seriously I like to be able to add more capabilities. So rather than putting everything in one big structure and dealing with too many things at once I like when the API offers integration points (e.g. method overloading, patterns,...) that I can explore when I want to.

But this is just my personal taste 😀