Does not pass all PouchDB tests

[brodycj]

As reported by @nolanlawson in #247:

Hm, it gets to 49% and then just stops (Android 4.4 emulator + SQLite
plugin). In logcat I see a lot of:

GC_FOR_ALLOC freed 1230K, 18% free 6185K/7492K, paused 2ms, total 2ms

But that's only while it's working.

Once it stops working, the last failed test is Test _conflicts key in the
replication tests. Also I see in the logs a lot of:

  1821         eglCodecCommon  E  **** ERROR unknown type 0x0 (glSizeof,72)
  1821         eglCodecCommon  E  glUtilsParamSize: unknow param 0x00000b44
  1821         eglCodecCommon  E  glUtilsParamSize: unknow param 0x00000bd0

^ This is repeated over and over again.

and another comment by @nolanlawson:

I'm guessing this is some kind of memory leak, because once I restart the tests it works fine again... for awhile.

NOTEs:

• We did not (so far) try this with the Crosswalk version on Cordova on Android.
• @nolanlawson we should try this with androidDatabaseImplementation option set to 2 in sqlitePlugin.openDatabase() (as specified in README.md) and check results (also did we check this for iOS?)
• Blob support is removed (from Android and iOS versions) and I would like to treat blobs with lower priority until we get some more urgent issues sorted

[nolanlawson]

Anybody who wants to help out on this: it is exceedingly easy to run the PouchDB test suite on your own machine, as described here.

[brodycj]

Quoted for reference here:

It's literally as easy as:

git clone https://github.com/pouchdb/pouchdb.git
cd pouchdb
npm install
# start up an Android emulator
SQLITE_PLUGIN=true ADAPTERS=websql npm run cordova

EDIT: you also have to start the CouchDB server on your localhost, as described at: https://github.com/pouchdb/pouchdb/blob/master/TESTING.md

Then it will run the test in your emulator like so:

[brodycj]

@nolanlawson I think it goes without saying that this should have been my task. Will take a look early next week.

[brodycj]

And for #236/#259: I am extremely grateful that J.F. Spencer is willing to help us with testing, measuring, and improving the performance. Let contributors in the community work where the like best :)

[nolanlawson]

@brodybits No prob; I was going to look into it this weekend as well. :) BTW I updated the code snippet above; you no longer need a special branch, because pouchdb master is now cordova-friendly.

[nolanlawson]

FWIW, I just manually confirmed 100% tests passing for Cordova apps in:

• Android 5.1, 4.4, and 4.2
• iOS 8.3
• Firefox OS 3.0.0

I found the fastest way with Android is to use Genymotion, then (assuming CouchDB is installed) using the option:

COUCH_HOST=http://10.0.3.2:5984 npm run cordova

(This was just a sanity check to make sure that the problem wasn't with PouchDB itself.)

[brodycj]

FWIW, I just manually confirmed 100% tests passing for Cordova apps in:

• Android 5.1, 4.4, and 4.2
• iOS 8.3
• Firefox OS 3.0.0

Do you mean that you tested with my plugin, or something else?

Also how are you running the tests with Firefox OS, which this plugin does not (yet) support?

[nolanlawson]

Ah sorry for the confusion, I meant I was testing without the plugin. :) Just a sanity check to make sure that the failures were really due to this plugin.

And yeah, I don't blame ya for not supporting FirefoxOS, given that they didn't make it possible. ;)

[brodycj]

I remember something now, which I discovered from BUG #232. If tx.executeSql() calls the error handler: the plugin will continue only if the error handler returns false, while Web SQL continues if the error handler returns a non-true handler. (I tested what if the error handler returns 1 or a non-empty string, and both the plugin and websql will abort the transaction in this case.) This was due to a change that I accepted from PR #170 (and missing test coverage as well).

[brodycj]

Of course, the problem in #232 would be very easy to fix, but I try to follow a test-first approach (which may not always be the best thing). The workaround (as I said in #232) is for the SQL error handler to explicitly return false if the transaction should be recovered.

Also, #226 uncovered another issue. If a transaction SQL error handler returns TRUE, no more SQL callbacks should be fired before aborting the transaction.

Today is my sabbath and I am taking off now, I am happy to work on this tonight. What I am thinking is to first try to get the full test suite working in Travis/Circle CI, then fix #232 and perhaps #226.

[brodycj]

@nolanlawson am I correct that it possible for PouchDB to store binary data using this plugin without using Base64? I need this answer for the discussion in #260.

