Generify typing to support OrderStatus subclass serialization #128
Conversation
philvarner
changed the title
philvarner
changed the title
|
@philvarner I'm sorry I have not had a chance to get to this review, I've been dealing with several rapidly-approaching deadlines. I just wanted to post that I have this on my radar and am trying to get to it, but practically I'm not sure I can until next week sometime. |
jkeifer
left a comment
jkeifer
left a comment
There was a problem hiding this comment.
This generally looks good to me, though I have a rather large concern about switching the callable types to protocols (conceptually I agree the protocols are certainly better, but in practice they have limitations that I think are problematic).
I'm marking this with "request changes", but nothing I flagged is necessarily a required change if you can convince me the concerns I pointed out are unfounded. I'd be happy to chat about this on a call or something if that would be easier than async conversation.
| from stapi_pydantic import OpportunitySearchRecord, OpportunitySearchStatus, Order, OrderStatusBound | ||
|
|
||
|
|
||
| class GetOrders(Protocol, Generic[OrderStatusBound]): |
There was a problem hiding this comment.
Why protocols here as opposed to individual callables? One key reason for the callables previously was to allow for using decorators in end-user implementations. This is especially relevant with the use of returns, because its decorators can greatly simplify result handling. Decorators are notoriously hard to get working with class methods, and unless something has changed in the returns implementation since I last looked, its decorators do not support the descriptor pattern necessary to accommodate class methods.
As a result, I'd strongly suggest reverting back to callables for these signatures.
There was a problem hiding this comment.
Just to refresh the historical context, we moved away from protocols in this PR: stapi-spec/stapi-fastapi#132. At that time the major motivation was to break up what endpoints were supported by the routers in response to what "behaviors" were passed in via these callables, but the secondary effect of using callables was to allow for more flexible implementations as I noted above.
| return self.url_for(request, f"{self.name}:{LIST_ORDER_STATUSES}", order_id=order_id) | ||
|
|
||
| def order_links(self, order: Order[OrderStatus], request: Request) -> list[Link]: | ||
| def order_links(self, order: Order[Any], request: Request) -> list[Link]: |
There was a problem hiding this comment.
Maybe just need an education here, but what is the difference between Order[Any] and just Order? I get nervous whenever I see Any because it can disable type checking unexpectedly. I'm not sure if it would have that effect here being used as a generic type specifier, but it does make me wonder what effect it has.
| self.root_provider = root_provider | ||
| self.conformances_support: ConformancesSupport = root_provider |
There was a problem hiding this comment.
What's the thinking behind having two properties for the same object? If a RootProvider necessarily is a ConformancesSupport, then why not just use the self.root_provider?
There was a problem hiding this comment.
Just a note: I do like this idea to use the RootProvider protocol here to decouple the ProductRouter from the RootRouter implementation. 👍
What I'm changing
A existing problem with the typing for various methods is that an implementer could not create their own implementation of OrderStatus and have additional properties in it be serialized in server responses. This is due to fastapi's use of type annotations for determining what serializer to use when returning an object from an endpoint. As defined, any object returned as the status (either with an Order or from the Order statuses endpoint) will be serialized as an Order Status, so any additional provider-specific attributes are not included. However, by adding the correct generic typing, a provider can redefine these endpoints so their own custom OrderStatus class will be serialized correctly.
How I did it
Checklist
./scripts/run-tests.shuv run pre-commit run --all-files