1073358 - Sqlite.jsm should return some measure of what happened regardless of whether a row handler was used

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

When using Sqlite.jsm's execute()/executeCached(), if you pass in a row handler, the promise returned always resolves to null. This is so large result sets aren't kept in memory. However, there are cases where it is useful to get some form of result returned, so you don't need to hook up something special in your row handler to track things. 

In bug 1067899 I have on such example use case.

Returning the entire result set is obviously bad. However, returning the number of rows is lightweight and gives adequate information about what happened - without needing to hook up some tracking in your row handler.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Marco: This is blocking bug 1067899. It's smallish, but it should be done in isolation.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Comment 3

[Marco Mucci [:MarcoM]]

Added to IT 35.2

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8495691 [details] [diff] [review]
Patch v1

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

I think it's confusing and not very useful to return the number of rows, because if one is executing an update or a delete, he might exect it to return the number of affected rows, and we don't have that information at hand.

My idea is that instead we return a bool, stating whether the onrow handler has been called at least once.

::: toolkit/modules/Sqlite.jsm
@@ +631,5 @@
>          self._pendingStatements.delete(index);
>  
>          switch (reason) {
>            case Ci.mozIStorageStatementCallback.REASON_FINISHED:
>              // If there is an onRow handler, we always resolve to null.

please remember to update this comment

Comment 5

[Marco Bonardo [:mak]]

please also update here

https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Sqlite.jsm

"If onRow is specified, the returned promise will be resolved with null. Else, the resolved value will be an array of mozIStorageRow."

should just take a few seconds.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1.1

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8495913 [details] [diff] [review]
Patch v1.1

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

::: toolkit/modules/tests/xpcshell/test_sqlite.js
@@ +282,5 @@
>      yield c.executeCached(sql, ["dir" + i]);
>    }
>  
>    let i = 0;
> +  let result = yield c.execute("SELECT * FROM DIRS", null, function onRow(row) {

nit: hasResult

@@ +317,3 @@
>    do_check_eq(i, 5);
>  
>    yield c.close();

could you please add a simple test that checks also the false case? like a SELECT * FROM dirs WHERE path = "nonexistent"?

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/fx-team/rev/6351ff630f1a

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6351ff630f1a