Copy of #2582, Added proxy parameters for ClientSession #3405

RatanShreshtha · 2018-11-22T19:31:44Z

Added proxy parameters for ClientSession so that subsequent requests can use the session proxy.

Are there changes in behavior for the user?

New parameters of ClientSession():

proxy
proxy_auth
proxy_headers

proxy_auth = aiohttp.BasicAuth('user', 'pass')
async with ClientSession(proxy="http://proxy.com", proxy_auth=proxy_auth) as session:
    async with session.get('http://test.com') as resp:
        print(await resp.text())

Related issue number

#2580
#2582

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

webknjaz · 2018-11-23T10:08:03Z

CONTRIBUTORS.txt

 Yusuke Tsutsumi
 Марк Коренберг
-Семён Марьясин
+Семён Марьясин


This change is unrelated. Fix your editor to preserve original EOF.

@RatanShreshtha ^

webknjaz · 2018-11-23T10:09:11Z

aiohttp/client.py

        version = self._version

+        # Merge with session proxy
+        proxy = self._proxy_info['proxy'] \


Please use brackets instead of escaping EOL

tests/test_proxy_session.py

asvetlov · 2018-11-23T11:10:02Z

Not sure if I like yet another params in ClientSession constructor.

Anyway, the biggest showstopper now is proxy tests.
I have a strong feeling that mocking proxy calls has close to zero value.
Our functional proxy tests are messy too.
What we need to unblock any proxy improvement is implementing a test proxy server, rewriting tests to use it and dropping the current mocking approach.

RatanShreshtha · 2018-11-23T15:37:26Z

@asvetlov I am thinking about sending a separate PR with test proxy server and new tests.

RatanShreshtha · 2018-11-23T17:19:31Z

@webknjaz I am unable to solve the issue with mypy I have not used mypy before, can you guide me through that I have done wrong ?

webknjaz · 2018-11-23T17:27:47Z

@RatanShreshtha

So the errors are:

aiohttp/client.py:293: error: Incompatible types in assignment (expression has type "Union[str, URL, None, BasicAuth, Mapping[str, str]]", variable has type "Union[str, URL, None]")
aiohttp/client.py:296: error: Incompatible types in assignment (expression has type "Union[str, URL, None, BasicAuth, Mapping[str, str]]", variable has type "Optional[BasicAuth]")
aiohttp/client.py:299: error: Incompatible types in assignment (expression has type "Union[str, URL, None, BasicAuth, Mapping[str, str]]", variable has type "Union[Mapping[str, str], CIMultiDict[str], CIMultiDictProxy[str], None]")

Let's look at the first one. The statement (partially) is

proxy = self._proxy_info['proxy']

So it says expression has type "Union[str, URL, None, BasicAuth, Mapping[str, str]]", which means that the expression self._proxy_info['proxy'] ... returns smth of type Union[str, URL, None, BasicAuth, Mapping[str, str]] but earlier on line 258:

proxy: Optional[StrOrURL]=sentinel,

you declared that allowed types for data to assign to the proxy variable are Optional[StrOrURL] (which unpacks to Union[str, URL, None] as stated in the error).
You need to modify type definitions or code so that there were no code assigning "forbidden"/non-matching types to vars.

asvetlov

Please reserve merge conflicts
Docs update is needed.
@webknjaz asked for changes, not all comments are addressed as I see.

proxy_test_server implementation suggestion:
In my mind the server fixture proxies requests to the app.
Why not pass a app server to the fixture to check the proxy? It can get rid of mocks in tests.

asvetlov · 2018-12-17T20:38:32Z

docs/client_reference.rst

                         connector_owner=True, \
-                         auto_decompress=True, proxies=None)
+                         auto_decompress=True, \
+                         proxy=None, proxy_auth=None, proxy_headers=None))


Please document these params, not only enumerate in constructor signature.

I have documented these changes.

asvetlov · 2018-12-17T20:45:15Z

aiohttp/client.py

+        if proxy is None and self._session_proxy is not None:
+            try:
+                proxy = self._session_proxy
+                proxy_auth = self._session_proxy_auth


By this line you are overriding explicitly passed proxy_auth and proxy_headers arguments with session default ones.

I have no idea how to resolve the conflict properly but proposed behavior looks confusing.

I am trying to use session proxy if proxy is not mentioned explicitly for a particular request.

webknjaz · 2019-07-29T12:55:49Z

Hey @RatanShreshtha, this PR needs updating

RatanShreshtha · 2019-07-29T13:04:44Z

@webknjaz let me update the PR

webknjaz · 2019-07-29T13:09:11Z

CONTRIBUTORS.txt

 Eugene Naydenov
 Eugene Tolmachev
 Evert Lammerts
+Fan Yu


Plz clean-up all unrelated changes.

Should I squash all the changes ?

Up to you. We'll use "Merge and squash" strategy on this PR anyway :)

asvetlov

Looks good, please address my comment

