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

Enhancement: Add root_path value to OpenAPI servers if present #2077

Open
floxay opened this issue Jul 27, 2023 · 10 comments
Open

Enhancement: Add root_path value to OpenAPI servers if present #2077

floxay opened this issue Jul 27, 2023 · 10 comments
Labels
Enhancement This is a new feature or request Good First Issue This is good for newcomers to take on Help Wanted 🆘 This is good for people to work on OpenAPI This is related to our OpenAPI schema

Comments

@floxay
Copy link
Contributor

floxay commented Jul 27, 2023

Summary

Currently the OpenAPI config servers URLs default to "/" even if a root_path is present, even though for example debug errors display the root_path. (ValueError on GET /api/ if a ValueError is raised in test_handler in the code below.)

It would be nice to have Litestar automatically add the root_path value --if present-- as the default instead.

Basic Example

Code:

from litestar import Litestar, get, Request


@get(path="/", sync_to_thread=False)
def test_handler(request: Request) -> dict[str, str | list]:
    return {
        "request.scope['root_path']": request.scope["root_path"],
        "app.openapi_config.servers": request.app.openapi_config.servers,
    }


app = Litestar(route_handlers=[test_handler], debug=True)

if __name__ == "__main__":
    import uvicorn

    uvicorn.run(app, port=5000, root_path="/api")

Current response (Litestar 2.0.0b4):

{
	"request.scope['root_path']": "/api",
	"app.openapi_config.servers": [
		{
			"url": "/",
			"description": null,
			"variables": null
		}
	]
}

Proposed:

{
	"request.scope['root_path']": "/api",
	"app.openapi_config.servers": [
		{
			"url": "/api",
			"description": null,
			"variables": null
		}
	]
}

Drawbacks and Impact

No response

Unresolved questions

No response


Funding

  • If you would like to see an issue prioritized, make a pledge towards it!
  • We receive the pledge once the issue is completed & verified
Fund with Polar
@floxay floxay added the Enhancement This is a new feature or request label Jul 27, 2023
@Goldziher
Copy link
Contributor

this is not something we can support. root path is a dynamic input sent during runtime by the asgi server.

You can configure this parameter manually by setting Servers on the openapi config you pass to litestar.

Also consider this value might be propagated via ENV variables etc.

@floxay
Copy link
Contributor Author

floxay commented Jul 27, 2023

root path is a dynamic input sent during runtime by the asgi server.

That makes sense.

You can configure this parameter manually by setting Servers on the openapi config you pass to litestar.

Would a root_path keyword argument for the Litestar constructor be possible as an alternative?
It just seems weird to me that the default OpenAPI config defaults to "/" but errors display the root_path value.

@Goldziher
Copy link
Contributor

root path is a dynamic input sent during runtime by the asgi server.

That makes sense.

You can configure this parameter manually by setting Servers on the openapi config you pass to litestar.

Would a root_path keyword argument for the Litestar constructor be possible as an alternative?
It just seems weird to me that the default OpenAPI config defaults to "/" but errors display the root_path value.

I think that's doable. I'm just afraid it will confuse people. I would rather that is added as an attribute to the OpenAPI config.

Thoughts?

@provinzkraut
Copy link
Member

I think that's doable. I'm just afraid it will confuse people. I would rather that is added as an attribute to the OpenAPI config.

I'm with you here

@Goldziher
Copy link
Contributor

yes its a similar thing. I dont particularly want to do it top level - we already have too many parameters there.

What do you guys think?

@JacobCoffee
Copy link
Member

OpenAPI config makes sense if that is all it affects.

@JacobCoffee JacobCoffee self-assigned this Jul 31, 2023
@Goldziher Goldziher added Help Wanted 🆘 This is good for people to work on Good First Issue This is good for newcomers to take on labels Sep 9, 2023
@JacobCoffee JacobCoffee added the OpenAPI This is related to our OpenAPI schema label Dec 7, 2023
@github-project-automation github-project-automation bot moved this to Triage in Overview Dec 8, 2023
@JacobCoffee JacobCoffee moved this from Triage to Ideas in Overview Dec 8, 2023
@peterschutt
Copy link
Contributor

peterschutt commented May 19, 2024

We took a look at this issue today, and its fairly trivial to set a Server(url=scope["root_path"]) when the openapi schema is requested via a call to a schema endpoint, if it hasn't already explicitly been set by the user. That part is easy enough.

However, the openapi schema can be accessed outside of a request context, and at that point, we have no idea what the server has been told is the "root_path", and so there is no way for us to generate a consistent schema from within a request context, and from outside of a request context (e.g., when generating the schema from the CLI).

This could probably then be solved by adding a --root-path option to our CLI, so that we have that available when generating the schema for output via cli, and this would also allow us to pass that through to the underlying server (uvicorn, whatever) so that "root_path" is delivered to us in the scope dict, which would allow us to set it in the generated schema in either the CLI or request access scenario.

E.g., accessing schema via cli:

$ litestar --app test_apps.test_app:app --root-path=/api/v1 schema openapi

Or, passing to litestar run:

$ litestar --app test_apps.test_app:app --root-path=/api/v1 run

This leaves one other case, where the user accesses the openapi schema directly on a app instance:

Litestar(..., openapi_config=...).openapi_schema

In this case, we have no way of knowing the root_path, and so are unable to automatically populate that servers=[...] field on the OpenAPI spec object. IMO, it should be up to the user to explicitly declare their Server objects on the OpenAPIConfig object if they intend to access the schema in this fashion.

So, the proposal is:

Change the default value on OpenapiConfig.servers from [Server(url="/")] to [] - this lets us determine if the value has been explicitly set by the user.

If servers has not been explicitly set, we will set it either via the value received from $ litestar --root-path=... in the case of CLI schema generation, or via the root_path key in the scope dict if schema is accessed within a request context, and default to [Server(url="/")] if no value is provided.

In any case, if the servers=[...] field on the OpenAPIConfig object has been set with a value - we just leave it alone, as configured.

@jaykv thanks for sitting in on the bug squash session today - this one turned out to have a little more nuance to it, but curious to hear if you have any feedback on the above. Also, @litestar-org/maintainers, thoughts?

@jaykv
Copy link
Contributor

jaykv commented May 20, 2024

Thanks @peterschutt, it was nice meeting you and @JacobCoffee!

That sounds intuitive from my perspective- not needing to pass a root_path param to OpenAPIConfig is great for the dynamic behavior.

@kigawas
Copy link

kigawas commented Oct 4, 2024

root_path is especially useful when mounting existing asgi applications. If I'd like to mount a django asgi application, in fastapi it's very ergonomic, just app.mount. But in litestar I have to do:

class DjangoASGIMiddleware:
    def __init__(self, app, *, path: str = "") -> None:
        self._app = app
        self._path = path

    async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
        scope["root_path"] = self._path
        scope["path"] = self._path + scope["path"]
        await self._app(scope, receive, send)

liteapp = Litestar()
django_app = asgi(path="/django", is_mount=True)(
    DjangoASGIMiddleware(application, path="/django")
)
static_files_router = create_static_files_router(
    path="/static", directories=["staticfiles"]
)
app.register(django_app)
app.register(static_files_router)

Otherwise django admin page will not work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement This is a new feature or request Good First Issue This is good for newcomers to take on Help Wanted 🆘 This is good for people to work on OpenAPI This is related to our OpenAPI schema
Projects
Status: Ideas
Development

No branches or pull requests

7 participants