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

sqlite: Escape '%' and '_' in search queries. #4487

Merged
merged 1 commit into from
Apr 12, 2022

Conversation

progval
Copy link
Contributor

@progval progval commented Feb 16, 2022

I picked '@' arbitrarily, it doesn't matter much.
I just don't like '' because it needs to be escaped itself in the JS code,
which is annoying.

Closes GH-4481

I picked '@' arbitrarily, it doesn't matter much.
I just don't like '\' because it needs to be escaped itself in the JS code,
which is annoying.
@MaxLeiter MaxLeiter added this to the 4.3.2 milestone Feb 16, 2022
@itsjohncs
Copy link
Member

Why do you think these characters ought to be escaped?

@itsjohncs itsjohncs self-assigned this Feb 17, 2022
@MaxLeiter
Copy link
Member

MaxLeiter commented Feb 17, 2022

@itsjohncs there's some discussion in #4481, here's my comment:

I agree with @brunnre8, there's a chance you want to search for % or _. We can implement more user-friendly and/or transparent methods that use them on the back-end in the future. I personally like the idea of github-style search, i.e. from:MaxLeiter in:#thelounge but that's for another issue

I think we might want to escape them with \, which is at least consistent with javascript

@itsjohncs
Copy link
Member

What an obvious thing to miss, of course there's more explanation in the linked issue 😅. Thank you @MaxLeiter. Sorry @progval for not looking at that issue.

I think we might want to escape them with , which is at least consistent with javascript

Since it's an implementation detail that isn't exposed to the user, I'm pretty indifferent to the actual escape character that's used. The forward slash would certainly be a bit more normal though so might cause a bit less confusion for folks reading the code 🤷‍♀️.

Copy link
Member

@brunnre8 brunnre8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks Val.

I don't care what the escape is, @ is fine and the variable naming doesn't leave much room for interpretation so 🤷

Copy link
Member

@itsjohncs itsjohncs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay. We're waiting to merge any more PRs until we make a release so this'll be sitting until then, but we'll merge this afterwards.

I did some quick testing just now to verify that % and _ are treated literally in search queries. LGTM.

@itsjohncs itsjohncs added the Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors. label Mar 2, 2022
@brunnre8 brunnre8 added the Status: release-after-next reviewed and ready to merge, postponed to release after next (feature freeze of current release) label Mar 9, 2022
@MaxLeiter
Copy link
Member

LGTM 🎉

thanks for your patience @progval!

@MaxLeiter MaxLeiter removed the Status: release-after-next reviewed and ready to merge, postponed to release after next (feature freeze of current release) label Apr 12, 2022
@MaxLeiter MaxLeiter merged commit 20ed3e6 into thelounge:master Apr 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

History search queries are not sanitized
4 participants