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

A way to group features or services to allow middleware to be applied to groups #551

Open
dannflor opened this issue Mar 25, 2024 · 7 comments
Labels
kind/feature New feature. status/triage Collecting information required to triage the issue.
Milestone

Comments

@dannflor
Copy link

dannflor commented Mar 25, 2024

Motivation

Using the OpenAPI generator loses a lot of expressivity on the server side. Once you're in generated-code-land, you lose access to the functionality you would be able to have with vanilla routes e.g. applying middleware to groups.

/// Before
final class AdminMiddleware: AsyncMiddleware
{
    func respond(to request: Request, chainingTo next: AsyncResponder) async throws -> Response {
        if request.auth.has(User.self) {
            if try await (UserService(request.db).user(from: request.auth).isAdmin {
                return try await next.respond(to: request)
            }
        }
        throw Abort(.forbidden, reason: "Admins only, fools")
    }
}
let adminProtected = app.grouped(AdminMiddleware())
adminProtected.group("api", configure: apiController)
func apiController(api: RoutesBuilder) {
    api.get("users") { req async throws -> [User] in
        try await UserService(req.db).allUsers
    }
    api.delete("user", ":name") { req async throws -> User in
        if let username = req.parameters.get("name") {
            try await UserService(req.db).deleteUser(username)
        } else { throw Abort(.badRequest) }
    }
}

/// After
struct APIProtocolImpl: APIProtocol {
    // I am using a middleware with swift-dependency to have access to the request
    @Dependency(\.request) var req
    
