Skip to content

fix: Don't compute relative URLs of already relative ones#15

Merged
pawamoy merged 1 commit intomasterfrom
fix-item-url
Mar 6, 2022
Merged

fix: Don't compute relative URLs of already relative ones#15
pawamoy merged 1 commit intomasterfrom
fix-item-url

Conversation

@pawamoy
Copy link
Copy Markdown
Member

@pawamoy pawamoy commented Feb 26, 2022

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:

  1. 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)
  1. 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.

  2. 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

@pawamoy pawamoy requested a review from oprypin February 26, 2022 23:01
@pawamoy
Copy link
Copy Markdown
Member Author

pawamoy commented Mar 6, 2022

@oprypin I intend to merge this in 2-3 days.

Copy link
Copy Markdown
Member

@oprypin oprypin left a comment

Choose a reason for hiding this comment

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

Seems good

Co-authored-by: Oleh Prypin <oleh@pryp.in>
@pawamoy pawamoy merged commit f6b861c into master Mar 6, 2022
@pawamoy pawamoy deleted the fix-item-url branch November 26, 2024 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants