Expand caching to cache remote refs

Several refinements are needed in the CacheDownloader to support this.
Primarily, support for sibling directories to the `downloads` dir, in
the cache dir. This allows the ref resolver to pass in `"refs"` as a
directory name.

As a related change in the course of this work, HTTP retries are
expanded in scope to also cover connection errors and timeouts.

Additionally, `disable_cache` gets passed down from the CLI through to
the ref resolution layer.

Tests are enhanced to better explore CacheDownloader behaviors, but
not to test the usage in ref resolution.

Author: sirosen

src/check_jsonschema/cachedownloader.py

@@ -11,14 +11,10 @@
 
 import requests
 
-# this will let us do any other caching we might need in the future in the same
-# cache dir (adjacent to "downloads")
-_CACHEDIR_NAME = os.path.join("check_jsonschema", "downloads")
-
 _LASTMOD_FMT = "%a, %d %b %Y %H:%M:%S %Z"
 
 
-def _get_default_cache_dir() -> str | None:
+def _base_cache_dir() -> str | None:
     sysname = platform.system()
 
     # on windows, try to get the appdata env var
@@ -34,9 +30,13 @@ def _get_default_cache_dir() -> str | None:
     else:
         cache_dir = os.getenv("XDG_CACHE_HOME", os.path.expanduser("~/.cache"))
 
-    if cache_dir:
-        cache_dir = os.path.join(cache_dir, _CACHEDIR_NAME)
+    return cache_dir
 
+
+def _resolve_cache_dir(dirname: str = "downloads") -> str | None:
+    cache_dir = _base_cache_dir()
+    if cache_dir:
+        cache_dir = os.path.join(cache_dir, "check_jsonschema", dirname)
     return cache_dir
 
 
@@ -55,18 +55,21 @@ def _lastmod_from_response(response: requests.Response) -> float:
 def _get_request(
     file_url: str, *, response_ok: t.Callable[[requests.Response], bool]
 ) -> requests.Response:
-    try:
-        r: requests.Response | None = None
-        for _attempt in range(3):
+    num_retries = 2
+    r: requests.Response | None = None
+    for _attempt in range(num_retries + 1):
+        try:
             r = requests.get(file_url, stream=True)
-            if r.ok and response_ok(r):
-                return r
-        assert r is not None
-        raise FailedDownloadError(
-            f"got response with status={r.status_code}, retries exhausted"
-        )
-    except requests.RequestException as e:
-        raise FailedDownloadError("encountered error during download") from e
+        except requests.RequestException as e:
+            if _attempt == num_retries:
+                raise FailedDownloadError("encountered error during download") from e
+            continue
+        if r.ok and response_ok(r):
+            return r
+    assert r is not None
+    raise FailedDownloadError(
+        f"got response with status={r.status_code}, retries exhausted"
+    )
 
 
 def _atomic_write(dest: str, content: bytes) -> None:
@@ -97,27 +100,19 @@ class FailedDownloadError(Exception):
 
 
 class CacheDownloader:
-    def __init__(
-        self,
-        cache_dir: str | None = None,
-        disable_cache: bool = False,
-        validation_callback: t.Callable[[bytes], t.Any] | None = None,
-    ):
-        self._cache_dir = cache_dir or _get_default_cache_dir()
+    def __init__(self, cache_dir: str | None = None, disable_cache: bool = False):
+        if cache_dir is None:
+            self._cache_dir = _resolve_cache_dir()
+        else:
+            self._cache_dir = _resolve_cache_dir(cache_dir)
         self._disable_cache = disable_cache
-        self._validation_callback = validation_callback
-
-    def _validate(self, response: requests.Response) -> bool:
-        if not self._validation_callback:
-            return True
-
-        try:
-            self._validation_callback(response.content)
-            return True
-        except ValueError:
-            return False
 
-    def _download(self, file_url: str, filename: str) -> str:
+    def _download(
+        self,
+        file_url: str,
+        filename: str,
+        response_ok: t.Callable[[requests.Response], bool],
+    ) -> str:
         assert self._cache_dir is not None
         os.makedirs(self._cache_dir, exist_ok=True)
         dest = os.path.join(self._cache_dir, filename)
@@ -129,7 +124,7 @@ def check_response_for_download(r: requests.Response) -> bool:
             if _cache_hit(dest, r):
                 return True
             # we now know it's not a hit, so validate the content (forces download)
-            return self._validate(r)
+            return response_ok(r)
 
         response = _get_request(file_url, response_ok=check_response_for_download)
         # check to see if we have a file which matches the connection
@@ -140,15 +135,31 @@ def check_response_for_download(r: requests.Response) -> bool:
         return dest
 
     @contextlib.contextmanager
-    def open(self, file_url: str, filename: str) -> t.Iterator[t.IO[bytes]]:
+    def open(
+        self,
+        file_url: str,
+        filename: str,
+        validate_response: t.Callable[[requests.Response], bool],
+    ) -> t.Iterator[t.IO[bytes]]:
         if (not self._cache_dir) or self._disable_cache:
