Validate band index in _read_cog_http for backend parity#1700
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR closes the backend-parity gap where the HTTP eager COG read path (_read_cog_http) accepted band= but did not validate it, leading to negative/out-of-range indices behaving differently than local/dask/GPU/VRT reads.
Changes:
- Add pre-flight
bandvalidation in_read_cog_http(mirroringread_to_array) so invalid indices raise consistentIndexErrormessages. - Update the
read_to_arrayNOTE/commentary to reflect that the HTTP parity gap is now closed. - Add a dedicated regression test suite covering invalid/valid
bandvalues for HTTP reads and local-vs-HTTP parity.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
xrspatial/geotiff/_reader.py |
Adds HTTP-side band validation and updates the related inline NOTE to reflect backend parity. |
xrspatial/geotiff/tests/test_http_band_validation_1695.py |
Adds regression tests ensuring HTTP band validation and error-message parity with the local eager path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1638
to
+1645
| # ``source.close()`` runs before raising since the HTTP source | ||
| # holds a network handle. See issue #1695. | ||
| if band is not None: | ||
| if ifd.samples_per_pixel <= 1: | ||
| if band != 0: | ||
| source.close() | ||
| raise IndexError( | ||
| f"band={band} requested on a single-band file.") |
The HTTP eager read path accepted band= but never validated the index. band=-1 silently returned the last channel via numpy negative indexing on the post-decode slice, band>=samples_per_pixel leaked a raw numpy IndexError, and band=N (N != 0) on a single-band HTTP COG was dropped on the floor because the post-decode slice was gated on arr.ndim == 3 and samples_per_pixel > 1. Mirror the validator added to read_to_array in PR #1673 inside _read_cog_http, after the orientation and window guards and before the tile fetch. source.close() runs before raising since the HTTP source holds a network handle. Update the NOTE comment in read_to_array (L2031-2034 before this change) that flagged the gap; the HTTP branch is now part of the "all backends agree" set. Add xrspatial/geotiff/tests/test_http_band_validation_1695.py with 11 tests covering negative band, out-of-range band, single-band nonzero band, single-band band=0, band=None, and cross-path parity with the local eager read on the IndexError messages. Fixes #1695
brendancol
force-pushed
the
fix-http-band-validation-2026-05-12
branch
from
4942e89 to
4b42fce
Compare
The previous comment claimed source.close() runs before raising because the HTTP source holds a network handle, but _HTTPSource.close() is currently a no-op. The urllib3 PoolManager is shared module-level, not per-source. Reword to state that source.close() is called for symmetry with the success-path teardown so future resource-holding sources work correctly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #1695. The HTTP eager read path (
_read_cog_http) acceptedband=but did not validate the index, so it diverged from the local, dask, GPU, and VRT paths after PR #1673:band=-1silently returned the last channel via numpy negative indexing on the post-decode slice (L1638).band=NwithN >= samples_per_pixelleaked a raw numpyIndexErrorwhose message exposed the internal slice shape.band=N(N != 0) on a single-band HTTP COG was dropped on the floor because the post-decode slice was gated onarr.ndim == 3 and samples_per_pixel > 1; the call returned the full single-band raster as ifbandhad beenNone.The in-file NOTE at
_reader.py:2031-2034already flagged the gap. PR #1669 / #1680 added the slice; the mirror-the-check follow-up was never landed.Changes
bandvalidator in_read_cog_httpafter the orientation and window guards and before the tile fetch. Same wording asread_to_array.source.close()runs before raising since the HTTP source holds a network handle.read_to_arrayso it documents the now-closed gap instead of pointing at unfinished work.xrspatial/geotiff/tests/test_http_band_validation_1695.pywith 11 tests.Test plan
pytest xrspatial/geotiff/tests/test_http_band_validation_1695.py-> 11 passed.pytest xrspatial/geotiff/tests/test_band_validation_1673.py xrspatial/geotiff/tests/test_http_window_band_planar_1669.py-> 19 passed (local-path validator and HTTP window/band parity unchanged).pytest xrspatial/geotiff/tests/-> 1700 passed, 7 skipped, 3 pre-existing matplotlib palette-deepcopy failures intest_features.py(unrelated to this change, fail on baseline too).