Repository navigation
fix(filters): validate range bounds against the column type - #55
Merged
Merged
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The validation is correctly parameterized, documented, and covered across both adapters.
Review effort: Balanced
Findings: None
What changed in this PR
Validates numeric and date range bounds before SQL execution, producing actionable REST/MCP errors.
Changes:
- Adds finite-number and ISO/DICOM date validation.
- Derives numeric attributes from schema metadata.
- Updates tests, user guidance, and changelog.
| File | Description |
|---|---|
src/idc_api/core/filters.py |
Validates and normalizes range bounds. |
src/idc_api/core/schema.py |
Classifies numeric and date range attributes. |
src/idc_api/mcp/server.py |
Documents accepted bound formats. |
tests/test_filter_shape.py |
Tests REST and MCP validation behavior. |
docs/user-guide.md |
Documents range-bound requirements. |
CHANGELOG.md |
Records the corrected behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
NumericRange admits strings because StudyDate/SeriesDate are string columns. That let two bad inputs through compile_filters: - A non-numeric string on a numeric column (e.g. instanceCount gte "1 AND 1=1", seen in production logs) failed inside DuckDB's cast, escaped every typed handler, and surfaced as an internal error / HTTP 500 with a logged traceback. The value was always a bound parameter, so this was never an injection vector. - A non-date string on a date column compared lexically and silently matched nothing. Numeric bounds are now coerced to finite floats and date bounds normalized to YYYY-MM-DD (DICOM YYYYMMDD accepted); anything else raises InvalidQueryError -> 400 / clean MCP ToolError. A correctly formatted but impossible date (2020-02-30) gets its own "not a real calendar date" message rather than being told to use the format it already has. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fedorov
force-pushed
the
fix/range-filter-bound-validation
branch
from
October 2, 2026 16:25
a81f719 to
30ae27f
Compare
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
Range-filter bounds (
ranges: {attr: {gte, lte}}) are now checked against the column's type incompile_filters, before any SQL runs.Why: the production logs showed
build_cohortfailing with:NumericRange.gte/lteis typedfloat | str, becauseStudyDate/SeriesDateare string columns. That allowed two bad inputs through:instanceCount,series_size_MB,series_*_idc_version). DuckDB's cast failed, and the error escaped every typed handler. The caller got MCP "Internal error" or REST HTTP 500, and a full traceback was logged on every request. The value was always a bound parameter, so this was not an injection vector. It was an unhandled error."nope","01/31/2020", a number). DuckDB compared it as text and quietly matched nothing, so the caller got a plausible-looking zero instead of an error.Changes
core/filters.py"5"are still accepted and show as5.0infilters_applied. Non-numbers, NaN and infinity raiseInvalidQueryError.YYYY-MM-DDform, and DICOMYYYYMMDDis accepted. Anything else raisesInvalidQueryErrorstating the expected format. An impossible date that is correctly formatted (2020-02-30) gets its own message, "…is not a real calendar date", so the caller isn't told to use the format it already used.core/schema.pynumeric_range_attributes()is built from the index schema's column types.dateflag, which definesDATE_RANGE_ATTRIBUTES.build_cohorttool description anddocs/user-guide.mdnow state the bound formats.CHANGELOG.md: two entries under Unreleased → Fixed.Errors are now a 400
invalid_queryon REST and a cleanToolErroron MCP, and the message names the field and the expected format.Testing
tests/test_filter_shape.pycover REST and MCP, numeric and date. The numeric tests were confirmed to fail onmainwithout the fix.ruff checkandruff format --checkare clean.🤖 Generated with Claude Code