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: allow for 'operation_class' to be added on an 'Litestar' (app) level, Router level as well as route #2675

Open
WilliamStam opened this issue Nov 14, 2023 · 14 comments
Labels
Enhancement This is a new feature or request OpenAPI This is related to our OpenAPI schema

Comments

@WilliamStam
Copy link
Contributor

WilliamStam commented Nov 14, 2023

Summary

at the moment the openapi route skeleton uses a dataclass that extends Operation. this is only set'able on the actual route.

@get(..., operation_class=CustomOperation)
Router(..., operation_class=CustomOperation)
Litestar(..., operation_class=CustomOperation)

this proposal is to add the operation_class to each "step" in the chain. by default set Litestar(operation_class=Operation). if the operation_class is set on the Router() then override it for all routes under that router. if its set on the route then override it for the route.

this follows on from #1396


i also think renaming the parameter to something more meaningful might be an option here. maybe something like openapi_output_structure.

Basic Example

from dataclasses import dataclass, field
from typing import Dict, List, Optional

from litestar.openapi.spec import Operation


@dataclass
class CustomOperation(Operation):
    x_scopes: Optional[List[str]] = field(default=None, metadata={"alias": "x-scopes"})
    x_requires_auth: Optional[bool] = field(default=False, metadata={"alias": "x-requires-authentication"})
    
    def __post_init__(self) -> None:
        self.x_scopes = []

@get(..., operation_class=CustomOperation)
Router(..., operation_class=CustomOperation)
Litestar(..., operation_class=CustomOperation)

Drawbacks and Impact

im not convinced this parameter is the most useful of the bunch. you can only specify the class name and the system will then instantiate it with default "extra parameters in the outputed json".

