Skip to content

fix(filters): validate range bounds against the column type - #55

Merged
fedorov merged 1 commit into
mainfrom
fix/range-filter-bound-validation
Oct 2, 2026
Merged

fedorov merged 1 commit into
mainfrom
fix/range-filter-bound-validation

Conversation

@fedorov

@fedorov fedorov commented Oct 2, 2026 •

Copy link
Copy Markdown
Member

Summary

Range-filter bounds (ranges: {attr: {gte, lte}}) are now checked against the column's type in compile_filters, before any SQL runs.

Why: the production logs showed build_cohort failing with:

_duckdb.ConversionException: Conversion Error: Could not convert string '1 AND 1=1' to INT64

NumericRange.gte/lte is typed float | str, because StudyDate/SeriesDate are string columns. That allowed two bad inputs through:

  1. A non-numeric string on a numeric column (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.
  2. A non-date string on a date column ("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
    • Numeric bounds are converted to finite floats. Numeric strings like "5" are still accepted and show as 5.0 in filters_applied. Non-numbers, NaN and infinity raise InvalidQueryError.
    • Date bounds are normalized to the stored YYYY-MM-DD form, and DICOM YYYYMMDD is accepted. Anything else raises InvalidQueryError stating 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.py
    • numeric_range_attributes() is built from the index schema's column types.
    • Date columns are marked explicitly with a date flag, which defines DATE_RANGE_ATTRIBUTES.
  • build_cohort tool description and docs/user-guide.md now state the bound formats.
  • CHANGELOG.md: two entries under Unreleased → Fixed.

Errors are now a 400 invalid_query on REST and a clean ToolError on MCP, and the message names the field and the expected format.

Testing

  • New tests in tests/test_filter_shape.py cover REST and MCP, numeric and date. The numeric tests were confirmed to fail on main without the fix.
  • Full suite: 98 passed. ruff check and ruff format --check are clean.

🤖 Generated with Claude Code

Copilot AI balanced review requested due to automatic review settings October 2, 2026 16:13

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
fedorov force-pushed the fix/range-filter-bound-validation branch from a81f719 to 30ae27f Compare October 2, 2026 16:25
@fedorov
fedorov merged commit ce893b2 into main Oct 2, 2026
6 checks passed
@fedorov
fedorov deleted the fix/range-filter-bound-validation branch October 2, 2026 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants