Skip to content

RequestUrl and ResponseUrl using yarl #45

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.

Sign up for GitHub

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

Open
Parent: RequestURL and ResponseURL
wants to merge 9 commits into
base: url-page-inputs
Choose a base branch
from
Open

RequestUrl and ResponseUrl using yarl #45

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
71bb150
use yarl underneath ResponseURL and RequestURL
BurnzZ Jun 1, 2022
b3c7a0a
fix naming, annotations, and cleanup files
BurnzZ Jun 1, 2022
2ebf7a3
Update the __repr__ of _Url class
BurnzZ Jun 1, 2022
b658ab5
update the internal yarl.URL reference to be private
BurnzZ Jun 1, 2022
be37f39
expose 'encoded' parameter in _Url class
BurnzZ Jun 1, 2022
4e0e103
revert dunder private attribute in _Url
BurnzZ Jun 1, 2022
292a3b4
handle equality on the base url
BurnzZ Jun 1, 2022
912ba77
prevent str and _Url instance when having the same value
BurnzZ Jun 3, 2022
d869024
fix type annotation on _Url init
BurnzZ Jun 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
'url-matcher',
'multidict',
'w3lib >= 1.22.0',
'yarl',
],
classifiers=[
'Development Status :: 2 - Pre-Alpha',
Expand Down
18 changes: 18 additions & 0 deletions tests/test_page_inputs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

import parsel
from web_poet.page_inputs import (
RequestUrl,
ResponseUrl,
HttpRequest,
HttpResponse,
HttpRequestBody,
Expand All @@ -16,6 +18,22 @@
)


@pytest.mark.parametrize("cls", [RequestUrl, ResponseUrl])
def test_url(cls):
url_value = "https://example.com/category/product?query=123&id=xyz#frag1"

url = cls(url_value)

assert str(url) == url_value
assert url.scheme == "https"
assert url.host == "example.com"
assert url.path == "/category/product"
assert url.query_string == "query=123&id=xyz"
assert url.fragment == "frag1"

new_url = cls(url)


@pytest.mark.parametrize("body_cls", [HttpRequestBody, HttpResponseBody])
def test_http_body_hashable(body_cls):
http_body = body_cls(b"content")
Expand Down
4 changes: 2 additions & 2 deletions web_poet/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
HttpRequestBody,
HttpResponseBody,
Meta,
RequestURL,
ResponseURL,
RequestUrl,
ResponseUrl,
)
from .overrides import PageObjectRegistry, consume_modules, OverrideRule

Expand Down
2 changes: 1 addition & 1 deletion web_poet/mixins.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ def base_url(self) -> str:
# FIXME: move it to HttpResponse
if self._cached_base_url is None:
text = self.html[:4096]
self._cached_base_url = get_base_url(text, self.url)
self._cached_base_url = get_base_url(text, str(self.url))
return self._cached_base_url

def urljoin(self, url: str) -> str:
Expand Down
4 changes: 2 additions & 2 deletions web_poet/page_inputs/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
HttpResponseHeaders,
HttpRequestBody,
HttpResponseBody,
RequestURL,
ResponseURL
RequestUrl,
ResponseUrl
)
from .browser import BrowserHtml
43 changes: 39 additions & 4 deletions web_poet/page_inputs/http.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
http_content_type_encoding
)

import yarl
from web_poet._base import _HttpHeaders
from web_poet.utils import memoizemethod_noargs
from web_poet.mixins import SelectableMixin
Expand All @@ -18,12 +19,46 @@
_AnyStrDict = Dict[AnyStr, Union[AnyStr, List[AnyStr], Tuple[AnyStr, ...]]]


class ResponseURL(str):
class _Url:
def __init__(self, url: Union[str, yarl.URL]):
self._url = yarl.URL(str(url))

def __str__(self) -> str:
return str(self._url)

def __repr__(self) -> str:
return str(self._url)

def __eq__(self, other) -> bool:
return str(self._url) == str(other)
Copy link
Member

@Gallaecio Gallaecio Jun 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We should probably make sure we implement this in line with our API expectations.

Mind the following:

>>> example_url_1 = yarl.URL("https://example.com")
>>> example_url_2 = yarl.URL("https://example.com/")
>>> example_url_1 == example_url_2
True
>>> str(example_url_1) == str(example_url_2)
False

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's a good point. Although I'm not quite sure if we should have the presence of a trailing / dictates an equality though.

For example, some sites redirect URLs like "https://example.com/" into "https://example.com". For efficiency, the trailing "/" needs to be stripped to prevent an extra request from being wasted due to redirections.

Because of the different behaviors/expectations, such URLs cannot really be equal as they don't point at the same resource (without the redirections).

Copy link
Member

@Gallaecio Gallaecio Jun 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

