geotiff: activate mixed-band-metadata fail-closed check (#1987 PR 5)

Registers ``_check_read_mixed_band_metadata`` so VRT reads whose bands
declare disagreeing per-band ``<NoDataValue>`` sentinels raise
``MixedBandMetadataError`` by default. The check and its
``band_nodata=`` opt-out were staged in #2031 but not registered: the
activation needed the existing VRT test sites that mosaic distinct
per-band sentinels to migrate to ``band_nodata='first'`` first.

Wires the opt-out into ``open_geotiff`` and ``read_geotiff_dask`` so
all three VRT entry points expose the same kwarg, with a clear
ValueError on non-VRT sources (same dispatch-kwarg guard pattern as
``missing_sources`` / ``on_gpu_failure`` / ``max_cloud_bytes``).

Migrates the four VRT test files whose fixtures intentionally mosaic
bands with distinct sentinels:

- ``test_vrt_band_nodata_1598.py``
- ``test_vrt_multiband_int_nodata_1611.py``
- ``test_vrt_int_source_float_dtype_1616.py``
- ``test_vrt_multiband_dtype_1696.py``

Each migrated call adds ``band_nodata='first'`` so the regression keeps
asserting the legacy flatten-to-first-band semantics, now via the
documented opt-out instead of implicitly.

Adds ``test_mixed_band_metadata_fail_closed_1987.py`` with 11 cases
pinning the fail-closed contract end-to-end: default raise, chunked
raise, opt-out passthrough, shared-sentinel acceptance, one-band-only
acceptance, ``open_geotiff`` / ``read_geotiff_dask`` propagation, and
the non-VRT-source rejection on both entry points.

Refs #1987.

Author: brendancol

xrspatial/geotiff/__init__.py

@@ -254,6 +254,7 @@ def open_geotiff(source: str | BinaryIO, *,
                  missing_sources: str = _MISSING_SOURCES_SENTINEL,
                  allow_rotated: bool = False,
                  allow_unparseable_crs: bool = False,
+                 band_nodata: str | None = None,
                  mask_nodata: bool = True,
                  ) -> xr.DataArray:
     """Read a GeoTIFF, COG, or VRT file into an xarray.DataArray.
@@ -325,6 +326,17 @@ def open_geotiff(source: str | BinaryIO, *,
         return a partial mosaic. Passing this kwarg with a non-VRT
         source raises ``ValueError`` because the policy only applies to
         the VRT pipeline. See ``read_vrt`` for the full description.
+    band_nodata : {'first', None}, optional
+        VRT-only. Opt-out for the fail-closed check that rejects VRT
+        sources whose bands declare disagreeing per-band nodata
+        sentinels (issue #1987 PR 5). When ``None`` (the default), a VRT
+        that mosaics bands with different sentinels raises
+        ``MixedBandMetadataError``; flattening to one value would let
+        one band's valid pixels collide with another band's sentinel.
+        Pass ``band_nodata='first'`` to keep the legacy behaviour of
+        using band 0's sentinel for the whole mosaic. Passing this
+        kwarg with a non-VRT source raises ``ValueError`` because the
+        policy only applies to the VRT pipeline.
     mask_nodata : bool, default True
         If True (the default), replace the nodata sentinel with ``NaN``;
         integer rasters get promoted to ``float64`` first so NaN can be
@@ -409,6 +421,19 @@ def open_geotiff(source: str | BinaryIO, *,
             "Pass a .vrt path to enable the VRT pipeline, or drop "
             "missing_sources to keep the default GeoTIFF path.")
 
+    # ``band_nodata`` is the #1987 PR 5 opt-out for the mixed-band
+    # metadata fail-closed check. It only has meaning on the VRT pipeline
+    # (a plain GeoTIFF has one nodata sentinel per file, not per band),
+    # so reject the kwarg up front on non-VRT sources rather than letting
+    # it leak into ``read_vrt`` and confuse the caller about what the
+    # opt-out actually controls.
+    if band_nodata is not None and not _is_vrt_source:
+        raise ValueError(
+            "band_nodata only applies to VRT sources. "
+            "Pass a .vrt path to enable the VRT pipeline, or drop "
+            "band_nodata to keep the default GeoTIFF path. "
+            "See issue #1987.")
+
     # ``max_cloud_bytes`` is the eager fsspec-read budget. Only
     # ``_read_to_array`` on the eager non-VRT, non-GPU, non-dask branch
     # consumes it; the GPU (``read_geotiff_gpu``), dask
@@ -473,6 +498,7 @@ def open_geotiff(source: str | BinaryIO, *,
                         max_pixels=max_pixels,
                         allow_rotated=allow_rotated,
                         allow_unparseable_crs=allow_unparseable_crs,
+                        band_nodata=band_nodata,
                         mask_nodata=mask_nodata,
                         **vrt_kwargs)
 

xrspatial/geotiff/_backends/dask.py

@@ -41,6 +41,7 @@ def read_geotiff_dask(source: str, *,
                       max_pixels: int | None = None,
                       allow_rotated: bool = False,
                       allow_unparseable_crs: bool = False,
+                      band_nodata: str | None = None,
                       mask_nodata: bool = True) -> xr.DataArray:
     """Read a GeoTIFF as a dask-backed DataArray for out-of-core processing.
 
@@ -74,6 +75,11 @@ def read_geotiff_dask(source: str, *,
         directly.
     name : str or None
         Name for the DataArray.
+    band_nodata : {'first', None}, optional
+        VRT-only opt-out for the fail-closed mixed-band-metadata check
+        (issue #1987 PR 5). Forwarded verbatim to ``read_vrt`` when the
+        source is a ``.vrt`` file. Passing it with a non-VRT GeoTIFF
+        source raises ``ValueError``.
     mask_nodata : bool, default True
         If True, replace the nodata sentinel with NaN per chunk (integer
         rasters get promoted to ``float64``). If False, skip the
@@ -114,8 +120,21 @@ def read_geotiff_dask(source: str, *,
         return read_vrt(
             source, dtype=dtype, window=window, band=band, name=name,
             chunks=chunks, max_pixels=max_pixels,
+            allow_rotated=allow_rotated,
+            allow_unparseable_crs=allow_unparseable_crs,
+            band_nodata=band_nodata,
             mask_nodata=mask_nodata,
         )
+    # ``band_nodata`` only has meaning for the VRT path (per-band sentinel
+    # ambiguity). Reject the kwarg up front on non-VRT GeoTIFF inputs so
+    # callers learn the opt-out is being dropped, matching the
+    # ``open_geotiff`` guard. See issue #1987 PR 5.
+    if band_nodata is not None:
+        raise ValueError(
+            "band_nodata only applies to VRT sources. "
+            "Pass a .vrt path to enable the VRT pipeline, or drop "
+            "band_nodata to keep the default GeoTIFF path. "
+            "See issue #1987.")
 
     # P5: HTTP COG sources used to fire one IFD/header GET per chunk
     # task. Parse metadata once here so every delayed task can reuse it.

xrspatial/geotiff/_validation.py

@@ -950,11 +950,13 @@ def _same_nodata(a: float, b: float) -> bool:
     return a == b
 
 
-# NOT registered by default. The mixed-band check has a much larger
-# migration cost than its sibling read-side checks (around 35 existing
-# test sites would need to opt in via ``band_nodata='first'`` to keep
-# their legacy assertions), so it lands as a follow-up PR that bundles
-# the check activation with the test migration. The check itself and
-# the ``band_nodata=`` VRT kwarg ship now so the follow-up is a
-# one-line registration call plus the test sweep.
-# register_read_metadata_check(_check_read_mixed_band_metadata)
+# Registered as of issue #1987 PR 5. The check was staged in the
+# preceding bundle (#2031) along with the ``band_nodata=`` VRT kwarg
+# but not registered, because activation needed a coordinated migration
+# of the VRT test sites that read fixtures with disagreeing per-band
+# sentinels. The migration sweep ships alongside this registration.
+# Callers that still want the legacy flatten-to-first-band behaviour
+# pass ``band_nodata='first'`` to ``read_vrt`` / ``open_geotiff`` /
+# ``read_geotiff_dask``; the explicit opt-in surfaces the per-band
+# ambiguity at the call site instead of papering over it silently.
+register_read_metadata_check(_check_read_mixed_band_metadata)

xrspatial/geotiff/tests/test_mixed_band_metadata_fail_closed_1987.py

@@ -0,0 +1,270 @@
+"""Fail-closed coverage for ``MixedBandMetadataError`` (issue #1987 PR 5).
+
+A VRT can mosaic source bands that declare disagreeing per-band
+``<NoDataValue>`` sentinels. The legacy reader picked band 0's sentinel
+for the whole mosaic, which let band N's valid pixels collide with
+band 0's sentinel after the flatten-to-scalar step. The fail-closed
+default refuses the ambiguity; ``band_nodata='first'`` keeps the legacy
+behaviour explicitly.
+
+The validator check itself, the ``band_nodata=`` kwarg on
+``read_vrt``, and the per-VRT-read context plumbing landed in
+the #2031 bundle. This PR registers the check and forwards
+``band_nodata`` through ``open_geotiff`` and ``read_geotiff_dask``.
+The tests below pin the four entry-point contracts:
+
+* default ``read_vrt`` raises on disagreeing per-band sentinels
+* default ``read_vrt(chunks=...)`` raises before any task fires
+* ``band_nodata='first'`` opts back into the legacy reader
+* ``band_nodata`` raises on non-VRT sources for ``open_geotiff`` and
+  ``read_geotiff_dask``
+"""
+from __future__ import annotations
+
+import os
+
+import numpy as np
+import pytest
+
+from xrspatial.geotiff import (
+    GeoTIFFAmbiguousMetadataError,
+    MixedBandMetadataError,
+    open_geotiff,
+    read_geotiff_dask,
+    read_vrt,
+    to_geotiff,
+)
+from xrspatial.geotiff._writer import write
+
+
+def _write_mixed_band_vrt(tmp_path, *, sentinel_a=65535, sentinel_b=65000):
+    """Two uint16 sources with distinct sentinels, mosaiced into one VRT.
+
+    ``[1, 1]`` is the sentinel pixel in each source so the legacy
+    reader's flatten-to-first-band step would mis-mask band 1.
+    """
+    band0 = np.array([[1, 2], [3, sentinel_a]], dtype=np.uint16)
+    band1 = np.array([[7, 8], [9, sentinel_b]], dtype=np.uint16)
+    p0 = str(tmp_path / 'mixed_band_1987_a.tif')
+    p1 = str(tmp_path / 'mixed_band_1987_b.tif')
+    write(band0, p0, nodata=sentinel_a, compression='none', tiled=False)
+    write(band1, p1, nodata=sentinel_b, compression='none', tiled=False)
+    vrt_path = str(tmp_path / 'mixed_band_1987.vrt')
+    vrt_xml = f"""<VRTDataset rasterXSize="2" rasterYSize="2">
+  <GeoTransform>0.0, 1.0, 0.0, 0.0, 0.0, -1.0</GeoTransform>
+  <VRTRasterBand dataType="UInt16" band="1">
+    <NoDataValue>{sentinel_a}</NoDataValue>
+    <SimpleSource>
+      <SourceFilename relativeToVRT="0">{p0}</SourceFilename>
+      <SourceBand>1</SourceBand>
+      <SrcRect xOff="0" yOff="0" xSize="2" ySize="2"/>
+      <DstRect xOff="0" yOff="0" xSize="2" ySize="2"/>
+    </SimpleSource>
+  </VRTRasterBand>
+  <VRTRasterBand dataType="UInt16" band="2">
+    <NoDataValue>{sentinel_b}</NoDataValue>
+    <SimpleSource>
+      <SourceFilename relativeToVRT="0">{p1}</SourceFilename>
+      <SourceBand>1</SourceBand>
+      <SrcRect xOff="0" yOff="0" xSize="2" ySize="2"/>
+      <DstRect xOff="0" yOff="0" xSize="2" ySize="2"/>
+    </SimpleSource>
+  </VRTRasterBand>
+</VRTDataset>"""
+    with open(vrt_path, 'w') as f:
+        f.write(vrt_xml)
+    return vrt_path
+
+
+def _write_shared_sentinel_vrt(tmp_path, *, sentinel=65535):
+    """Two uint16 sources mosaiced with the *same* per-band sentinel.
+
+    The fail-closed check must not trigger on a VRT whose per-band
+    nodata agrees; the check is about ambiguity, not about per-band
+    declarations.
+    """
+    band0 = np.array([[1, 2], [3, sentinel]], dtype=np.uint16)
+    band1 = np.array([[7, 8], [9, sentinel]], dtype=np.uint16)
+    p0 = str(tmp_path / 'shared_band_1987_a.tif')
+    p1 = str(tmp_path / 'shared_band_1987_b.tif')
+    write(band0, p0, nodata=sentinel, compression='none', tiled=False)
+    write(band1, p1, nodata=sentinel, compression='none', tiled=False)
+    vrt_path = str(tmp_path / 'shared_band_1987.vrt')
+    vrt_xml = f"""<VRTDataset rasterXSize="2" rasterYSize="2">
+  <GeoTransform>0.0, 1.0, 0.0, 0.0, 0.0, -1.0</GeoTransform>
+  <VRTRasterBand dataType="UInt16" band="1">
+    <NoDataValue>{sentinel}</NoDataValue>
+    <SimpleSource>
+      <SourceFilename relativeToVRT="0">{p0}</SourceFilename>
+      <SourceBand>1</SourceBand>
+      <SrcRect xOff="0" yOff="0" xSize="2" ySize="2"/>
+      <DstRect xOff="0" yOff="0" xSize="2" ySize="2"/>
+    </SimpleSource>
+  </VRTRasterBand>
+  <VRTRasterBand dataType="UInt16" band="2">
+    <NoDataValue>{sentinel}</NoDataValue>
+    <SimpleSource>
+      <SourceFilename relativeToVRT="0">{p1}</SourceFilename>
+      <SourceBand>1</SourceBand>
+      <SrcRect xOff="0" yOff="0" xSize="2" ySize="2"/>
+      <DstRect xOff="0" yOff="0" xSize="2" ySize="2"/>
+    </SimpleSource>
+  </VRTRasterBand>
+</VRTDataset>"""
+    with open(vrt_path, 'w') as f:
+        f.write(vrt_xml)
+    return vrt_path
+
+
+def _write_one_band_no_sentinel_vrt(tmp_path, *, sentinel=65535):
+    """One band declares a sentinel, the other does not.
+
+    ``[b.nodata for b in vrt.bands]`` is ``[sentinel, None]``. The
+    check considers ``None`` entries "no declaration" rather than a
+    distinct sentinel, so only one concrete value is present and the
+    check must accept.
+    """
+    band0 = np.array([[1, 2], [3, sentinel]], dtype=np.uint16)
+    band1 = np.array([[7, 8], [9, 99]], dtype=np.uint16)
+    p0 = str(tmp_path / 'one_band_sentinel_a.tif')
+    p1 = str(tmp_path / 'one_band_sentinel_b.tif')
+    write(band0, p0, nodata=sentinel, compression='none', tiled=False)
+    write(band1, p1, nodata=None, compression='none', tiled=False)
+    vrt_path = str(tmp_path / 'one_band_sentinel.vrt')
+    vrt_xml = f"""<VRTDataset rasterXSize="2" rasterYSize="2">
+  <GeoTransform>0.0, 1.0, 0.0, 0.0, 0.0, -1.0</GeoTransform>
+  <VRTRasterBand dataType="UInt16" band="1">
+    <NoDataValue>{sentinel}</NoDataValue>
+    <SimpleSource>
+      <SourceFilename relativeToVRT="0">{p0}</SourceFilename>
+      <SourceBand>1</SourceBand>
+      <SrcRect xOff="0" yOff="0" xSize="2" ySize="2"/>
+      <DstRect xOff="0" yOff="0" xSize="2" ySize="2"/>
+    </SimpleSource>
+  </VRTRasterBand>
+  <VRTRasterBand dataType="UInt16" band="2">
+    <SimpleSource>
+      <SourceFilename relativeToVRT="0">{p1}</SourceFilename>
+      <SourceBand>1</SourceBand>
+      <SrcRect xOff="0" yOff="0" xSize="2" ySize="2"/>
+      <DstRect xOff="0" yOff="0" xSize="2" ySize="2"/>
+    </SimpleSource>
+  </VRTRasterBand>
+</VRTDataset>"""
+    with open(vrt_path, 'w') as f:
+        f.write(vrt_xml)
+    return vrt_path
+
+
+def test_read_vrt_rejects_mixed_per_band_nodata(tmp_path):
+    """Default ``read_vrt`` raises on disagreeing per-band sentinels."""
+    vrt_path = _write_mixed_band_vrt(tmp_path)
+    with pytest.raises(MixedBandMetadataError) as exc_info:
+        read_vrt(vrt_path)
+    msg = str(exc_info.value)
+    assert "65535" in msg and "65000" in msg
+    assert "band_nodata='first'" in msg
+    assert "#1987" in msg
+
+
+def test_read_vrt_chunked_rejects_mixed_per_band_nodata(tmp_path):
+    """The chunked dispatch validates before scheduling any task."""
+    vrt_path = _write_mixed_band_vrt(tmp_path)
+    with pytest.raises(MixedBandMetadataError):
+        read_vrt(vrt_path, chunks=1)
+
+
+def test_read_vrt_band_nodata_first_opts_back_in(tmp_path):
+    """``band_nodata='first'`` keeps the legacy flatten-to-first path."""
+    vrt_path = _write_mixed_band_vrt(tmp_path)
+    r = read_vrt(vrt_path, band_nodata='first')
+    # Legacy semantics: band 0's sentinel becomes ``attrs['nodata']``.
+    assert r.attrs.get('nodata') == 65535.0
+
+
+def test_read_vrt_shared_sentinel_accepts(tmp_path):
+    """A VRT whose bands agree on a single sentinel is not ambiguous."""
+    vrt_path = _write_shared_sentinel_vrt(tmp_path)
+    r = read_vrt(vrt_path)
+    assert r.attrs.get('nodata') == 65535.0
+
+
+def test_read_vrt_only_one_band_declares_sentinel_accepts(tmp_path):
+    """``None`` per-band entries count as "no declaration", not a value.
+
+    A VRT with one declared sentinel and one undeclared band has only
+    one concrete sentinel so the fail-closed check must accept.
+    """
+    vrt_path = _write_one_band_no_sentinel_vrt(tmp_path)
+    # Should not raise.
+    read_vrt(vrt_path)
+
+
+def test_open_geotiff_propagates_mixed_band_rejection(tmp_path):
+    """``open_geotiff`` on a ``.vrt`` source routes through ``read_vrt``."""
+    vrt_path = _write_mixed_band_vrt(tmp_path)
+    with pytest.raises(MixedBandMetadataError):
+        open_geotiff(vrt_path)
+
+
+def test_open_geotiff_band_nodata_first_passes_through(tmp_path):
+    """``open_geotiff(band_nodata='first')`` forwards to ``read_vrt``."""
+    vrt_path = _write_mixed_band_vrt(tmp_path)
+    r = open_geotiff(vrt_path, band_nodata='first')
+    assert r.attrs.get('nodata') == 65535.0
+
+
+def test_read_geotiff_dask_band_nodata_first_passes_through(tmp_path):
+    """``read_geotiff_dask`` on a ``.vrt`` source forwards the kwarg."""
+    vrt_path = _write_mixed_band_vrt(tmp_path)
+    r = read_geotiff_dask(vrt_path, chunks=1, band_nodata='first')
+    assert r.attrs.get('nodata') == 65535.0
+
+
+def test_open_geotiff_band_nodata_rejected_on_non_vrt_source(tmp_path):
+    """``band_nodata`` is VRT-only; reject up front on plain GeoTIFFs.
+
+    Same dispatch-kwarg pattern as ``missing_sources``, ``on_gpu_failure``,
+    and ``max_cloud_bytes``: silently dropping a backend-specific kwarg
+    is the bug class #1561 / #1605 / #1685 / #1795 / #1810 / #1974 closed
+    on the rest of the surface.
+    """
+    arr = np.zeros((2, 2), dtype=np.uint16)
+    arr_da = _wrap_2d(arr)
+    p = tmp_path / 'plain.tif'
+    to_geotiff(arr_da, str(p), compression='none', tiled=False)
+    with pytest.raises(ValueError, match="band_nodata only applies to VRT"):
+        open_geotiff(str(p), band_nodata='first')
+
+
+def test_read_geotiff_dask_band_nodata_rejected_on_non_vrt_source(tmp_path):
+    """``read_geotiff_dask`` matches ``open_geotiff``'s VRT-only guard."""
+    arr = np.zeros((2, 2), dtype=np.uint16)
+    arr_da = _wrap_2d(arr)
+    p = tmp_path / 'plain.tif'
+    to_geotiff(arr_da, str(p), compression='none', tiled=False)
+    with pytest.raises(ValueError, match="band_nodata only applies to VRT"):
+        read_geotiff_dask(str(p), chunks=1, band_nodata='first')
+
+
+def test_mixed_band_metadata_error_subclasses_base(tmp_path):
+    """``MixedBandMetadataError`` is catchable via the base class."""
+    vrt_path = _write_mixed_band_vrt(tmp_path)
+    with pytest.raises(GeoTIFFAmbiguousMetadataError):
+        read_vrt(vrt_path)
+
+
+def _wrap_2d(arr):
+    """Wrap a 2D numpy array as a minimal DataArray for ``to_geotiff``."""
+    import xarray as xr
+    h, w = arr.shape
+    return xr.DataArray(
+        arr,
+        dims=('y', 'x'),
+        coords={
+            'y': np.arange(h, dtype=np.float64),
+            'x': np.arange(w, dtype=np.float64),
+        },
+        attrs={'crs': 4326,
+               'transform': (1.0, 0.0, 0.0, 0.0, -1.0, 0.0)},
+    )

xrspatial/geotiff/tests/test_reader_kwarg_order_1935.py

@@ -44,6 +44,7 @@
     "missing_sources",
     "allow_rotated",
     "allow_unparseable_crs",
+    "band_nodata",
     "mask_nodata",
 )