Linting fixes #1004
Linting fixes #1004
Conversation
| *, | ||
| request: Request, | ||
| primary_url: str, | ||
| ) -> bytes: | ||
| LOG.info("Forwarding %s request to remote url: %r since we're not the master", request.method, request.url) | ||
| timeout = 60.0 | ||
| headers = request.headers.mutablecopy() |
There was a problem hiding this comment.
There's a piece of code for the authorization header, do we want to bring this back?
i.e.
# auth_header = request.headers.get("Authorization")
# if auth_header is not None:
# headers["Authorization"] = auth_headerThere was a problem hiding this comment.
There's also this piece of code: func = getattr(self._get_forward_client(), request.method.lower()), this would mean that we generate a new client session every time we want to forward a request. Maybe we move the session initialization to the __init__() function, assign directly to self._forward_client and that will get reused for the whole app runtime and we can even add a close() function that cleans up the client session in the application lifespan.
There was a problem hiding this comment.
Yes, probably not in this PR though.
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| headers=response.headers, | ||
| ) | ||
| if self._acceptable_response_content_type(content_type=response.headers.get("Content-Type")): | ||
| return await response.text() |
There was a problem hiding this comment.
Would there be anytime when we expect text/plain?
There was a problem hiding this comment.
Error cases, I can't think of anything else.
|
|
||
| async def forward_request_remote(self, *, request: Request, primary_url: str, response_type: type[T] | type[P]) -> T | P: | ||
| body = await self._forward_request_remote(request=request, primary_url=primary_url) | ||
| if response_type == int: |
There was a problem hiding this comment.
Why not using isinstance()?
There was a problem hiding this comment.
The response_type is a type not an int object.
There was a problem hiding this comment.
Ah yh makes sense, hence the type vars 👍
src/karapace/forward_client.py
Outdated
| if response_type == list[int]: | ||
| return json_decode(body, assume_type=list[int]) # type: ignore[return-value] | ||
| if issubclass(response_type, BaseModel): | ||
| return response_type.model_validate_json(body) # type: ignore[return-value] |
There was a problem hiding this comment.
If it's a BaseModel, it should already have been validated from the source response, unless there's no other way to load the text into the object.
There was a problem hiding this comment.
Uh, Pydantic 1.x has parse_raw which is deprecated in 2.x. I think we got to support 1.x so I'll change to parse_raw.
| status_code=response.status, | ||
| headers=response.headers, | ||
| ) | ||
| if self._acceptable_response_content_type(content_type=response.headers.get("Content-Type")): |
There was a problem hiding this comment.
What if the server cannot be reached, i.e. there's even no response, etc, or 5xx status.
There was a problem hiding this comment.
Handling improvement to do later.
jjaakola-aiven
force-pushed
the
jjaakola-aiven-fastapi-lint-fixes
branch
from
d7e9684 to
37a17fa
Compare
jjaakola-aiven
force-pushed
the
jjaakola-aiven-fastapi-lint-fixes
branch
from
76f2f7e to
5f77f09
Compare
| ) -> BaseModelResponse | SimpleTypeResponse: | ||
| body = await self._forward_request_remote(request=request, primary_url=primary_url) | ||
| if response_type == int: | ||
| return int(body) # type: ignore[return-value] |
There was a problem hiding this comment.
Maybe a cast will work to avoid the # type: ignore[return-value], we can cast to the SimpleTypeResponse, but not sure if this works, will test in another PR.
| @@ -14,16 +16,30 @@ | |||
| LOG = logging.getLogger(__name__) | |||
|
|
|||
|
|
|||
| BaseModelResponse = TypeVar("BaseModelResponse", bound=BaseModel) | |||
| SimpleTypeResponse = TypeVar("SimpleTypeResponse", bound=Union[int, list[int]]) | |||
There was a problem hiding this comment.
Do we need the Union, maybe | works, will test it later.
| ], | ||
| ids=str, | ||
| ) | ||
| async def test_forward_request_with_basemodel_response(testcase: ContentTypeTestCase) -> None: |
About this change - What it does
References: #xxxxx
Why this way