Make URL from extract_link safe and add missing scheme #19
Conversation
|
Hi @wRAR please take a look at the suggested fix. |
Co-authored-by: Andrey Rakhmatullin <[email protected]>
|
Hi @lopuhin , could you also take a look? |
zyte_parsers/utils.py
Outdated
| # add scheme (https) when missing schema and no base url | ||
| safe_link = add_https_to_url(safe_link) |
There was a problem hiding this comment.
Super minor: I think "no base url" part of the comment does not quite apply, as this can have effect even if base URL is supplied. Also if we take that away we'd just repeat the function name so maybe we could remove the comment completely.
There was a problem hiding this comment.
Ah, in case of existing base_url this link gets scheme from base_url here strip_urljoin.
I just wanted to point out that this append scheme is kind of an add-on to strip_urljoin.
Do you think it's better to remove this to avoid confusion?
UPDATE: For the simplicity I removed this function.
|
|
||
|
|
||
| def extract_link( | ||
| a_node: SelectorOrElement, base_url: str, force_safe: bool = False |
There was a problem hiding this comment.
As I understand you're adding force_safe to make new behavior optional, I wonder if it's possible to apply that unconditionally to keep API simpler, or there are any cases where you are worried it can break working code?
There was a problem hiding this comment.
Yes, actually I don't know where and in what cases this feature is used and decided to add it in such a way to prevent any unpredictable problems. And also for the cases if someone wants to use this without changing the link at all.
zyte_parsers/utils.py
Outdated
| def add_https_to_url(url: str) -> str: | ||
| if url.startswith(("http://", "https://")): | ||
| return url | ||
|
|
||
| parsed_url = urlparse(url) | ||
| if not parsed_url.scheme and parsed_url.netloc: | ||
| parsed_url = parsed_url._replace(scheme="https") | ||
|
|
||
| return str(urlunparse(parsed_url)) |
There was a problem hiding this comment.
Hm, why do we need this function? If a URL is written without http or https, it won't work anyways in the browser, right (unless it's a relative URL, which uses different rules - adding http/https is wrong for them)?
If so, why are we adding the missing schema? My concern is that it'd change some invalid URLs to URLs which look valid, but which are not.
There was a problem hiding this comment.
Hm, or is it the // example, i.e. the relative schema ("use whatever protocol the page is using")? In this case, adding https could be not 100% precise, as it should be the protocol of the web page itself.
There was a problem hiding this comment.
Yes, the idea is to handle such cases like //example.com when base_url here https://github.com/zytedata/zyte-parsers/pull/19/files#diff-0db3ea2158d725e2de6295c907fbe44928efc0c94057ef6ca85dbbee4c733819R83 is not provided.
But this function doesn't seem to be a very important feature at the moment and we can easily ignore it.
UPDATE: For the simplicity I removed this function.
There was a problem hiding this comment.
Thanks @PyExplorer looks good to me. Perhaps we could make this default eventually, but it's better to have it here and it's nice that we have more tests now.
Problem:
In some cases, the
extract_linkfunction returns links that are not fully valid or not entirely safe to use in requests.Suggested fix:
There is an additional function that makes the link safe using the safe_url_string function, and also adds a scheme for cases where we have a link that looks valid but lacks a scheme and base URL. (e.g.
//example.com)