Check for silent lexicographic comparison against string-typed value#240
Draft
thodson-usgs wants to merge 1 commit intoDOI-USGS:mainfrom
Draft
Check for silent lexicographic comparison against string-typed value#240thodson-usgs wants to merge 1 commit intoDOI-USGS:mainfrom
value#240thodson-usgs wants to merge 1 commit intoDOI-USGS:mainfrom
Conversation
559b466 to
68d1a81
Compare
Move all CQL filter and chunking logic out of api.py / utils.py into a dedicated dataretrieval/waterdata/filters.py module (with chunked as a decorator on the per-request fetch), and extract get_nearest_continuous into a sibling nearest.py — so the entire filter feature can be removed by deleting two source files, two test files, and two re-export lines. Adds a pre-flight check that raises on unquoted-numeric comparisons (value > 1000, parameter_code IN (60, 61), value BETWEEN 5 AND 10), since every Water Data API queryable is string-typed and the server either returns HTTP 500 or silently produces lexicographically-sorted wrong rows. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fbd0f51 to
d81dd33
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 (draft — for discussion)
Every queryable field on every OGC collection in the Water Data API is
type=stringserver-side. So any unquoted numeric comparison in a CQL-text filter —value >= 1000,parameter_code = 60,district_code = 1— isn't a valid numeric comparison: empirically the server returns HTTP 500 Internal Server Error. Even if the user does quote the literal, the comparison is lexicographic (e.g.value > '10'returns rows wherevalue='34.52'because first-char'3'>'1'), and zero-padded codes likeparameter_code = '60'silently match nothing because the real values are'00060'-shaped.Either way, the user's intent was numeric, they get either 500 or silent wrong rows, and the failure is opaque. This PR catches the pattern client-side and raises a clear
ValueErrorbefore the request fires.Motivation
Flagged during review of DOI-USGS/dataRetrieval#880 (R side), where
ldecicco-USGSpushed back on exposing a genericfilterkwarg:This is the smallest change that addresses her concern without removing the
filterfeature.What the check does
Runs once per cql-text filter, inside
_plan_filter_chunks, before any HTTP traffic:Scope: universal, not a watchlist
Every queryable property across every OGC endpoint is
type=string— confirmed empirically:continuousvalue,parameter_code,statistic_iddailyvalue,parameter_code,statistic_idfield-measurementsvalue,parameter_codelatest-continuousvalue,parameter_code,statistic_idlatest-dailyvalue,parameter_code,statistic_idtime-series-metadataparameter_code,statistic_id,hydrologic_unit_codemonitoring-locationsmonitoring_location_number,district_code,state_code,county_codechannel-measurementsmeasurement_number,channel_flow,channel_width,channel_area,channel_velocitySince there's no such thing as a legitimate numeric comparison on this API, the regex flags any
<identifier> <op> <unquoted numeric literal>(or the reverse), regardless of field. Quoted literals (value >= '1000') are not flagged — the caller has signalled they want sort-order semantics.Live evidence
Test plan
ruff check/ruff format --checkpass.pytest tests/waterdata_utils_test.py— 66/66 pass (32 prior + 34 new). The new tests cover:>=,>,<=,<,=,!=) × both orderings (x OP NandN OP x), floats, negatives, multiple real-world fields (value, parameter_code, statistic_id, district_code, county_code, hydrologic_unit_code, channel_flow, channel_velocity), and nested in AND/OR expressions.INlists, and false-positive guards (identifiers appearing only inside quoted string literals likename = 'see district_code = 1 in docs').get_continuousbefore_construct_api_requestsis ever called (mock-verified).USGS-02238500/continuous— unquoted numeric RHS consistently returns 500; quoted literal returns 200 with lex-sorted results.Open for discussion
_plan_filter_chunksalongside the other filter validation (chunkability, URL-budget). Could hoist to theget_ogc_dataentry.CAST(value AS FLOAT) > 1000): the regex is scoped to simple\b<ident>\b \s* <op> \s* <num>patterns and — empirically — the server doesn't support CAST or CQL2 functions on these endpoints anyway, so the false-positive rate should be near zero in practice.Marked draft so it can ride along with the R-side discussion before landing.
🤖 Generated with Claude Code