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

RequestUrl and ResponseUrl using yarl #45

Open
wants to merge 9 commits into
base: url-page-inputs
Choose a base branch
from
1 change: 1 addition & 0 deletions setup.py
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
29 changes: 29 additions & 0 deletions tests/test_page_inputs.py
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,33 @@
)


@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("cls", [RequestUrl, ResponseUrl])
def test_url_encoding(cls):
url_value = "http://εμπορικόσήμα.eu/путь/這裡"

url = cls(url_value)
str(url) == url_value

url = cls(url_value, encoded=False)
str(url) == "http://xn--jxagkqfkduily1i.eu/%D0%BF%D1%83%D1%82%D1%8C/%E9%80%99%E8%A3%A1"


@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
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
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
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
59 changes: 53 additions & 6 deletions web_poet/page_inputs/http.py
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,13 +19,59 @@
_AnyStrDict = Dict[AnyStr, Union[AnyStr, List[AnyStr], Tuple[AnyStr, ...]]]


class ResponseURL(str):
""" URL of the response """
class _Url:
def __init__(self, url: Union[str, yarl.URL], encoded=True):
Copy link
Member

Choose a reason for hiding this comment

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

url: Union[str, yarl.URL] - this type annotation doesn't allow to create RequestUrl from RequestUrl or from ResponseUrl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! Fixed it in d869024.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot to annotate the other methods in other modules. In any case, we'd be rebasing to #42 if we should proceed with this route. This would include the annotations like 5b0e889#diff-e91005d8dfc7a61689963aafad8b742554243e973f4943ec394cab425bbb61f6R81.

self.__url = yarl.URL(str(url), encoded=encoded)

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

def __repr__(self) -> str:
return f'{type(self).__name__}({str(self.__url)!r})'

def __eq__(self, other) -> bool:
return str(self.__url) == str(other)

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

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

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

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

:param url: a string representation of a URL.
:param encoded: If set to False, the given ``url`` would be auto-encoded.
However, there's no guarantee that correct encoding is used. Thus,
it's recommended to set this in the *default* ``False`` value.
"""
pass


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

:param url: a string representation of a URL.
:param encoded: If set to False, the given ``url`` would be auto-encoded.
However, there's no guarantee that correct encoding is used. Thus,
it's recommended to set this in the *default* ``False`` value.
"""
pass


Expand Down Expand Up @@ -162,7 +209,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 +242,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