Fix intersphinx cache handling

sphinx/ext/intersphinx/_load.py

@@ -6,6 +6,7 @@
 import functools
 import posixpath
 import time
+from operator import itemgetter
 from os import path
 from typing import TYPE_CHECKING
 from urllib.parse import urlsplit, urlunsplit
@@ -138,36 +139,38 @@ def load_mappings(app: Sphinx) -> None:
     intersphinx_cache: dict[InventoryURI, InventoryCacheEntry] = inventories.cache
     intersphinx_mapping: IntersphinxMapping = app.config.intersphinx_mapping
 
+    expected_uris = {uri for _name, (uri, _invs) in intersphinx_mapping.values()}
+
+    # If the current cache contains some (project, uri) pair
+    # say ("foo", "foo.com") and if the new intersphinx dict
+    # contains the pair ("foo", "bar.com"), we need to remove
+    # the ("foo", "foo.com") entry and use ("foo", "bar.com").
+    for uri in frozenset(intersphinx_cache):
+        if intersphinx_cache[uri][0] not in intersphinx_mapping or uri not in expected_uris:
+            # remove a cached inventory if the latter is no more used by intersphinx
+            del intersphinx_cache[uri]
+
     with concurrent.futures.ThreadPoolExecutor() as pool:
-        futures = []
-        for name, (uri, invs) in intersphinx_mapping.values():
-            futures.append(pool.submit(
-                fetch_inventory_group, name, uri, invs, intersphinx_cache, app, now,
-            ))
+        futures = [
+            pool.submit(fetch_inventory_group, name, uri, invs, intersphinx_cache, app, now)
+            for name, (uri, invs) in app.config.intersphinx_mapping.values()
+        ]
         updated = [f.result() for f in concurrent.futures.as_completed(futures)]
 
     if any(updated):
+        # clear the local inventories
         inventories.clear()
 
         # Duplicate values in different inventories will shadow each
