In the last changes I made, I wanted to optimize things a bit as to avoid recomputing URLs again and again by interrogating handlers. But I also introduced a bug: URLs are recorded, but are always made relative. So an already relative URL would be stored, and made relative again, which made it incorrect.
We have three ways to fix this:
simply don't record URLs:
with contextlib.suppress(KeyError):
- url = self.get_item_url(new_identifier, from_url)
- self._url_map[identifier] = url # update the map to avoid doing all this again
- return url
+ return self.get_item_url(new_identifier, from_url)
split the function in two so that URLs are made relative outside of the recursive function, where URLs are recorded: the change in this PR. Note that the fallback mechanism is able to return an absolute URL (from a loaded inventory). This PR keeps that behavior but maybe that's not something we want (I can't a find a use-case where a handler returns an identifier that is found in an inventory). We must check if an URL is absolute to avoid trying to make it relative.
same as 2 but absolute URLs (from inventories) are never picked up when falling back. We can get rid of the "is absolute?" check:
def _get_item_url( # noqa: WPS234
self,
identifier: str,
fallback: Optional[Callable[[str], Sequence[str]]] = None,
) -> str:
try:
return self._url_map[identifier]
except KeyError:
if fallback:
new_identifiers = fallback(identifier)
for new_identifier in new_identifiers:
with contextlib.suppress(KeyError):
url = self._get_item_url(new_identifier)
self._url_map[identifier] = url
return url
raise
def get_item_url( # noqa: WPS234
self,
identifier: str,
from_url: Optional[str] = None,
fallback: Optional[Callable[[str], Sequence[str]]] = None,
) -> str:
try:
url = self._get_item_url(identifier, fallback)
except KeyError:
if identifier in self._abs_url_map:
return self._abs_url_map[identifier]
raise
if from_url is not None:
return relative_url(from_url, url)
return url
In the last changes I made, I wanted to optimize things a bit as to avoid recomputing URLs again and again by interrogating handlers. But I also introduced a bug: URLs are recorded, but are always made relative. So an already relative URL would be stored, and made relative again, which made it incorrect.
We have three ways to fix this:
split the function in two so that URLs are made relative outside of the recursive function, where URLs are recorded: the change in this PR. Note that the fallback mechanism is able to return an absolute URL (from a loaded inventory). This PR keeps that behavior but maybe that's not something we want (I can't a find a use-case where a handler returns an identifier that is found in an inventory). We must check if an URL is absolute to avoid trying to make it relative.
same as 2 but absolute URLs (from inventories) are never picked up when falling back. We can get rid of the "is absolute?" check: