feat(BA-759): Revamp api handler #3714
Conversation
github-actions
bot
added
comp:common
| @@ -156,6 +184,7 @@ def status_code(self) -> int: | |||
|
|
|||
|
|
|||
| _ParamType: TypeAlias = BodyParam | QueryParam | PathParam | HeaderParam | MiddlewareParam | |||
| _ValidatorType: TypeAlias = BodyParam | QueryParam | PathParam | HeaderParam | type[MiddlewareParam] | |||
There was a problem hiding this comment.
Just a question, what is the difference between two?
There was a problem hiding this comment.
You can initialize all ...Param types before evaluating request data except MiddlewareParam, which requires input data for initializing. _ValidatorType represents the types that can evaluate and validate input data.
| signature_validator_map: dict[str, _ValidatorType] = {} | ||
| for name, param in signature.parameters.items(): | ||
| if param.annotation is inspect.Parameter.empty: | ||
| raise ValueError(f"Empty type hint is not allowed (handler:{handler_name},name:{name})") |
There was a problem hiding this comment.
Defining and using a custom error rather than simply using ValueError would be better for future error convention modification
There was a problem hiding this comment.
This error occurs when bootstrapping the server, not on runtime if any API handler function is defined incorrectly. You can test it
There was a problem hiding this comment.
I added an exception for this case
| match param_instance_or_class: | ||
| case PathParam() | BodyParam() | HeaderParam() | QueryParam(): |
There was a problem hiding this comment.
I would like to ask why you changed from comparing classes to comparing instances?
There was a problem hiding this comment.
It is recommended to use pattern matching rather than using multiple isinstance() if it is possible
https://peps.python.org/pep-0636/#adding-a-ui-matching-objects
| case _: | ||
| try: | ||
| param_instance = param_instance_or_class.from_request(request) | ||
| except ValidationError: | ||
| raise MiddlewareParamParsingFailed( | ||
| f"Failed while parsing {param_instance_or_class}" | ||
| ) |
There was a problem hiding this comment.
I think there might be some confusion because MiddlewareParamParsingFailed is thrown in all cases of non_ValidatorType - are we trusting the type checker to assume a non_ValidatorType class or instance is coming in?
There was a problem hiding this comment.
As type checkers check, only ValidatorType input is allowed here. Thats why we use type checkers.
resolves #3713 (BA-759)
Checklist: (if applicable)