Change of PhoneGap SQLitePlugin API to be closer to the W3 SQL API

[brodybits]

From issue #11: it would be great to use almost the same API as the HTML5/W3 SQL API.

I hereby propose a couple small changes to the plugin in the executeSql() member function of the transaction object and the PGSQLitePlugin database object, so that the array of values is kept separate from the SQL string itself. This change makes the SQLitePlugin API closer to the HTML5/W3 SQL API.

This change would also make the application code more portable to the existing Android Storage plugin from: https://github.com/apache/incubator-cordova-android/blob/master/framework/src/org/apache/cordova/Storage.java

I made some changes to the lawnchair adapter but I did not have a chance to test them. Perhaps it would be better to put these changes together with the Cordova version.

[brodybits]

This pull request is now merged with #23 and is hereby closed.

[brodybits]

Thanks @marcucio I will adapt the CoffeeScript version myself.

[brodybits]

@marcucio I do not have much time to test these changes, however I found a few things that I would like to do differently. I made these changes in the CoffeeScript version and compiled to Javascript but I do not have time to do much testing. Here are some of the things I would do differently:

• I checked a few places on the Internet and I believe the success and error callbacks were already in the right order. Please take a quick look at the example in: http://www.w3.org/TR/offline-webapps/#sql
• In PGSQLitePluginTransaction.prototype.executeSql it is possible to save the reference to the current transaction object in a "self" variable and pass that reference into the success and error callback functions.
• In PGSQLitePluginTransaction.prototype.complete incude the tx reference in the callbacks
• In PGSQLitePlugin.prototype.executeSql returning the result object just like PGSQLitePluginTransaction.prototype.executeSql

Following your suggestion, I tried making the following changes to pgsqlite_plugin.coffee and I hope you can follow them. Please let me know if you want me to post a special commit with the changes to the Javascript version.

diff --git a/src/pgsqlite_plugin.coffee b/src/pgsqlite_plugin.coffee
index a3f230f..26ed6e7 100644
--- a/src/pgsqlite_plugin.coffee
+++ b/src/pgsqlite_plugin.coffee
@@ -50,7 +50,13 @@ class root.PGSQLitePlugin

   executeSql: (sql, params, success, error) ->
     throw new Error "Cannot executeSql without a query" unless sql
-    opts = getOptions({ query: [sql].concat(params || []), path: @dbPath }, success, error)
+    successcb = (execres) ->
+      res =
+        item: (i) ->
+          execres[i]
+        length: execres.length
+      success(res)
+    opts = getOptions({ query: [sql].concat(params || []), path: @dbPath }, successcb, error)
     PhoneGap.exec("PGSQLitePlugin.backgroundExecuteSql", opts)
     return

@@ -79,14 +85,28 @@ class root.PGSQLitePluginTransaction
     @executes = []

   executeSql: (sql, params, success, error) ->
-    @executes.push getOptions({ query: [sql].concat(params || []), path: @dbPath }, success, error)
+    txself = @
+    successcb = (execres) ->
+      res =
+        item: (i) ->
+          execres[i]
+        length: execres.length
+      success(txself, res)
+    errorcb = (res) ->
+      error(txself, res)
+    @executes.push getOptions({ query: [sql].concat(params || []), path: @dbPath }, successcb, errorcb)
     return

   complete: (success, error) ->
     throw new Error "Transaction already run" if @__completed
     @__completed = true
+    txself = @
+    successcb = (res) ->
+      success(txself, res)
+    errorcb = (res) ->
+      error(txself, res)
     begin_opts = getOptions({ query: [ "BEGIN;" ], path: @dbPath })
-    commit_opts = getOptions({ query: [ "COMMIT;" ], path: @dbPath }, success, error)
+    commit_opts = getOptions({ query: [ "COMMIT;" ], path: @dbPath }, successcb, errorcb)
     executes = [ begin_opts ].concat(@executes).concat([ commit_opts ])
     opts = { executes: executes }
     PhoneGap.exec("PGSQLitePlugin.backgroundExecuteSqlBatch", opts)

Chris

[marcucio]

I agree with all of your changes except for one, here is the spec I was going off of (and webkit follows it):

http://www.w3.org/TR/webdatabase/

Only in the transaction is the error callback before the success callback, everywhere else success comes before error:

4.3 Asynchronous database API

