WilliamStam
commented
1 year ago 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.
Open WilliamStam opened 1 year ago
WilliamStam
commented
1 year ago 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
commented
1 year ago 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
commented
1 year ago 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 = operationwould 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
commented
1 year ago 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
commented
1 year ago 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: https://github.com/litestar-org/litestar/pull/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
commented
1 year ago 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
commented
1 year ago 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.
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
commented
1 year ago 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
commented
1 year ago 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
commented
1 year ago 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:
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
commented
1 year ago 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
commented
12 months ago I count 3 separate issues that have been brought up in this discussion:
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
commented
12 months ago 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 = MyOpenAPISchemathis 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.
guards=[Authorize(...)] guard. the "permissions" get saved on the init to the auth class. 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.
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_classto each "step" in the chain. by default setLitestar(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 https://github.com/litestar-org/litestar/issues/1396
i also think renaming the parameter to something more meaningful might be an option here. maybe something like openapi_output_structure.
Basic Example
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