-            yield io.BytesIO(_get_request(file_url, response_ok=self._validate).content)
+            yield io.BytesIO(
+                _get_request(file_url, response_ok=validate_response).content
+            )
         else:
-            with open(self._download(file_url, filename), "rb") as fp:
+            with open(
+                self._download(file_url, filename, response_ok=validate_response), "rb"
+            ) as fp:
                 yield fp
 
-    def bind(self, file_url: str, filename: str | None = None) -> BoundCacheDownloader:
-        return BoundCacheDownloader(file_url, filename, self)
+    def bind(
+        self,
+        file_url: str,
+        filename: str | None = None,
+        validation_callback: t.Callable[[bytes], t.Any] | None = None,
+    ) -> BoundCacheDownloader:
+        return BoundCacheDownloader(
+            file_url, filename, self, validation_callback=validation_callback
+        )
 
 
 class BoundCacheDownloader:
@@ -157,12 +168,29 @@ def __init__(
         file_url: str,
         filename: str | None,
         downloader: CacheDownloader,
+        *,
+        validation_callback: t.Callable[[bytes], t.Any] | None = None,
     ):
         self._file_url = file_url
         self._filename = filename or file_url.split("/")[-1]
         self._downloader = downloader
+        self._validation_callback = validation_callback
 
     @contextlib.contextmanager
     def open(self) -> t.Iterator[t.IO[bytes]]:
-        with self._downloader.open(self._file_url, self._filename) as fp:
+        with self._downloader.open(
+            self._file_url,
+            self._filename,
+            validate_response=self._validate_response,
+        ) as fp:
             yield fp
+
+    def _validate_response(self, response: requests.Response) -> bool:
+        if not self._validation_callback:
+            return True
+
+        try:
+            self._validation_callback(response.content)
+            return True
+        except ValueError:
+            return False

src/check_jsonschema/cli/main_command.py

@@ -300,8 +300,8 @@ def build_schema_loader(args: ParseResult) -> SchemaLoaderBase:
         assert args.schema_path is not None
         return SchemaLoader(
             args.schema_path,
-            args.cache_filename,
-            args.disable_cache,
+            cache_filename=args.cache_filename,
+            disable_cache=args.disable_cache,
             base_uri=args.base_uri,
             validator_class=args.validator_class,
         )

src/check_jsonschema/schema_loader/main.py

