Skip to content
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

Add a base_url option to ClientSession #6129

Merged
merged 20 commits into from Oct 31, 2021
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES/6013.feature
@@ -0,0 +1 @@
Added ``base_url`` parameter to the initializer of :class:`~aiohttp.ClientSession`.
15 changes: 14 additions & 1 deletion aiohttp/client.py
Expand Up @@ -169,6 +169,7 @@ class ClientSession:
"""First-class interface for making HTTP requests."""

__slots__ = (
"_base_url",
"_source_traceback",
"_connector",
"_loop",
Expand All @@ -193,6 +194,7 @@ class ClientSession:

def __init__(
self,
base_url: Optional[StrOrURL] = None,
*,
connector: Optional[BaseConnector] = None,
cookies: Optional[LooseCookies] = None,
Expand All @@ -216,6 +218,11 @@ def __init__(
trace_configs: Optional[List[TraceConfig]] = None,
read_bufsize: int = 2 ** 16,
) -> None:
if base_url is None or isinstance(base_url, URL):
self._base_url: Optional[URL] = base_url
else:
self._base_url = URL(base_url)
asvetlov marked this conversation as resolved.
Show resolved Hide resolved

loop = asyncio.get_running_loop()

if connector is None:
Expand Down Expand Up @@ -304,6 +311,12 @@ def request(
"""Perform HTTP request."""
return _RequestContextManager(self._request(method, url, **kwargs))

def _build_url(self, str_or_url: StrOrURL) -> URL:
if self._base_url is None:
return URL(str_or_url)
else:
return self._base_url.join(URL(str_or_url))
asvetlov marked this conversation as resolved.
Show resolved Hide resolved

async def _request(
self,
method: str,
Expand Down Expand Up @@ -363,7 +376,7 @@ async def _request(
proxy_headers = self._prepare_headers(proxy_headers)

try:
url = URL(str_or_url)
url = self._build_url(str_or_url)
except ValueError as e:
raise InvalidURL(str_or_url) from e

Expand Down
12 changes: 12 additions & 0 deletions docs/client_quickstart.rst
Expand Up @@ -55,6 +55,18 @@ Other HTTP methods are available as well::
session.options('http://httpbin.org/get')
session.patch('http://httpbin.org/patch', data=b'data')

To make several requests to the same site more simple, the parameter ``base_url``
of :class:`ClientSession` constructor can be used. For example to request different
endpoints of ``http://httpbin.org`` can be used the following code::

async with aiohttp.ClientSession('http://httpbin.org') as session:
async with session.get('/get'):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is correct, sure.

I think an example (or .. note::) that demonstrates the result of joining base URL with partial path and a relative path can be helpful also.
httpbin.org supports /status/{codes} and /image/jpeg / /image/png urls.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is an example that uses base_url feature of the code above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like a doctest could be more visual here.

>>> invocation_block()
result

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this is the problem.
If I call URL.join for http://httpbin.org/status and 200 I get http://httpbin.org/200 which is probably not expected result :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooh.

Tests for the standard urllib.parse.urljoin():

In [1]: from urllib.parse import urljoin

In [2]: urljoin("http://a.b.c:/path", "to")
Out[2]: 'http://a.b.c:/to'

In [3]: urljoin("http://a.b.c:/path/", "to")
Out[3]: 'http://a.b.c:/path/to'

In [4]: urljoin("http://a.b.c:/path/thing", "to")
Out[4]: 'http://a.b.c:/path/to'

In [5]: urljoin("http://a.b.c:/path/thing/foo", "to")
Out[5]: 'http://a.b.c:/path/thing/to'

Sorry, I should go now.
Tomorrow I'll check the compliance of behavior above with RFC. I suspect it matches :(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, works as expected by RFC.

Copy link
Contributor Author

@derlih derlih Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked one more time:

>>> from yarl import URL
>>> URL("http://httpbin.org/status").join(URL("/200"))
URL('http://httpbin.org/200')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asvetlov
It works as expected by RFC, but I think the feature wasn't about RFC implementation.
May be it would be better if base_url='http://httpbin.org/status' and url='/200' build http://httpbin.org/status'/200.
Not RFC complaint, but I would expect such behavior.

Copy link
Member

@asvetlov asvetlov Oct 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option is following the current url / path behavior:
base_url='http://httpbin.org/status' and url='/200' is forbidden
base_url='http://httpbin.org/status' and url='200' builds http://httpbin.org/status/200

BTW, should we forbid query/fragment parts in base_url? They are ignored anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's wait for the user feedback on it.

pass
async with session.post('/post', data=b'data'):
pass
async with session.put('/put', data=b'data'):
pass

.. note::

Don't create a session per request. Most likely you need a session
Expand Down
8 changes: 7 additions & 1 deletion docs/client_reference.rst
Expand Up @@ -38,7 +38,8 @@ Usage example::

The client session supports the context manager protocol for self closing.

.. class:: ClientSession(*, connector=None, loop=None, cookies=None, \
.. class:: ClientSession(base_url=None, *, \
connector=None, cookies=None, \
headers=None, skip_auto_headers=None, \
auth=None, json_serialize=json.dumps, \
version=aiohttp.HttpVersion11, \
Expand All @@ -56,6 +57,11 @@ The client session supports the context manager protocol for self closing.
The class for creating client sessions and making requests.


:param base_url: Base part of the URL (optional)
If set it allows to skip the base part in request calls.

.. versionadded:: 3.8
derlih marked this conversation as resolved.
Show resolved Hide resolved

:param aiohttp.BaseConnector connector: BaseConnector
sub-class instance to support connection pooling.

Expand Down
70 changes: 70 additions & 0 deletions tests/test_client_session.py
Expand Up @@ -706,3 +706,73 @@ async def test_requote_redirect_url_default_disable() -> None:
session = ClientSession(requote_redirect_url=False)
assert not session.requote_redirect_url
await session.close()


@pytest.mark.parametrize(
("base_url", "url", "expected_url"),
[
pytest.param(
None,
"http://example.com/test",
URL("http://example.com/test"),
id="base_url=None url='http://example.com/test'",
),
pytest.param(
None,
URL("http://example.com/test"),
URL("http://example.com/test"),
id="base_url=None url=URL('http://example.com/test')",
),
pytest.param(
"http://example.com",
"/test",
URL("http://example.com/test"),
id="base_url='http://example.com' url='/test'",
),
pytest.param(
"http://example.com",
"test",
URL("http://example.com/test"),
id="base_url='http://example.com' url='test'",
),
pytest.param(
URL("http://example.com"),
"/test",
URL("http://example.com/test"),
id="base_url=URL('http://example.com') url='/test'",
),
pytest.param(
URL("http://example.com"),
"test",
URL("http://example.com/test"),
id="base_url=URL('http://example.com') url='test'",
),
],
)
async def test_build_url_returns_expected_url(
create_session, base_url, url, expected_url
) -> None:
session = await create_session(base_url)
assert session._build_url(url) == expected_url


async def test_request_uses_base_url_when_url_is_str(create_session) -> None:
request_class = mock.MagicMock()
webknjaz marked this conversation as resolved.
Show resolved Hide resolved
session = await create_session("http://example.com", request_class=request_class)
with contextlib.suppress(Exception):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not be more precise here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exception seems too broad. What sort of error do you expect really?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was exactly the point. In this test I don't care about the exception, I care only about the call of the request_class.
Actually quite many things happen inside ClientSession._request, and I don't want this test to fail because something unrelated to it has been changed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree with this perception. There are things that we'd want to fail the test. For example, we make pytest error out on any warnings (namely deprecation warnings). This will silently shadow those while they must bubble up and show up as an error.

await session.get("/test")

args, _ = request_class.call_args
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use called_with_args()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to check only the url param. The others I don't care.
For called_with_args you have to specify all of them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you could specify mocks for those IIRC.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I dislike the current approach is that having a check to "extract an element from a list and compare it with some value" is semantically ambiguous. It doesn't mean anything. While called_with_args() communicates the intent of the test better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same idea as with supress(Exception). Here the only thing that matters is the url param of the request_class constructor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not the only thing that matters. Shadowing the underlying warnings shouldn't happen because it's important for us to see any deprecations as soon as any runtime deps get updated: https://github.com/aio-libs/aiohttp/pull/6129/files#r738312860.

url = args[1]
assert url == URL("http://example.com/test")


async def test_request_not_uses_base_url_when_url_is_URL(create_session) -> None:
request_class = mock.MagicMock()
session = await create_session("http://example.com", request_class=request_class)
with contextlib.suppress(Exception):
await session.get(URL("http://sample.com"))

args, _ = request_class.call_args
url = args[1]
assert url == URL("http://sample.com")