Strict input validation family-wide (issue #194) #213
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,116 @@ | ||
| """Shared strict input validators for word and offset arrays (issue #194). | ||
|
|
||
| The toc module's validation discipline, hoisted to one home so the whole | ||
| family answers bad input the same way: refuse float-typed words and offsets | ||
| instead of truncating them, range-check before any narrowing cast instead of | ||
| wrapping, and name the parameter and the offending value. The strict form | ||
| has caught two real bug classes -- the issue #185 uncatchable-panic arc and | ||
| PR #192's silent uint64 wrap -- so with issue #187's consolidation giving | ||
| each operation one polymorphic entry point, that posture is applied at every | ||
| choke point rather than kept as a toc-only stance. | ||
|
|
||
| Zero-size input is the one deliberate acceptance: an untyped empty container | ||
| (``[]``, ``()``, ``np.array([])``) is float64 by numpy's default, but it is | ||
| not numeric, it is empty (the ruling :class:`~mortie.toc_object.Toc` already | ||
| applied to its source argument) -- so it passes through as a typed empty | ||
| array rather than being refused for a dtype it never chose. | ||
| """ | ||
|
|
||
| import numpy as np | ||
|
|
||
|
|
||
| def _as_u64(values, name): | ||
| """Validate non-negative integer input and return it as uint64. | ||
|
|
||
| Float input is refused rather than truncated, and negative input is | ||
| refused rather than wrapped -- packed words are unsigned, and a negative | ||
| here is almost always a signed *reinterpretation* of a word whose top | ||
| bit is set (base cells 7-11; spec section 1) or a legacy signed id, both | ||
| of which would wrap into a different, possibly valid, word. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| values : array_like | ||
| Integer-typed values (any shape); zero-size input of any dtype is | ||
| accepted as empty. | ||
| name : str | ||
| Parameter name to blame in refusal messages. | ||
|
|
||
| Returns | ||
| ------- | ||
| numpy.ndarray | ||
| The values as ``uint64``, at least 1-D; no copy when the input is | ||
| already ``uint64``. | ||
|
|
||
| Raises | ||
| ------ | ||
| ValueError | ||
| If ``values`` is not integer-typed, or any value is negative -- | ||
| naming ``name`` and the first offending value. | ||
| """ | ||
| arr = np.atleast_1d(np.asarray(values)) | ||
| if arr.size == 0: | ||
| return arr.astype(np.uint64) | ||
| if arr.dtype.kind not in "iu": | ||
| raise ValueError( | ||
| f"{name} must be integer-typed, got dtype {arr.dtype}") | ||
| if arr.dtype.kind == "i": | ||
| flat = arr.ravel() | ||
| neg = flat[flat < 0] | ||
| if neg.size: | ||
| raise ValueError( | ||
| f"{name} must be non-negative, got {int(neg[0])}") | ||
| return arr.astype(np.uint64, copy=False) | ||
|
|
||
|
|
||
| def _as_offsets(offsets): | ||
| """Validate arrow list offsets and return them as contiguous int64. | ||
|
|
||
| Integer-typed by the same rule :func:`_as_u64` applies to words: a float | ||
| offset array would otherwise cast silently, truncating ``2.9`` to a group | ||
| boundary at 2 rather than saying so. The same standard rules out the | ||
| ``uint64`` values the cast cannot represent -- at or above ``2**63`` they | ||
| would wrap negative, and the Rust validator would then describe the | ||
| wrapped copy rather than the offset that was passed. Monotonicity and | ||
| bounds stay the Rust validator's job -- it names the offending group. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| offsets : array_like | ||
| Integer-typed arrow list offsets; zero-size input of any dtype is | ||
| accepted and left for the kernel's own emptiness refusal. | ||
|
|
||
| Returns | ||
| ------- | ||
| numpy.ndarray | ||
| The offsets as a contiguous 1-D ``int64`` array. | ||
|
|
||
| Raises | ||
| ------ | ||
| ValueError | ||
| If ``offsets`` is not integer-typed, or a value is at or above | ||
| ``2**63`` -- naming the first offending value. | ||
| """ | ||
| arr = np.atleast_1d(np.asarray(offsets)) | ||
| if arr.size == 0: | ||
| return np.ascontiguousarray(arr.astype(np.int64).ravel()) | ||
| if arr.dtype.kind not in "iu": | ||
| # A Python int past int64 coerces to float64 in the untyped asarray | ||
| # above; re-try the exact cast so an oversized *integer* is refused as | ||
| # out of range rather than as a float. | ||
| try: | ||
| np.asarray(offsets, dtype=np.int64) | ||
|
Owner
Author
There was a problem hiding this comment. 🤖 from Claude (review) should-fix — this probe cast runs for every non-integer dtype, not just the Python-int-past-int64 case it is commented for, and it leaks numpy's own failure out of the family's message register. >>> from mortie._validate import _as_offsets
>>> _as_offsets(['a'])
ValueError: invalid literal for int() with base 10: 'a' # was, pre-hoist:
# offsets must be integer-typed, got dtype <U1
>>> _as_offsets([None, 1])
TypeError: int() argument must be a string, ... not 'NoneType' # not even a ValueError
>>> _as_offsets(np.array([0.0, np.nan]))
RuntimeWarning: invalid value encountered in cast # spurious warning on a refusal path
ValueError: offsets must be integer-typed, got dtype float64The old >>> warnings.simplefilter("error"); _as_offsets(np.array([0.0, np.nan]))
RuntimeWarning: invalid value encountered in castSuggest gating the probe to the case it exists for —
Owner
Author
There was a problem hiding this comment. 🤖 from Claude Folded in if arr.dtype.kind not in "iu":
# A Python int past int64 lands as float64 (or, further out, object)
# in the untyped asarray above, so an oversized *integer* would
# otherwise be blamed on its promoted dtype. Look for one directly
# rather than probe-casting: a trial cast would leak numpy's own
# message for strings and None, and would raise a RuntimeWarning on
# NaN that becomes the exception under warnings-as-errors -- in every
# case burying this family's named refusal (issue #194 review).
if arr.dtype.kind in "fO":
flat = np.atleast_1d(np.asarray(offsets, dtype=object)).ravel()
bad = next((v for v in flat.tolist() if isinstance(v, int)
and not -2**63 <= v < 2**63), None)
if bad is not None:
raise ValueError(f"offsets must fit in int64, got {bad}")
raise ValueError(
f"offsets must be integer-typed, got dtype {arr.dtype}")A scan beats a suppressed probe here because it needs no All three of your cases, under Including Tests in On the drift you noted: |
||
| except OverflowError: | ||
| flat = np.atleast_1d(np.asarray(offsets, dtype=object)).ravel() | ||
| bad = next((v for v in flat if isinstance(v, int) | ||
| and not -2**63 <= v < 2**63), None) | ||
| raise ValueError( | ||
| f"offsets must fit in int64, got {bad}") from None | ||
| raise ValueError( | ||
| f"offsets must be integer-typed, got dtype {arr.dtype}") | ||
| if arr.dtype.kind == "u": | ||
| too_big = arr > np.iinfo(np.int64).max | ||
| if too_big.any(): | ||
| raise ValueError( | ||
| f"offsets must fit in int64, got {int(arr[too_big][0])}") | ||
| return np.ascontiguousarray(arr.astype(np.int64).ravel()) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 from Claude (review)
should-fix — this row is not accurate as written. Only the
dissolve=Falsearm ofto_geometry/to_wkb/to_wktgained validation (the validator sits in_per_cell_polygons); the defaultdissolve=Truearm goes throughdissolve.py's untouchednp.asarray(..., dtype=np.uint64)and still truncates/wraps — see the repro onmortie/geometry.py:700. Either fix the seam (preferred) or narrow this row and the corresponding inventory row in the PR body so the breaking-register does not over-promise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 from Claude
Fixed the seam (preferred option) in
8f11d8d, then tightened this row inb3a727erather than narrowing it.The row now pins the property by name instead of leaving "word intake" to be read charitably:
So the breaking-register no longer over-promises, and it now says the thing that was actually at issue — the default spelling — out loud, which is the part a future reader would otherwise have to re-derive.