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

Deprecate URL.is_ssl #1128

Merged
merged 5 commits into from
Aug 5, 2020
Merged

Deprecate URL.is_ssl #1128

merged 5 commits into from
Aug 5, 2020

Conversation

tomchristie
Copy link
Member

This is one of those niggly little API bits that have only ended up in here because of implementation details.

We're not using url.is_ssl anywhere in our codebase, and now that we're agnostic to protocols except at the transport layer, this API doesn't feel right. (Eg ftps://... is an SSL URL, but would return false here.)

If any users are using this, they should migrate to a more explicit if url.scheme == "https":.

@tomchristie tomchristie added this to the v0.14 milestone Aug 5, 2020
@tomchristie tomchristie added the refactor Issues and PRs related to code refactoring label Aug 5, 2020
@StephenBrown2
Copy link
Contributor

ftps:// is also an invalid scheme, isn't it? Since httpx is now enforcing only http or https?

@florimondmanca
Copy link
Member

florimondmanca commented Aug 5, 2020

@StephenBrown2 Not, not an invalid URL per se anymore. :-)

>>> import httpx  # @ master
>>> httpx.URL('ftps://localhost/test.txt')
URL('ftps://localhost/test.txt')  # Would have raised an exception on 0.13.x.

The decision of "is this scheme valid?" has been deferred to the transport layer - see #1080.

@@ -140,6 +140,8 @@ def raw(self) -> typing.Tuple[bytes, bytes, typing.Optional[int], bytes]:

@property
def is_ssl(self) -> bool:
message = 'URL.is_ssl() is pending deprecation. Use url.scheme == "https"'
warnings.warn(message, DeprecationWarning)
Copy link
Member

Choose a reason for hiding this comment

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

Note: we have a handy util in _utils.py so we don't have to remember which warning category should be passed (planning to issue a PR to update all usage to this shortly!).

Suggested change
warnings.warn(message, DeprecationWarning)
warn_deprecated(message)

@StephenBrown2
Copy link
Contributor

The decision of "is this scheme valid?" has been deferred to the transport layer - see #1080.

Yes, but that still means it's an invalid URL as far as httpx is concerned:

In [1]: import httpx

In [2]: httpx.get('ftps://localhost/test.txt')
---------------------------------------------------------------------------
InvalidURL                                Traceback (most recent call last)
<ipython-input-2-f5499711c87f> in <module>
----> 1 httpx.get('ftps://localhost/test.txt')

~/.pyenv/versions/pydantic/lib/python3.8/site-packages/httpx/_api.py in get(url, params, headers, cookies, auth, allow_redirects, cert, verify, timeout, trust_env)
    157     this function, as `GET` requests should not include a request body.
    158     """
--> 159     return request(
    160         "GET",
    161         url,

~/.pyenv/versions/pydantic/lib/python3.8/site-packages/httpx/_api.py in request(method, url, params, data, files, json, headers, cookies, auth, timeout, allow_redirects, verify, cert, trust_env)
     84         cert=cert, verify=verify, timeout=timeout, trust_env=trust_env,
     85     ) as client:
---> 86         return client.request(
     87             method=method,
     88             url=url,

~/.pyenv/versions/pydantic/lib/python3.8/site-packages/httpx/_client.py in request(self, method, url, data, files, json, params, headers, cookies, auth, allow_redirects, timeout)
    581             cookies=cookies,
    582         )
--> 583         return self.send(
    584             request, auth=auth, allow_redirects=allow_redirects, timeout=timeout,
    585         )

~/.pyenv/versions/pydantic/lib/python3.8/site-packages/httpx/_client.py in send(self, request, stream, auth, allow_redirects, timeout)
    596         if request.url.scheme not in ("http", "https"):
    597             message = 'URL scheme must be "http" or "https".'
--> 598             raise InvalidURL(message, request=request)
    599
    600         timeout = self.timeout if isinstance(timeout, UnsetType) else Timeout(timeout)

InvalidURL: URL scheme must be "http" or "https".

Or are you implying that one could make their own transport handler to deal with non-http schemes?

@florimondmanca
Copy link
Member

florimondmanca commented Aug 5, 2020

Sure, the default connection pool transport can't handle ftp:// schemes, but yes this would in theory allow someone to build an FTPTransport that does… 👀

Edit: oh, wait, I think we haven't made the switch to "defer scheme validation to transports" just yet (see how the error comes from an assertion made in _client.py). I do remember that this was a planned tweak though, just can't member where it was discussed… Edit: the switch will be made once HTTPCore 0.10 is out: #1126

@tomchristie
Copy link
Member Author

Yes, but that still means it's an invalid URL as far as httpx is concerned:

In either case the point would still stand, which is that we've got a nicer API now that httpx.URL(...) deals with URLs in a scheme-agnostic manner.

@tomchristie tomchristie merged commit 0e73be8 into master Aug 5, 2020
@tomchristie tomchristie deleted the deprecate-url-is-ssl branch August 5, 2020 18:11
@tomchristie tomchristie mentioned this pull request Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Issues and PRs related to code refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants