Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Not escaping ' in iLIKE query properly #1114

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Not escaping ' in iLIKE query properly #1114

wants to merge 9 commits into from

Conversation

trzysiek
Copy link
Member

@trzysiek trzysiek commented Dec 15, 2024

#1038

As new tests show for a little bit, the issue seems fixed. Instead of doing nothing, I escape all \ and ' in all string literals. There might be some edge case where it's not the correct behaviour, but I thought for a bit and haven't found any.
Even in regexp, where we escape _ (Clickhouse's equivalent of . in regex), escaping it twice because of this change (sending \\_ to Clickhouse instead of \_ before), seems to work just as fine.

Because of array joins issue, sample panel still doesn't work, but there are no more any escaping error messages, like in the issue description.
So, when running on main, we get a lot of:

quesma-1                 | Dec 22 15:36:13.876 ERR quesma/quesma/dual_write_proxy.go:252 > quesma request failed: Q3006: Database query has failed. You may get more details in the Quesma console.  clickhouse: query failed. err: code: 62, message: Syntax error: failed at position 488 ('(') (line 1, col 488): (((arrayJoin("category") __quesma_match 'Men's Clothing')

But when running on this branch, we only have errors like this (notice ' properly escaped):

quesma-1                 | Dec 22 15:32:01.714 ERR quesma/quesma/dual_write_proxy.go:252 > quesma request failed: Q3006: Database query has failed. You may get more details in the Quesma console.  clickhouse: query failed. err: code: 62, message: Syntax error: failed at position 278 ('__quesma_match') (line 1, col 278): __quesma_match 'Men\'s Clothing')

Additional info:
We discussed with Rafał some other way to make it work: stop sending any strings in the query itself, but pass all of them as parameters, where the driver will take care of any escaping issues, etc.
It's probably a good idea for the future, but would require some non-trivial amount of work, unlike this straightforward solution, which I already had done.

Copy link

cloudflare-workers-and-pages bot commented Dec 21, 2024

Deploying quesma with  Cloudflare Pages  Cloudflare Pages

Latest commit: e384249
Status: ✅  Deploy successful!
Preview URL: https://eca0c455.quesma.pages.dev
Branch Preview URL: https://issue-1038.quesma.pages.dev

View logs

@trzysiek trzysiek marked this pull request as ready for review December 22, 2024 15:58
@trzysiek trzysiek requested a review from a team as a code owner December 22, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant