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

Add error callback to list() on DbQueryCollection in store.sql as sql might fail #131

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

miklschmidt
Copy link

All the sql stores has error callbacks in their executeSql methods, but the list() method in store.sql doesn't use them. So when a query fails, you'll never know.

I ran into this issue while using the migration plugin, i accidentally created a table with the plural version of the entity's name, and didn't get any callbacks while using the list() function after doing Entity.all().

I added support for error callbacks in .list() on DbQueryCollection, to fix this issue.

@@ -579,19 +579,21 @@ function config(persistence, dialect) {
* @param callback function to be called taking an array with
* result objects as argument
*/
persistence.DbQueryCollection.prototype.list = function (tx, callback) {
persistence.DbQueryCollection.prototype.list = function (tx, callback, errorCallback) {

Choose a reason for hiding this comment

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

Conflicting, for instance, with the convention of persistence.flush (callback should check second arg for error)

@AndreaSpinelli
Copy link

I support the issue, because .list(/) and .count() do fail! (see below for an example)

However, IMHO, a better solution would be just to use the same callback on line 711 of persistence.store.sql.js:

      }, callback );

by the way, se same applies to .count() and again a simple fix is changing line 844 to

      }, callback );

This is consistent with the general convention of persistence.js that callbacks should test their second argument; if it is not-null, it is a SQL error.

A simple index.html for testing:

<title>Test</title> <script type="text/javascript" src="persistence.store.sql.js"></script> <script type="text/javascript" src="persistence.store.websql.js"></script> <script type="text/javascript" src="persistence.js"></script> <script> persistence.store.websql.config(persistence, 'test_test', 'test', 10000); var schema_of_foo = { bar: "TEXT NOT NULL", baz: 'INTEGER' }; var foo = persistence.define( 'foo', schema_of_foo ); persistence.schemaSync(function(tx) { foo.all().filter("foobaz","=","x").list(function(result, error) { if( error ){ alert("OK! Error detected in list: " + error.message ); } else { alert("KO! error not detected..."); } }); foo.all().filter("foobaz","=","x").count(function(result, error) { if( error ){ alert("OK! Error detected in count: " + error.message ); } else { alert("KO! error not detected..."); } }); }); </script>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants