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

DOCS: Improve docs around alternative Cadwyn use #149

Open
isaacharrisholt opened this issue Feb 26, 2024 · 10 comments
Open

DOCS: Improve docs around alternative Cadwyn use #149

isaacharrisholt opened this issue Feb 26, 2024 · 10 comments
Labels
enhancement New feature or request

Comments

@isaacharrisholt
Copy link

Describe the solution you'd like

The docs mention that you can "use Cadwyn directly through cadwyn.generate_versioned_routers". This is something I'd like to do to emulate Stripe's model and make the version header optional. However, I'm not entirely sure how to do that, and there's no good documentation on the topic.

I tried adding some extra middleware to set the version header manually if missing, which worked for most requests, but resulted in a 404 on the /docs and /openapi.json routes.

Basically, it would be good to have an example or two of using other methods :)

@isaacharrisholt isaacharrisholt added the enhancement New feature or request label Feb 26, 2024
@zmievsa
Copy link
Owner

zmievsa commented Feb 26, 2024

Makes a lot of sense! Let's use this task for tracking progress on the "alternative uses" issue. As for the 404 on /docs and /openapi.json -- this problem is sadly in verselect, not cadwyn. But I'm also a maintainer of that package so I'll be happy to help here too.

Though right now I am not sure how we would approach it. I am not sure if it makes sense to add header-based routing to /docs endpoint. However, it seems to make a lot of sense to add header-based routing to /openapi.json endpoint; or maybe even duplicate this endpoint in multiple versions.

I propose to brainstorm the interface that we would like to see and then, once decided, I can guide you with fixing it yourself or put it into my personal backlog and see whether I'll be able to get to it any time soon.

@isaacharrisholt
Copy link
Author

I think the query params for those endpoints are fine - it just seems like they break if you make a request with the header. This is 100% something I can work around in the middleware, but it feels like a strange behaviour to me.

Happy to help, and I'm going to take a look at some of the other open issues too!

@zmievsa
Copy link
Owner

zmievsa commented Feb 29, 2024

Almost finished adding versioned openapi.json routes to cadwyn -- finishing unit tests right now. Will be done with it some time this week.

@zmievsa
Copy link
Owner

zmievsa commented Mar 1, 2024

Added versioned /openapi.json in 3.9.0. Note that /docs and /redoc were not added as I do not see the use case for them being versioned outside of browser yet.

@zmievsa
Copy link
Owner

zmievsa commented Mar 3, 2024

Writing the proper docs for custom Cadwyn use cases is going to be resource intensive so let me just summarize it here.

Cadwyn needs four fundamental parts to function:

  1. VersionBundle and the things we send there
  2. generate_code_for_versioned_schemas for code generation
  3. generate_versioned_routers for versioned router generation (i.e. creating alternative versions of latest routes with versioned schemas used instead of latest and this is the step where converters are added to routes)
  4. VersionBundle.api_version_var that is the ContextVar we set for each received request. We use it for request/response migrations and for side effects checking.

By default, Cadwyn provides its own header-based routing and a middleware for that routing. The main job of this middleware is to alter VersionBundle.api_version_var whenever a new request comes.

Essentially if you are able to set api_version_var using your own approach, then the other fundamental parts of Cadwyn are fairly straightforward to integrate into any API versioning solution.

@isaacharrisholt Does this answer your question?

@isaacharrisholt
Copy link
Author

@zmievsa it does. I did try and do that, but I was unsuccessful. I'll have another play and see what I can do, and then maybe write some docs up for it

@zmievsa
Copy link
Owner

zmievsa commented Mar 3, 2024

Okay! Feel free to ask for help. I'm also available for a call in Discord (see the discord status badge in README).

@gabloe
Copy link

gabloe commented May 29, 2024

@isaacharrisholt does this middleware solve your problem? Essentially this is making the API version header optional, and it would default to the most recent API version if no API version were present in the request headers.

from starlette.middleware.base import BaseHTTPMiddleware
from fastapi import Request
from cadwyn.structure import VersionBundle
from starlette.types import ASGIApp

def should_set_default_version(request: Request) -> bool:
    """
    Check if the request matches any of the routes except the swagger, openapi or redoc routes
    """
    for route in request.app.routes:
        if request.url.path.startswith(route.path) and any(
            route.name.startswith(name) for name in ["swagger", "openapi", "redoc"]
        ):
            return False
    return True


class DefaultVersionMiddleware(BaseHTTPMiddleware):
    def __init__(self, app: ASGIApp, api_version_header_name: str, version_bundle: VersionBundle):
        super().__init__(app)
        self.version_bundle = version_bundle
        self.api_version_header_name = api_version_header_name

    async def dispatch(self, request: Request, call_next):
        # Check if the API version is set in the request headers
        api_version = request.headers.get(self.api_version_header_name, None)
        if not api_version and should_set_default_version(request):
            # Set the API version to the latest version if not set
            latest_version = max([v.value for v in self.version_bundle.versions])
            self.version_bundle.api_version_var.set(latest_version)
        return await call_next(request)

@isaacharrisholt
Copy link
Author

Hey @gabloe, thanks for that. I managed to set up something similar, but a little more custom. I essentially have the concept of a team in my application and each team has a stored API version. I use this:

class DefaultVersioningMiddleware(BaseHTTPMiddleware):
    def __init__(
        self,
        app: ASGIApp,
        *,
        api_version_var: ContextVar[date] | ContextVar[date | None],
        latest_version: date,
    ):
        super().__init__(app)
        self.api_version_var = api_version_var
        self.latest_version = latest_version

    async def dispatch(self, request: Request, call_next: RequestResponseEndpoint):
        if request.url.path.startswith('/docs') or request.url.path.startswith(
            '/openapi.json'
        ):
            return await call_next(request)

        # Set default version to latest
        self.api_version_var.set(self.latest_version)

        header_version = request.headers.get(settings.version_header, None)

        if header_version is not None:
            return await call_next(request)

        auth_header = request.headers.get('Authorization', None)
        if auth_header is None:
            return await call_next(request)

        if not auth_header.startswith('Bearer '):
            return await call_next(request)

        api_key = auth_header.split(' ')[1]
        supabase_admin = await get_supabase_admin()
        api_key_and_team = await get_team_from_api_key(supabase_admin, api_key)
        if api_key_and_team is None:
            return await call_next(request)

        _, team = api_key_and_team

        version = team.api_version

        self.api_version_var.set(version)

        response = await call_next(request)
        response.headers.append(settings.version_header, version.isoformat())
        return response

@zmievsa
Copy link
Owner

zmievsa commented May 29, 2024

Feel free to contribute "default_api_version" flag to Cadwyn or even something like "default_api_version_resolver" which would be able to handle @isaacharrisholt 's use case as well. It seems like not including it to begin with was a major mistake from my side :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants