1111284 - Improve error messages of Sqlite.jsm

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

In Sqlite.jsm, whenever the underlying library mozStorage returns an error, we hide the error message, which loses information for developers.

Fixing this would be quite simple. The error message is hardcoded as "Error(s) encountered during statement execution" in Sqlite.jsm, but local variable `errors` is an array of objects with a field `message` that contains additional information.

Therefore, it should be sufficient to replace the hardcoded message with
"Error(s) encountered during statement execution: " + [error.message for (error of errors)].join(", ")".

The code lives here: http://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Sqlite.jsm?from=Sqlite.jsm#685

Comment 1

[Jacob Clark]

Attachment: Patch - Bug 1111284 - Adds error message within Sqllite.jsm to provide further details of error

Suggested patch for the above issue, please could I have a review and feedback?

Comment 2

[Jacob Clark]

Attachment: Patch - Bug 1111284 - Adds error message within Sqllite.jsm to provide further details of error

Comment 3

[Jacob Clark]

Attachment: Patch - Bug 1111284 - Adds error message within Sqllite.jsm to provide further details of error

Comment 4

[Jacob Clark]

Attachment: Modifies error message within Sqllite.jsm to provide further details of error

Comment 5

[David Teller [:Yoric] - still alive but not very active]

(In the future, you should use `review` or `feedback` flags if you wish to request review/feedback)

Comment 6

[Jacob Clark]

No problem, thanks!

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8536187 [details] [diff] [review]
Modifies error message within Sqllite.jsm to provide further details of error

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

This looks good. Let's take the opportunity to test the behavior.

You can find the relevant test in file test_sqlite.js, function `test_readOnly_clone` http://dxr.mozilla.org/mozilla-central/source/toolkit/modules/tests/xpcshell/test_sqlite.js#928-943

Let's replace the entire `try/catch` block with

Assert.rejects(
  () => clone.execute(...),
  /read-only/
);

This will check that `clone.execute(...)` throws an asynchronous error that contains the words "read-only", which should indeed appear in the string you have just modified.

To rebuild and test that it works, run
./mach build toolkit/modules
./mach xpcshell-test toolkit/modules/tests/xpcshell/test_sqlite.js

Comment 8

[Jacob Clark]

Attachment: Bug 1111284 - Improves error messages of Sqlite.jsm by including specific error message;r=Yoric

New patch to include functionality tests

Comment 9

[Jacob Clark]

Attachment: Bug 1111284 - Improves error messages of Sqlite.jsm by including specific error message;r=Yoric

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8536220 [details] [diff] [review]
Bug 1111284 - Improves error messages of Sqlite.jsm by including specific error message;r=Yoric

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

Almost there :)

::: toolkit/modules/tests/xpcshell/test_sqlite.js
@@ -931,5 @@
>  
>    let clone = yield c.clone(true);
>    // Just check that it works.
>    yield clone.execute("SELECT 1");
> -  // But should not be able to write.

You removed the comment here, can you put it back?

@@ +934,5 @@
>    yield clone.execute("SELECT 1");
> +
> +  yield Assert.rejects(clone.execute("CREATE TABLE test (id INTEGER PRIMARY KEY)"),
> +                       /readonly/);
> +  

Nit: can you remove the whitespace on this line?

Comment 11

[Jacob Clark]

Attachment: Bug 1111284 - Improves error messages of Sqlite.jsm by including specific error message;r=Yoric

Comment 12

[Jacob Clark]

Attachment: Bug 1111284 - Improves error messages of Sqlite.jsm by including specific error message;r=Yoric

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8536225 [details] [diff] [review]
Bug 1111284 - Improves error messages of Sqlite.jsm by including specific error message;r=Yoric

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

This looks good.
Let's Try it: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=59bc30e6eba0

Comment 14

[Jacob Clark]

Looks like the Try was green :)

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Let's go :)

By the way, do you have an account on Mozillians.org?

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/97af67305dab

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/97af67305dab