fix!: reject invalid filter input and make the parser correct & concurrency-safe#9
Merged
Merged
Conversation
…rrency-safe
The filter parser accepted several classes of invalid input and produced a
misleading AST, undermining its role as a validator. This fixes the core
correctness bugs and modernizes a few idioms.
Fixes:
- Reject trailing/incomplete input. Parse now requires the whole input to be
consumed; previously it stopped at the first complete expression and silently
ignored the rest (e.g. "name = 'John' garbage", "age > 30 40", "1 = 1" all
validated).
- Make FilterParser safe for concurrent use. Per-parse state moved out of the
shared struct into a per-call filterParseState, removing a data race
(confirmed under -race). SortParser/FieldsParser were already stateless.
- Negation is now recorded on the operator node. NOT IN / NOT BETWEEN /
NOT SIMILAR TO / NOT DISTINCT FROM set IsNot=true instead of being wrapped in
a UnaryOperatorNode(NOT) around a node whose IsNot was always false; NOT LIKE
uses TokenOperatorNotLike. Type() and String() now honor negation.
- Preserve the DISTINCT FROM value in a new DistinctNode.Value field; it was
parsed and discarded before.
- Lex a stray '+' as ILLEGAL instead of an empty-type token that slipped past
the illegal-token check.
Idioms:
- Use map[string]struct{} sets (was map[string]any) across all three parsers.
- FilterParser.Parse returns errors.Join of *QFVFilterError values so callers
can use errors.As/errors.Is.
- Bump go directive to 1.25.0; use range-over-int in new tests.
Adds regression tests for trailing tokens, DISTINCT value retention, concurrent
reuse (-race), and '+' lexing. Existing AST tests updated for the new negation
representation. A migration guide is documented in the README.
BREAKING CHANGE: The filter AST for negated operators changed (IsNot on the
operator node instead of a wrapping UnaryOperatorNode; NOT LIKE via
TokenOperatorNotLike), DistinctNode gained a Value field, and FilterParser.Parse
returns a joined error. Inputs that were previously accepted by mistake are now
rejected. See the "Breaking Changes & Migration" section in the README.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
The filter parser was accepting several classes of invalid input and producing a misleading AST, which undermines its role as a query validator. This PR fixes the core correctness bugs, removes a data race, and modernizes a few idioms. All findings were reproduced with tests before fixing.
Bugs fixed
Trailing / incomplete input silently accepted.
Parsestopped at the first complete expression and ignored the rest.Parsenow requires the entire input to be consumed.first_name = 'John' garbageage > 30 40first_name = 'John')1 = 1FilterParserwas not safe for concurrent use — confirmed data race undergo test -race. Per-parse state (lexer,currentToken,errors) moved off the shared struct into a per-callfilterParseState.SortParser/FieldsParserwere already stateless; now all three can be created once and shared.Dead
IsNotfields → misleading AST.NOT IN/NOT BETWEEN/NOT SIMILAR TO/NOT DISTINCT FROMwere wrapped in aUnaryOperatorNode{NOT}around a node whose ownIsNotwas alwaysfalse. They now setIsNot = trueon the operator node itself (consistent with howRegexMatchNodealready worked);NOT LIKEusesTokenOperatorNotLike.Type()andString()now honor negation.DISTINCT FROM <value>discarded the value. AddedDistinctNode.Value, which now holds the compared value.Stray
+lexed to an empty-type token that slipped past the illegal-token check. It now lexes toILLEGAL.Idiom / modernization
map[string]struct{}sets instead ofmap[string]any(the previous "avoid allocating" comment was backwards) across all three parsers.FilterParser.Parsereturnserrors.Join(...)of*QFVFilterErrorso callers can useerrors.As/errors.Is.godirective bumped to1.25.0(already pending in the working tree);range-over-int in new tests.Tests
filter_parser_regression_test.go: trailing tokens, DISTINCT value retention, concurrent reuse (run under-race), and+lexing.go test -race ./...green;go vetclean;gofmtclean; coverage 80.1%.This is a pre-1.0 breaking change (suggested tag:
v0.1.0). Full migration guide is in the README under "Breaking Changes & Migration". In short:IsNoton the node;NOT LIKE→TokenOperatorNotLike).DistinctNodegained aValuefield.Parsereturns a joined error instead of one formatted string.Code that only calls
Parseand checkserr != nilneeds no changes (beyond fixing inputs that were never actually valid).🤖 Generated with Claude Code