Conversation
|
I would love to hear feedback about this proxy integration, I could overlook something or my implementation is way to barbaric. My plan is to fix all of the known limitations and todo's |
| # if proxies: | ||
| # raise NotImplementedError("proxies not supported") |
There was a problem hiding this comment.
You can remove these commented-out lines now that support is implemented.
| if self.proxies.get("https", None): | ||
| self.curl.setopt(pycurl.PROXY, self.proxies.get("https")) | ||
| else: | ||
| # http? could be there anything else in the dict we would have to use? | ||
| self.curl.setopt(pycurl.PROXY, self.proxies.get("http")) |
There was a problem hiding this comment.
I'd recommend also setting PROXYTYPE based on the proxy selected:
https_proxy = self.proxies.get("https")
http_proxy = self.proxies.get("http")
if https_proxy is not None:
self.curl.setopt(pycurl.PROXYTYPE, pycurl.PROXYTYPE_HTTPS)
self.curl.setopt(pycurl.PROXY, https_proxy)
elif http_proxy is not None:
self.curl.setopt(pycurl.PROXYTYPE, pycurl.PROXYTYPE_HTTP)
self.curl.setopt(pycurl.PROXY, http_proxy)
else:
self.curl.setopt(pycurl.PROXYTYPE, 0) # default is HTTP
self.curl.setopt(pycurl.PROXY, None)| # TODO: add support for the envoirement variables | ||
| # $ export HTTP_PROXY="http://10.10.1.10:3128" | ||
| # $ export HTTPS_PROXY="http://10.10.1.10:1080" | ||
| # $ export ALL_PROXY="socks5://10.10.1.10:3434" |
There was a problem hiding this comment.
By default libcurl respects the standard proxy environment variables, so we probably don't need to handle it ourselves.
| # TODO: I've just cleaned up the settings, so now we will get the stream and cert errors | ||
| # I don't really need streams or certs myself, so it's not the best thing to ship to production |
There was a problem hiding this comment.
I would remove this comment. While it's helpful for me to understand the code review, it will likely confuse other people who look at the code in the future.
| timeout=None, | ||
| allow_redirects=True, | ||
| max_redirects=-1, | ||
| proxies={}, |
There was a problem hiding this comment.
It's generally not recommended to use mutable types as default values as it can lead to surprising results (that one dict instance is shared for the entire program, so any changes made to it affect everyone).
Usually the pattern I like is:
def my_function(my_dict_param = None):
my_dict_param = my_dict_param or {}
…This takes advantage of or's short-circuit behaviour of returning the first truth-y value.
| # settings.update( | ||
| # self.merge_environment_settings(prep.url, proxies, stream, verify, cert) | ||
| # ) | ||
| send_kwargs = { |
There was a problem hiding this comment.
Requests does it this way to allow the user to subclass Session and override the behaviour.
By commenting out this code you would prevent their overridden behaviour being called.
|
|
||
| assert isinstance(origin_response, requests.Response) | ||
| assert isinstance(proxy_response, requests.Response) | ||
| # TODO: this is not the best way to test proxy. Ideally test should look for other params that imply the proxy is bieng used |
There was a problem hiding this comment.
Self-contained testing this is a bit tricky.
One idea would be to use http_server as the proxy server and check that the proxy request is the right format. You can tell a request is intended for a proxy server since the request is either a GET with the full URL not just the path (e.g. GET http://httpbin.org/get) for HTTP requests; or CONNECT with a host:port for HTTPS requests (e.g. CONNECT httpbin.org:443).
2 participants
I've added contribution section to the readme.md file (simple stuff just to run the tests)
And i've added rough proxy support.
These changes are not ideal because they could fuck up backwards compatibility. I removed the settings filter for the PyCurlHttpAdapter, so if
streamorcertare set, there will be an error.It's possible return some sort of filtering back to the pycurl_requests/sessions.py request func, however I do not like the empty dict solution at all.