interface Database { void transaction(in SQLTransactionCallback callback, in optional SQLTransactionErrorCallback errorCallback, in optional SQLVoidCallback successCallback);

I am not sure why in this one situation it is backwards but this is how it works in webkit

[marcucio]

if you want to build the .js I can test it out

[marcucio]

I know why it seems backwards to me,

void transaction(in SQLTransactionCallback callback, in optional SQLTransactionErrorCallback errorCallback, in optional SQLVoidCallback successCallback);

it has the usual SQLTransactionCallback and SQLTransactionErrorCallback but there is an optional successCallback as the third callback that is called after the entire transaction completes.

[brodybits]

Sorry Mike, I missed that one. Give me a few minutes and I will post a version for you to test.

[brodybits]

Can you please check the changes from the following commit: https://github.com/chbrody/Phonegap-SQLitePlugin/commit/d864bae2eb80a005ac327053983b36ec5e4d79b4 (W3-SQL-API-test-1 branch)?

[brodybits]

@marcucio I have reorganized my fork quite a bit and it has a new location: https://github.com/chbrody/Cordova-SQLiteNative so just look at its default master branch.

You will see both "Legacy PhoneGap" and new Cordova versions, and I have checked in Lawnchair automatic testing suites to help verify that I don't break anything major. I have checked in a couple API changes in the Legacy-PhoneGap-iPhone subdirectory to make db.transaction() and tx.executeSql() follow the HTML5 SQL API more closely. Here are the changes in the CoffeeScript:

--- a/Legacy-PhoneGap-iPhone/src/pgsqlite_plugin.coffee
+++ b/Legacy-PhoneGap-iPhone/src/pgsqlite_plugin.coffee
@@ -54,7 +54,7 @@ class root.PGSQLitePlugin
     PhoneGap.exec("PGSQLitePlugin.backgroundExecuteSql", opts)
     return

-  transaction: (fn, success, error) ->
+  transaction: (fn, error, success) ->
     t = new root.PGSQLitePluginTransaction(@dbPath)
     fn(t)
     t.complete(success, error)
@@ -79,14 +79,36 @@ class root.PGSQLitePluginTransaction
     @executes = []

   executeSql: (sql, values, success, error) ->
-    @executes.push getOptions({ query: [sql].concat(values || []), path: @dbPath }, success, error)
+    txself = @
+    successcb = null
+    if success
+      successcb = (execres) ->
+        saveres = execres
+        res =
+          rows:
+            item: (i) ->
+              saveres.rows[i]
+            length: saveres.rows.length
+          rowsAffected: saveres.rowsAffected
+          insertId: saveres.insertId || null
+        success(txself, res)
+    errorcb = null
+    if error
+      errorcb = (res) ->
+        error(txself, res)
+    @executes.push getOptions({ query: [sql].concat(values || []), path: @dbPath }, successcb, errorcb)
     return

