fix: escape the union operator in tag filter values - #717
Draft
vishal-bala wants to merge 1 commit into
Draft
Conversation
TokenEscaper covered braces, backslash, whitespace, comma and the
wildcards, but neither character class covered `|`. RediSearch reads it
as a union, so `Tag("f") == "a|b"` matched either value instead of the
literal one, and under DIALECT 2 `|` binds looser than the implicit
intersection -- so a union reaching the top level of a combined filter
stopped the surrounding clauses constraining the result.
Add `|` to DEFAULT_ESCAPED_CHARS. Tag values are escaped element by
element before being joined, so the list form that renders a union
deliberately is unaffected. ESCAPED_CHARS_NO_WILDCARD is left alone
because the `%` operator documents `|` as a union between wildcard
patterns; the comment there now records the asymmetry.
Text filters are not covered: Text.__str__ never calls the escaper.
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.
Motivation
A
|inside a tag filter value reaches the query string unescaped, where RediSearch reads it as its union operator.TokenEscapercovers{,},\, whitespace,,,*and?, but neither of its character classes covers|, so a value carrying one widens its own clause into a union rather than matching literally.Measured on 0.27.0:
That is a match on either value, not on the one asked for. It matters most where a tag value comes from outside the caller's control and is combined with other clauses: under DIALECT 2
|binds looser than the implicit intersection, so a union appearing at the top level of a combined expression stops the surrounding clauses constraining the result at all.The same gap affects the two full-text tokenizers, which split a raw user query on whitespace, escape each token, and join the results with
" | ". A single whitespace-delimited token containingfoo|barcurrently contributes its own union to the generated query.Changes
|joins the default escaped setTag values are escaped element by element before being joined, so the list form that renders a union deliberately is unaffected:
ESCAPED_CHARS_NO_WILDCARDis deliberately left aloneThat class backs
preserve_wildcards=True, which is what the%operator uses, andTag.__mod__'s own docstring documentsTag("category") % "elec*|*soft"as multiple wildcard patterns. Escaping|there would turn a documented union into a literal that matches nothing, and it would do so silently. The comment above the class now records that reasoning so the asymmetry is not read as an oversight.Secondary changes
|, the preserved wildcard union, and the list form.redisvl/mcp/filters.pycorrected: it claimed no escaper in RedisVL touches|, which is no longer true of the default path.Notes
This changes the rendering of an existing public API, so it is worth being precise about who sees a difference. Any caller passing a literal
|inside a singleTagequality or inequality value gets a literal match where they previously got a union. A grep oftests/,docs/andredisvl/found nothing relying on that behaviour, and the list form remains the documented way to express a tag union. The%operator is unchanged.Textfilters are not covered here and remain unescaped:Text.__str__interpolates its value straight into@field:("%s")without calling the escaper at all, so adding a character to the escaper's class does not reach them. That is a separate defect with a different fix, since escaping a text value has to keep spaces live for phrase matching. The MCP server already guards its own text boundary inredisvl/mcp/filters.py.Release Notes
This is a backwards-incompatible change to how tag filter values render. Tag filter values containing a
|are now escaped rather than interpreted as RediSearch's union operator, soTag("field") == "a|b"matches the single literal valuea|bwhere it previously matched documents tagged eitheraorb. Queries relying on that implicit union will return fewer results, or none, after upgrading.Passing a list remains the supported way to express a tag union and is unchanged:
Tag("field") == ["a", "b"]still matches either value. Code depending on the old behaviour should pass a list. The%pattern operator is also unchanged, and continues to treat|as a union between wildcard patterns.The previous behaviour meant a tag value drawn from outside the caller's control could widen its own clause. Under DIALECT 2
|binds looser than the implicit intersection, so such a union appearing at the top level of a combined filter stopped the surrounding clauses constraining the result.Next Steps