[nolanlawson]

PouchDB stores binary strings in WebSQL/this plugin, not base64-encoded strings or Blobs. I actually added some tests to confirm the the behavior of this plugin matched WebSQL: 05ff6cc

[brodycj]

PouchDB stores binary strings in WebSQL/this plugin, not base64-encoded strings or Blobs. I actually added some tests to confirm the the behavior of this plugin matched WebSQL: 05ff6cc

Okay, when I look at the test in the commit it says "unicode" and not "binary data". Do we simply infer that if a "Unicode" test is passing then we can expect a binary data test to pass?

Actually, after reading your blog entries, I think we can consider "binary data" to be very similar to Unicode, in that when passing back and forth between Javascript and any data-storage library, we should be able to deal with a limited number of "trouble cases" and otherwise just store and retrieve it as a normal Javascript string. So my answer to the question above would be "yes". @nolanlawson do you agree with me or not and do you have any more comments?

[nolanlawson]

Okay, when I look at the test in the commit it says "unicode" and not "binary data"

Sorry, that's a misnomer. That test is also checking whether the database supports UTF-16 (Safari <8) or UTF-8 (everything else), hence my focus on unicode.

As long as you can store strings in JavaScript exactly as-is, this plugin will pass the tests just fine. :) So yes.

[aarononeal]

I looked at that before creating the blob solution in PR #170. The two solutions are not mutually exclusive because Nolan's solution is not actually storing data using the sqlite blob type.

PR #170 gives you the option to work with real binary data in sqlite which is required in scenarios where your databases are created by 3rd party authoring tools--something much more likely in Cordova apps than WebSQL.

If you don't need byte for byte accuracy when data is at rest in the database, then sure, you can twiddle a few bytes like PouchDB does and store a binary string in the database but then you're incompatible with any tools expecting that data to be in a blob column and have byte for byte accuracy.

Even after the blob support added in PR #170 you still have the option of storing binary data like PouchDB using string columns. In fact, there is a toBinaryString() method on the SQLBlob for this purpose that you can use (with custom twiddling logic like PouchDB) when binding the column.

I think it's worth supporting both and leaving it to applications to decide what is the optimum solution for their scenario.

[aarononeal]

To clarify some implementation details, the SQLBlob in PR #170 stores data in memory as either a Base64 or url encoded UTF-8 string. The latter should perform fairly well in most circumstances.

Data can be extracted for column binding with either toString or transparently through valueOf if an app needs full binary compatibility and a sqlite blob column type, or an app can use toBase64, toBinaryString, and toUnicodeString for a sqlite string column type.

[aarononeal]

One more clarification. :) Regardless of SQLBlob's implementation, the actual changes on the native side from PR #170 continue to support storing binary data in sqlite string columns, they just add a path for using real sqlite blob columns too.

[brodycj]

I would like to address a couple points you make about PouchDB:

The two solutions are not mutually exclusive because Nolan's solution is not actually storing data using the sqlite blob type.