    func getUsers(_ input: Operations.getUsers.Input) async throws -> Operations.getUsers.Output {
        // I would need to copy this block for every single route that would be behind AdminMiddleware
        guard try await req.auth.has(User.self), (UserService(req.db).user(from: req.auth).isAdmin else {
            return  .forbidden(.init("Admins only, fools"))
        }
        return try await .ok(.init(body: .json(
            .init(UserService(req.db).allUsers)
        )))
    }
}

Repeating the middleware logic for every single route that should be behind it can quickly grow completely untenable if you have middleware that is supposed to be in front of dozens or hundreds of endpoints.
However you can apply middleware to the routesBuilder you pass to the transport:

let transport = VaporTransport(
    routesBuilder: app.grouped(OpenAPIRequestInjectionMiddleware()).grouped(AdminMiddleware())
)

But we're artificially limited to one document, which means I can only apply a middleware if I'm willing to put it in front of all routes. I've heard that this project intends to implement authorization as part of the API spec, which would alleviate the problem in this specific example, but this isn't the only situation I would want to arbitrarily group routes for a middleware.

Proposed solution

Leverage the openapi-generator-config.yaml configuration file to generate code from multiple documents rather than just one. Let us supply a list of document names that will each generate a protocol with associated types. I envision it looking something like this:

generate:
    - types
    - server
documents:
    - api/admin.yaml
    - api/public.yaml

This config would look for an api/admin.yaml and an api/public.yaml document in the target. It would then generate two protocols: AdminAPIProtocol and PublicAPIProtocol. We could then do something like:

let requestAttached = app.grouped(OpenAPIRequestInjectionMiddleware())

let adminTransport = VaporTransport(routesBuilder: requestAttached.grouped(AdminMiddleware()))
let publicTransport = VaporTransport(routesBuilder: requestAttached)

let adminHandler = AdminAPIProtocolImpl()
let publicHandler = PublicAPIProtocolImpl()

try adminHandler.registerHandlers(on: adminTransport, serverURL: Servers.server1())
try publicHandler.registerHandlers(on: publicTransport, serverURL: Servers.server1())

try await app.execute()

If the documents list isn't specified in the config, we could default to the normal behavior of looking for an openapi.yaml document and generating a single protocol from that.

Alternatives considered

Ideally, we'd be able to integrate with some unified serverside middleware framework to let us specify middleware in generated-code-land. SSWG is supposed to be working on one, but the repo isn't even public, much less finished. In the meantime, the requirement to run middleware operations on every single route is extremely onerous and this seems like a major blocker to migrating large applications to use it.

Additional information

No response

@dannflor dannflor added kind/feature New feature. status/triage Collecting information required to triage the issue. labels Mar 25, 2024
@dannflor
Copy link
Author

dannflor commented Mar 25, 2024

I am aware that middleware could respond in a way that is undocumented, however that was already the case with whatever middleware is placed in front of the single route builder (for instance the request injection middleware could respond with an undocumented response if it pleased).

@czechboy0
Copy link
Contributor

If I understood your ask correctly, you can already achieve this by putting each document into a separate Swift module, and then having your executable attach the generated routes from both OpenAPI docs to the same server.

That would allow you to do exactly this, with two extra imports:

import Admin
import Public

let requestAttached = app.grouped(OpenAPIRequestInjectionMiddleware())

let adminTransport = VaporTransport(routesBuilder: requestAttached.grouped(AdminMiddleware()))
let publicTransport = VaporTransport(routesBuilder: requestAttached)

let adminHandler = AdminAPIProtocolImpl()
let publicHandler = PublicAPIProtocolImpl()

try adminHandler.registerHandlers(on: adminTransport, serverURL: Servers.server1())
try publicHandler.registerHandlers(on: publicTransport, serverURL: Servers.server1())

try await app.execute()

@dannflor
Copy link
Author

Thanks for the workaround. Are there any plans to more cleanly support middleware, possibly by allowing the middleware to be documented and have their responses enforced by some sort of protocol as well?

@czechboy0
Copy link
Contributor

Middlewares are a concept of the runtime library, which is shared by all adopters with their OpenAPI docs.

Maybe there could be a concept of a project-specific middleware, but we haven't done any thinking around this area.

Ideas and proposals are always welcome 🙂

@czechboy0
Copy link
Contributor

@dannflor have you had a chance to think about this more since we last talked? If so, could you share a snippet of code that illustrates what you'd like to see?

Or, if you solve the issue another way, that's okay too and we can close this.

@czechboy0 czechboy0 added this to the Post-1.0 milestone Oct 29, 2024
@dannflor
Copy link
Author

This remains a concern for me. The fact that I need to restructure my project and create a whole new module + openapi document each time I want to add any scoped middleware doesn’t seem scalable. What I’ve been doing to work around this is exactly that: creating new modules, as well as writing middleware that takes in an set of paths to match against so I can apply it universally but have it only affect certain paths. It works for now, but incurs a cost of repetitive hashing and matching for every request, erasing any efficiency gained by using Vapor’s Trie-based router. This project is planned to grow to hundreds or thousands of routes, and serve many concurrent requests, so I’m very hopeful for a solution that will scale better.

But as for what it will look like, I’m stumped. I think it should use the still forthcoming unified SSWG middleware interface. I think whatever we do to fix it could also look into addressing the fact we need to hack in the request injection using a task local middleware. I no longer think allowing multiple specs is a good enough solution. The answer could possibly lie within either that config.yaml or offering an api via the VaporTransport that looks like vapor’s builtin router and letting us register middleware the way we usually do it.

But if you want to avoid maintaining a bunch of custom code for all the different libraries’ versions of middleware, you should probably wait for the unified implementation to come out.

@czechboy0
Copy link
Contributor

You're right that there's a lot of potential tie-in with the other efforts in the ecosystem, but I think we can still incrementally improve the experience for Swift OpenAPI Generator adopters. Let me get back to some of your points below.

Are there any plans to more cleanly support middleware, possibly by allowing the middleware to be documented and have their responses enforced by some sort of protocol as well?

No current plans, because it's not clear what that would look like. Currently, middlewares operate on raw HTTP requests and responses, that's the "runtime" layer, which is shared between all adopters of Swift OpenAPI Generator. What would you like this to look like? If you provide a snippet of OpenAPI, how would you like to express those constraints? (Note that OpenAPI doesn't have any concept of a middleware, it just documents HTTP operations in a transport-agnostic way.)

The fact that I need to restructure my project and create a whole new module + openapi document each time I want to add any scoped middleware doesn’t seem scalable. What I’ve been doing to work around this is exactly that: creating new modules, as well as writing middleware that takes in an set of paths to match against so I can apply it universally but have it only affect certain paths.

I think the latter solution is what I'd recommend - that's one of the reasons that operationId is one of the parameters of middlewares, to allow easy dynamic behavior switching. I think that's better than parsing the path and as you point out, will be much faster.

This project is planned to grow to hundreds or thousands of routes, and serve many concurrent requests, so I’m very hopeful for a solution that will scale better.

If I understand correctly, the concern here is about the number of categories of routes, rather than the number of routes themselves, right? If you only have two (admin and public), hopefully the number in each category won't impact the overall workflow.

But as for what it will look like, I’m stumped. I think it should use the still forthcoming unified SSWG middleware interface.

That's orthogonal here, whatever we end up doing with the SSWG middleware, we can already do with the OpenAPI middleware type - we just collectively need to agree first on what is it we want to do to help with your use case.

I think whatever we do to fix it could also look into addressing the fact we need to hack in the request injection using a task local middleware.

That's a fair concern - do you have ideas of how to make that better, while preserving the property of APIProtocol that it doesn't contain any network types nor has dependencies on the concrete framework, and for the protocol to be the same between the generated client and server?

I no longer think allowing multiple specs is a good enough solution.

Can you elaborate on why allowing multiple specs would be a problem? Isn't it a valid use case for a single server to implement APIs from multiple OpenAPI documents?

The answer could possibly lie within either that config.yaml or offering an api via the VaporTransport that looks like vapor’s builtin router and letting us register middleware the way we usually do it.

The tricky part here is that with Vapor, you manually register your handlers, and as part of that, you can inject middlewares at any level.

When the handlers are generated from an OpenAPI document, you don't register them by hand - so there isn't a similarly natural place to register your middlewares at an arbitrary level. If you have an API in mind that you'd like to see here, please share.

But if you want to avoid maintaining a bunch of custom code for all the different libraries’ versions of middleware, you should probably wait for the unified implementation to come out.

Just like we did when adopting swift-http-types, we're fully committed to adopting any SSWG-supported libraries that overlap with what is today implemented in swift-openapi-runtime. We just need to wait for any such package to exist and reach 1.0, since we already are at 1.0, so need to maintain API stability.

Thanks again for your insights, we want to improve things here for you, we just need your help arriving at what exactly you'd like to see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature. status/triage Collecting information required to triage the issue.
Projects
None yet
Development

No branches or pull requests

2 participants