question: connector get/post parity

[dimaqq]

How come get has a timeout= mandatory keyword argument, but post does not have a timeout= argument?

[jarikujansuu]

Both underlying libraries aiohttp and httpx support timeout for both verbs so would make sense to have support in both.

[ojii]

get is used by credentials which has a different timeout logic than post which is used in combination with ThrottleConfig. I think that's why those are different.

To be honest, I think the whole aiodynamo.http.base.HTTP interface was a mistake (at least the way it was implemented). I still think that not tying aiodynamo tightly to a specific http library/version is a good goal (since those usually move quite fast and version conflicts would be quite probable), but the interface is terrible. The API disparity between get and post is a good example of that. It kind of makes sense if you look at the call sites why they return different things, but it's just confusing.

I think the actual interface for pluggable HTTP should be something more like the following and maybe it should be changed to that. Then, the timeout logic and handling dynamo-level errors (400s, exception_from_response) could be handled internally and the http-glue code doesn't have to worry about it:

from dataclasses import dataclass
from typing import Optional, Callable, Awaitable, Literal, Union, Dict

import aiohttp
import httpx


@dataclass(frozen=True)
class Request:
    method: Union[Literal["GET"], Literal["POST"]]
    url: str
    headers: Dict[str, str]  # Mapping is more appropriate but httpx doesn't like it...
    body: Optional[bytes]


@dataclass(frozen=True)
class Response:
    status: int
    body: bytes  # may be empty bytes


@dataclass
class RequestFailed(Exception):
    inner: Exception


HTTPInterface = Callable[
    [Request], Awaitable[Response]
]  # may throw RequestFailed for connection based errors.


# Example implementations, actual implementations would likely be a class with an async __call__ so sessions etc
# can be re-used, for simplicity, that is ignored here.


async def aiohttp_impl(request: Request) -> Response:
    session: aiohttp.ClientSession
    try:
        async with session.request(
            method=request.method,
            url=request.url,
            headers=request.headers,
            body=request.body,
        ) as response:
            return Response(response.status, await response.read())
    except aiohttp.ClientError as exc:
        raise RequestFailed(exc)


async def httpx_impl(request: Request) -> Response:
    client: httpx.AsyncClient
    try:
        response = await client.request(
            method=request.method,
            url=request.url,
            headers=request.headers,
            content=request.body,
        )
        return Response(response.status_code, await response.aread())
    except httpx.HTTPError as exc:
        raise RequestFailed(exc)

Would that be better and would this change be worth it?

[dimaqq]

I quite like it!

Now is a good time to consider breaking changes, since 2022 is coming up and that changes the major version :)