some of the ways that you would be able to even set it would involve middleware / guards / dependencies (anywhere where you get the request object and then set the values. this would probably mean a workaround could be to define opts and then have a function that gets the request look at opts and set it from there.

this would greatly impact setting it on the app level and the router level since then you would again have to define opts on each route then anyways. (but you could probably set a dependency on whichever layer you're on to hard code set it in anycase)

proposal instead is to include an openapi_extensions={} parameter and to handle it all "automaticaly?" with the current structures. again allowing it to be set any any level and the next level overrites the previous all the way to route level.

im not the biggest fan of the "thousand parameters" approach tho. wouldn't it be pertinent then to maybe expose the rendering of the json for the open api spec and let the devs break it however they choose. "after aaaallll the steps and you still want to break it.. you sir.. deserve the pain"

Unresolved questions

  1. can the parameter be renamed to something meaningful?
  2. is it the most useful parameter out there considering how you have to use some other way to set it / use opts etc
  3. aren't there better ways to handle adding openapi extensions

Note

While we are open for sponsoring on GitHub Sponsors and
OpenCollective, we also utilize Polar.sh to engage in pledge-based sponsorship.

Check out all issues funded or available for funding on our Polar.sh Litestar dashboard

  • If you would like to see an issue prioritized, make a pledge towards it!
  • We receive the pledge once the issue is completed & verified
  • This, along with engagement in the community, helps us know which features are a priority to our users.
Fund with Polar
@WilliamStam WilliamStam added the Enhancement This is a new feature or request label Nov 14, 2023
@WilliamStam
Copy link
Contributor Author

is there any place to set a value on this? like if you want set 1 of the keys to a value for each route. i can only get it to show the default value on that class.

@WilliamStam
Copy link
Contributor Author

WilliamStam commented Nov 20, 2023

so i got thinking about this. what would a good "interface" be for this. what about something like:

@dataclass
class CustomOperation(Operation):
    x_scopes: Optional[List[str]] = field(default=None, metadata={"alias": "x-scopes"})
@get("/authenticated", operation_class=CustomOperation, x_scopes=["x","y","z"])

obviously this gets more tricky cause then as a dev you REALLY have to keep your opt dict in check (make sure you arent specifying stuff that gets used in unexpected places).

im not totaly sure what the best way for this to work at the moment. you almost want to allow the developer "full" abilities to change the operation "schema"(seems more what it is) for whatever reason they want. so like "merge" keys. o you want to programmatically change the tags? sure.. here go right ahead.

would would the interface be like to do that? just root item fields seems easy enough.

# /litestar/_openapi/path_item.py:123

      operation = route_handler.operation_class(
              operation_id=operation_id,
              tags=tags,
              summary=route_handler.summary or SEPARATORS_CLEANUP_PATTERN.sub("", route_handler.handler_name.title()),
              description=get_description_for_handler(route_handler, use_handler_docstrings),
              deprecated=route_handler.deprecated,
              responses=create_responses(
                  route_handler=route_handler,
                  raises_validation_error=raises_validation_error,
                  schema_creator=response_schema_creator,
              ),
              request_body=request_body,
              parameters=parameters,  # type: ignore[arg-type]
              security=security,
          )
        # add this part to take opt items and add them to the operation_class
         for k,v in route_handler.opt.items():
              if hasattr(operation,k):
                  setattr(operation,k, v)

(reason im adding it outside the initialization part (cause other wise just **handler.opt) is so that any dataclass __post_init__ can still run. if its part of the init then the post init takes over)

this however still lands us in how do we change the other values if we so wish. adding in any code like this just to cover 1 use case like adding openapi vendor extensions seems like a good way to land up in a totally bloated framework.

@WilliamStam
Copy link
Contributor Author

WilliamStam commented Nov 20, 2023

Proposal:

How to use the operation callable:

from litestar.openapi.spec import Operation
from litestar.router import HTTPRouteHandler


@dataclasses.dataclass
class CustomOperation(Operation):
    x_scopes: Optional[List[str]] = dataclasses.field(default=None, metadata={"alias": "x-scopes"})
    x_requires_auth: Optional[bool] = dataclasses.field(default=False, metadata={"alias": "x-requires-authentication"})
    
    def __post_init__(self) -> None:
        self.x_scopes = []


def do_something_to_the_operation(operation: CustomOperation, route_handler: HTTPRouteHandler) -> None:
    # set fields in the operation_class object from both setting it as well as getting it from other sources like opts
    operation.x_scopes = route_handler.opt.get("x_scopes")
    operation.x_requires_auth = True
    # can set it
    operation.tags = ["whatever"]
    
    # python by reference means we can actually change stuff on the route handler itself
    route_handler.include_in_schema = False


class MyController(Controller):
    
    @get(
        "/test",
        x_scopes=["x", "y"], 
        operation=do_something_to_the_operation,
        operation_class=CustomOperation
    )
    async def test_operation_stuff(self, user: User) -> str:
        return "woof"

this would output: (with commenting out the include_in_schema part in the callable)

"/segment1/test": {
            "get": {
                "tags": [
                    "whatever"
                ],
                "summary": "TestOperationStuff",
                "operationId": "Segment1TestTestOperationStuff",
                "responses": {
                    "200": {
                        "description": "Request fulfilled, document follows",
                        "headers": {},
                        "content": {
                            "text/plain": {
                                "schema": {
                                    "type": "string"
                                }
                            }
                        }
                    }
                },
                "deprecated": false,
                "x-scopes": [
                    "x",
                    "y"
                ],
                "x-requires-authentication": true
            }
        },

changes needed to make this work:

/litestar/_openapi/path_item.py:123

operation = route_handler.operation_class(
   # boring stuff
)
# if no operation is specified we dont need to bother. just call it as it is and be done with it
if route_handler.operation is not None:
    route_handler.operation(operation, route_handler)

# this takes care of my discord thing where i want to hide a route if the user doesnt have auth to see it. 
# untested but thinking  if route_handler.resolve_include_in_schema(): would be better here. 
if route_handler.include_in_schema:
    operation_ids.append(operation_id)
    setattr(path_item, http_method.lower(), operation)

/litestar/http_handlers/base.py:

__slots__ = (
        # boring stuff
        #"operation_class",
        "operation",
        #"operation_id",
        #more boring stuff
    )
 def __init__(
        # boring stuff
        # operation_class: type[Operation] = Operation,
        # operation_id: str | OperationIDCreator | None = None,
        operation: Optional[Callable] = None,
        # more boring stuff
        
    ) -> None:
    # blah blah
    self.operation = operation

Impact

  • Backwards compatibility: None
  • Testing: Low (would probs need a few tests for this like if the operation_class = route stuff with extras added)
  • Documentation: I dont think anything exists around the operation_class other than a issue here and a PR so think this whole part might need some explaining.

@WilliamStam
Copy link
Contributor Author

WilliamStam commented Nov 20, 2023

would still be in line with this ticket in that the operation_class and operation callable should be able to be set on app, router, and route levels. and other than route im not sure how to set it in higher levels and being able to "hand it down" to the next level down if that level doesn't have its own.

the lowest level set wins. so an operation on the route would overwrite the operation on the router etc

@provinzkraut
Copy link
Member

provinzkraut commented Nov 21, 2023

Can you, maybe with an example, explain what exact problem this solves? Your proposal seems a bit tangential to the issue originally described - being able to set the operation_class on different application layers.

im not sure how to set it in higher levels and being able to "hand it down" to the next level down if that level doesn't have its own.

We already have a mechanism for this; The layered parameters. It could work in the same way things like middlewares, dependencies, cache, etc. do.

@WilliamStam
Copy link
Contributor Author

WilliamStam commented Nov 21, 2023

the other issue was the ability to set operation_class onto other layers. and in the investigation on "how" to do it i realised even if you have it, its pretty useless at being "generic".

my proposal (and PR) focuses on how are you even able to change anything inside that operation_class based on anything in the route.

the examples of operation_class show examples of using it for x-examples but the value of the example is fixed in that operation class

ie:

from litestar.openapi.spec import Operation
@dataclass
class CustomOperation(Operation):
    x_scopes: Optional[List[str]] = field(default=None, metadata={"alias": "x-scopes"})
    x_requires_auth: Optional[bool] = field(default=False, metadata={"alias": "x-requires-authentication"})
    
    def __post_init__(self) -> None:
        self.x_scopes = []

i havent found any way to set any of the "extra" field's values inside that operation_class other than defining it in the dataclass.

if i set CustomOperation to a route ie:

class MyController(Controller):
    
    @get(
        "/test",
        operation_class=CustomOperation
    )
    async def test_operation_stuff(self, user: User) -> str:
        return "woof"

there isnt any way (that i know of) to set "x_scopes" (or x_requires_auth) for this route other than to have an operation_class for every combination.

going back to the original "solution" where the oepration_class was brought in: #1732 you would be out of luck (again.. AFAIK) if you wanted to have a self.x_codeSamples for each route thats different from the previous route.

another example of being able to "after" the route is setup run a callable (this might be stupid and there are other ways of doing it, im just using it as an eg). rapidoc has this nasty tendency to duplicate every route into every tag. so if you add a tag to the Router and another tag to the route that route is basicaly duplicated twice. there isnt a nested tag structure with it (again probably totaly the right thing, its 100% following spec but it makes it slightly less useful) with the ability to control the route in a callable like this you can for instance do something like request.tags = " | ".join(request.tags) - again a stupid example but it highlights the ability to use the framework however you see fit.

@provinzkraut
Copy link
Member

Maybe I'm missing something here but what is the value of being able to do this:

@dataclass
class CustomOperation(Operation):
    x_my_value: str | None = None
    x_default_value: str | None = field(default=None, metadata={"alias": "x-default-value"})

    def __post_init__(self) -> None:
        self.x_my_value = "some_value"


def custom_operation_callable(operation: CustomOperation, route: HTTPRouteHandler):
    operation.x_my_value = "some_other_value"

over this:

@dataclass
class CustomOperation(Operation):
    x_my_value: str | None = None
    x_default_value: str | None = field(default=None, metadata={"alias": "x-default-value"})

    def __post_init__(self) -> None:
        self.x_my_value = "some_value"


@dataclass
class SomeOtherCustomOperation(CustomOperation):
    def __post_init__(self) -> None:
        self.x_my_value = "some_other_value"

The only thing your solution adds is that it's being passed the route handler:

def custom_operation_callable(operation: CustomOperation, route: HTTPRouteHandler):
    operation.description = route.opt.get("value_from_opts")

which doesn't seem to be all that useful to me, since you still have to have a custom function defined for your route handler, at which point you also know which opts are set on there, which in turn means you don't actually need to fetch them dynamically, since they're known beforehand.


its pretty useless at being "generic"

Maybe you could try to explain why this is an issue

my proposal (and PR) focuses on how are you even able to change anything inside that operation_class based on anything in the route.

And why that is something that you need to do / what concrete problems this solves (other than that you're now able to do that), so I can fully understand your intent here :)

@WilliamStam
Copy link
Contributor Author

WilliamStam commented Nov 21, 2023

if you have 2 or maybe 6 total values for x_my_value then sure. having a base Operation class and 5 sub classes works fine. but say you want to include a specific value for x_my_value for each route then the only way would eb to subclass and have a ton of those.

https://redocly.com/docs/api-reference-docs/specification-extensions/x-code-samples/#code-sample-object

check the example give, its on the "get" method but the example shows curl --request POST if you wanted to include an example for every 1 of your routes then you would have to have that many operation_class objects. with the above you can just setup a generic operation_class say OperationWithSchemaClass(Operation) and then using the operation (cause its nicely formatted at this point and all the parent stuff has been added in) and the internal route object cause that gets stipped away when the operation object gets created. and to then automatically set the x_codeSamples object inside the oepration_class.

basically there is an operation_class but no way to actually do anything with it. it might for all intense and purposes have been 100% static.

@provinzkraut
Copy link
Member

Couldn't the same be achieved by being able to directly supply arguments passed to the operation class when it's instantiated?

basically there is an operation_class but no way to actually do anything with it. it might for all intense and purposes have been 100% static.

Well you are not really supposed to do anything with it, that's why it is the way it is. It's not a user facing API. The operation_class, much like the response_class is just a way to customise, and thereby extend, a datastructure then used by Litestar internally.

The proposed solution seems to be a rather convoluted way to solve a particular edge case (at least it hasn't come up so far), and I think it would be best addressed differently. For this specific case, it might be enough to allow the customisation of examples, or, more generally, it would good to have a solution that allows to customise various aspects of the schema, without having to add a new parameter to every app layer.

Another issue I see with the proposal is that it's impossible to type / type check correctly:

You have typed your example like this

def custom_operation_callable(operation: CustomOperation, route: HTTPRouteHandler):

but if operation is a layered parameter, you can't actually know that it's going to receive a CustomOperation. All you can guarantee is that it receives an Operation. But if you type it as Operation, then it won't have the custom attributes defined. The only solution to this would be to force every operation_callable to be like this:

def custom_operation_callable(operation: Operation, route: HTTPRouteHandler):
    if isinstance(operation, CustomOperation):
        operation.description = route.opt.get("value_from_opts")

which again, makes things a bit more cumbersome than they need to be IMO.


@litestar-org/members @litestar-org/maintainers I'd appreciate some input on this. I feel like there's a useful feature in there somewhere but I don't quite know how it would look.

@WilliamStam
Copy link
Contributor Author

i was trying to help limit how many parameters you need in the code base. the above came it at less than 20 lines of code needing to be added to the code base to gain a whole lot of extra functionality and future customization.

my thinking on this was that "last set wins" in the hierarchy of app router route space. but i honestly didn't give it much thought other than the original post where i couldn't define the oepration_class on a higher level than just the route. so would have had to add operation_class in every route to have the vendor extensions added, and then i realized theres no way to actually even set those as explained in hopefully detail above.

the alternative would be something like instantiate the operation (thereby being able to check thats its a child of Operation) and then merge the 2 together with any changes. another alternative would be to add something like if opts['xxx'] matches the property in the operation class alternatively have a parameter for x_codeExamples="xxx" but then the next person comes along and says "i dont use redoc but i want x-badges" instead. alternatively add a parameter of a dict of all extensions you want. but i feel thats just unnecessary bloat.

since oepration_class is ment as an internal api, wouldn't this fall into the same vein? its there.. for edge cases you can use it.. and then hopefully it can save someone's keyboard in future with not having to type these pages of text.

but if operation is a layered parameter

then dont make it a layered parameter. or alternatively do what it does in other places where it throws an error. or make operation_class a callable class that you can specify "body" on. idk. its like 2000 degrees here and im way past deadlines :(

all this just because i wanted to hide the endpoints in the docs that a user doesnt have permission to use.

@provinzkraut
Copy link
Member

i was trying to help limit how many parameters you need in the code base. the above came it at less than 20 lines of code needing to be added to the code base to gain a whole lot of extra functionality and future customization.

Totally fine, there's generally no issues with that.

my thinking on this was that "last set wins" in the hierarchy of app router route space.

Also just fine, we already have this mechanism; It's called "layered parameters", and it would absolutely make sense that, if this parameter was added, it would be a layered parameter as well. You can see how this works for example by taking a look at the resolve_type_encoders method:

def resolve_type_encoders(self) -> TypeEncodersMap:

but i honestly didn't give it much thought other than the original post where i couldn't define the oepration_class on a higher level than just the route. so would have had to add operation_class in every route to have the vendor extensions added

This is where you kinda lost me, since your PR doesn't really do that. This would be fixed by simply making operation_class a layered parameter, available on the app, router, controller and handler layer. Then you could set it there, and it would propagate down. But I struggle to see how having this parameter available on all layers is connected to the proposal of the addition of the operation_class_callable.

Just to clarify that I understood what you wrote there correctly: Your initial problem was not being able to set the operation_class on a higher level?

all this just because i wanted to hide the endpoints in the docs that a user doesnt have permission to use.

I think this is a very different topic, and actually a feature request we have gotten several times now. I haven't invested a lot of time into this, but to me it feels like there should be a more elegant way to do this than to repurpose these parameters. Maybe a callback on the OpenAPI config object or something.

@guacs
Copy link
Member

guacs commented Nov 24, 2023

If I'm understanding it correctly, the reason for operation_class_callable was to be able to get the RouteHandler and then change values on the operation class based on the values set in the router.

Couldn't the same be achieved by being able to directly supply arguments passed to the operation class when it's instantiated?

If my understanding is correct, then I think this suggestion by @provinzkraut should work.

@peterschutt
Copy link
Contributor

I count 3 separate issues that have been brought up in this discussion:

  1. cannot set operation_class on layers.
  2. cannot dynamically modify the operation instance after we have created it (true, and while not the OP I feel this has evolved into the main area of discussion).
  3. hiding routes from schema based on auth.

For 1., we can easily solve this - its just a matter of doing the work.

For 2., I don't like the idea of adding another parameter to the handlers for this. I would support configuring some sort of callback on the openapi config, or maybe extending the openapi plugin protocol where that callable receives the route handler, the method and the operation instance as a way for users to make last-resort modifications to the schema.

For 3., I would like this sort of capability, however b/c we generate the schema on app startup time and cache it, I think it would be simplest for us to support generation of multiple schemas, and then have some way to discriminate which schema is served to the client at time of the request.

@WilliamStam
Copy link
Contributor Author

for 2 i would "almost" disagree with you there. to make the whole thing more "useful" (in the scope of this discussion) would mean you need to be able to go down to a per route level. by adding it to the main app as a callback type thing you wont be able to add "additional data" to the route other than through opts["..."] which cant really be type hinted.

@get("/page",opts['scopes'] = ['a','b'])
@get("/page",scopes=["a","b"]) 

i like the idea of the openapi config getting the route handler.

wouldnt a solution be where you can attach custom Router, Route, OpenAPI on the app?

ie:

class MyRouter(Router):
  async def __call__(self,blah,blah):
    ...

class MyRoute(RouteHandler):
  async def __call__(self,blah,blah):
    ...

class MyOpenAPISchema(OpenAPISchema):
  async def __call__(self,blah,blah):
    ...

app.router_handler = MyRouter
app.route_handler = MyRoute
app.openapi_schema_handler = MyOpenAPISchema

this doesn't have to be an advertised feature but it could potentially make you life sooo much easier in that the dev can do whatever they want. it does allow for some pretty interesting ways of breaking the entire thing tho.

i'm more the type of "this is a tool for the dev" vs "this is a product" i think. so probs not the greatest opinion around -_-


for 3:

i solved this by doing a few things.

  1. adding an guards=[Authorize(...)] guard. the "permissions" get saved on the init to the auth class.
  2. create a normal route to openapi.json
@get("/openapi.json", include_in_schema=False)
    async def openapi(self, request: Request) -> ASGIResponse:
        request.logger.info(f"User: {request.user}")
        
        schemas = []
        for key, value in request.app.openapi_config.components.security_schemes.items():
            schemas.append({key: []})
        secure_route_security = schemas
        
        for route in request.app.routes:
            for handle in route.route_handlers:
                handle.operation_class = CustomOperation
        
                all_permission = []
                authed_route = False
                if handle.guards is not None:
                    for guard in handle.guards:
                        # if the guard is a subclass of Authorize then we use its scopes
                        # might be a good idea to lookup "all" authorize scopes and combine them incase somone goes wierd and guard=[Authorize("a"),Authorize("b")
                        if isinstance(guard, Authorize):
                            authed_route = True
                            for permission in guard.permissions:
                                all_permission.append(str(permission))
                    if all_permission:
                        request.logger.info(f"Scopes required for route {route.path} - {all_permission}")
                        
                    if authed_route:
                        handle.security = secure_route_security
                        if not request.user.has_permissions(all_permission):
                            pass
                            # handle.include_in_schema = False
                    # handle.opt["x-scopes"] = all_permission
                    # handle.opt["x-requires-authentication"] = True
                    # print(handle.opt.get("x_scopes"))
                    pass
                
                # handle.operation_id.description = "woof"
                    # handle.operation_class.x_scopes = all_permission
                    # handle.operation_class.x_requires_auth = authed_route
            pass
        schema = request.app.openapi_schema.to_schema()
        json_encoded_schema = encode_json(schema, request.route_handler.default_serializer)
        return ASGIResponse(
            body=json_encoded_schema,
            media_type=OpenAPIMediaType.OPENAPI_JSON,
        )

this was a prototype. setting "include_in_schema" is a trap. dont do it. simply create a new list of routes instead. having a just before call funtion could be used quite nicely with something like this i think.

@JacobCoffee JacobCoffee added the OpenAPI This is related to our OpenAPI schema label Dec 7, 2023
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 OpenAPI This is related to our OpenAPI schema
Projects
Status: Ideas
Development

No branches or pull requests

5 participants