Add a `base_url` option to `ClientSession` #6129

derlih · 2021-10-24T17:11:09Z

What do these changes do?

Add base_url to ClientSession constructor that makes several requests to the same server look simpler.

Are there changes in behavior for the user?

New optional parameter in ClientSession constructor. If not set, the behavior is not changed.

Related issue number

Fixes #6013

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the CHANGES folder
- name it <issue_id>.<type> for example (588.bugfix)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

codecov · 2021-10-25T08:57:32Z

Codecov Report

Merging #6129 (3a85902) into master (4fcfb8a) will decrease coverage by 0.00%.
The diff coverage is 68.75%.

@@            Coverage Diff             @@
##           master    #6129      +/-   ##
==========================================
- Coverage   93.29%   93.29%   -0.01%     
==========================================
  Files         102      102              
  Lines       30238    30351     +113     
  Branches     2712     2729      +17     
==========================================
+ Hits        28212    28315     +103     
- Misses       1849     1858       +9     
- Partials      177      178       +1

Flag	Coverage Δ
unit	`93.21% <65.62%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiohttp/client.py	`24.56% <9.09%> (-0.33%)`	⬇️
tests/test_client_session.py	`99.56% <100.00%> (+0.07%)`	⬆️
aiohttp/http_parser.py	`92.45% <0.00%> (-0.15%)`	⬇️
tests/test_http_parser.py	`99.05% <0.00%> (+<0.01%)`	⬆️
aiohttp/web_urldispatcher.py	`97.55% <0.00%> (+<0.01%)`	⬆️
tests/test_helpers.py	`98.88% <0.00%> (+0.03%)`	⬆️
tests/test_web_urldispatcher.py	`98.14% <0.00%> (+0.07%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4fcfb8a...3a85902. Read the comment docs.

tests/test_client_session.py

webknjaz · 2021-10-27T01:18:34Z

tests/test_client_session.py

+async def test_request_uses_base_url_when_url_is_str() -> None:
+    request_class = mock.MagicMock()
+    session = ClientSession("http://example.com", request_class=request_class)
+    with contextlib.suppress(Exception):


Why not be more precise here?

What do you mean?

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

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.

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.

tests/test_client_session.py

webknjaz · 2021-10-27T01:20:39Z

tests/test_client_session.py

+    with contextlib.suppress(Exception):
+        await session.get("/test")
+
+    args, _ = request_class.call_args


Why not use called_with_args()?

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.

Yeah, you could specify mocks for those IIRC.

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.

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

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.

tests/test_client_session.py

CHANGES/6013.feature

aiohttp/client.py

docs/client_reference.rst

tests/test_client_session.py

aiohttp/client.py

CHANGES/6013.feature

tests/test_client_session.py

asvetlov

I have a couple of suggestions if we agree to follow URL.join() behavior.

Maybe urljoin is not the fastest approach but it can be improved by the next yarl release easily.

aiohttp/client.py

asvetlov · 2021-10-28T16:34:55Z

docs/client_quickstart.rst

+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'):


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.

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

Something like a doctest could be more visual here.

>>> invocation_block() result

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 :-)

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 :(

Ok, works as expected by RFC.

Checked one more time:

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

@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.

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.

Let's wait for the user feedback on it.

tests/test_client_session.py

asvetlov

LGTM.

I prefer functional tests over mocks (test server that handles all requests and returns request.url in response body can be used) but I can live with the current state.

Mocking increases the chance of dropping in internals are changed (I want to drop request_class argument eventually in 4.x for example).

Co-authored-by: Sviatoslav Sydorenko <[email protected]>

Co-authored-by: Andrew Svetlov <[email protected]>

Co-authored-by: Sviatoslav Sydorenko <[email protected]>

aiohttp/client.py

for more information, see https://pre-commit.ci

asvetlov · 2021-10-31T18:09:00Z

I'm merging as-is.
We can improve the feature support in future

patchback · 2021-10-31T18:09:33Z

Backport to 3.8: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 527b1b9 on top of patchback/backports/3.8/527b1b9b6f60b0becae3e63550c1b76de1b189e7/pr-6129

Backporting merged PR #6129 into master

Ensure you have a local repo clone of your fork. Unless you cloned it
from the upstream, this would be your origin remote.
Make sure you have an upstream repo added as a remote too. In these
instructions you'll refer to it by the name upstream. If you don't
have it, here's how you can add it:
```
$ git remote add upstream https://github.com/aio-libs/aiohttp.git
```

Ensure you have the latest copy of upstream and prepare a branch
that will hold the backported code:

$ git fetch upstream
$ git checkout -b patchback/backports/3.8/527b1b9b6f60b0becae3e63550c1b76de1b189e7/pr-6129 upstream/3.8

Now, cherry-pick PR Add a base_url option to ClientSession #6129 contents into that branch:
```
$ git cherry-pick -x 527b1b9b6f60b0becae3e63550c1b76de1b189e7
```
If it'll yell at you with something like fatal: Commit 527b1b9b6f60b0becae3e63550c1b76de1b189e7 is a merge but no -m option was given., add -m 1 as follows intead:
```
$ git cherry-pick -m1 -x 527b1b9b6f60b0becae3e63550c1b76de1b189e7
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR Add a base_url option to ClientSession #6129 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.8/527b1b9b6f60b0becae3e63550c1b76de1b189e7/pr-6129

Create a PR, ensure that the CI is green. If it's not — update it so that
the tests and any other checks pass. This is it!
Now relax and wait for the maintainers to process your pull request
when they have some cycles to do reviews. Don't worry — they'll tell you if
any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

aio-libs-github-bot · 2021-10-31T18:14:34Z

💔 Backport was not successful

The PR was attempted backported to the following branches:

❌ 3.8: Commit could not be cherrypicked due to conflicts

Co-authored-by: Andrew Svetlov <[email protected]> Co-authored-by: Sviatoslav Sydorenko <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

asvetlov · 2021-10-31T18:30:19Z

Backported by 59bbcac

derlih requested review from asvetlov and webknjaz as code owners October 24, 2021 17:11

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 24, 2021

derlih linked an issue Oct 24, 2021 that may be closed by this pull request

Add a base_url option to ClientSession like httpx does #6013

Closed

1 task

derlih removed a link to an issue Oct 24, 2021

Add a base_url option to ClientSession like httpx does #6013

Closed

1 task