Allow ESCAPE in LIKE clauses to be valid SQL#31222
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #31222 +/- ##
==========================================
- Coverage 64.30% 64.30% -0.01%
==========================================
Files 1889 1898 +9
Lines 185430 185863 +433
Branches 5351 5629 +278
==========================================
+ Hits 119245 119522 +277
- Misses 56825 56980 +155
- Partials 9360 9361 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jacobshandling
left a comment
There was a problem hiding this comment.
Confirmed the test query is now interpreted as valid. OSS FTW.
sharon-fdm
left a comment
There was a problem hiding this comment.
Based on @jacobshandling's approval and minor changes after it.
for #30109
Details
This PR fixes an issue in our current SQL parsing library that was causing queries like this to be marked invalid:
This is valid in SQLite because the
\is not considered an escape character by default. From the SQLite docs (see section 3 "Literal Values (Constants)"; emphasis mine):Use of forked code
Part of the fix for this was submitted as a PR to the node-sql-parser library we now use, and merged. I then found that another fix was needed, which I submitted as a separate PR. As these fixes have yet to be made part of an official release of the library, I made a fork off of the release we were using (5.3.10) and bundled the necessary build artifacts with Fleet. We have an ADR proposing the use of submodules for this purpose; I'm happy to implement that instead if we approve that, although for a front-end module with a build step it's a bit more complicated. Hopefully this code will be released in
node-sql-parsersoon and we can revert back to using the dependency.Here is the full set of changes.
Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.