From 7c7f827db9f86053a698f0fe1fe2cc49c8083fc6 Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Tue, 26 May 2026 10:12:42 -0700 Subject: [PATCH 1/2] Reject VRT sources under stable_only=True (#2443) Add a `stable_only: bool = False` kwarg to `open_geotiff`, `read_geotiff_dask`, `read_geotiff_gpu`, and `read_vrt`. When True, the VRT path raises a typed `VRTStableSourcesOnlyError` (a `GeoTIFFAmbiguousMetadataError` subclass) before any pixel decode. The message names the offending VRT path and the `allow_experimental_codecs` unlock so callers can opt into the broader tier set explicitly. Flips the `test_release_gate_negative_mixed_tier_vrt_children` xfail from epic #2342. --- xrspatial/geotiff/__init__.py | 21 ++- xrspatial/geotiff/_backends/dask.py | 11 ++ xrspatial/geotiff/_backends/gpu.py | 8 ++ xrspatial/geotiff/_backends/vrt.py | 28 ++++ xrspatial/geotiff/_errors.py | 21 +++ xrspatial/geotiff/_validation.py | 66 +++++++++- .../release_gates/test_stable_features.py | 29 +---- xrspatial/geotiff/tests/test_features.py | 5 + .../tests/test_reader_kwarg_order_1935.py | 6 + .../tests/test_vrt_stable_only_2443.py | 120 ++++++++++++++++++ 10 files changed, 291 insertions(+), 24 deletions(-) create mode 100644 xrspatial/geotiff/tests/test_vrt_stable_only_2443.py diff --git a/xrspatial/geotiff/__init__.py b/xrspatial/geotiff/__init__.py index e6d50e3c..1c9bc521 100644 --- a/xrspatial/geotiff/__init__.py +++ b/xrspatial/geotiff/__init__.py @@ -76,7 +76,8 @@ MixedBandMetadataError, NonRepresentableEPSGCRSError, NonUniformCoordsError, RotatedTransformError, UnknownCRSModelTypeError, - UnparseableCRSError, UnsupportedGeoTIFFFeatureError) + UnparseableCRSError, UnsupportedGeoTIFFFeatureError, + VRTStableSourcesOnlyError) from ._geotags import RASTER_PIXEL_IS_AREA, RASTER_PIXEL_IS_POINT, GeoTransform # noqa: F401 from ._reader import _MAX_CLOUD_BYTES_SENTINEL, CloudSizeLimitError, UnsafeURLError from ._reader import read_to_array as _read_to_array @@ -122,6 +123,7 @@ 'UnparseableCRSError', 'UnsafeURLError', 'UnsupportedGeoTIFFFeatureError', + 'VRTStableSourcesOnlyError', 'open_geotiff', 'read_geotiff_gpu', 'read_geotiff_dask', @@ -389,6 +391,7 @@ def open_geotiff(source: str | BinaryIO, *, allow_unparseable_crs: bool = False, allow_inconsistent_geokeys: bool = False, allow_invalid_nodata: bool = False, + stable_only: bool = False, allow_experimental_codecs: bool = False, allow_internal_only_jpeg: bool = False, band_nodata: str | None = None, @@ -611,6 +614,19 @@ def open_geotiff(source: str | BinaryIO, *, pre-rejection no-op behaviour for files known to carry such sentinels (e.g. external tooling that writes ``"nan"`` on integer outputs). See issue #2441 (#1774 follow-up). + stable_only : bool, default False + [advanced] Read-side opt-in for stable-tier sources only. When + ``True``, a ``.vrt`` source raises + :class:`VRTStableSourcesOnlyError` because ``reader.vrt`` and + the VRT child-source pipeline sit at the ``advanced`` / + ``experimental`` tiers in + :data:`xrspatial.geotiff.SUPPORTED_FEATURES`. Non-VRT sources + on this entry point already ride the stable ``reader.local_file`` + path and the per-source codec gate, so the flag is a no-op for + them. The rejection names the file path and the + ``allow_experimental_codecs`` opt-in so the caller can unlock + the broader tier set explicitly when needed. See epic #2342 + and ``docs/source/reference/release_gate_geotiff.rst``. allow_experimental_codecs : bool, default False Read-side opt-in for sources compressed with the Tier 3 experimental codecs (``lerc``, ``jpeg2000`` / ``j2k``, ``lz4``). @@ -761,6 +777,7 @@ def open_geotiff(source: str | BinaryIO, *, allow_inconsistent_geokeys=( allow_inconsistent_geokeys), allow_invalid_nodata=allow_invalid_nodata, + stable_only=stable_only, allow_experimental_codecs=allow_experimental_codecs, allow_internal_only_jpeg=allow_internal_only_jpeg, band_nodata=band_nodata, @@ -786,6 +803,7 @@ def open_geotiff(source: str | BinaryIO, *, allow_inconsistent_geokeys=( allow_inconsistent_geokeys), allow_invalid_nodata=allow_invalid_nodata, + stable_only=stable_only, allow_experimental_codecs=( allow_experimental_codecs), allow_internal_only_jpeg=( @@ -804,6 +822,7 @@ def open_geotiff(source: str | BinaryIO, *, allow_inconsistent_geokeys=( allow_inconsistent_geokeys), allow_invalid_nodata=allow_invalid_nodata, + stable_only=stable_only, allow_experimental_codecs=( allow_experimental_codecs), allow_internal_only_jpeg=( diff --git a/xrspatial/geotiff/_backends/dask.py b/xrspatial/geotiff/_backends/dask.py index fd7f240c..8e3a3aa3 100644 --- a/xrspatial/geotiff/_backends/dask.py +++ b/xrspatial/geotiff/_backends/dask.py @@ -44,6 +44,7 @@ def read_geotiff_dask(source: str, *, allow_unparseable_crs: bool = False, allow_inconsistent_geokeys: bool = False, allow_invalid_nodata: bool = False, + stable_only: bool = False, allow_experimental_codecs: bool = False, allow_internal_only_jpeg: bool = False, band_nodata: str | None = None, @@ -142,6 +143,13 @@ def read_geotiff_dask(source: str, *, ``InvalidIntegerNodataError`` at graph-build time. See ``open_geotiff`` for the full description (#1774 follow-up, #2441). + stable_only : bool, default False + [advanced] Read-side opt-in for stable-tier sources only. + Forwarded to ``read_vrt`` when the source ends in ``.vrt`` so + the rejection fires at graph-build time. Non-VRT sources on + this entry point already ride the stable ``reader.local_file`` + path, so the flag is a no-op for them. See ``open_geotiff`` for + the full description (epic #2342). allow_experimental_codecs : bool, default False [advanced] Read-side opt-in for Tier 3 experimental codecs (``lerc``, ``jpeg2000`` / ``j2k``, ``lz4``). Fires at graph @@ -227,6 +235,9 @@ def read_geotiff_dask(source: str, *, allow_unparseable_crs=allow_unparseable_crs, allow_inconsistent_geokeys=allow_inconsistent_geokeys, allow_invalid_nodata=allow_invalid_nodata, + stable_only=stable_only, + allow_experimental_codecs=allow_experimental_codecs, + allow_internal_only_jpeg=allow_internal_only_jpeg, band_nodata=band_nodata, mask_nodata=mask_nodata, **vrt_kwargs, diff --git a/xrspatial/geotiff/_backends/gpu.py b/xrspatial/geotiff/_backends/gpu.py index bd9238a4..26db314d 100644 --- a/xrspatial/geotiff/_backends/gpu.py +++ b/xrspatial/geotiff/_backends/gpu.py @@ -77,6 +77,7 @@ def read_geotiff_gpu(source: str, *, allow_unparseable_crs: bool = False, allow_inconsistent_geokeys: bool = False, allow_invalid_nodata: bool = False, + stable_only: bool = False, allow_experimental_codecs: bool = False, allow_internal_only_jpeg: bool = False, band_nodata: str | None = None, @@ -217,6 +218,13 @@ def read_geotiff_gpu(source: str, *, eager and dask paths; default raises ``InvalidIntegerNodataError``. See ``open_geotiff`` for the full description (#1774 follow-up, #2441). + stable_only : bool, default False + [experimental] Read-side opt-in for stable-tier sources only. + The GPU read path does not consume VRT sources directly (VRT + routing happens in ``open_geotiff``), so this kwarg is accepted + for cross-backend signature symmetry and is a no-op on the GPU + eager / chunked paths. See ``open_geotiff`` for the full + description (epic #2342). allow_experimental_codecs : bool, default False [experimental] Read-side opt-in for Tier 3 experimental codecs (``lerc``, ``jpeg2000`` / ``j2k``, ``lz4``). The GPU read path diff --git a/xrspatial/geotiff/_backends/vrt.py b/xrspatial/geotiff/_backends/vrt.py index c6161cd9..d0384425 100644 --- a/xrspatial/geotiff/_backends/vrt.py +++ b/xrspatial/geotiff/_backends/vrt.py @@ -130,6 +130,7 @@ def read_vrt(source: str, *, allow_unparseable_crs: bool = False, allow_inconsistent_geokeys: bool = False, allow_invalid_nodata: bool = False, + stable_only: bool = False, allow_experimental_codecs: bool = False, allow_internal_only_jpeg: bool = False, band_nodata: str | None = None, @@ -276,6 +277,17 @@ def read_vrt(source: str, *, the per-source GeoTIFF reads built by the VRT planner. See ``open_geotiff`` for the full description (#1774 follow-up, #2441). + stable_only : bool, default False + [advanced] Read-side opt-in for stable-tier sources only. When + ``True``, ``read_vrt`` raises :class:`VRTStableSourcesOnlyError` + before any pixel decode because ``reader.vrt`` itself sits at + the ``advanced`` tier in :data:`SUPPORTED_FEATURES` and VRT + child sources can declare any codec the GeoTIFF reader supports + (including experimental and internal-only tiers). The message + names the file path and the ``allow_experimental_codecs`` + unlock so the caller can opt into the broader tier set + explicitly. See epic #2342 and + ``docs/source/reference/release_gate_geotiff.rst``. allow_experimental_codecs : bool, default False [advanced] Read-side opt-in for Tier 3 experimental codecs in any source file referenced by the VRT. Forwarded to the @@ -373,6 +385,22 @@ def read_vrt(source: str, *, source = _coerce_path(source) + # Epic #2342: reject the read up front when the caller asked for + # stable-only sources. ``reader.vrt`` sits at the ``advanced`` tier + # and VRT children can declare any codec the GeoTIFF reader + # supports, so a stable-only request cannot be served from a VRT + # mosaic without the documented ``allow_experimental_codecs`` + # unlock. Runs before the dispatcher-kwarg validator so the typed + # error surfaces before any other validation noise (a malformed VRT + # path, an unsupported ``overview_level``, etc.) competes for the + # raise site. + from .._validation import _validate_stable_only_vrt + _validate_stable_only_vrt( + source, + stable_only=stable_only, + allow_experimental_codecs=allow_experimental_codecs, + ) + # Shared dispatcher-kwarg validator so direct callers see the same # rejections as ``open_geotiff`` (issue #2175 / parent #2162). For # ``read_vrt`` the helper rejects ``on_gpu_failure`` (VRT reads do diff --git a/xrspatial/geotiff/_errors.py b/xrspatial/geotiff/_errors.py index 47a4aa78..cc2ea85f 100644 --- a/xrspatial/geotiff/_errors.py +++ b/xrspatial/geotiff/_errors.py @@ -166,6 +166,26 @@ class InvalidIntegerNodataError(GeoTIFFAmbiguousMetadataError): """ +class VRTStableSourcesOnlyError(GeoTIFFAmbiguousMetadataError): + """VRT source opened under ``stable_only=True`` (epic #2342). + + Raised when a caller opens a ``.vrt`` file with ``stable_only=True``. + The VRT reader (``reader.vrt``) and its child sources sit at the + ``advanced`` / ``experimental`` tiers in + :data:`xrspatial.geotiff.SUPPORTED_FEATURES`, so a request for + stable-only sources cannot be served from a VRT mosaic without an + explicit opt-in. The message names the offending VRT path and the + matching opt-in flag (``allow_experimental_codecs``) so the caller + learns the unlock at the boundary rather than from the docs. + + Pass ``stable_only=False`` (the default) to keep the legacy + behaviour, or pass ``allow_experimental_codecs=True`` to opt into + the broader tier set explicitly. See the release contract document + at ``docs/source/reference/release_gate_geotiff.rst`` and epic + #2342 for the full rationale. + """ + + class UnknownCRSModelTypeError(GeoTIFFAmbiguousMetadataError): """Can't classify an EPSG as geographic or projected on write (#2277). @@ -230,6 +250,7 @@ class UnsupportedGeoTIFFFeatureError(ValueError): "ConflictingCRSError", "ConflictingNodataError", "InvalidIntegerNodataError", + "VRTStableSourcesOnlyError", "VRTUnsupportedError", "UnknownCRSModelTypeError", "NonRepresentableEPSGCRSError", diff --git a/xrspatial/geotiff/_validation.py b/xrspatial/geotiff/_validation.py index 944b5989..46c0cd8d 100644 --- a/xrspatial/geotiff/_validation.py +++ b/xrspatial/geotiff/_validation.py @@ -28,7 +28,7 @@ from ._coords import _BAND_DIM_NAMES from ._errors import (ConflictingCRSError, ConflictingNodataError, InconsistentGeoKeysError, InvalidIntegerNodataError, MixedBandMetadataError, NonUniformCoordsError, - RotatedTransformError, UnparseableCRSError) + RotatedTransformError, UnparseableCRSError, VRTStableSourcesOnlyError) from ._runtime import (_MISSING_SOURCES_SENTINEL, _ON_GPU_FAILURE_SENTINEL, _TIME_DIM_NAMES, _X_DIM_NAMES, _Y_DIM_NAMES) @@ -668,6 +668,70 @@ def _validate_int_nodata_for_dtype( ) +def _validate_stable_only_vrt( + source: str, + *, + stable_only: bool, + allow_experimental_codecs: bool = False, +) -> None: + """Reject a VRT source when the caller asks for stable-only sources. + + Implements the read-side gate the release-contract test + ``test_release_gate_negative_mixed_tier_vrt_children`` pins from + epic #2342. The ``reader.vrt`` entry point sits at the ``advanced`` + tier in :data:`xrspatial.geotiff.SUPPORTED_FEATURES`, and VRT child + sources can declare any codec / capability the underlying GeoTIFF + reader supports (including the ``experimental`` and ``internal_only`` + tiers). A caller who asks for stable-only sources via + ``stable_only=True`` therefore cannot be served from a VRT mosaic + without naming the broader-tier opt-in (``allow_experimental_codecs``). + Reject up front at graph-build / eager-read setup time so the + failure surfaces before any pixel decode work. + + Parameters + ---------- + source : str + Path to the ``.vrt`` file. Embedded in the rejection message so + the caller can locate the offending file without re-parsing. + stable_only : bool + Caller's opt-in for stable-only sources. ``False`` is a no-op; + ``True`` raises :class:`VRTStableSourcesOnlyError`. + allow_experimental_codecs : bool, default False + Companion opt-in. When the caller passes both ``stable_only=True`` + and ``allow_experimental_codecs=True`` the request is internally + contradictory, but ``allow_experimental_codecs=True`` is the + documented unlock so we honour it: the gate becomes a no-op and + the per-source codec gate downstream handles the rest. Pure + ``stable_only=True`` (without the unlock) raises. + + Raises + ------ + VRTStableSourcesOnlyError + When ``stable_only=True`` and the source is a VRT and the + caller did not pass ``allow_experimental_codecs=True``. The + message names the offending VRT path, both flags, and cites the + release-contract document plus epic #2342. + """ + if not stable_only: + return + if allow_experimental_codecs: + return + raise VRTStableSourcesOnlyError( + f"VRT source '{source}' cannot be opened under stable_only=True. " + f"The VRT reader (``reader.vrt``) sits at the advanced tier in " + f"SUPPORTED_FEATURES, and VRT child sources can declare any " + f"codec or capability the GeoTIFF reader supports, including " + f"the experimental and internal-only tiers. The stable-only " + f"request cannot be served from a VRT mosaic without an " + f"explicit broader-tier opt-in. Pass " + f"allow_experimental_codecs=True to opt in to the advanced / " + f"experimental tiers, or drop stable_only=True to keep the " + f"default behaviour. See " + f"docs/source/reference/release_gate_geotiff.rst for the " + f"release contract and epic #2342 for the tracking issue." + ) + + def _validate_no_rotated_affine(attrs, *, drop_rotation: bool, entry_point: str = "to_geotiff") -> None: """Refuse writes that would silently drop ``attrs['rotated_affine']``. diff --git a/xrspatial/geotiff/tests/release_gates/test_stable_features.py b/xrspatial/geotiff/tests/release_gates/test_stable_features.py index 9584e37d..64e3a4f7 100644 --- a/xrspatial/geotiff/tests/release_gates/test_stable_features.py +++ b/xrspatial/geotiff/tests/release_gates/test_stable_features.py @@ -2415,31 +2415,16 @@ def test_release_gate_negative_rotated_gpu( @pytest.mark.release_gate -@pytest.mark.xfail( - reason=( - "The VRT stable-only knob is owned by epic #2342 and has not " - "landed yet. The release promise: when the caller asks for " - "stable-only sources and a VRT child uses an experimental codec, " - "the reader names the offending child and the opt-in flag. This " - "xfail flips to a pass when #2342 ships the knob." - ), - strict=False, -) def test_release_gate_negative_mixed_tier_vrt_children(tmp_path) -> None: """The reader must refuse mixed-tier VRT children when stable-only is asked. - XFAIL-to-PASS transition note - ----------------------------- - Today this test fails with ``TypeError: unexpected keyword argument - 'stable_only'`` because epic #2342 has not landed the kwarg yet. The - strict=False xfail swallows that TypeError. When #2342 lands, the - test will start raising :class:`GeoTIFFAmbiguousMetadataError` (or - fail to raise) and the xfail will report XPASS. Before removing the - xfail marker, confirm the new code path satisfies both inline - assertions: the error message must mention either ``stable_only`` or - ``allow_experimental_codecs``, and it must cite the release contract - docs. If either assertion would not pass, fix the production message - in the same PR that removes the xfail. + Pinned by epic #2342 / issue #2443: when the caller asks for + stable-only sources via ``stable_only=True`` and the source is a + VRT, the read raises :class:`VRTStableSourcesOnlyError` (a + :class:`GeoTIFFAmbiguousMetadataError` subclass) before any pixel + decode. The message must name either ``stable_only`` or + ``allow_experimental_codecs`` and cite the release-contract docs + or the tracking issue. """ path = _neg_tmp(tmp_path, "case4_mixed_tier_vrt", suffix=".vrt") Path(path).write_text( diff --git a/xrspatial/geotiff/tests/test_features.py b/xrspatial/geotiff/tests/test_features.py index d30acb4b..8f74039d 100644 --- a/xrspatial/geotiff/tests/test_features.py +++ b/xrspatial/geotiff/tests/test_features.py @@ -2804,6 +2804,11 @@ def test_all_lists_supported_functions(self): # pansharpened / derived VRT subclasses, unknown VRT band # children, rotated source transforms on a VRT mosaic). 'UnsupportedGeoTIFFFeatureError', + # Issue #2443 (epic #2342): typed rejection when a caller + # opens a VRT under ``stable_only=True``. The VRT reader + # itself is advanced-tier so the request cannot be served + # without naming the broader-tier opt-in. + 'VRTStableSourcesOnlyError', 'GeoTIFFFallbackWarning', 'UnsafeURLError', # Canonical georef_status constants (issue #2136). Exposed diff --git a/xrspatial/geotiff/tests/test_reader_kwarg_order_1935.py b/xrspatial/geotiff/tests/test_reader_kwarg_order_1935.py index b54fbffa..206123cd 100644 --- a/xrspatial/geotiff/tests/test_reader_kwarg_order_1935.py +++ b/xrspatial/geotiff/tests/test_reader_kwarg_order_1935.py @@ -47,6 +47,12 @@ # opt-outs so the canonical order keeps the typed-error gates # grouped. "allow_invalid_nodata", + # Issue #2443 (epic #2342) added the stable-tier-only read-side + # gate. Sits alongside the other ambiguous-metadata opt-outs and + # immediately before the experimental-codec unlock it pairs with + # in the rejection message, so the canonical order tracks the + # release-contract grouping. + "stable_only", # PR 4 of epic #2340 added the experimental / internal-only codec # opt-ins on the read side, mirroring the writer surface from #2137 # / #1845. They sit after the other ``allow_*`` flags so the diff --git a/xrspatial/geotiff/tests/test_vrt_stable_only_2443.py b/xrspatial/geotiff/tests/test_vrt_stable_only_2443.py new file mode 100644 index 00000000..318ccc36 --- /dev/null +++ b/xrspatial/geotiff/tests/test_vrt_stable_only_2443.py @@ -0,0 +1,120 @@ +"""Default-rejection tests for the VRT ``stable_only=True`` gate (#2443). + +Companion to ``test_release_gate_negative_mixed_tier_vrt_children`` in +``release_gates/test_stable_features.py``. These tests pin the +release-contract upgrade from epic #2342: a caller who asks for +stable-tier sources via ``stable_only=True`` on a VRT source must see a +typed :class:`VRTStableSourcesOnlyError` (a +:class:`GeoTIFFAmbiguousMetadataError` subclass) before any pixel +decode, and the message must name the file path and the +``allow_experimental_codecs`` opt-in unlock. +""" +from __future__ import annotations + +from pathlib import Path + +import pytest + +from xrspatial.geotiff import (GeoTIFFAmbiguousMetadataError, VRTStableSourcesOnlyError, + open_geotiff, read_geotiff_dask, read_vrt) + + +_MINIMAL_VRT_XML = '\n' + + +def _write_minimal_vrt(tmp_path: Path, name: str = "stable_only_2443") -> str: + path = tmp_path / f"{name}.vrt" + path.write_text(_MINIMAL_VRT_XML, encoding="utf-8") + return str(path) + + +def test_open_geotiff_vrt_stable_only_rejected_by_default(tmp_path): + """``open_geotiff(vrt, stable_only=True)`` raises the typed error.""" + path = _write_minimal_vrt(tmp_path, "open_geotiff_default") + with pytest.raises(VRTStableSourcesOnlyError) as excinfo: + open_geotiff(path, stable_only=True) + msg = str(excinfo.value) + assert path in msg, ( + f"expected the offending VRT path in the rejection message; " + f"got: {msg!r}" + ) + assert "stable_only" in msg + assert "allow_experimental_codecs" in msg + assert "release_gate_geotiff" in msg + assert "#2342" in msg + + +def test_open_geotiff_vrt_stable_only_default_false_does_not_reject(tmp_path): + """The default ``stable_only=False`` does not fire the new gate. + + The minimal-VRT body has no ```` children so the read + still raises a downstream validator error, but it must NOT be the + new typed :class:`VRTStableSourcesOnlyError` -- that one is gated on + the explicit opt-in. + """ + path = _write_minimal_vrt(tmp_path, "open_geotiff_default_false") + with pytest.raises(Exception) as excinfo: + open_geotiff(path) + assert not isinstance(excinfo.value, VRTStableSourcesOnlyError), ( + f"stable_only default should not fire VRTStableSourcesOnlyError; " + f"got: {excinfo.value!r}" + ) + + +def test_open_geotiff_vrt_stable_only_with_experimental_unlock(tmp_path): + """``allow_experimental_codecs=True`` is the documented unlock. + + When the caller passes both ``stable_only=True`` and + ``allow_experimental_codecs=True`` the gate is a no-op (the per-source + codec gate downstream handles the rest). The read still raises a + downstream "no " rejection on this minimal-VRT + fixture, but it must NOT be the new typed error. + """ + path = _write_minimal_vrt(tmp_path, "open_geotiff_unlock") + with pytest.raises(Exception) as excinfo: + open_geotiff( + path, + stable_only=True, + allow_experimental_codecs=True, + ) + assert not isinstance(excinfo.value, VRTStableSourcesOnlyError), ( + f"allow_experimental_codecs=True must unlock the gate; " + f"got: {excinfo.value!r}" + ) + + +def test_read_vrt_stable_only_rejected_by_default(tmp_path): + """Direct ``read_vrt(stable_only=True)`` raises the typed error too.""" + path = _write_minimal_vrt(tmp_path, "read_vrt_direct") + with pytest.raises(VRTStableSourcesOnlyError): + read_vrt(path, stable_only=True) + + +def test_read_geotiff_dask_vrt_stable_only_rejected(tmp_path): + """``read_geotiff_dask`` forwards the kwarg to ``read_vrt`` for VRT sources.""" + path = _write_minimal_vrt(tmp_path, "read_dask_vrt") + with pytest.raises(VRTStableSourcesOnlyError): + read_geotiff_dask(path, stable_only=True) + + +def test_vrt_stable_only_error_is_geotiff_ambiguous_metadata_error(): + """The typed error subclasses :class:`GeoTIFFAmbiguousMetadataError`. + + Callers that already ``except GeoTIFFAmbiguousMetadataError`` keep + catching this case without an import-list change. The release-gate + test in ``release_gates/test_stable_features.py`` relies on this + inheritance to assert on the base class. + """ + assert issubclass(VRTStableSourcesOnlyError, GeoTIFFAmbiguousMetadataError) + + +def test_read_vrt_stable_only_no_op_on_default(tmp_path): + """``stable_only=False`` (the default) is a no-op on the direct VRT path. + + Same fixture as the rejection test, but the absence of the flag + means the read proceeds to the existing band-count validator. + """ + path = _write_minimal_vrt(tmp_path, "read_vrt_default") + with pytest.raises(Exception) as excinfo: + read_vrt(path) + assert not isinstance(excinfo.value, VRTStableSourcesOnlyError) From 2a642dd29316908b55d60e87962c493e06e846a5 Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Tue, 26 May 2026 10:16:08 -0700 Subject: [PATCH 2/2] Address review nits: defensive VRT-extension check, tighter test asserts (#2443) - ``_validate_stable_only_vrt`` now passes through silently when the source is not a ``.vrt`` path, so a future call site that forwards a non-VRT path cannot trip a mislabeled "VRT source" rejection. The docstring spells out the pass-through contract. - ``test_vrt_stable_only_2443.py`` pins the downstream ``VRTUnsupportedError`` on the default-false and ``allow_experimental_codecs``-unlock paths so a future refactor cannot silently broaden either branch past intent. --- xrspatial/geotiff/_validation.py | 23 ++++++++++--- .../tests/test_vrt_stable_only_2443.py | 34 ++++++++----------- 2 files changed, 32 insertions(+), 25 deletions(-) diff --git a/xrspatial/geotiff/_validation.py b/xrspatial/geotiff/_validation.py index 46c0cd8d..5e0cbfee 100644 --- a/xrspatial/geotiff/_validation.py +++ b/xrspatial/geotiff/_validation.py @@ -693,9 +693,15 @@ def _validate_stable_only_vrt( source : str Path to the ``.vrt`` file. Embedded in the rejection message so the caller can locate the offending file without re-parsing. + Non-string sources (eager file-like buffers) and string paths + that do not end in ``.vrt`` are silently passed through; the + gate only fires for sources the caller could reasonably have + intended as a VRT mosaic, so a future call site that routes a + non-VRT path through this helper does not mislabel the failure. stable_only : bool Caller's opt-in for stable-only sources. ``False`` is a no-op; - ``True`` raises :class:`VRTStableSourcesOnlyError`. + ``True`` raises :class:`VRTStableSourcesOnlyError` (provided + ``source`` looks like a VRT path). allow_experimental_codecs : bool, default False Companion opt-in. When the caller passes both ``stable_only=True`` and ``allow_experimental_codecs=True`` the request is internally @@ -707,15 +713,22 @@ def _validate_stable_only_vrt( Raises ------ VRTStableSourcesOnlyError - When ``stable_only=True`` and the source is a VRT and the - caller did not pass ``allow_experimental_codecs=True``. The - message names the offending VRT path, both flags, and cites the - release-contract document plus epic #2342. + When ``stable_only=True`` and the source path ends in ``.vrt`` + (case-insensitive) and the caller did not pass + ``allow_experimental_codecs=True``. The message names the + offending VRT path, both flags, and cites the release-contract + document plus epic #2342. """ if not stable_only: return if allow_experimental_codecs: return + # Defensive extension check: every public-API call site routes only + # ``.vrt`` paths into this helper, but a future call site could + # forward a non-VRT path. Pass through silently in that case so the + # rejection message never mislabels a non-VRT source as a VRT. + if not (isinstance(source, str) and source.lower().endswith('.vrt')): + return raise VRTStableSourcesOnlyError( f"VRT source '{source}' cannot be opened under stable_only=True. " f"The VRT reader (``reader.vrt``) sits at the advanced tier in " diff --git a/xrspatial/geotiff/tests/test_vrt_stable_only_2443.py b/xrspatial/geotiff/tests/test_vrt_stable_only_2443.py index 318ccc36..86ee9f60 100644 --- a/xrspatial/geotiff/tests/test_vrt_stable_only_2443.py +++ b/xrspatial/geotiff/tests/test_vrt_stable_only_2443.py @@ -17,6 +17,7 @@ from xrspatial.geotiff import (GeoTIFFAmbiguousMetadataError, VRTStableSourcesOnlyError, open_geotiff, read_geotiff_dask, read_vrt) +from xrspatial.geotiff._errors import VRTUnsupportedError _MINIMAL_VRT_XML = '\n' @@ -47,18 +48,14 @@ def test_open_geotiff_vrt_stable_only_rejected_by_default(tmp_path): def test_open_geotiff_vrt_stable_only_default_false_does_not_reject(tmp_path): """The default ``stable_only=False`` does not fire the new gate. - The minimal-VRT body has no ```` children so the read - still raises a downstream validator error, but it must NOT be the - new typed :class:`VRTStableSourcesOnlyError` -- that one is gated on - the explicit opt-in. + The minimal-VRT body has no ```` children so the + read still raises the downstream :class:`VRTUnsupportedError` + band-count check; pinning the exact class confirms the new gate is + not stealing the raise site at the default flag value. """ path = _write_minimal_vrt(tmp_path, "open_geotiff_default_false") - with pytest.raises(Exception) as excinfo: + with pytest.raises(VRTUnsupportedError): open_geotiff(path) - assert not isinstance(excinfo.value, VRTStableSourcesOnlyError), ( - f"stable_only default should not fire VRTStableSourcesOnlyError; " - f"got: {excinfo.value!r}" - ) def test_open_geotiff_vrt_stable_only_with_experimental_unlock(tmp_path): @@ -66,21 +63,18 @@ def test_open_geotiff_vrt_stable_only_with_experimental_unlock(tmp_path): When the caller passes both ``stable_only=True`` and ``allow_experimental_codecs=True`` the gate is a no-op (the per-source - codec gate downstream handles the rest). The read still raises a - downstream "no " rejection on this minimal-VRT - fixture, but it must NOT be the new typed error. + codec gate downstream handles the rest). The read still raises the + downstream "no " :class:`VRTUnsupportedError` on + this minimal-VRT fixture; pinning the exact downstream class keeps + a future refactor from silently broadening the unlock past intent. """ path = _write_minimal_vrt(tmp_path, "open_geotiff_unlock") - with pytest.raises(Exception) as excinfo: + with pytest.raises(VRTUnsupportedError): open_geotiff( path, stable_only=True, allow_experimental_codecs=True, ) - assert not isinstance(excinfo.value, VRTStableSourcesOnlyError), ( - f"allow_experimental_codecs=True must unlock the gate; " - f"got: {excinfo.value!r}" - ) def test_read_vrt_stable_only_rejected_by_default(tmp_path): @@ -112,9 +106,9 @@ def test_read_vrt_stable_only_no_op_on_default(tmp_path): """``stable_only=False`` (the default) is a no-op on the direct VRT path. Same fixture as the rejection test, but the absence of the flag - means the read proceeds to the existing band-count validator. + means the read proceeds to the existing band-count validator and + raises :class:`VRTUnsupportedError`. """ path = _write_minimal_vrt(tmp_path, "read_vrt_default") - with pytest.raises(Exception) as excinfo: + with pytest.raises(VRTUnsupportedError): read_vrt(path) - assert not isinstance(excinfo.value, VRTStableSourcesOnlyError)