While that is true for https://example.com/foo and https://example.com/foo/, the case of https://example.com and https://example.com/ is special, since the path of a URL, when not specified, is /.

>>> URL('https://example.com').path
'/'
>>> URL('https://example.com/').path
'/'
>>> URL('https://example.com/foo').path
'/foo'
>>> URL('https://example.com/foo/').path
'/foo/'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhh thanks for making the distinction clear! Handled this on 292a3b4.


Copy link
Contributor Author

Choose a reason for hiding this comment

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

The property methods below can be added mapped dynamically with yarl's. However, we lose the benefit of defining docstrings within them.

Copy link
Member

@Gallaecio Gallaecio Jun 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We would also lose API governance, so +1 to manual definition.

However, I wonder if we should define them at all for the initial implementation. We want to make sure we get the API right encoding-wise, and if we expose a part of the Yarl interface already as is, I imagine we are introducing the encoding issue in our implementation, with the caveat of not supporting encoded=True in __init__ to at least prevent Yarl from messing things up.

Maybe the initial implementation should use a string internally instead, and we can convert it into Yarl later.

Copy link
Contributor Author

@BurnzZ BurnzZ Jun 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

A good point about handling the encoding. What do you think about setting encoded=False by default to prevent yarl from messing things up due to incorrect encoding? be37f39. This would be equivalent to having a str internally, aside from the "smart" helper methods.

Copy link
Member

@Gallaecio Gallaecio Jun 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The thing is, Yarl exposes encoded in __init__ as a workaround for proper encoding handling, which they set off not to implement. But I believe what @kmike has in mind is for us to have a URL class that does proper encoding handling, in which case we should probably not expose encoded at all (maybe encoding instead, defaulting to "utf8").

I would wait for feedback from @kmike before making more API decisions. I am personally not sure of the best approach here, what parts of w3lib.url we want to apply and how.

Copy link
Member

Choose a reason for hiding this comment

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

I think the goal shouldn't be to implement a general URL class; the goal is to implement URL class useful for web scraping.

If that's hard to use yarl or other library's URL class directly, and we're defining the API anyways, we probably should think about it from the API point of view: what's the API we want, what are the features commonly used in web scraping? After figuring out how we'd like API to look like, we can see what's the best way to implement it - wrap yarl, wrap w3lib, do something else.

Based on our previous discussions, I think a scraping-ready URL class should have:

  • a way to manipulate query string: add/remove/get/update query parameters
  • some kind of urljoin method, probably via / operation
  • probably - a way to extract the domain name?
  • anything else?

In addition to this, there is whole bunch of questions about encoding, normalization, converting URLs to ascii-only encoded strings suitable for downloading, etc. The best API to handle all that might require some thought. I wonder if we can side-step it for now somehow.

At the same time, I'm not sure properties like .scheme are that essential. They're are essential for a general-purpose URL class, but are people who write scraping code commonly parse URLs to get their scheme? We can add such methods and properties for sure, but we can do it later. These methods are probably useful for authors of web scraping frameworks / http clients, but less so for people who write web scraping code.

Copy link
Member

Choose a reason for hiding this comment

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

The property methods below can be added mapped dynamically with yarl's. However, we lose the benefit of defining docstrings within them.

It's also about return types - some yarl.URL methods are going to return yarl.URL objects, while here it would make more sense to return _Url objects.

@property
def scheme(self) -> str:
return self._url.scheme

@property
def host(self) -> Optional[str]:
return self._url.host

@property
def path(self) -> str:
return self._url.path

@property
def query_string(self) -> str:
return self._url.query_string

@property
def fragment(self) -> str:
return self._url.fragment


class ResponseUrl(_Url):
""" URL of the response """
pass


class RequestURL(str):
class RequestUrl(_Url):
""" URL of the request """
pass

Expand Down Expand Up @@ -162,7 +197,7 @@ class HttpRequest:
**web-poet** like :class:`~.HttpClient`.
"""

url: RequestURL = attrs.field(converter=RequestURL)
url: RequestUrl = attrs.field(converter=RequestUrl)
method: str = attrs.field(default="GET", kw_only=True)
headers: HttpRequestHeaders = attrs.field(
factory=HttpRequestHeaders, converter=HttpRequestHeaders, kw_only=True
Expand Down Expand Up @@ -195,7 +230,7 @@ class HttpResponse(SelectableMixin):
is auto-detected from headers and body content.
"""

url: ResponseURL = attrs.field(converter=ResponseURL)
url: ResponseUrl = attrs.field(converter=ResponseUrl)
body: HttpResponseBody = attrs.field(converter=HttpResponseBody)
status: Optional[int] = attrs.field(default=None, kw_only=True)
headers: HttpResponseHeaders = attrs.field(factory=HttpResponseHeaders,
Expand Down