Add a file-based cache for remote intersphinx inventories (#13684)

Author: AA-Turner

CHANGES.rst

@@ -54,6 +54,10 @@ Features added
   Patch by Adam Turner.
 * #13647: LaTeX: allow more cases of table nesting.
   Patch by Jean-François B.
+* #13684: intersphinx: Add a file-based cache for remote inventories.
+  The location of the cache directory must not be relied upon externally,
+  as it may change without notice or warning in future releases.
+  Patch by Adam Turner.
 
 Bugs fixed
 ----------

sphinx/ext/intersphinx/_cli.py

@@ -5,7 +5,11 @@
 import sys
 from pathlib import Path
 
-from sphinx.ext.intersphinx._load import _fetch_inventory, _InvConfig
+from sphinx.ext.intersphinx._load import (
+    _fetch_inventory_data,
+    _InvConfig,
+    _load_inventory,
+)
 
 
 def inspect_main(argv: list[str], /) -> int:
@@ -28,12 +32,14 @@ def inspect_main(argv: list[str], /) -> int:
     )
 
     try:
-        inv = _fetch_inventory(
+        raw_data, _ = _fetch_inventory_data(
             target_uri='',
             inv_location=filename,
             config=config,
             srcdir=Path(),
+            cache_path=None,
         )
+        inv = _load_inventory(raw_data, target_uri='')
         for key in sorted(inv.data):
             print(key)
             inv_entries = sorted(inv.data[key].items())

sphinx/ext/intersphinx/_load.py

@@ -181,6 +181,9 @@ def load_mappings(app: Sphinx) -> None:
                 now=now,
                 config=inv_config,
                 srcdir=app.srcdir,
+                # the location of this cache directory must not be relied upon
+                # externally, it may change without notice or warning.
+                cache_dir=app.doctreedir / '__intersphinx_cache__',
             )
             for project in projects
         ]
@@ -230,6 +233,7 @@ def _fetch_inventory_group(
     now: int,
     config: _InvConfig,
     srcdir: Path,
+    cache_dir: Path | None,
 ) -> bool:
     if config.intersphinx_cache_limit >= 0:
         # Positive value: cache is expired if its timestamp is below
