mattpolzin / VaporOpenAPI

MIT License
57 stars 7 forks source link

Is description on a Path parameter supported? #3

Closed m-barthelemy closed 4 years ago

m-barthelemy commented 4 years ago

Hi, me again. with another question.

Does VaporOpenAPI current support setting a description on Path parameters, like it does for query parameters? I had a quick look at the source code, and it looks like it doesn't for now, but just wanted your confirmation in case I missed something obvious.

mattpolzin commented 4 years ago

That is correct. Last I checked there was not an obvious way to store any metadata alongside path parameters so I did not implement yet. I can imagine a not-so-bad way to do this using the route metadata, but no code written for that just yet.

mattpolzin commented 4 years ago

Have not tested it yet, but https://github.com/mattpolzin/VaporOpenAPI/releases/tag/0.0.9 is aimed at addressing this. that release pulls in some breaking changes from OpenAPIKit and also requires you to use a new routes builder accessed via the Application.openAPI property instead of adding your routes directly to app with Application.get/post/etc..

It'll look something like:

app.openAPI.get(
  "widgets",
  ":type".description("The type of widget"),
  ":id".description("The widget's ID"),
  use: WidgetController.show
).tags("Widgets")
  .summary("Get a single widget")
  .description("Get a widget by its type and ID.")
m-barthelemy commented 4 years ago

Thanks for the update and implementation!

I couldn't seem to find how to use it with routes defined inside a .group(...) so I ended up defining the following extension:

extension RoutesBuilder {
    @discardableResult
    public func openApiGet<Context, Response>(
        _ path: OpenAPIPathComponent...,
        use closure: @escaping (TypedRequest<Context>) throws -> Response
    ) -> Route
        where Context: RouteContext, Response: ResponseEncodable
    {
        return self.on(.GET, path, use: closure)
    }

    @discardableResult
    public func openApiPost<Context, Response>(
        _ path: OpenAPIPathComponent...,
        use closure: @escaping (TypedRequest<Context>) throws -> Response
    ) -> Route
        where Context: RouteContext, Response: ResponseEncodable
    {
        return self.on(.POST, path, use: closure)
    }

    @discardableResult
    public func openApiPatch<Context, Response>(
        _ path: OpenAPIPathComponent...,
        use closure: @escaping (TypedRequest<Context>) throws -> Response
    ) -> Route
        where Context: RouteContext, Response: ResponseEncodable
    {
        return self.on(.PATCH, path, use: closure)
    }

    @discardableResult
    public func openApiPut<Context, Response>(
        _ path: OpenAPIPathComponent...,
        use closure: @escaping (TypedRequest<Context>) throws -> Response
    ) -> Route
        where Context: RouteContext, Response: ResponseEncodable
    {
        return self.on(.PUT, path, use: closure)
    }

    @discardableResult
    public func openApiDelete<Context, Response>(
        _ path: OpenAPIPathComponent...,
        use closure: @escaping (TypedRequest<Context>) throws -> Response
    ) -> Route
        where Context: RouteContext, Response: ResponseEncodable
    {
        return self.on(.DELETE, path, use: closure)
    }

    @discardableResult
    public func on<Context, Response>(
        _ method: HTTPMethod,
        _ path: [OpenAPIPathComponent],
        body: HTTPBodyStreamStrategy = .collect,
        use closure: @escaping (TypedRequest<Context>) throws -> Response
    ) -> Route
        where Context: RouteContext, Response: ResponseEncodable
    {
        let wrappingClosure = { (request: Vapor.Request) -> Response in
            return try closure(.init(underlyingRequest: request))
        }

        let responder = BasicResponder { request in
            if case .collect(let max) = body, request.body.data == nil {
                return request.body.collect(max: max?.value ?? request.application.routes.defaultMaxBodySize.value).flatMapThrowing { _ in
                    return try wrappingClosure(request)
                }.encodeResponse(for: request)
            } else {
                return try wrappingClosure(request)
                    .encodeResponse(for: request)
            }
        }

        let route = Route(
            method: method,
            path: path.map(\.vaporPathComponent),
            responder: responder,
            requestType: Context.RequestBodyType.self,
            responseType: Context.self
        )

        for component in path {
            guard case let .parameter(name, optionalDescription) = component,
                let description = optionalDescription else {
                    continue
            }
            route.userInfo["openapi:parameter:\(name)"] = description
        }

        self.add(route)
        return route
    }
}

If I missed something or if you have a more elegant solution, please let me know :)

If I enter nitpicking mode, something that I don't think is possible is to set the type of the OpenAPIPathComponent (String, Int, UUID...), do you confirm?

mattpolzin commented 4 years ago

Interesting. I simply had not used .group yet so I had not hit that unfortunate problem.

The solution you came up with makes sense -- I originally did something similar in the first place before deciding I thought I liked app.openAPI.get(...) more than app.openAPIGet(...) for the symmetry with the non-typed equivalent methods.

The whole situation only arises via the unfortunate ambiguity between the helpers defined in VaporTypedRoutes and VaporOpenAPI. After you mentioned the issue with .group() and the limitation of not being able to specify a type for the parameter, I decided to move things into VaporTypedRoutes to fix the problem by entirely removing the need to replicate the overloads in VaporOpenAPI.

The TLDR is that VaporOpenAPI version 0.0.11 fixes the problem more cleanly but at the cost of you needing to change your code back to using methods more similar to the ones offered by Vapor in the first place. See the release notes for more and feel free to follow up with me on any questions.

mattpolzin commented 4 years ago

Oh, and the example in the README for this repo has been updated to show off both the description and parameterType functionality of TypedPathComponent. The two can be combined, as well:

":id".parameterType(UUID.self).description("The Id of the widget")
m-barthelemy commented 4 years ago

This is great, thanks! However after upgrading to 0.0.11, while the Yaml is still valid, ReDoc is now throwing the following error:

Screenshot 2020-05-12 at 7 58 43 PM

The only change on my end, besides renaming everything back to app.get/post/put... and deleting my extensions, was to add a few .parameterType(UUID.self). I haven't tested yet if Redoc is happy again without any parameterType

m-barthelemy commented 4 years ago

I pasted the Yaml into https://editor.swagger.io, and it is parsed and rendered fine, although there are warnings looking like this: Structural error at paths./v1/users/{id}.delete.parameters.0.description should be string These description fields look like this in my Yaml:

      - description: 
        in: path
        name: id
        required: true
        schema:
          type: string

On the other hand, the descriptions that I now set as UUIDs thanks to your latest improvements do not trigger any warning:

      - description: The ID of the CertificateAuthority
        in: path
        name: id
        required: true
        schema:
          format: uuid
          type: string

But to be honest I don't know if it has anything to do with the Redoc crash.

mattpolzin commented 4 years ago

Huh. My gut without having opened Xcode yet this morning is that we’ve uncovered an error with the YAML encoder that is encoding an empty string as nothing instead of using empty single quotes, which would be the way to signal that the description field is a string instead of an empty object.

Regardless, the better behavior here would be to omit the description when it is empty instead of emitting an empty description. Should be a straight forward fix that I’m sure I’ll push before work.

mattpolzin commented 4 years ago

This should fix that: https://github.com/mattpolzin/VaporOpenAPI/releases/tag/0.0.12

m-barthelemy commented 4 years ago

This did fix that indeed, thanks a lot for the quick fix!