-
Notifications
You must be signed in to change notification settings - Fork 71
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
Linting fixes #1004
Linting fixes #1004
Changes from all commits
8acc092
360a1f5
220f45d
0d6074c
37a17fa
5f77f09
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,9 +3,11 @@ | |
See LICENSE for details | ||
""" | ||
|
||
from fastapi import Request, Response | ||
from fastapi.responses import JSONResponse, PlainTextResponse | ||
from fastapi import HTTPException, Request, status | ||
from karapace.utils import json_decode | ||
from karapace.version import __version__ | ||
from pydantic import BaseModel | ||
from typing import overload, TypeVar, Union | ||
|
||
import aiohttp | ||
import async_timeout | ||
|
@@ -14,16 +16,30 @@ | |
LOG = logging.getLogger(__name__) | ||
|
||
|
||
BaseModelResponse = TypeVar("BaseModelResponse", bound=BaseModel) | ||
SimpleTypeResponse = TypeVar("SimpleTypeResponse", bound=Union[int, list[int]]) | ||
|
||
|
||
class ForwardClient: | ||
USER_AGENT = f"Karapace/{__version__}" | ||
|
||
def __init__(self): | ||
def __init__(self) -> None: | ||
self._forward_client: aiohttp.ClientSession | None = None | ||
|
||
def _get_forward_client(self) -> aiohttp.ClientSession: | ||
return aiohttp.ClientSession(headers={"User-Agent": ForwardClient.USER_AGENT}) | ||
|
||
async def forward_request_remote(self, *, request: Request, primary_url: str) -> Response: | ||
def _acceptable_response_content_type(self, *, content_type: str) -> bool: | ||
return ( | ||
content_type.startswith("application/") and content_type.endswith("json") | ||
) or content_type == "application/octet-stream" | ||
|
||
async def _forward_request_remote( | ||
self, | ||
*, | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a piece of code for the authorization header, do we want to bring this back? # auth_header = request.headers.get("Authorization")
# if auth_header is not None:
# headers["Authorization"] = auth_header There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's also this piece of code: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, probably not in this PR though. |
||
|
@@ -37,18 +53,52 @@ async def forward_request_remote(self, *, request: Request, primary_url: str) -> | |
forward_url = f"{forward_url}?{request.url.query}" | ||
logging.error(forward_url) | ||
|
||
with async_timeout.timeout(timeout): | ||
async with async_timeout.timeout(timeout): | ||
body_data = await request.body() | ||
async with func(forward_url, headers=headers, data=body_data) as response: | ||
if response.headers.get("Content-Type", "").startswith("application/json"): | ||
return JSONResponse( | ||
content=await response.text(), | ||
status_code=response.status, | ||
headers=response.headers, | ||
) | ||
else: | ||
return PlainTextResponse( | ||
content=await response.text(), | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handling improvement to do later. |
||
return await response.text() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would there be anytime when we expect There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Error cases, I can't think of anything else. |
||
LOG.error("Unknown response for forwarded request: %s", response) | ||
raise HTTPException( | ||
status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
detail={ | ||
"error_code": status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
"message": "Unknown response for forwarded request.", | ||
}, | ||
) | ||
|
||
@overload | ||
async def forward_request_remote( | ||
self, | ||
*, | ||
request: Request, | ||
primary_url: str, | ||
response_type: type[BaseModelResponse], | ||
) -> BaseModelResponse: | ||
... | ||
|
||
@overload | ||
async def forward_request_remote( | ||
self, | ||
*, | ||
request: Request, | ||
primary_url: str, | ||
response_type: type[SimpleTypeResponse], | ||
) -> SimpleTypeResponse: | ||
... | ||
|
||
async def forward_request_remote( | ||
self, | ||
*, | ||
request: Request, | ||
primary_url: str, | ||
response_type: type[BaseModelResponse] | type[SimpleTypeResponse], | ||
) -> BaseModelResponse | SimpleTypeResponse: | ||
body = await self._forward_request_remote(request=request, primary_url=primary_url) | ||
if response_type == int: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yh makes sense, hence the type vars 👍 |
||
return int(body) # type: ignore[return-value] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe a cast will work to avoid the |
||
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.parse_raw(body) # type: ignore[return-value] | ||
raise ValueError("Did not match any expected type") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the
Union
, maybe|
works, will test it later.