1188760 - Add LIKE support to Sqlite.jsm

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect)

Description

[Marco Bonardo [:mak]]

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.

Comment 1

[Kalpesh Krishna [:martianwars]]

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.

Comment 2

[Kalpesh Krishna [:martianwars]]

(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?

Comment 3

[Marco Bonardo [:mak]]

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.

Comment 4

[Kalpesh Krishna [:martianwars]]

Attachment: sqlite_fix.patch

I didn't fully understand the regex you desired. Do let me know the changes needed.

Comment 5

[Marco Bonardo [:mak]]

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.

Comment 6

[Kalpesh Krishna [:martianwars]]

Attachment: 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

Comment 7

[Marco Bonardo [:mak]]

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.

Comment 8

[Kalpesh Krishna [:martianwars]]

Attachment: sqlite_fix.patch

Did pretty much as you told me to. I hope it is correct this time

Comment 9

[Marco Bonardo [:mak]]

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

Comment 10

[Marco Bonardo [:mak]]

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

Comment 11

[Kalpesh Krishna [:martianwars]]

Attachment: sqlite_fix.patch

Added the things you told me to.

Comment 12

[Marco Bonardo [:mak]]

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 });

Comment 13

[Kalpesh Krishna [:martianwars]]

Attachment: sqlite_fix.patch

I hope it can be pushed now.

Comment 14

[Marco Bonardo [:mak]]

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

Comment 15

[Kalpesh Krishna [:martianwars]]

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?

Comment 16

[Marco Bonardo [:mak]]

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.

Comment 17

[Carsten Book [:Tomcat]]

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!

Comment 18

[Marco Bonardo [:mak]]

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.

Comment 19

[Marco Bonardo [:mak]]

(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.

Comment 20

[Kalpesh Krishna [:martianwars]]

So I just change the regex type to "var"?

Comment 21

[Kalpesh Krishna [:martianwars]]

Attachment: sqlite_fix.patch

changing let to var

Comment 22

[Marco Bonardo [:mak]]

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...

Comment 23

[Kalpesh Krishna [:martianwars]]

Attachment: sqlite_fix.patch

I hope this will do.

Comment 24

[Marco Bonardo [:mak]]

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.

Comment 25

[Marco Bonardo [:mak]]

Attachment: sqlite_fix.diff

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a279a50e4bd

Comment 26

[Marco Bonardo [:mak]]

thank you for your contribution, feel free to look around for more bugs to fix.

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5aeeb7979d4f

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5aeeb7979d4f