@@ -250,6 +254,25 @@ def _fetch_inventory_group(
         else:
             inv_location = location
 
+        if cache_dir is not None:
+            cache_path = cache_dir / f'{project.name}_{INVENTORY_FILENAME}'
+        else:
+            cache_path = None
+
+        if (
+            cache_path is not None
+            and '://' in inv_location
+            and project.target_uri not in cache
+            and cache_path.is_file()
+            # the saved 'objects.inv' is not older than the cache expiry time
+            and cache_path.stat().st_mtime >= cache_time
+        ):
+            raw_data = cache_path.read_bytes()
+            inv = _load_inventory(raw_data, target_uri=project.target_uri)
+            cache_path_mtime = int(cache_path.stat().st_mtime)
+            cache[project.target_uri] = project.name, cache_path_mtime, inv.data
+            break
+
         # decide whether the inventory must be read: always read local
         # files; remote ones only if the cache time is expired
         if (
@@ -264,17 +287,18 @@ def _fetch_inventory_group(
             )
 
             try:
-                inv = _fetch_inventory(
+                raw_data, target_uri = _fetch_inventory_data(
                     target_uri=project.target_uri,
                     inv_location=inv_location,
                     config=config,
                     srcdir=srcdir,
+                    cache_path=cache_path,
                 )
+                inv = _load_inventory(raw_data, target_uri=target_uri)
             except Exception as err:
                 failures.append(err.args)
                 continue
-
-            if inv:
+            else:
                 cache[project.target_uri] = project.name, now, inv.data
                 updated = True
                 break
@@ -302,18 +326,25 @@ def _fetch_inventory_group(
 
 def fetch_inventory(app: Sphinx, uri: InventoryURI, inv: str) -> Inventory:
     """Fetch, parse and return an intersphinx inventory file."""
-    return _fetch_inventory(
+    raw_data, uri = _fetch_inventory_data(
         target_uri=uri,
         inv_location=inv,
         config=_InvConfig.from_config(app.config),
         srcdir=app.srcdir,
-    ).data
+        cache_path=None,
+    )
+    return _load_inventory(raw_data, target_uri=uri).data
 
 
-def _fetch_inventory(
-    *, target_uri: InventoryURI, inv_location: str, config: _InvConfig, srcdir: Path
-) -> _Inventory:
-    """Fetch, parse and return an intersphinx inventory file."""
+def _fetch_inventory_data(
+    *,
+    target_uri: InventoryURI,
+    inv_location: str,
+    config: _InvConfig,
+    srcdir: Path,
+    cache_path: Path | None,
+) -> tuple[bytes, str]:
+    """Fetch inventory data from a local or remote source."""
     # both *target_uri* (base URI of the links to generate)
     # and *inv_location* (actual location of the inventory file)
     # can be local or remote URIs
@@ -324,9 +355,17 @@ def _fetch_inventory(
         raw_data, target_uri = _fetch_inventory_url(
             target_uri=target_uri, inv_location=inv_location, config=config
         )
+        if cache_path is not None:
+            cache_path.parent.mkdir(parents=True, exist_ok=True)
+            cache_path.write_bytes(raw_data)
     else:
         raw_data = _fetch_inventory_file(inv_location=inv_location, srcdir=srcdir)
+    return raw_data, target_uri
+
 
+def _load_inventory(raw_data: bytes, /, *, target_uri: InventoryURI) -> _Inventory:
+    """Parse and return an intersphinx inventory file."""
+    # *target_uri* (base URI of the links to generate) can be a local or remote URI
     try:
         inv = InventoryFile.loads(raw_data, uri=target_uri)
     except ValueError as exc:

tests/test_extensions/test_ext_intersphinx.py

@@ -18,10 +18,11 @@
 from sphinx.ext.intersphinx import setup as intersphinx_setup
 from sphinx.ext.intersphinx._cli import inspect_main
 from sphinx.ext.intersphinx._load import (
-    _fetch_inventory,
+    _fetch_inventory_data,
     _fetch_inventory_group,
     _get_safe_url,
     _InvConfig,
+    _load_inventory,
     _strip_basic_auth,
     load_mappings,
     validate_intersphinx_mapping,
@@ -85,12 +86,15 @@ def test_fetch_inventory_redirection(get_request, InventoryFile, app):
 
     # same uri and inv, not redirected
     mocked_get.url = 'https://hostname/' + INVENTORY_FILENAME
-    _fetch_inventory(
-        target_uri='https://hostname/',
+    target_uri = 'https://hostname/'
+    raw_data, target_uri = _fetch_inventory_data(
+        target_uri=target_uri,
         inv_location='https://hostname/' + INVENTORY_FILENAME,
         config=_InvConfig.from_config(app.config),
         srcdir=app.srcdir,
+        cache_path=None,
     )
+    _load_inventory(raw_data, target_uri=target_uri)
     assert 'intersphinx inventory has moved' not in app.status.getvalue()
     assert InventoryFile.loads.call_args[1]['uri'] == 'https://hostname/'
 
@@ -99,12 +103,15 @@ def test_fetch_inventory_redirection(get_request, InventoryFile, app):
     app.status.truncate(0)
     mocked_get.url = 'https://hostname/new/' + INVENTORY_FILENAME
 
-    _fetch_inventory(
-        target_uri='https://hostname/',
+    target_uri = 'https://hostname/'
+    raw_data, target_uri = _fetch_inventory_data(
+        target_uri=target_uri,
         inv_location='https://hostname/' + INVENTORY_FILENAME,
         config=_InvConfig.from_config(app.config),
         srcdir=app.srcdir,
+        cache_path=None,
     )
+    _load_inventory(raw_data, target_uri=target_uri)
     assert app.status.getvalue() == (
         'intersphinx inventory has moved: '
         'https://hostname/%s -> https://hostname/new/%s\n'
@@ -117,12 +124,15 @@ def test_fetch_inventory_redirection(get_request, InventoryFile, app):
     app.status.truncate(0)
     mocked_get.url = 'https://hostname/new/' + INVENTORY_FILENAME
 
-    _fetch_inventory(
-        target_uri='https://hostname/',
+    target_uri = 'https://hostname/'
+    raw_data, target_uri = _fetch_inventory_data(
+        target_uri=target_uri,
         inv_location='https://hostname/new/' + INVENTORY_FILENAME,
         config=_InvConfig.from_config(app.config),
         srcdir=app.srcdir,
+        cache_path=None,
     )
+    _load_inventory(raw_data, target_uri=target_uri)
     assert 'intersphinx inventory has moved' not in app.status.getvalue()
     assert InventoryFile.loads.call_args[1]['uri'] == 'https://hostname/'
 
@@ -131,12 +141,15 @@ def test_fetch_inventory_redirection(get_request, InventoryFile, app):
     app.status.truncate(0)
     mocked_get.url = 'https://hostname/other/' + INVENTORY_FILENAME
 
-    _fetch_inventory(
-        target_uri='https://hostname/',
+    target_uri = 'https://hostname/'
+    raw_data, target_uri = _fetch_inventory_data(
+        target_uri=target_uri,
         inv_location='https://hostname/new/' + INVENTORY_FILENAME,
         config=_InvConfig.from_config(app.config),
         srcdir=app.srcdir,
+        cache_path=None,
     )
+    _load_inventory(raw_data, target_uri=target_uri)
     assert app.status.getvalue() == (
         'intersphinx inventory has moved: '
         'https://hostname/new/%s -> https://hostname/other/%s\n'
@@ -774,13 +787,16 @@ def test_intersphinx_cache_limit(app, monkeypatch, cache_limit, expected_expired
     now = 2 * 86400
     monkeypatch.setattr('time.time', lambda: now)
 
-    # `_fetch_inventory_group` calls `_fetch_inventory`.
+    # `_fetch_inventory_group` calls `_fetch_inventory_data`.
     # We replace it with a mock to test whether it has been called.
     # If it has been called, it means the cache had expired.
-    mock_fake_inventory = _Inventory({})  # must be truthy
-    mock_fetch_inventory = mock.Mock(return_value=mock_fake_inventory)
     monkeypatch.setattr(
-        'sphinx.ext.intersphinx._load._fetch_inventory', mock_fetch_inventory
+        'sphinx.ext.intersphinx._load._fetch_inventory_data',
+        mock.Mock(return_value=(b'', '')),
+    )
+    mock_fetch_inventory = mock.Mock(return_value=_Inventory({}))
+    monkeypatch.setattr(
+        'sphinx.ext.intersphinx._load._load_inventory', mock_fetch_inventory
     )
 
     for name, (uri, locations) in app.config.intersphinx_mapping.values():
@@ -791,8 +807,9 @@ def test_intersphinx_cache_limit(app, monkeypatch, cache_limit, expected_expired
             now=now,
             config=_InvConfig.from_config(app.config),
             srcdir=app.srcdir,
+            cache_dir=None,
         )
-        # If we hadn't mocked `_fetch_inventory`, it would've made
+        # If we hadn't mocked `_fetch_inventory_data`, it would've made
         # a request to `https://example.org/` and found no inventory
         # file. That would've been an error, and `updated` would've been
         # False even if the cache had expired. The mock makes it behave
@@ -826,8 +843,14 @@ def log_message(*args, **kwargs):
         }
 
         now = int(time.time())
-        # we can use 'srcdir=None' since we are raising in _fetch_inventory
-        kwds = {'cache': {}, 'now': now, 'config': config, 'srcdir': None}
+        # we can use 'srcdir=None' since we are raising in _fetch_inventory_data
+        kwds = {
+            'cache': {},
+            'now': now,
+            'config': config,
+            'srcdir': None,
+            'cache_dir': None,
+        }
         # We need an exception with its 'args' attribute set (see error
         # handling in sphinx.ext.intersphinx._load._fetch_inventory_group).
         side_effect = ValueError('')
@@ -836,38 +859,44 @@ def log_message(*args, **kwargs):
             name='1', target_uri=url1, locations=(url1, None)
         )
         with mock.patch(
-            'sphinx.ext.intersphinx._load._fetch_inventory', side_effect=side_effect
+            'sphinx.ext.intersphinx._load._fetch_inventory_data',
+            side_effect=side_effect,
         ) as mockfn:
             assert not _fetch_inventory_group(project=project1, **kwds)
         mockfn.assert_any_call(
             target_uri=url1,
             inv_location=url1,
             config=config,
             srcdir=None,
+            cache_path=None,
         )
         mockfn.assert_any_call(
             target_uri=url1,
             inv_location=url1 + '/' + INVENTORY_FILENAME,
             config=config,
             srcdir=None,
+            cache_path=None,
         )
 
         project2 = _IntersphinxProject(
             name='2', target_uri=url2, locations=(url2, None)
         )
         with mock.patch(
-            'sphinx.ext.intersphinx._load._fetch_inventory', side_effect=side_effect
+            'sphinx.ext.intersphinx._load._fetch_inventory_data',
+            side_effect=side_effect,
         ) as mockfn:
             assert not _fetch_inventory_group(project=project2, **kwds)
         mockfn.assert_any_call(
             target_uri=url2,
             inv_location=url2,
             config=config,
             srcdir=None,
+            cache_path=None,
         )
         mockfn.assert_any_call(
             target_uri=url2,
             inv_location=url2 + INVENTORY_FILENAME,
             config=config,
             srcdir=None,
+            cache_path=None,
         )

tests/test_extensions/test_ext_intersphinx_cache.py

@@ -4,6 +4,7 @@
 
 import posixpath
 import re
+import shutil
 import zlib
 from http.server import BaseHTTPRequestHandler
 from io import BytesIO
@@ -261,12 +262,14 @@ def test_load_mappings_cache_update(tmp_path):
         app1 = SphinxTestApp('dummy', srcdir=tmp_path, confoverrides=confoverrides1)
         app1.build()
         app1.cleanup()
+        shutil.rmtree(app1.doctreedir / '__intersphinx_cache__', ignore_errors=True)
 
         # switch to new url and assert that the old URL is no more stored
         confoverrides2 = BASE_CONFIG | {'intersphinx_mapping': new_project.record}
         app2 = SphinxTestApp('dummy', srcdir=tmp_path, confoverrides=confoverrides2)
         app2.build()
         app2.cleanup()
+        shutil.rmtree(app2.doctreedir / '__intersphinx_cache__', ignore_errors=True)
 
     entry = new_project.make_entry()
     item = dict((new_project.normalise(entry),))