[8.x] Remove Intersphinx Legacy Format and Fix intersphinx cache loading in incremental builds

CHANGES.rst

@@ -77,6 +77,9 @@ Bugs fixed
 * #11925: Blacklist the ``sphinxprettysearchresults`` extension; the functionality
   it provides was merged into Sphinx v2.0.0.
   Patch by James Addison.
+* #11466: intersphinx: fix cache loading when the inventory URI is changed
+  in an incremental build.
+  Patch by Bénédikt Tran.
 
 Testing
 -------

sphinx/ext/intersphinx.py

@@ -24,6 +24,7 @@
 import re
 import sys
 import time
+from itertools import chain
 from os import path
 from typing import TYPE_CHECKING, cast
 from urllib.parse import urlsplit, urlunsplit
@@ -233,11 +234,31 @@ def fetch_inventory_group(
                     failures.append(err.args)
                     continue
                 if invdata:
+                    old_uris = set()
+                    # Find the URIs of the *existing* projects that are to
+                    # be removed because there are being overwritten.
+                    #
+                    # If the current cache contains some (project, uri) pair
+                    # say ("foo", "foo.com") and if the new intersphinx dict
+                    # contains the pair ("bar", "foo.com"), we need to remove
+                    # the first entry and use the second one.
+                    for old_uri, (old_name, _expiry, _data) in cache.items():
+                        if old_name == name:
+                            old_uris.add(old_uri)
+
+                    for old_uri in old_uris:
+                        del cache[old_uri]
+
+                    if uri in cache:
+                        # ensure that the cache data is moved to the end
+                        # when setting `cache[uri] = ...`
+                        del cache[uri]
+
                     cache[uri] = name, now, invdata
                     return True
         return False
     finally:
-        if failures == []:
+        if not failures:
             pass
         elif len(failures) < len(invs):
             logger.info(__("encountered some issues with some of the inventories,"
@@ -271,24 +292,48 @@ def load_mappings(app: Sphinx) -> None:
         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 = []
+        # other; which one will override which may vary between builds,
+        # but we can ensure using the latest inventory data.
+        #
+        # When we encounter a named inventory that already exists,
+        # this means that we had two entries for the same inventory,
+        # but with different URIs (e.g., the remote URL was updated).
+        #
+        # In particular, the newest URI is inserted in the cache *after*
+        # the one that existed from a previous build (dict are ordered
+        # by insertion time since Python 3.6).
+        #
+        # Example: assume that we use URI_A to generate the inventory of "foo",
+        # so that we have
+        #
+        #   intersphinx_cache = {URI_A: ('foo', timeout, DATA_FROM_URI1)}
+        #
+        # If we rebuild the project but change URI_A to URI_B while keeping
+        # the build directory (i.e. incremental build), the cache becomes
+        #
+        #   intersphinx_cache = {URI_A: ('foo', ..., DATA_FROM_URI_A),
+        #                        URI_B: ('foo', ..., DATA_FROM_URI_B)}
+        #
+        # So if we iterate the *values* of ``intersphinx_cache`` and not
+        # the keys, we correctly replace old (in time) inventory cache data
+        # with the same name but different URIs (and/or data). If the URI is
+        # the same or if we force-reload the cache, only the data is updated.
+        named_vals: dict[str, Inventory] = {}
+        unnamed_vals: list[tuple[str | None, Inventory]] = []
         for name, _expiry, invdata in intersphinx_cache.values():
             if name:
-                named_vals.append((name, invdata))
+                named_vals[name] = invdata
             else:
                 unnamed_vals.append((name, invdata))
-        for name, invdata in sorted(named_vals) + unnamed_vals:
+
+        # we do not sort the named inventories by their name so that two builds
+        # with same intersphinx_mapping value but ordered differently reflect
+        # the order correctly for reproducible builds
+        for name, invdata in chain(sorted(named_vals.items()), unnamed_vals):
             if name:
                 inventories.named_inventory[name] = invdata
-            for type, objects in invdata.items():
-                inventories.main_inventory.setdefault(type, {}).update(objects)
+            for objtype, objects in invdata.items():
+                inventories.main_inventory.setdefault(objtype, {}).update(objects)
 
 
 def _create_element_from_result(domain: Domain, inv_name: str | None,

sphinx/util/typing.py

@@ -87,6 +87,7 @@ def is_invalid_builtin_class(obj: Any) -> bool:
     str,  # URL
     str,  # display name
 ]
+# referencable role => (reference name => inventory item)
 Inventory = dict[str, dict[str, InventoryItem]]
 
 

tests/test_extensions/test_ext_intersphinx.py

@@ -1,6 +1,9 @@
 """Test the intersphinx extension."""
 
+from __future__ import annotations
+
 import http.server
+import zlib
 from unittest import mock
 
 import pytest
@@ -427,6 +430,81 @@ 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):
+    def make_invdata(i: int) -> bytes:
+        headers = f'''\
+# Sphinx inventory version 2
+# Project: foo
+# Version: {i}
+# The remainder of this file is compressed with zlib.
+'''.encode()
+        line = f'module{i} py:module 0 foo.html#module-$ -\n'.encode()
+        return headers + zlib.compress(line)
+
+    class InventoryHandler(http.server.BaseHTTPRequestHandler):
+        def do_GET(self):
+            self.send_response(200, 'OK')
+
+            if self.path.startswith('/old/'):
+                data = make_invdata(1)
+            elif self.path.startswith('/new/'):
+                data = make_invdata(2)
+            else:
+                data = b''
+
+            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']}
+    url_old = 'http://localhost:7777/old'
+    url_new = 'http://localhost:7777/new'
+
+    with http_server(InventoryHandler):
+        confoverrides1 = baseconfig | {'intersphinx_mapping': {'foo': (url_old, None)}}
+        app1 = make_app(*args, confoverrides=confoverrides1, **kwargs)
+        app1.build()
+        # the inventory when querying the 'old' URL
+        entry1 = {'module1': ('foo', '1', f'{url_old}/foo.html#module-module1', '-')}
+
+        assert list(app1.env.intersphinx_cache) == [url_old]
+        assert app1.env.intersphinx_cache[url_old][0] == 'foo'
+        assert app1.env.intersphinx_cache[url_old][2] == {'py:module': entry1}
+        assert app1.env.intersphinx_named_inventory == {'foo': {'py:module': entry1}}
+
+        # switch to new url and assert that the old URL is no more stored
+        confoverrides2 = baseconfig | {'intersphinx_mapping': {'foo': (url_new, None)}}
+        app2 = make_app(*args, confoverrides=confoverrides2, **kwargs)
+        app2.build()
+        entry2 = {'module2': ('foo', '2', f'{url_new}/foo.html#module-module2', '-')}
+
+        assert list(app2.env.intersphinx_cache) == [url_new]
+        assert app2.env.intersphinx_cache[url_new][0] == 'foo'
+        assert app2.env.intersphinx_cache[url_new][2] == {'py:module': entry2}
+        assert app2.env.intersphinx_named_inventory == {'foo': {'py:module': entry2}}
+
+        # switch back to old url
+        confoverrides3 = baseconfig | {'intersphinx_mapping': {'foo': (url_old, None)}}
+        app3 = make_app(*args, confoverrides=confoverrides3, **kwargs)
+        app3.build()
+        # same as entry 1
+        entry3 = {'module1': ('foo', '1', f'{url_old}/foo.html#module-module1', '-')}
+
+        assert list(app3.env.intersphinx_cache) == [url_old]
+        assert app3.env.intersphinx_cache[url_old][0] == 'foo'
+        assert app3.env.intersphinx_cache[url_old][2] == {'py:module': entry3}
+        assert app3.env.intersphinx_named_inventory == {'foo': {'py:module': entry3}}
+
+
 class TestStripBasicAuth:
     """Tests for sphinx.ext.intersphinx._strip_basic_auth()"""