-        # other; which one will override which can vary between builds
-        # since they are specified using an unordered dict.  To make
-        # it more consistent, we sort the named inventories and then
-        # add the unnamed inventories last.  This means that the
-        # unnamed inventories will shadow the named ones but the named
-        # ones can still be accessed when the name is specified.
-        named_vals = []
-        unnamed_vals = []
-        for name, _expiry, invdata in intersphinx_cache.values():
-            if name:
-                named_vals.append((name, invdata))
-            else:
-                unnamed_vals.append((name, invdata))
-        for name, invdata in sorted(named_vals) + unnamed_vals:
-            if name:
-                inventories.named_inventory[name] = invdata
-            for type, objects in invdata.items():
-                inventories.main_inventory.setdefault(type, {}).update(objects)
+        # other and which one will override which varies between builds.
+        #
+        # We can however order the cache by (NAME, EXPIRY) for reproducibility.
+        by_name_and_time = itemgetter(0, 1)  # 0: name, 1: expiry
+        cache_values = sorted(intersphinx_cache.values(), key=by_name_and_time)
+        for name, _expiry, invdata in cache_values:
+            inventories.named_inventory[name] = invdata
+            for objtype, objects in invdata.items():
+                inventories.main_inventory.setdefault(objtype, {}).update(objects)
 
 
 def fetch_inventory_group(
@@ -179,39 +182,43 @@ def fetch_inventory_group(
     now: int,
 ) -> bool:
     cache_time = now - app.config.intersphinx_cache_limit * 86400
+
+    updated = False
     failures = []
-    try:
-        for inv in invs:
-            if not inv:
-                inv = posixpath.join(uri, INVENTORY_FILENAME)
-            # decide whether the inventory must be read: always read local
-            # files; remote ones only if the cache time is expired
-            if '://' not in inv or uri not in cache or cache[uri][1] < cache_time:
-                safe_inv_url = _get_safe_url(inv)
-                inv_descriptor = name or 'main_inventory'
-                LOGGER.info(__("loading intersphinx inventory '%s' from %s..."),
-                            inv_descriptor, safe_inv_url)
-                try:
-                    invdata = fetch_inventory(app, uri, inv)
-                except Exception as err:
-                    failures.append(err.args)
-                    continue
-                if invdata:
-                    cache[uri] = name, now, invdata
-                    return True
-        return False
-    finally:
-        if not failures:
-            pass
-        elif len(failures) < len(invs):
-            LOGGER.info(__('encountered some issues with some of the inventories,'
-                           ' but they had working alternatives:'))
-            for fail in failures:
-                LOGGER.info(*fail)
-        else:
-            issues = '\n'.join(f[0] % f[1:] for f in failures)
-            LOGGER.warning(__('failed to reach any of the inventories '
-                              'with the following issues:') + '\n' + issues)
+
+    for location in invs:
+        inv: str = location or posixpath.join(uri, INVENTORY_FILENAME)
+        # decide whether the inventory must be read: always read local
+        # files; remote ones only if the cache time is expired
+        if '://' not in inv or uri not in cache or cache[uri][1] < cache_time:
+            safe_inv_url = _get_safe_url(inv)
+            inv_descriptor = name or 'main_inventory'
+            LOGGER.info(__("loading intersphinx inventory '%s' from %s..."),
+                        inv_descriptor, safe_inv_url)
+
+            try:
+                invdata = fetch_inventory(app, uri, inv)
+            except Exception as err:
+                failures.append(err.args)
+                continue
+
+            if invdata:
+                cache[uri] = name, now, invdata
+                updated = True
+                break
+
+    if not failures:
+        pass
+    elif len(failures) < len(invs):
+        LOGGER.info(__("encountered some issues with some of the inventories,"
+                       " but they had working alternatives:"))
+        for fail in failures:
+            LOGGER.info(*fail)
+    else:
+        issues = '\n'.join(f[0] % f[1:] for f in failures)
+        LOGGER.warning(__("failed to reach any of the inventories "
+                          "with the following issues:") + "\n" + issues)
+    return updated
 
 
 def fetch_inventory(app: Sphinx, uri: InventoryURI, inv: str) -> Inventory:

tests/test_extensions/test_ext_intersphinx.py

@@ -3,6 +3,10 @@
 from __future__ import annotations
 
 import http.server
+import posixpath
+import re
+import zlib
+from io import BytesIO
 from typing import TYPE_CHECKING
 from unittest import mock
 
@@ -31,7 +35,125 @@
 from tests.utils import http_server
 
 if TYPE_CHECKING:
-    from typing import NoReturn
+    from collections.abc import Iterable
+    from typing import BinaryIO, NoReturn
+
+    from sphinx.util.typing import InventoryItem
+
+
+class InventoryEntry:
+    __slots__ = (
+        'name', 'display_name', 'domain_name', 'object_type', 'uri', 'anchor', 'priority',
+    )
+
+    def __init__(
+        self,
+        name: str = 'this',
+        *,
+        display_name: str | None = None,
+        domain_name: str = 'py',
+        object_type: str = 'obj',
+        uri: str = 'index.html',
+        anchor: str = '',
+        priority: int = 0,
+    ):
+        if anchor.endswith(name):
+            anchor = anchor[:-len(name)] + '$'
+
+        if anchor:
+            uri += '#' + anchor
+
+        if display_name is None or display_name == name:
+            display_name = '-'
+
+        self.name = name
+        self.display_name = display_name
+        self.domain_name = domain_name
+        self.object_type = object_type
+        self.uri = uri
+        self.anchor = anchor
+        self.priority = priority
+
+    def format(self) -> str:
+        return (f'{self.name} {self.domain_name}:{self.object_type} '
+                f'{self.priority} {self.uri} {self.display_name}\n')
+
+    def __repr__(self) -> str:
+        fields = (f'{attr}={getattr(self, attr)!r}' for attr in self.__slots__)
+        return f"{self.__class__.__name__}({', '.join(fields)})"
+
+
+class IntersphinxProject:
+    def __init__(
+        self,
+        name: str = 'foo',
+        version: str | int = 1,
+        baseurl: str = '',
+        baseuri: str = '',
+        file: str | None = None,
+    ) -> None:
+        #: The project name.
+        self.name = name
+        #: The escaped project name.
+        self.safe_name = re.sub(r'\\s+', ' ', name)
+
+        #: The project version as a string.
+        self.version = version = str(version)
+        #: The escaped project version.
+        self.safe_version = re.sub(r'\\s+', ' ', version)
+
+        #: The project base URL (e.g., http://localhost:7777).
+        self.baseurl = baseurl
+        #: The project base URI, relative to *baseurl* (e.g., 'foo').
+        self.uri = baseuri
+        #: The project URL, as specified in :confval:`intersphinx_mapping`.
+        self.url = posixpath.join(baseurl, baseuri)
+        #: The project local file, if any.
+        self.file = file
+
+    @property
+    def record(self) -> dict[str, tuple[str | None, str | None]]:
+        """The :confval:`intersphinx_mapping` record for this project."""
+        return {self.name: (self.url, self.file)}
+
+    def normalize(self, entry: InventoryEntry) -> tuple[str, InventoryItem]:
+        url = posixpath.join(self.url, entry.uri)
+        return entry.name, (self.safe_name, self.safe_version, url, entry.display_name)
+
+
+class FakeInventory:
+    protocol_version: int
+
+    def __init__(self, project: IntersphinxProject | None = None) -> None:
+        self.project = project or IntersphinxProject()
+
+    def serialize(self, entries: Iterable[InventoryEntry] | None = None) -> bytes:
+        buffer = BytesIO()
+        self._write_headers(buffer)
+        entries = entries or [InventoryEntry()]
+        self._write_body(buffer, (item.format().encode() for item in entries))
+        return buffer.getvalue()
+
+    def _write_headers(self, buffer: BinaryIO) -> None:
+        buffer.write((f'# Sphinx inventory version {self.protocol_version}\n'
+                      f'# Project: {self.project.safe_name}\n'
+                      f'# Version: {self.project.safe_version}\n').encode())
+
+    def _write_body(self, buffer: BinaryIO, lines: Iterable[bytes]) -> None:
+        raise NotImplementedError
+
+
+class FakeInventoryV2(FakeInventory):
+    protocol_version = 2
+
+    def _write_headers(self, buffer: BinaryIO) -> None:
+        super()._write_headers(buffer)
+        buffer.write(b'# The remainder of this file is compressed using zlib.\n')
+
+    def _write_body(self, buffer: BinaryIO, lines: Iterable[bytes]) -> None:
+        compressor = zlib.compressobj(9)
+        buffer.writelines(map(compressor.compress, lines))
+        buffer.write(compressor.flush())
 
 
 class FakeList(list):
@@ -502,6 +624,92 @@ def test_load_mappings_fallback(tmp_path, app, status, warning):
     assert isinstance(rn, nodes.reference)
 
 
+@pytest.mark.sphinx('dummy', testroot='basic')
+def test_load_mappings_cache_update(make_app, app_params):
+    ITEM_NAME = 'bar'
+    DOMAIN_NAME = 'py'
+    OBJECT_TYPE = 'module'
+    REFTYPE = f'{DOMAIN_NAME}:{OBJECT_TYPE}'
+    PORT = 7777  # needd since otherwise it's an automatic port
+
+    PROJECT_NAME, PROJECT_BASEURL = 'foo', f'http://localhost:{PORT}'
+    old_project = IntersphinxProject(PROJECT_NAME, 1337, PROJECT_BASEURL, 'old')
+    assert old_project.name == PROJECT_NAME
+    assert old_project.url == f'http://localhost:{PORT}/old'
+
+    new_project = IntersphinxProject(PROJECT_NAME, 1701, PROJECT_BASEURL, 'new')
+    assert new_project.name == PROJECT_NAME
+    assert new_project.url == f'http://localhost:{PORT}/new'
+
+    def make_entry(project: IntersphinxProject) -> InventoryEntry:
+        name = f'{ITEM_NAME}_{project.version}'
+        return InventoryEntry(name, domain_name=DOMAIN_NAME, object_type=OBJECT_TYPE)
+
+    def make_invdata(project: IntersphinxProject) -> bytes:
+        return FakeInventoryV2(project).serialize([make_entry(project)])
+
+    class InventoryHandler(http.server.BaseHTTPRequestHandler):
+        def do_GET(self):
+            self.send_response(200, 'OK')
+
+            data = b''
+            for project in (old_project, new_project):
+                if self.path.startswith(f'/{project.uri}/'):
+                    data = make_invdata(project)
+                    break
+
+            self.send_header('Content-Length', str(len(data)))
+            self.end_headers()
+            self.wfile.write(data)
+
+        def log_message(*args, **kwargs):
+            pass
+
+    # clean build
+    args, kwargs = app_params
+    _ = make_app(*args, freshenv=True, **kwargs)
+    _.build()
+
+    baseconfig = {'extensions': ['sphinx.ext.intersphinx']}
+
+    with http_server(InventoryHandler, port=PORT):
+        confoverrides1 = baseconfig | {'intersphinx_mapping': old_project.record}
+        app1 = make_app(*args, confoverrides=confoverrides1, **kwargs)
+        app1.build()
+
+        # the inventory when querying the 'old' URL
+        old_entry = make_entry(old_project)
+        old_item = dict([old_project.normalize(old_entry)])
+
+        assert list(app1.env.intersphinx_cache) == [old_project.url]
+        assert app1.env.intersphinx_cache[old_project.url][0] == old_project.name
+        assert app1.env.intersphinx_cache[old_project.url][2] == {REFTYPE: old_item}
+        assert app1.env.intersphinx_named_inventory == {old_project.name: {REFTYPE: old_item}}
+
+        # switch to new url and assert that the old URL is no more stored
+        confoverrides2 = baseconfig | {'intersphinx_mapping': new_project.record}
+        app2 = make_app(*args, confoverrides=confoverrides2, **kwargs)
+        app2.build()
+
+        new_entry = make_entry(new_project)
+        new_item = dict([new_project.normalize(new_entry)])
+
+        assert list(app2.env.intersphinx_cache) == [new_project.url]
+        assert app2.env.intersphinx_cache[new_project.url][0] == PROJECT_NAME
+        assert app2.env.intersphinx_cache[new_project.url][2] == {REFTYPE: new_item}
+        assert app2.env.intersphinx_named_inventory == {PROJECT_NAME: {REFTYPE: new_item}}
+
+        # switch back to old url (re-use 'old_item')
+        confoverrides3 = baseconfig | {'intersphinx_mapping': old_project.record}
+        app3 = make_app(*args, confoverrides=confoverrides3, **kwargs)
+        app3.build()
+
+        assert list(app3.env.intersphinx_cache) == [old_project.url]
+        assert app3.env.intersphinx_cache[old_project.url][0] == PROJECT_NAME
+        assert app3.env.intersphinx_cache[old_project.url][2] == {REFTYPE: old_item}
+        assert app3.env.intersphinx_named_inventory == {PROJECT_NAME: {REFTYPE: old_item}}
+
+
 class TestStripBasicAuth:
     """Tests for sphinx.ext.intersphinx._strip_basic_auth()"""