asvetlov · 2019-08-02T10:01:59Z

aiohttp/client.py

                skip_headers.add(istr(i))

-        if proxy is not None:
+        if proxy is not None and self._session_proxy is None:


What is the behavior if proxy is not None and self._session_proxy is not None?
I think passed proxy parameter should take precedence.

Copy of #2582, Added proxy parameters for ClientSession

8055b96

RatanShreshtha requested review from asvetlov and webknjaz as code owners November 22, 2018 19:31

RatanShreshtha added 2 commits November 23, 2018 10:26

Removed extra line from aiohttp/client.py and sorted CONTRIBUTORS.txt

d3416a3

Fixed tests for session proxy.

f23b9ee

webknjaz reviewed Nov 23, 2018

View reviewed changes

tests/test_proxy_session.py Outdated Show resolved Hide resolved

webknjaz reviewed Nov 23, 2018

View reviewed changes

tests/test_proxy_session.py Outdated Show resolved Hide resolved

Merge branch 'master' of https://github.com/aio-libs/aiohttp

19ccf12

This comment has been minimized.

Sign in to view

RatanShreshtha added 4 commits November 24, 2018 19:34

Merge branch 'master' of https://github.com/aio-libs/aiohttp

5821bc0

Fixed mypy errors

5c37878

Merge branch 'master' of https://github.com/aio-libs/aiohttp

66c4ce7

Added proxy params in client session attrs

8e79f99

This comment has been minimized.

Sign in to view

asvetlov requested changes Dec 15, 2018

View reviewed changes

asvetlov requested changes Dec 17, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

Merge branch 'master' of https://github.com/aio-libs/aiohttp

8c249e1

This comment has been minimized.

Sign in to view

Ratan KULSHRESHTHA and others added 4 commits February 24, 2019 18:46

Merge branch 'master' of https://github.com/aio-libs/aiohttp

3defdef

Merge branch 'master' of https://github.com/aio-libs/aiohttp

dc2115c

Merge branch 'master' of github.com:RatanShreshtha/aiohttp

1f11214

Resolved conflict and making tests pass.

7370f8a

This comment has been minimized.

Sign in to view

RatanShreshtha and others added 6 commits April 5, 2019 00:59

Merge branch 'master' of github.com:RatanShreshtha/aiohttp

0233316

Merge branch 'master' of https://github.com/aio-libs/aiohttp

e592dd6

Merge branch 'master' of github.com:RatanShreshtha/aiohttp

578de43

Merge branch 'master' of https://github.com/aio-libs/aiohttp

9c75827

Merge branch 'master' of https://github.com/aio-libs/aiohttp

bcd94cf

Merge branch 'master' of github.com:RatanShreshtha/aiohttp

36c2b1b

This comment has been minimized.

Sign in to view

RatanShreshtha and others added 6 commits May 13, 2019 23:25

Merge branch 'master' of https://github.com/aio-libs/aiohttp

f1fba4d

Merge branch 'master' of github.com:RatanShreshtha/aiohttp

1936e55

Merge branch 'master' of https://github.com/aio-libs/aiohttp

18d9dde

Merge branch 'master' of github.com:RatanShreshtha/aiohttp

90333ca

Merge branch 'master' of https://github.com/aio-libs/aiohttp

4ee84d7

Merge branch 'master' of github.com:RatanShreshtha/aiohttp

508541e

webknjaz closed this Jul 29, 2019

webknjaz reopened this Jul 29, 2019

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jul 29, 2019

webknjaz closed this Jul 29, 2019

webknjaz reopened this Jul 29, 2019

webknjaz reviewed Jul 29, 2019

View reviewed changes

Ratan KULSHRESHTHA added 2 commits July 29, 2019 18:49

Merge branch 'master' of https://github.com/aio-libs/aiohttp

f266e15

Merge branch 'master' of github.com:RatanShreshtha/aiohttp

d767587

asvetlov reviewed Aug 2, 2019

View reviewed changes

asvetlov mentioned this pull request Aug 2, 2019

Added proxy parameters for ClientSession #2582

Closed

5 tasks

ntextreme3 mentioned this pull request Jan 18, 2020

[python] asyncio aiohttp.ClientSession doesn't have proxy in __init__ swagger-api/swagger-codegen#9995

Open

RatanShreshtha closed this Sep 4, 2020

Uh oh!

Copy of #2582, Added proxy parameters for ClientSession #3405

Copy of #2582, Added proxy parameters for ClientSession #3405

Uh oh!

Conversation

RatanShreshtha commented Nov 22, 2018

Are there changes in behavior for the user?

Related issue number

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

asvetlov commented Nov 23, 2018

Uh oh!

RatanShreshtha commented Nov 23, 2018

Uh oh!

RatanShreshtha commented Nov 23, 2018

Uh oh!

webknjaz commented Nov 23, 2018

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

webknjaz commented Jul 29, 2019

Uh oh!

RatanShreshtha commented Jul 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants