Add LIKE support to Sqlite.jsm

RESOLVED FIXED in Firefox 44

Status

()

Toolkit
Async Tooling
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mak, Assigned: martianwars, Mentored)

Tracking

({dev-doc-needed})

Trunk
mozilla44
dev-doc-needed
Points:
---

Firefox Tracking Flags

(firefox42 affected, firefox44 fixed)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

2 years ago
We don't have good support for escaped LIKE parameters in Sqlite.jsm
Usually like parameters should be passed to stmt.escapeStringForLike, but what that method does is very simple (add the escape char before %, _ or the escape char itself).

I think we could just rely on the consumer doing the right thing, but we need to verify he's doing right.
We must verify that any LIKE is followed by ?, : or @ and that it also specifies ESCAPE '/' (or any other char)
For example

value LIKE :token ESCAPE '/'
value LIKE @token ESCAPE '\'
value LIKE ?token ESCAPE '!'

If it's not in this shape we should throw.

A regex should be able to do that.
Keywords: dev-doc-needed
(Assignee)

Comment 1

2 years ago
Hey Marco, I'm new to Bugzilla. I came across this bug as a "good-first-bug". Could I be assigned this bug? I do have a fair knowledge of JS / regex.
Assignee: nobody → kalpeshk2011
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
(In reply to Marco Bonardo [::mak] from comment #0)
> We don't have good support for escaped LIKE parameters in Sqlite.jsm
> Usually like parameters should be passed to stmt.escapeStringForLike, but
> what that method does is very simple (add the escape char before %, _ or the
> escape char itself).
> 
> I think we could just rely on the consumer doing the right thing, but we
> need to verify he's doing right.
> We must verify that any LIKE is followed by ?, : or @ and that it also
> specifies ESCAPE '/' (or any other char)
> For example
> 
> value LIKE :token ESCAPE '/'
> value LIKE @token ESCAPE '\'
> value LIKE ?token ESCAPE '!'
> 
> If it's not in this shape we should throw.
> 
> A regex should be able to do that.

Hey Marco, I hope you are back. I've understood the error, but not sure about where to start. I couldn't find any escapeStringForLike in Sqlite.jsm . Where is the existing support for LIKE parameters?
(Reporter)

Comment 3

2 years ago
there is no specific support for LIKE parameters in Sqlite.jsm and we won't add one, here we should just check the passed-in query and throw if the consumer is trying to hardcode a LIKE parameter instead of using binding.
basically execute and executeCached should check the query before creating a statement, a regular expression can be used to check LIKE is followed by one of the binding characters, as explained in comment 0.
(Assignee)

Comment 4

2 years ago
Created attachment 8655460 [details] [diff] [review]
sqlite_fix.patch

I didn't fully understand the regex you desired. Do let me know the changes needed.
Attachment #8655460 - Flags: review?(mak77)
(Reporter)

Comment 5

2 years ago
Comment on attachment 8655460 [details] [diff] [review]
sqlite_fix.patch

Review of attachment 8655460 [details] [diff] [review]:
-----------------------------------------------------------------

Some general hints:
1. it would be nice to move the code to a global helper function, since it's reused in 2 places.
2. similarly, that function could cache the regex in a global var on first use, so there's no need to parse it every time
3. we need to add a test for this to toolkit/modules/tests/xpcshell/test_sqlite.js, should be easy to just pass in invalid SQL to both methods and check the thrown error is as expected. you can run the test using "./mach test toolkit/modules/tests/xpcshell/test_sqlite.js"
4. please use let instead of var, per coding style

::: toolkit/modules/Sqlite.jsm
@@ +1274,5 @@
>     */
>    executeCached: function (sql, params=null, onRow=null) {
> +    var likeExists = /LIKE/i;
> +    if (sql.search(likeExists)!=-1) {
> +      var likePattern = /LIKE.+[@:?].+ESCAPE\s+'\S'/i;

I think a regex like /\bLIKE\b\s(?![@:?])/i could be used once to figure if the query contains a LIKE that is not properly bound. You may test it.
Attachment #8655460 - Flags: review?(mak77)
(Assignee)

Comment 6

2 years ago
Created attachment 8657228 [details] [diff] [review]
sqlite_fix.patch

I couldn't understand the format of the test javascript file. Did the rest as you mentioned to the best of my ability. Do let me know the further changes
Attachment #8655460 - Attachment is obsolete: true
Attachment #8657228 - Flags: review?(mak77)
(Reporter)

Comment 7

2 years ago
Comment on attachment 8657228 [details] [diff] [review]
sqlite_fix.patch

Review of attachment 8657228 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/Sqlite.jsm
@@ +1134,5 @@
>    TRANSACTION_EXCLUSIVE: "EXCLUSIVE",
>  
>    TRANSACTION_TYPES: ["DEFERRED", "IMMEDIATE", "EXCLUSIVE"],
>  
> +  validQueryCache: true,

this is not what I meant, what you should cache is the regular expression.

You should basically add a global variable, for example

let likeSqlRegex = /\bLIKE\b\s(?![@:?])/i;

then later just reuse this instead of creating a new regular expression object every time.

@@ +1219,5 @@
>    /**
> +   * Helper function to check whether LIKE is implemented using proper bindings.
> +   *
> +   * @param sql
> +   *        (string) The name of the registered statement to execute.

it's not the name of the statement, it's "The SQL query to be verified."
This also needs a @return

@@ +1221,5 @@
> +   *
> +   * @param sql
> +   *        (string) The name of the registered statement to execute.
> +  */
> +  checkLikeInQuery: function(sql) {

please move this as a simple function in the global scope, it's not an API we should expose. I'd also name it checkBoundLikeQuery

@@ +1223,5 @@
> +   *        (string) The name of the registered statement to execute.
> +  */
> +  checkLikeInQuery: function(sql) {
> +    let badLikePattern = /\bLIKE\b\s(?![@:?])/i;
> +    if (badLikePattern!=-1) {

you should just use .test() on the regular expression and return the negation of its result, cause the regular expression is checking "LIKE not followed by a bound variable" that is what we don't want. the function should return false if the query is invalid, true otherwise.

@@ +1293,5 @@
> +      throw new Error("Please enter a LIKE clause with bindings");
> +    } 
> +    else if (this.checkLikeInQuery(sql)) {
> +      throw new Error("Please enter a LIKE clause with bindings");
> +    }

you just need one check

::: toolkit/modules/tests/xpcshell/test_sqlite.js
@@ +277,5 @@
>  
> +add_task(function* test_incorrect_like_bindings() {
> +  let c = yield getDummyDatabase("incorrect_like_bindings");
> +
> +});

well, you got a database, now you should execute and executeCached an invalid query (containing an unbound LIKE) and verify that those 2 calls will throw an exception.

then remember to close the connection.
Attachment #8657228 - Flags: review?(mak77)
(Assignee)

Comment 8

2 years ago
Created attachment 8658830 [details] [diff] [review]
sqlite_fix.patch

Did pretty much as you told me to. I hope it is correct this time
Attachment #8657228 - Attachment is obsolete: true
Attachment #8658830 - Flags: review?(mak77)
(Reporter)

Comment 9

2 years ago
Comment on attachment 8658830 [details] [diff] [review]
sqlite_fix.patch

Review of attachment 8658830 [details] [diff] [review]:
-----------------------------------------------------------------

yes, it looks much better, a couple things yet, but nothing critical

::: toolkit/modules/Sqlite.jsm
@@ +70,5 @@
> + *        (string) The SQL query to be verified.
> + * @return boolean value telling us whether query was correct or not
> +*/
> +function checkBoundLikeQuery(sql) {
> +  return !likeSqlRegex.test(sql);

so, after looking at how this is uses, I think it's worth to rename to isInvalidBoundLikeQuery, remove the negation, so later checks will look like 

if (isInvalid...)
  throw...

Sorry for the bouncing, I didn't notice the double negation before...

@@ +1227,5 @@
>    executeBeforeShutdown: function(name, task) {
>      return this._connectionData.executeBeforeShutdown(this, name, task);
>    },
>  
> +

please remove the newline added here since it's not needed

::: toolkit/modules/tests/xpcshell/test_sqlite.js
@@ +277,5 @@
>  
> +add_task(function* test_incorrect_like_bindings() {
> +  let c = yield getDummyDatabase("incorrect_like_bindings");
> +
> +  let sql = "select * from dirs where path like non%";

this would be an invalid query cause the like argument should be wrapped by apices, so please use LIKE 'non%'

@@ +286,5 @@
> +  }
> +  catch (ex if ex.message.startsWith("Please enter a LIKE clause with bindings")) {
> +    success = true;
> +  }
> +  do_check_true(success);

I think you can use  Assert.throws() here to simplify the test, it will automatically try/catch and check the exception, you can find a lot of examples in the codebase
http://mxr.mozilla.org/mozilla-central/search?string=Assert.throws&case=on

(note you won't be able to use yield with it, but doesn't matter in this case cause we expect the exception to fire synchronously)

@@ +296,5 @@
> +  }
> +  catch (ex if ex.message.startsWith("Please enter a LIKE clause with bindings")) {
> +    successCached = true;
> +  }
> +  do_check_true(successCached);

ditto
Attachment #8658830 - Flags: review?(mak77) → feedback+
(Reporter)

Comment 10

2 years ago
or even isUnboundLikeQuery could be a valid name (shorter too). whatever you prefer.
(Assignee)

Comment 11

2 years ago
Created attachment 8660299 [details] [diff] [review]
sqlite_fix.patch

Added the things you told me to.
Attachment #8660299 - Flags: review?(mak77)
(Reporter)

Comment 12

2 years ago
Comment on attachment 8660299 [details] [diff] [review]
sqlite_fix.patch

Review of attachment 8660299 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/Sqlite.jsm
@@ +37,5 @@
>                                    "resource://gre/modules/PromiseUtils.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "console",
>                                    "resource://gre/modules/devtools/Console.jsm");
>  
> +// Regular expression used by checkBoundLikeQuery

comment needs updated function name

::: toolkit/modules/tests/xpcshell/test_sqlite.js
@@ +277,5 @@
>  
> +add_task(function* test_incorrect_like_bindings() {
> +  let c = yield getDummyDatabase("incorrect_like_bindings");
> +
> +  let sql = "select * from dirs where path LIKE non%";

please wrap non% in single apices

@@ +280,5 @@
> +
> +  let sql = "select * from dirs where path LIKE non%";
> +  Assert.throws(() => c.execute(sql),
> +                /Please enter a LIKE clause/);
> +  

trailing spaces

@@ +286,5 @@
> +                /Please enter a LIKE clause/);
> +
> +  yield c.close();
> +
> +});

please remove the newline before });
Attachment #8660299 - Flags: review?(mak77) → review+
(Reporter)

Updated

2 years ago
Attachment #8658830 - Attachment is obsolete: true
(Assignee)

Comment 13

2 years ago
Created attachment 8661462 [details] [diff] [review]
sqlite_fix.patch

I hope it can be pushed now.
Attachment #8660299 - Attachment is obsolete: true
Attachment #8661462 - Flags: review?(mak77)
(Reporter)

Comment 14

2 years ago
Comment on attachment 8661462 [details] [diff] [review]
sqlite_fix.patch

Review of attachment 8661462 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, I pushed your patch to Try to very tests. Once done if tests did pass, a tree sheriff will merge your patch to the main repository.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=258e7e808e29
Attachment #8661462 - Flags: review?(mak77) → review+
(Reporter)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 15

2 years ago
Finally :) @mak - I really enjoyed the process. What should be my next step? Should I fix some more bugs? Would it be possible to start contributing actively somewhere?
(Reporter)

Comment 16

2 years ago
The previous push didn't work for some infrastructure reason, here's another one
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ef31646aa5f

(In reply to Kalpesh Krishna from comment #15)
> Finally :) @mak - I really enjoyed the process. What should be my next step?
> Should I fix some more bugs? Would it be possible to start contributing
> actively somewhere?

I'd suggest to look into the list of mentored bugs for something more involving, when you feel like you're good enough for bigger tasks, you may try to look for "diamond" bugs or select an area of the product you're interested into and ask the owner for things to do in that area.
See https://developer.mozilla.org/en-US/docs/Introduction#Find_a_bug_we%27ve_identified_as_a_good_fit_for_new_contributors.
Hi, this didn't apply cleanly to fx-team:

applying sqlite_fix.patch
patching file toolkit/modules/Sqlite.jsm
Hunk #1 FAILED at 32
Hunk #2 succeeded at 57 with fuzz 2 (offset 0 lines).
1 out of 4 hunks FAILED -- saving rejects to file toolkit/modules/Sqlite.jsm.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh sqlite_fix.patch

could you take a look? Thanks!
Flags: needinfo?(kalpeshk2011)
Keywords: checkin-needed
(Reporter)

Comment 18

2 years ago
looks like due to bug 1202902, and looks like from now on we should use var in the global object... I wonder if this change was announced somewhere and which kind of protection we have to avoid reintroducing the problem.

Btw, you likely just need to merge the patch on top of the current tree, and use let when you define something (like the regex) in the global object.
(Reporter)

Comment 19

2 years ago
(In reply to Marco Bonardo [::mak] from comment #18)
>  and use let when you define something (like the regex) in the global object.

ehr, I meant "var", use var when defining in the global scope.
(Assignee)

Comment 20

2 years ago
So I just change the regex type to "var"?
Flags: needinfo?(kalpeshk2011)
(Assignee)

Comment 21

2 years ago
Created attachment 8662346 [details] [diff] [review]
sqlite_fix.patch

changing let to var
Attachment #8662346 - Flags: review?(mak77)
(Reporter)

Comment 22

2 years ago
Comment on attachment 8662346 [details] [diff] [review]
sqlite_fix.patch

Review of attachment 8662346 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/Sqlite.jsm
@@ +44,3 @@
>  // Counts the number of created connections per database basename(). This is
>  // used for logging to distinguish connection instances.
>  let connectionCounters = new Map();

did you merge the patch on top of the current tree? doesn't look like...
Attachment #8662346 - Flags: review?(mak77)
(Assignee)

Comment 23

2 years ago
Created attachment 8662421 [details] [diff] [review]
sqlite_fix.patch

I hope this will do.
Attachment #8661462 - Attachment is obsolete: true
Attachment #8662346 - Attachment is obsolete: true
Attachment #8662421 - Flags: review?(mak77)
(Reporter)

Comment 24

2 years ago
Comment on attachment 8662421 [details] [diff] [review]
sqlite_fix.patch

Review of attachment 8662421 [details] [diff] [review]:
-----------------------------------------------------------------

There's still a small bitrot, but it's fur to my delay, so I'll take care of that.
Attachment #8662421 - Flags: review?(mak77) → review+
(Reporter)

Comment 25

2 years ago
Created attachment 8664903 [details] [diff] [review]
sqlite_fix.diff

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a279a50e4bd
Attachment #8662421 - Attachment is obsolete: true
Attachment #8664903 - Flags: review+
(Reporter)

Comment 26

2 years ago
thank you for your contribution, feel free to look around for more bugs to fix.
Keywords: checkin-needed

Comment 27

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/5aeeb7979d4f
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5aeeb7979d4f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.