@@ -57,14 +57,16 @@ def get_validator(
 
 class SchemaLoader(SchemaLoaderBase):
     validator_class: type[jsonschema.protocols.Validator] | None = None
+    disable_cache: bool = True
 
     def __init__(
         self,
         schemafile: str,
+        *,
         cache_filename: str | None = None,
-        disable_cache: bool = False,
         base_uri: str | None = None,
         validator_class: type[jsonschema.protocols.Validator] | None = None,
+        disable_cache: bool = True,
     ) -> None:
         # record input parameters (these are not to be modified)
         self.schemafile = schemafile
@@ -140,7 +142,7 @@ def get_validator(
         # reference resolution
         # with support for YAML, TOML, and other formats from the parsers
         reference_registry = make_reference_registry(
-            self._parsers, retrieval_uri, schema
+            self._parsers, retrieval_uri, schema, self.disable_cache
         )
 
         if self.validator_class is None:
@@ -171,7 +173,7 @@ def get_validator(
 
 
 class BuiltinSchemaLoader(SchemaLoader):
-    def __init__(self, schema_name: str, base_uri: str | None = None) -> None:
+    def __init__(self, schema_name: str, *, base_uri: str | None = None) -> None:
         self.schema_name = schema_name
         self.base_uri = base_uri
         self._parsers = ParserSet()
@@ -187,7 +189,7 @@ def get_schema(self) -> dict[str, t.Any]:
 
 
 class MetaSchemaLoader(SchemaLoaderBase):
-    def __init__(self, base_uri: str | None = None) -> None:
+    def __init__(self, *, base_uri: str | None = None) -> None:
         if base_uri is not None:
             raise NotImplementedError(
                 "'--base-uri' was used with '--metaschema'. "

src/check_jsonschema/schema_loader/readers.py

@@ -80,8 +80,7 @@ def __init__(
         self.parsers = ParserSet()
         self.downloader = CacheDownloader(
             disable_cache=disable_cache,
-            validation_callback=self._parse,
-        ).bind(url, cache_filename)
+        ).bind(url, cache_filename, validation_callback=self._parse)
         self._parsed_schema: dict | _UnsetType = _UNSET
 
     def _parse(self, schema_bytes: bytes) -> t.Any:

src/check_jsonschema/schema_loader/resolver.py

@@ -1,18 +1,19 @@
 from __future__ import annotations
 
+import hashlib
 import typing as t
 import urllib.parse
 
 import referencing
-import requests
 from referencing.jsonschema import DRAFT202012, Schema
 
+from ..cachedownloader import CacheDownloader
 from ..parsers import ParserSet
 from ..utils import filename2path
 
 
 def make_reference_registry(
-    parsers: ParserSet, retrieval_uri: str | None, schema: dict
+    parsers: ParserSet, retrieval_uri: str | None, schema: dict, disable_cache: bool
 ) -> referencing.Registry:
     id_attribute_: t.Any = schema.get("$id")
     if isinstance(id_attribute_, str):
@@ -26,7 +27,9 @@ def make_reference_registry(
     # mypy does not recognize that Registry is an `attrs` class and has `retrieve` as an
     # argument to its implicit initializer
     registry: referencing.Registry = referencing.Registry(  # type: ignore[call-arg]
-        retrieve=create_retrieve_callable(parsers, retrieval_uri, id_attribute)
+        retrieve=create_retrieve_callable(
+            parsers, retrieval_uri, id_attribute, disable_cache
+        )
     )
 
     if retrieval_uri is not None:
@@ -38,13 +41,17 @@ def make_reference_registry(
 
 
 def create_retrieve_callable(
-    parser_set: ParserSet, retrieval_uri: str | None, id_attribute: str | None
+    parser_set: ParserSet,
+    retrieval_uri: str | None,
+    id_attribute: str | None,
+    disable_cache: bool,
 ) -> t.Callable[[str], referencing.Resource[Schema]]:
     base_uri = id_attribute
     if base_uri is None:
         base_uri = retrieval_uri
 
     cache = ResourceCache()
+    downloader = CacheDownloader("refs", disable_cache)
 
     def get_local_file(uri: str) -> t.Any:
         path = filename2path(uri)
@@ -62,10 +69,19 @@ def retrieve_reference(uri: str) -> referencing.Resource[Schema]:
 
         full_uri_scheme = urllib.parse.urlsplit(full_uri).scheme
         if full_uri_scheme in ("http", "https"):
-            data = requests.get(full_uri, stream=True)
-            parsed_object = parser_set.parse_data_with_path(
-                data.content, full_uri, "json"
+
+            def validation_callback(content: bytes) -> None:
+                parser_set.parse_data_with_path(content, full_uri, "json")
+
+            bound_downloader = downloader.bind(
+                full_uri,
+                hashlib.md5(full_uri.encode()).hexdigest(),
+                validation_callback,
             )
+            with bound_downloader.open() as fp:
+                data = fp.read()
+
+            parsed_object = parser_set.parse_data_with_path(data, full_uri, "json")
         else:
             parsed_object = get_local_file(full_uri)
 

tests/acceptance/test_remote_ref_resolution.py

@@ -35,17 +35,6 @@
 }
 
 
-@pytest.fixture(autouse=True)
-def _mock_schema_cache_dir(monkeypatch, tmp_path):
-    def _fake_default_cache_dir():
-        return str(tmp_path)
-
-    monkeypatch.setattr(
-        "check_jsonschema.cachedownloader._get_default_cache_dir",
-        _fake_default_cache_dir,
-    )
-
-
 @pytest.mark.parametrize("check_passes", (True, False))
 @pytest.mark.parametrize("casename", ("case1", "case2"))
 def test_remote_ref_resolution_simple_case(run_line, check_passes, casename, tmp_path):

tests/acceptance/test_special_filetypes.py

@@ -79,20 +79,11 @@ def test_schema_and_instance_in_fifos(tmp_path, run_line, check_succeeds):
 
 
 @pytest.mark.parametrize("check_passes", (True, False))
-def test_remote_schema_requiring_retry(run_line, check_passes, tmp_path, monkeypatch):
+def test_remote_schema_requiring_retry(run_line, check_passes, tmp_path):
     """
     a "remote schema" (meaning HTTPS) with bad data, therefore requiring that a retry
     fires in order to parse
     """
-
-    def _fake_default_cache_dir():
-        return str(tmp_path)
-
-    monkeypatch.setattr(
-        "check_jsonschema.cachedownloader._get_default_cache_dir",
-        _fake_default_cache_dir,
-    )
-
     schema_loc = "https://example.com/schema1.json"
     responses.add("GET", schema_loc, body="", match_querystring=None)
     responses.add(

tests/conftest.py

@@ -46,3 +46,17 @@ def in_tmp_dir(request, tmp_path):
     os.chdir(str(tmp_path))
     yield
     os.chdir(request.config.invocation_dir)
+
+
+@pytest.fixture
+def cache_dir(tmp_path):
+    return tmp_path / ".cache"
+
+
+@pytest.fixture(autouse=True)
+def patch_cache_dir(monkeypatch, cache_dir):
+    with monkeypatch.context() as m:
+        m.setattr(
+            "check_jsonschema.cachedownloader._base_cache_dir", lambda: str(cache_dir)
+        )
+        yield m