feat: Add contains-substring filter for String fields#3392
Conversation
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a new ChangesContains Substring Constraint
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant User as Dashboard User
participant Filter as Filters UI
participant Builder as addConstraintFromValues
participant Query as Parse.Query
User->>Filter: select "contains substring" constraint
Filter->>Builder: addConstraintFromValues(field, 'contains', value)
Builder->>Query: query.contains(field, String(value))
Query-->>Filter: returns matching records via escaped regex
Related Issues: Suggested labels: enhancement, filters Suggested reviewers: dblythy, mtrezza 🐰 A whisker twitches, sniffing through strings so tight, 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
I don't understand what this PR is trying to achieve? What's the purpose of adding a "contains" filter, which was removed in #2991 because it was just a subset of "matches regex"? |
|
Oh, I didn't realise this previously existed. I thought the ticket was about adding the FWIW Happy to close though if you prefer |
|
@coderabbitai how are startsWith, endsWith implemented? Do they have the same issue as #2991 with a fixed "i" regex option? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "---Filters.js startsWith/endsWith---"
rg -n -B2 -A5 "startsWith|endsWith" src/lib/Filters.js
echo "---queryFromFilters.js startsWith/endsWith---"
rg -n -B2 -A10 "startsWith|endsWith" src/lib/queryFromFilters.js
echo "---full switch block---"
rg -n "case '" src/lib/queryFromFilters.jsLength of output: 665 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "---Filters.js full---"
cat -n src/lib/Filters.js
echo "---queryFromFilters.js full---"
cat -n src/lib/queryFromFilters.jsLength of output: 20102 🏁 Script executed: #!/bin/bash
set -euo pipefail
fd -type f "ParseQuery.js" 2>/dev/null || true
fd . node_modules/parse/lib -e js | rg -i "ParseQuery" || true
echo "---search startsWith/endsWith/contains/quote in parse package---"
rg -n -A15 "startsWith\s*\(" node_modules/parse/lib/node/ParseQuery.js 2>/dev/null || rg -n -A15 "startsWith\s*\(" $(fd -a ParseQuery.js node_modules 2>/dev/null | head -1)
echo "---"
rg -n -A15 "endsWith\s*\(" $(fd -a ParseQuery.js node_modules 2>/dev/null | head -1)
echo "---contains---"
rg -n -A15 "\bcontains\s*\(" $(fd -a ParseQuery.js node_modules 2>/dev/null | head -1)
echo "---quote---"
rg -n -A10 "quote\s*\(" $(fd -a ParseQuery.js node_modules 2>/dev/null | head -1)Length of output: 1981 🏁 Script executed: #!/bin/bash
set -euo pipefail
F=node_modules/parse/lib/node/ParseQuery.js
echo "---_regexStartWith---"
rg -n -A10 "_regexStartWith" "$F"
echo "---endsWith---"
rg -n -A15 "^\s*endsWith\s*\(" "$F"
echo "---contains(---"
rg -n -A15 "^\s*contains\s*\(" "$F"
echo "---matches(---"
rg -n -A20 "^\s*matches\s*\(" "$F"
echo "---quote/escape---"
rg -n -A15 "quote\(|_quote\(" "$F"
echo "---function quote definition---"
rg -n -B2 -A15 "function quote" "$F"Length of output: 6990
In
Neither call passes a Inside the Parse JS SDK (
Conclusion: |
|
@coderabbitai The issue in #2991 was also that no options can be customized, so emojis (/u option) for example are not recognized. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "---modifiers refs---"
rg -n "modifiers" src --type=js -C3
echo "---gear icon / settings component in filter row---"
rg -n "gear|Gear" src/components -i -C2 | head -80Length of output: 12098
So the practical consequence: If you'd like, I can treat this as a follow-up scope item — e.g., either (a) exposing an |
|
@coderabbitai We need a unified solution; the same issue exists with startsWith, endsWith, no matching flexibility, which is exactly why #2991 was merged. These kind of filter conditions with hard-coded, arbitrary, unintuitive matching limitations are impractical. How can we allow the user to quickly access a startsWith/endsWith/contains functionality and optionally use unescaped strings, given the already implemented features? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "---Filter.react.js modifiers/gear---"
rg -n -B3 -A10 "modifiers|RegexOptionsButton" src/components/Filter/Filter.react.js
echo "---FilterRow.react.js gear icon render condition---"
rg -n -B3 -A15 "RegexOptionsButton" src/components/BrowserFilter/FilterRow.react.jsLength of output: 3836
The core idea: Keep the existing safety property (starts/ends/contains are literal-by-default, no manual escaping needed) but let all four "regex-family" operators ( 1. // Instead of:
case 'contains':
query.contains(field, String(compareTo));
break;
// Do (mirrors what SDK's contains()/quote() does, but keeps modifiers):
case 'contains': {
const escaped = String(compareTo).replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
query.matches(field, escaped, modifiers);
break;
}For case 'starts':
query.startsWith(field, compareTo, modifiers);
break;
case 'ends':
query.endsWith(field, compareTo, modifiers);
break;2. // Line 80, currently:
modifiers: newConstraint === 'matches' ? 'i' : undefined,
// Change to cover the whole regex family:
modifiers: ['matches', 'starts', 'ends', 'contains'].includes(newConstraint) ? 'i' : undefined,(Open question: should the default still auto-check 3. Whatever the current guard is (something like Net effect:
If this direction looks right to you, I'm ready to fold it directly into this PR's scope (updating the three files above) rather than opening a separate follow-up — just say the word and I'll kick off the planning. Failed to handle agent chat message. Please try again. |
|
@coderabbitai Should the strings entered in starts, ends, contains be automatically regex-escaped? Is that the current behavior? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
F="node_modules/parse/lib/node/ParseQuery.js"
echo "---startsWith/endsWith/contains source (confirm quote usage)---"
rg -n -B2 -A8 "^\s*startsWith\s*\(|^\s*endsWith\s*\(|^\s*contains\s*\(" "$F"
echo "---quote function---"
rg -n -B2 -A12 "function quote" "$F"
echo "---dashboard queryFromFilters.js starts/ends/contains cases---"
rg -n -B2 -A6 "case 'starts'|case 'ends'|case 'contains'" src/lib/queryFromFilters.jsLength of output: 898 🏁 Script executed: #!/bin/bash
set -euo pipefail
fd -a ParseQuery.js node_modules 2>/dev/null | head -5
echo "---fallback: search anywhere---"
fd -a "ParseQuery.js" / 2>/dev/null | head -5
echo "---dashboard queryFromFilters.js starts/ends/contains cases---"
rg -n -B2 -A6 "case 'starts'|case 'ends'|case 'contains'" src/lib/queryFromFilters.jsLength of output: 633 🏁 Script executed: #!/bin/bash
set -euo pipefail
cat -n src/lib/queryFromFilters.jsLength of output: 10290
case 'starts':
...
query.startsWith(field, compareTo);
break;
case 'ends':
query.endsWith(field, compareTo);
break;
...
case 'contains':
query.contains(field, String(compareTo));
break;None of these three cases do any manual escaping in the dashboard code itself — they pass the raw user-typed string straight to the Parse JS SDK's But that doesn't mean the input is treated as regex. As I verified earlier from
So yes — this is already the current behavior: This also means my earlier suggestion to manually rebuild the regex-escape logic for ✏️ Learnings added
|
|
@coderabbitai What if we add an "escape" option to the "matches regex" options list. Then we change the startsWith/endsWith/contains to be a shortcut to a version of the regex filter, with "escape" checked and disabled, so the user can't change it. So for all 3 filters we'd use the regex condition since it supports the modifiers and they are all disguised regex conditions anyway. This also solves the issue that startsWith/endsWith/contains don't currently allow modifiers. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
fd -a "RegexOptionsButton" src
echo "---RegexOptionsButton full---"
F=$(fd -a "RegexOptionsButton.react.js" src | head -1)
cat -n "$F"
echo "---Filter.react.js full---"
cat -n src/components/Filter/Filter.react.js
echo "---FilterRow.react.js relevant---"
rg -n -B5 -A20 "RegexOptionsButton" src/components/BrowserFilter/FilterRow.react.jsLength of output: 741 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "---grep RegexOptionsButton---"
rg -n "RegexOptionsButton" src -l
echo "---list components dirs---"
fd -a "FilterRow" src
fd -a "Filter.react.js" src
echo "---cat Filter.react.js---"
cat -n src/components/Filter/Filter.react.js 2>/dev/null || true
echo "---cat FilterRow.react.js---"
FR=$(fd -a "FilterRow.react.js" src | head -1)
echo "FR=$FR"
cat -n "$FR" 2>/dev/null || trueLength of output: 27409
The i/u/m/x/s modifiers are not the same kind of thing as "escape." Confirmed in So making "escape" a checkbox living in the same
That's not less branching than today's per-constraint Leaner alternative that gets you the same end result: Keep // queryFromFilters.js
const escapeRegex = s => s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
switch (constraint) {
case 'matches':
query.matches(field, String(compareTo), modifiers);
break;
case 'starts':
query.matches(field, '^' + escapeRegex(String(compareTo)), modifiers);
break;
case 'ends':
query.matches(field, escapeRegex(String(compareTo)) + '$', modifiers);
break;
case 'contains':
query.matches(field, escapeRegex(String(compareTo)), modifiers);
break;
}And on the UI side, just drop the Net effect is identical to what you want — one unified regex-family implementation, modifiers available everywhere, literal/no-escaping-required-by-user preserved for If you still want the visible "escape" checkbox for transparency (so users explicitly see "this operator is secretly a regex under the hood"), I can build that instead — it's not wrong, just more moving parts for the same behavior. Let me know which direction you want, and I'll kick off `` for whichever one you pick — including the default- Failed to handle agent chat message. Please try again. |
2 participants
Closes #2153
Problem
For a String column, the only substring-style filter is matches regex, which passes the input to
Parse.Query#matchesverbatim as a regular expression. Regex metacharacters therefore have special meaning:.matches any character, and metacharacters like(are invalid on their own, so a plain literal search either returns the wrong rows or fails with a server error. There is no way to do a simple literal substring search.Fix
Add a separate contains substring operator for String fields, backed by the SDK's
Parse.Query#contains, which wraps the input in\Q...\Eso the whole value is matched literally (the same quotingstartsWith/endsWithalready use). The existing matches regex operator is unchanged, so raw regex remains available.The "rename the confusing operator" half of the issue was already handled in #2991 (the old mislabelled entry was replaced by the explicit matches regex), so this PR only adds the new safe operator. This does not touch the Array-field labels discussed in #2052.
Unit test added in
src/lib/tests/queryFromFilters.test.jscovering literal substring matching, metacharacter escaping, and a contrast assertingmatchesstays a raw regex.The new operator
The String field condition list now offers contains substring below matches regex:
Literal
.(data:a(b,c.d,hello,world)matches regex treats
.as a wildcard and returns every row:contains substring matches
.literally and returns onlyc.d:Literal
a(bmatches regex with
a(bis an invalid regular expression, so the query fails (500) and nothing is shown:contains substring with
a(bmatches the parenthesis literally and returns the correct row:Summary by CodeRabbit
New Features
Bug Fixes