   complete: (success, error) ->
     throw new Error "Transaction already run" if @__completed
     @__completed = true
+    txself = @
+    successcb = (res) ->
+      success(txself, res)
+    errorcb = (res) ->
+      error(txself, res)
     begin_opts = getOptions({ query: [ "BEGIN;" ], path: @dbPath })
-    commit_opts = getOptions({ query: [ "COMMIT;" ], path: @dbPath }, success, error)
+    commit_opts = getOptions({ query: [ "COMMIT;" ], path: @dbPath }, successcb, errorcb)
     executes = [ begin_opts ].concat(@executes).concat([ commit_opts ])
     opts = { executes: executes }
     PhoneGap.exec("PGSQLitePlugin.backgroundExecuteSqlBatch", opts)

There is a section in the README to describe the change to tx.executeSql(). Can you take a look at these changes and perhaps test Legacy-PhoneGap-iPhone/build/pgsqlite_plugin.js whenever you get a chance?

Thanks, Chris

[marcucio]

ok, I will check it out this afternoon, thanks

[brodybits]

@marcucio did you get a chance to test it? Can I continue with the new Cordova 1.5 version or do you need support for a legacy PhoneGap version?

[marcucio]

I tested it out briefly but I would like to do more in depth testing. This is a busy day but hopefully I can get to it today, if not, tomorrow I have all day to look into it.

You can continue with Cordova, I plan on taking your .m and .js files and renaming them to be consistent with the other phonegap plugins (maybe SQLitePlugin?) and just maintaining those files in my githib. I will be on the old PhoneGap for a while because I am at the tail end of a release and do not want to switch over yet for this version.

I will let you know if I find any bugs so that you can fix it in your branch, thanks

On Apr 3, 2012, at 6:00 AM, Chris Brody wrote:

@marcucio did you get a chance to test it? Can I continue with the new Cordova 1.5 version or do you need support for a legacy PhoneGap version?

---

Reply to this email directly or view it on GitHub: https://github.com/davibe/Phonegap-SQLitePlugin/pull/24#issuecomment-4897938

[brodybits]

@marcucio I really appreciate your efforts to test the changes for me. We are all of us busy so we just do things the best we can. Your comment is a confirmation to me to keep the old Legacy PhoneGap version for a while, for those who still need it.

[brodybits]

@marcucio I have just pushed some more changes to my fork, namely to use sqliteNative.openDatabase() instead of a constructor, and with some added parameters, to maintain better consistency with the HTML5/W3 SQL API. I am happy whether or not you want to test this change.

[brodybits]

And I want to rename (PG)SQLitePlugin to SQLiteNative also in the Objective-C class and in [PhoneGap|Cordova].plist. I had trouble doing this in the past and don't have much time to figure this one out right now.

[marcucio]

I was debating on using the term 'Plugin' or 'Native'. I think native is assumed by being a phonegap plugin, it looks like most people just use the name for what it is without 'Native' or 'Plugin':

https://github.com/phonegap/phonegap-plugins/tree/master/iOS

On Apr 3, 2012, at 8:44 AM, Chris Brody wrote:

And I want to rename (PG)SQLitePlugin to SQLiteNative also in the Objective-C class and in [PhoneGap|Cordova].plist. I had trouble doing this in the past and don't have much time to figure this one out right now.

---

Reply to this email directly or view it on GitHub: https://github.com/davibe/Phonegap-SQLitePlugin/pull/24#issuecomment-4900168

[brodybits]

Whatever makes the most sense for people to use. The idea is that people know they are getting SQLite, they are getting something that is "almost like" the HTML5 SQL API but in a form in which they can control where the data is being stored. The only thing I don't like about using the term 'Plugin' in the name is that most of the plugins are being "installed" in the window.plugins object. BTW I was a little too fast to rename my fork, I may change it back sometime.

[brodybits]

Also, do you see any reason to keep the db.executeSql() function? It is not like the HTML5/W3 standard and I don't really want to support that one in the Android version.

[brodybits]

@marcucio my idea right now is to rename back to SQLitePlugin, this will make its purpose very clear, and I still want to get rid of db.executeSql() since it is non-standard. I have no time right now, maybe later.

[marcucio]

I agree with removing db.executeSql() I do not this this is in the spec. SQLitePlugin seems like the best name. We might also want to put in the read me something like this:

The reason why this plugin is necessary is because iOS does not store its web SQL database in a location which will be backed up. If the user upgrades to a newer version of iOS the databases will be lost. This plugin gives you the web SQL API to store data on the device where it will be backed up by iOS.

[marcucio]

https://github.com/chbrody/Cordova-SQLiteNative/blob/master/Legacy-PhoneGap-iPhone/build/pgsqlite_plugin.js seems to work pretty good for my app, I think we got a winner.

[brodybits]

@marcucio thank you for your test and your feedback, the Cordova version is working well for me too so I plan to make another baseline later today or tonight. In https://github.com/davibe/Phonegap-SQLitePlugin/pull/23 people want me to get rid of the Legacy PG version but I am not ready for this if people like you still want to use this for pre-Cordova. Are you willing to add a comment to https://github.com/davibe/Phonegap-SQLitePlugin/pull/23 that you still need the legacy PhoneGap version?

[marcucio]

added comment... one bug I am looking into. when I save null values into the db and then retrieving them I get back the string "". I am trying to find out if this is a bug in getting the value or setting the value, I will report back anything I find.

[marcucio]

I really don't like this fix but it works for me:

https://github.com/marcucio/Phonegap-SQLitePlugin/commit/8660ba16fd796ad764e0c8a1eda906c47dbf8b57

I don't have time to debug the phonegap code to see exactly where the null value from the js sql parameters becomes "" in the .m but this is the first place in the plugin code where it happens.

Also since I do not have 1.5 installed I cannot see if this is an issue in that version too or if it is just an issue in 1.4

[brodybits]

@marcucio thank you for your time and attention to do the testing, fixing and reporting. To be honest, we really should be using automatic testing to check these kinds of problems. I had already included a test suite from Lawnchair that should be able to test both the Lawnchair adapter and the plugin itself at a basic level. I am thinking about making a special test to reproduce this problem and verify the fix.

[marcucio]

@chbrody I agree, setting/getting null values should be in the test suite. This fix probably shouldn't be put in until someone else can repro the issue.

[brodybits]

And I do not do anything with this one for legacy PG 1.4-, I am sure it should not be a problem for you. I suspect you could do a workaround at the application level though it is probably better to just to keep the fix this one in your own fork.

[brodybits]

I took a quick look at the issue list and found #15 seems to report this one. I do want to come up with a test in the Cordova 1.5+ version but probably not for today.

[marcucio]

sounds good, I will be interested to know if this is a bug in 1.5 too when you get around to testing it.

[brodybits]

I renamed my project (again) to https://github.com/chbrody/Cordova-SQLitePlugin and added an Android version which is in a very rough form but at least tested at a very basic level. I will work on a common Lawnchair adapter (probably from the original WebKit version) and some tests to verify compatibility between both versions but not tonight. BTW I do not expect to do anything more to the legacy PhoneGap version unless someone flags a really serious problem.

[marcucio]

I am curious what the advantage to using native Android sql over the browser web sql? I haven't had a problem with the android web sql yet but I had to switch for ios because the web sql db no longer is gaurenteed to persist on ios On Apr 4, 2012 7:23 PM, "Chris Brody" < reply@reply.github.com> wrote:

I renamed my project (again) to https://github.com/chbrody/Cordova-SQLitePlugin and added an Android version which is in a very rough form but at least tested at a very basic level. I will work on a common Lawnchair adapter (probably from the original WebKit version) and some tests to verify compatibility between both versions but not tonight. BTW I do not expect to do anything more to the legacy PhoneGap version unless someone flags a really serious problem.

---

Reply to this email directly or view it on GitHub:

https://github.com/davibe/Phonegap-SQLitePlugin/pull/24#issuecomment-4965440

[brodybits]

The key word is "yet", who knows what the Android platform will do in the future. Some people have been wanting this to do things like reading an existing database and controlling where the data is stored.

[marcucio]

true, and it would probably make creating widgets easier too.

[marcucio]

FYI I my code is now located here: https://github.com/marcucio/Cordova-Plugins

I also am in the middle of an Android version of this this plugin, I tried your Android version but it did not take advantage of transactions on the native side so it was too slow for me.

[brodybits]

I wish you didn't start a separate repository, it will be harder to merge and keep us all in sync. I was busy with some things this weekend but I have proven my Android version against the Lawnchair test suite and I want to try your Android version against the same suite. I want to make a common API, Lawnchair, and test suite for all versions then I hope we can find a way to have a common base.

[marcucio]

I have no problem changing the way it is set up, I did it this way because I was pretty much rewriting the entire Android plugin and I was also renaming both platforms so that it was more consistant.

If we want to keep them both in sync we might want to come up with a consistant naming convention, maybe something like this:

Project: Cordova-SQLitePlugin -- iOS ------SQLitePlugin.js (.m, .h)

-- Android ------SQLitePlugin.js (.java)

[marcucio]

looking over your project it actually looks a lot like this, maybe if we just rename DroidGap to Android and Cordova-iOS to iOS this might be what I was looking to do

[marcucio]

also Cordova-SQLitePlugin / DroidGap / src / com / phonegap / plugin

should be:

Cordova-SQLitePlugin / DroidGap / src / com / phonegap / plugins

to be more consistant with the other projects out there (needs to be changed in the .java file package name too)

[brodybits]

Mike these are good. I read quickly through your emails. I just finished church and I will look through these more carefully when I get home. My idea is to make the restructurings you suggested and include your changes assuming they pass the regressions. I will give you the first credit for the Android version since you did some real programming work. All I did was copy and adapt.

On Sunday, April 8, 2012, Mike wrote:

I have no problem changing the way it is set up, I did it this way because I was pretty much rewriting the entire Android plugin and I was also renaming both platforms so that it was more consistant. I am

If we want to keep them both in sync we might want to come up with a consistant naming convention, maybe something like this:

Project: Cordova-SQLitePlugin -- iOS ------SQLitePlugin.js (.m, .h)

-- Android ------SQLitePlugin.js (.java)

---

Reply to this email directly or view it on GitHub:

https://github.com/davibe/Phonegap-SQLitePlugin/pull/24#issuecomment-5016124

[brodybits]

For your work are you good to keep it under the mit license?

On Sunday, April 8, 2012, Mike wrote:

FYI I my code is now located here: https://github.com/marcucio/Cordova-Plugins

I also am in the middle of an Android version of this this plugin, I tried your Android version but it did not take advantage of transactions on the native side so it was too slow for me.

---

Reply to this email directly or view it on GitHub:

https://github.com/davibe/Phonegap-SQLitePlugin/pull/24#issuecomment-5015424

[brodybits]

@marcucio a few issues. I tried running my small test program (see https://github.com/chbrody/Cordova-SQLitePlugin/blob/master/DroidGap/assets/www/index.html lines 25-46, ignore lines 50-62), changing from my_openDatabase() to new SQLitePlugin() and also from res.rows.item(0).cnt to res.rows[0].cnt, and it looks like some plugin SQL is executed but I do not end getting any response to the select SQL. Can you post a small test program so I can check how to run your version of the Android plugin?

Also I looked again through some plugins under https://github.com/phonegap/phonegap-plugins/tree/master/Android and it looked like they to use src.com.phonegap.plugin (no 's'). Doesn't make such a big difference to me, I am just as happy if we use something like com.phonegap.sqlitePlugin.SQLitePlugin.

Finally, it looks like you are taking generated Javascript and making adaptations for the Android version. If I get this working, I would like to readapt from CoffeeScript sometime in the near future. I am happy to do this, probably sometime next weekend.

Chris

[marcucio]

I'm fine with MIT licence, dosen't matter to me.

I'll check out your tests, we should be able to use the same exact test code from iOS since the API should be exactly the same (except that I took out the executeSql function that wasn't part of transaction). I really didn't test it except for using it in my app so I will try to make it pass the same test that we use for iOS.

As far as the plugin (or plugins) name, I guess the random projects I looked at had plugins, but I just checked out the phoneGap wiki and they had it your way with plugin:

http://wiki.phonegap.com/w/page/36753494/How%20to%20Create%20a%20PhoneGap%20Plugin%20for%20Android

I figured you were going to make the .coffee for this, it would probably clean up some inconsistencies with my coding style.

[marcucio]

@chbrody I investigated that bug that you talked about, I fixed one small bug which prevented it from running but it seems like there is a larger issue with nested callbacks with both the iOS and Android plugins. The situation is as follows:

the following code should output:

222 333 444 555

but with the android AND the iOS plugin it outputs:

222 333 555

db.transaction(function(tx) 
{
    write_log('app->do_test - 222');
    tx.executeSql('DROP TABLE IF EXISTS test_table');
    tx.executeSql('CREATE TABLE IF NOT EXISTS test_table (id integer primary key, data text, data_num integer)');
    return tx.executeSql("INSERT INTO test_table (data, data_num) VALUES (?,?)", ["test", 100], function(tx, res) 
    {
        write_log('app->do_test - 333');
        tx.executeSql("select count(id) as cnt from test_table;", [], function(tx, res) 
        {
            write_log('app->do_test - 444');
            console.log("rows.length: " + res.rows.length + " -- should be 1");
            return console.log("rows[0].cnt: " + res.rows.item(0).cnt + " -- should be 1");
        });

    }, function(e) 
    {
        write_log('app->do_test error - 666');
        return console.log("ERROR: " + e.message);
    });

}, function()
{
    write_log('app->do_test error - 777');
}, function()
{
    write_log('app->do_test success - 555');
});

The problem is that by the time we get to the callback with 444 the transaction is already submitted via PhoneGap.exec.

This is a tricky issue because we want to batch our transactions together before sending them to PhoneGap for speed reasons but it makes this situation tricky to code.

I don't need this functionality for my apps but both the iOS and Android plugins do not do this correctly. I am looking into a few ways to fix this but I do not have time to fix it if it is going to take a few days to come up with a solution.

[marcucio]

ps, sorry about "write_log" it is my internal logging function

[brodybits]

@marcucio don't worry about write_log() any half-decent hacker should be able to figure that one out LOL. I was thinking about this issue and to be honest I am questioning if my sample is really supported by the H5 SQL API. I will take another look and maybe do some more testing sometime later today.

[brodybits]

Hi @marcucio I tried running my test on normal HTML5/Web SQL API, using the database object returned by window.openDatabase() (iOS version only) and my test was working OK. So perhaps you should make a clear label on your version that it is working only with batches. I am thinking about how I can change my version to work between individual queries and batches to get the best of both worlds, sometime in the future.

Chris

[marcucio]

I think i have a fix for Android but I have to test it out, hopefully I will have time today

[brodybits]

Awesome, please keep me posted, I will use it if it is working.

[marcucio]

Here is my fix:

https://github.com/marcucio/Cordova-Plugins/commit/03511745daaac51de0a8ac1a63cbef0b8e19146b

basically when the final transaction callback is run i check to see if there were any more queries that have not been run yet and if so I submit the transaction again with the queries that were not run yet.

[brodybits]

Mike @marcucio that sounds very good. I have just finished some changes to my fork to use sqlitePlugin.openDatabase() for both iOS and Android versions, and also a common Lawnchair adapter for both iOS and Android, so I will try your change and incorporate it if it is working for me.