Looking carefully at some of the code changed by pouchdb/pouchdb#2900:

   var type = attachment.content_type;
    var sql = 'SELECT escaped, ' +
      'CASE WHEN escaped = 1 THEN body ELSE HEX(body) END AS body FROM ' +
      ATTACH_STORE + ' WHERE digest=?';
    tx.executeSql(sql, [digest], function (tx, result) {
      // websql has a bug where \u0000 causes early truncation in strings
      // and blobs. to work around this, we used to use the hex() function,
      // but that's not performant. after migration 6, we remove \u0000
      // and add it back in afterwards
      var item = result.rows.item(0);
      var data = item.escaped ? websqlUtils.unescapeBlob(item.body) :
        parseHexString(item.body, encoding);

we see that the binary data is coming form the body column.

And here is the SQL that I found where the body column is actually created:

      var attach = 'CREATE TABLE IF NOT EXISTS ' + ATTACH_STORE +
        ' (digest UNIQUE, escaped TINYINT(1), body BLOB)';

So I would argue that the PouchDB adapter is using BLOB in the sql.

If you don't need byte for byte accuracy when data is at rest in the database, then sure, you can twiddle a few bytes like PouchDB does and store a binary string in the database but then you're incompatible with any tools expecting that data to be in a blob column and have byte for byte accuracy.

Can you think of a case that can prove the approach taken by PouchDB does not get the byte for byte accuracy?

In general, I do really like the approach of the SQLBlob class that you talk about (in another project).

[aarononeal]

https://www.sqlite.org/datatype3.html

In SQLite, the datatype of a value is associated with the value itself, not with its container.

What this means is that the SQLite column types specified when the table is created are for informational purposes only. Column type is actually determined at the time the row is inserted and if you step through the code as I did when creating SQLBlob, you'll see that the plugin in the PouchDB case is doing a string binding on the blob column. You can do a select on the data afterwards and retrieve the column type and sqlite will report to you that the column type is TEXT or VARCHAR and not BLOB.

Additionally, the PouchDB code above is removing `\u0000' values when persisting and putting them back when reading. That means the data at rest is not byte compatible and a 3rd party tool reading (or providing) the data will not be aware of this.

[aarononeal]

Also, it's fine if we move SQLBlob to another project, but there were other changes from PR #170 on the native side for Android that I think were never integrated. Those would need to be in place for a solution like SQLBlob to support true blob columns.

[brodycj]

https://www.sqlite.org/datatype3.html

In SQLite, the datatype of a value is associated with the value itself, not with its container.

What this means is that the SQLite column types specified when the table is created are for informational purposes only.

Yup!

Additionally, the PouchDB code above is removing `\u0000' values when persisting and putting them back when reading. That means the data at rest is not byte compatible and a 3rd party tool reading (or providing) the data will not be aware of this.

Correct. We definitely don't want to take the same approach as PouchDB when storing binary data.

Moving further discussion about dealing with blobs (back) to #260.

[nolanlawson]

The \u0000 hack is to work around bugs in the WebSQL implementation. I've filed bugs on both Chromium and Webkit, and they've assigned the issues but haven't fixed them yet.

You can do a select on the data afterwards and retrieve the column type and sqlite will report to you that the column type is TEXT or VARCHAR and not BLOB.

I didn't believe this at first, so I checked it out in the debugger:

And you're right! :) PouchDB is actually storing text, not SQLite BLOBs. I should update our documentation to reflect that. Thanks!

In general, though, if the SQLite plugin were able to use normal HTML5 Blobs and accept them as arguments, we could rewrite PouchDB to use them instead of binary strings. Would be a nice improvement to storage efficiency!

[aarononeal]

Indeed, I had added to that WebKit bug you filed to add more context about the root cause when I fixed the same issue in this plugin.

Regarding column types, thanks for confirming. :-) It was a surprise to me too initially.

The problem with HTML 5 Blobs as far as I have been able to tell is that there is no way to marshal them across the JS / native boundary using the techniques that Cordova employs (which could be XHR, frame inserts, or various other hacks). At some point, Cordova will convert the data to a string and run an encoder over it.

The compromise I settled on in PR #170 was to have a SQLBlob type that the plugin could automatically serialize to a sqlite blob column during column binding. It stores data in memory using either base64 or url encoding which makes the additional Cordova encoding a no-op, and you can create one directly from an ArrayBuffer if you like. I'd love to come up with something more efficient, but the built-in WebView APIs on various platforms tend to only allow string marshaling.

[aarononeal]

Would be a nice improvement to storage efficiency!

I forgot to mention, regardless of the marshaling constraints I mentioned above, PR #170 actually does store data in sqlite blob format, so depending on your data, there are storage efficiency gains.

[brodycj]

PouchDB is actually storing text, not SQLite blobs.

@nolanlawson, that actually makes things easier for me, for the time being. Plenty of other backlog for me to deal with!

Would be a nice improvement to storage efficiency!

I forgot to mention, regardless of the marshaling constraints I mentioned above, PR #170 actually does store data in sqlite blob format, so depending on your data, there are storage efficiency gains.

Absolutely! I just file #263 for this.

[brodycj]

@nolanlawson I followed the steps you gave me to clone and do local npm install of PouchDB (but with manual installation of node-ffi and node-exec-sync due to the issues I encountered in pouchdb/pouchdb#3841), but encountered the following error output when I tried SQLITE_PLUGIN=true ADAPTERS=websql npm run cordova:

Running command: /Users/cbrody/Documents/brodybits/sqltest1/pouchtest1/pouchdb/tests/integration/cordova/platforms/android/cordova/run --emulator




Buildfile: /Users/cbrody/Documents/brodybits/sqltest1/pouchtest1/pouchdb/tests/integration/cordova/platforms/android/build.xml does not exist!
Build failed

/Users/cbrody/Documents/brodybits/sqltest1/pouchtest1/pouchdb/tests/integration/cordova/platforms/android/cordova/node_modules/q/q.js:126
                    throw e;
                          ^
Error code 1 for command: ant with args: debug,-f,/Users/cbrody/Documents/brodybits/sqltest1/pouchtest1/pouchdb/tests/integration/cordova/platforms/android/build.xml,-Dout.dir=ant-build,-Dgen.absolute.dir=ant-gen
Error: /Users/cbrody/Documents/brodybits/sqltest1/pouchtest1/pouchdb/tests/integration/cordova/platforms/android/cordova/run: Command failed with exit code 1
    at ChildProcess.whenDone (/Users/cbrody/Documents/brodybits/sqltest1/pouchtest1/pouchdb/node_modules/cordova/src/superspawn.js:126:23)
    at ChildProcess.emit (events.js:110:17)
    at maybeClose (child_process.js:1015:16)
    at Process.ChildProcess._handle.onexit (child_process.js:1087:5)

Will try another way to run the tests.

[nolanlawson]

@brodybits Currently our build only supports Node 0.10. If you use nvm you can switch to 0.10 and that should fix your issues.

[brodycj]

I used tj/n to switch to node 0.10.38, and the npm install issues went away. (I cannot imagine it should be too hard to fix the problems, though suspect no-one has any extra time.)

I discovered that I (still) have to start the Couch server (as described in the PouchDB TESTING.md), and just updated the comment with the testing instructions above to reflect this.

I had to fix tests/integration/cordova/config.xml to install the Cordova test, as follows (thanks to http://stackoverflow.com/questions/28461985/andorid-cordova-phonegap-config-xml-unbound-prefix):

--- a/tests/integration/cordova/config.xml
+++ b/tests/integration/cordova/config.xml
@@ -1,5 +1,5 @@
 <?xml version='1.0' encoding='utf-8'?>
-<widget id="com.pouchdb.tests" version="0.0.1" xmlns="http://www.w3.org/ns/widgets" xmlns:cdv="http://cordova.apache.org/ns/1.0">
+<widget id="com.pouchdb.tests" version="0.0.1" xmlns="http://www.w3.org/ns/widgets" xmlns:cdv="http://cordova.apache.org/ns/1.0" xmlns:gap="http://phonegap.com/ns/1.0">
     <name>PouchDB Test Runner</name>
     <description>
       Runs the unit tests suite for the PouchDB project on Cordova.

But then when I run the app on the emulator, I get an alert that the connection to the server was unsuccessful, with a reference to file:///android_asset/www/tests/integration/index.html

I really cannot understand this because for the installation, index.html is there in my project under both tests/integration/cordova/www/tests/integration and tests/integration/cordova/platforms/android/assets/www/tests/integration.

Before switching to node 0.10(.38), I did create the test app project using cordova-android and a modified version of bin/run-cordova.sh that uses plugman to install the plugin. It did start but would not connect to the PouchDB.

What I am thinking to try now is to move index.html to the root of tests/integration/cordova/www (will be copied to tests/integration/cordova/platforms/android/assets/www) and update all the references.

[nolanlawson]

I get an alert that the connection to the server was unsuccessful

You mean the connection to CouchDB? Make sure it's accessible on localhost:5984 and that you have run add-cors-to-couchdb to set up CORS: https://github.com/pouchdb/add-cors-to-couchdb

You shouldn't need to move the index.html anywhere. For me, the tests work fine by just starting an Android emulator (NOT Genymotion - the 10.0.2.2 path to localhost doesn't work; it's 10.0.3.2 instead) and running npm run cordova.

You can also specify the path to CouchDB by doing COUCH_HOST=http://path.to.couch:5984 when you run it.

As for xmlns:gap="http://phonegap.com/ns/1.0" that is news to me; which version of the cordova cli are you using? I'll open a pull request to add i.

[nolanlawson]

Oh right that gap thing was added recently to fix blackberrry. My bad!

[brodycj]

This was only a problem when using the Android version with sqlite4java (by default). According to the comment in nolanlawson/sqlite-plugin-fork#1 (comment), the workaround was to use androidDatabaseImplementation (as documented). The Android version now uses Android-sqlite-connector by default, which has not been tested with PouchDB. As documented in README.md, it may still be necessary to use use the androidDatabaseImplementation option, leaving it up to the user community to test this.