Batch of fixes 20141231 #170
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The plugin id used here must match the one in plugin.xml or the test runner will fail to remove it when deployed.
|
Happy New Year! Many thanks for your efforts to fix the support for Base64, I do really like the cleanup of the Base64 code. I suspect it triggers a I am also wondering what will happen if we try the Base64 on WP8. If it Chris On Thursday, January 1, 2015, Aaron Oneal [email protected] wrote:
Sent from my mobile |
The test was closing the database in the middle of an execute transaction. This would cause the transaction to fail because at the exit of the transaction handler a commit would be issued on a closed database. This fix moves the close to the success handler of the transaction so that the final close occurs after the transaction ends. Additionally, the db variable is now obtained from the open callback instead of the function result, which is a more consistent pattern for async. Finally, the close method was updated to fail if an attempt is made to call it while a transaction is active and a test was added to confirm this behavior.
According to the W3C spec, if a transaction failure callback *does not return false* then the error is propagated to a transaction failure. This means that if a transaction returns true, but more importantly, if it has no return value (undefined) then the error must propagate to the transaction. This fix explicitly requires false to correct the propagation. A subsequent fix will address the fact that the original statement error is not propagated and is replaced here, but that is unrelated to this issue.
The aforementioned methods receive errors as string messages from the native layer, so these strings must be wrapped in a regular `Error` type before passing on to API callbacks. This fix also makes 2 minor changes to open: 1) remove a semicolon per doc convention 2) only call the success callback if there is one (in case the default handler that logs is ever removed)
aarononeal
force-pushed
the
hotfix/batch-20141231
branch
from
ec00c6a to
96d5db8
Compare
|
Happy New Year! :-) It looks like the NSData+Base64 files have been around in one shape or another since early 2012, but I only tested against the latest Cordova build. On WP8, what I expect will happen is the ArrayBuffer will encode to a new Also, I made 2 minor updates to the pull tonight. I updated the close commit 8c3bdcc so that the close method verifies it's not in a transaction. This would have avoided the original test bug had it been in place. I also added commit 96d5db8 so that executeSql throws when it's supposed to in order to make it more obvious why the plugin fails when used with ES6 promises. |
|
Very cool, and the new tests are highly appreciated. |
aarononeal
force-pushed
the
hotfix/batch-20141231
branch
8 times, most recently
from
33bbfa2 to
f95f5d5
Compare
aarononeal
force-pushed
the
hotfix/batch-20141231
branch
from
f95f5d5 to
4df15e7
Compare
Sometimes the native layer returns errors as {message:string, code:number} and other times as just a string. This fix wraps key places to ensure that errors returned through the API always conform to SQLError per the API contract documented in the W3C spec.
In short, errors consistently have a message and code now and propagate from statement failure up to the transaction.
Moving data between JavaScript and native is extremely slow, especially for binary which Cordova has to base64 encode and then transmit over XHR or as an URL parameter. The last thing we want to do is send more data. For some reason SQL statements and their parameters were being sent twice because they were duplicated to a `query` member. That member was only used by the iOS plugin, so this fix removes it and updates the iOS plugin to use the sql and params members like the others. Additionally, the iOS plugin is updated to handle unsupported types during binding and a test was added to verify.
The regex function for iOS was reading arguments before checking that the correct number were specified.
This fix enables end-to-end encoding of binary data using strings. Strings with Unicode values like `\u0000` were being truncated because the string constructor was using a null terminator to determine size instead of the stored byte length. The fix uses a different constructor and the stored byte length so that the full length string is returned.
Parameters use `,` not `;`.
If executeSql is called on a finalized transaction it should throw. If it does not, timing errors get a lot harder to debug. ES6 promises, for example, generally execute on the next tick and then it becomes unclear why a statement fails to execute.
Web SQL doesn't actually support storing binary data in SQL blobs using the SQLite blob methods. It serializes binary data using strings with the SQLite text methods which results in data not being stored as a blob. This can be problematic in Cordova if you need to work with existing SQLite databases.
This change adds more robust support for binary serialization. `SQLBlob` objects can be used in statements and these will be converted to a `sqlblob` URI with base64 or URL encoded data that is unpacked on the native side and stored as a proper binary blob.
When reading a blob back from the native side, the previous behavior was to return the blob as base64. Now, the blob can be decoded as a `SQLBlob` object which can then be unpacked in JavaScript to base64, ArrayBuffer, or binary string.
tx.executeSql('INSERT INTO test_table VALUES (?)', [new SQLBlob(arrayBuffer)]);
tx.executeSql("SELECT foo FROM test_table", [], function(tx, res) {
var blob = new SQLBlob(res.rows.item(0).foo);
blob.toBase64();
blob.toBinaryString();
blob.toArrayBuffer();
});
Tests were added to verify the behavior and to demonstrate usage.
Only iOS and Android were updated. Windows Phone did not previously have blob support; saving for a future update.
aarononeal
force-pushed
the
hotfix/batch-20141231
branch
from
4df15e7 to
b62bdff
Compare
brodycj
pushed a commit
that referenced
this pull request
Feb 6, 2015
Moving data between JavaScript and native is extremely slow, especially for binary which Cordova has to base64 encode and then transmit over XHR or as an URL parameter. The last thing we want to do is send more data. For some reason SQL statements and their parameters were being sent twice because they were duplicated to a `query` member. That member was only used by the iOS plugin, so this fix removes it and updates the iOS plugin to use the sql and params members like the others. Additionally, the iOS plugin is updated to handle unsupported types during binding and a test was added to verify. (@brodybits) from pull #170, adding www/SQLitePlugin.js updated from SQLitePlugin.coffee.md
brodycj
pushed a commit
that referenced
this pull request
Feb 6, 2015
The plugin id used here must match the one in plugin.xml or the test runner will fail to remove it when deployed. (@brodybits) from pull request #170 thanks @aarononeal
brodycj
pushed a commit
that referenced
this pull request
Feb 6, 2015
The test was closing the database in the middle of an execute transaction. This would cause the transaction to fail because at the exit of the transaction handler a commit would be issued on a closed database. This fix moves the close to the success handler of the transaction so that the final close occurs after the transaction ends. Additionally, the db variable is now obtained from the open callback instead of the function result, which is a more consistent pattern for async. Finally, the close method was updated to fail if an attempt is made to call it while a transaction is active and a test was added to confirm this behavior. (@brodybits) from pull #170 thanks @aarononeal, adding www/SQLitePlugin.js as updated from SQLitePlugin.coffee.md
|
Thanks @aarononeal, I will include the changes using My only major comment is that if you propose more changes in the future, changes to the Javascript need to be included when we change the CoffeeScript for the Cordova CLI installation to work. I will take care of this for the changes that you have already proposed. |
|
Thanks. I wasn't sure how you wanted to handle integration so I left out the auto-generated JS, but I can definitely include that in the future. |
brodycj
pushed a commit
that referenced
this pull request
Feb 18, 2015
brodycj
pushed a commit
that referenced
this pull request
Feb 28, 2015
brodycj
pushed a commit
that referenced
this pull request
Feb 28, 2015
According to the W3C spec, if a transaction failure callback *does not return false* then the error is propagated to a transaction failure. This means that if a transaction returns true, but more importantly, if it has no return value (undefined) then the error must propagate to the transaction. This fix explicitly requires false to correct the propagation. A subsequent fix will address the fact that the original statement error is not propagated and is replaced here, but that is unrelated to this issue. (@brodybits) from PR #170 thanks @aarononeal, adding www/SQLitePlugin.js as updated from SQLitePlugin.coffee.md
brodycj
pushed a commit
that referenced
this pull request
Feb 28, 2015
Sometimes the native layer returns errors as {message:string, code:number} and other times as just a string. This fix wraps key places to ensure that errors returned through the API always conform to SQLError per the API contract documented in the W3C spec.
In short, errors consistently have a message and code now and propagate from statement failure up to the transaction.
(@brodybits) from PR #170 thanks @aarononeal, adding www/SQLitePlugin.js as updated from SQLitePlugin.coffee.md
brodycj
pushed a commit
that referenced
this pull request
Feb 28, 2015
Parameters use `,` not `;`. (@brodybits) from PR #170 thanks @aarononeal
brodycj
pushed a commit
that referenced
this pull request
Mar 1, 2015
If executeSql is called on a finalized transaction it should throw. If it does not, timing errors get a lot harder to debug. ES6 promises, for example, generally execute on the next tick and then it becomes unclear why a statement fails to execute. (@brodybits) from PR #170 thanks @aarononeal, adding www/SQLitePlugin.js as updated from SQLitePlugin.coffee.md
brodycj
pushed a commit
that referenced
this pull request
Mar 1, 2015
brodycj
pushed a commit
that referenced
this pull request
Mar 1, 2015
brodycj
pushed a commit
that referenced
this pull request
Mar 1, 2015
Strings with Unicode values like `\u0000` ARE being truncated because the string constructor was using a null terminator to determine size instead of the stored byte length. (@brodybits) from PR #170 thanks @aarononeal. The fix will be committed separately.
brodycj
pushed a commit
that referenced
this pull request
Mar 1, 2015
This fix enables end-to-end encoding of binary data using strings. Strings with Unicode values like `\u0000` were being truncated because the string constructor was using a null terminator to determine size instead of the stored byte length. The fix uses a different constructor and the stored byte length so that the full length string is returned. (@brodybits) from PR #170 thanks @aarononeal. Test was already committed separately.
brodycj
pushed a commit
that referenced
this pull request
Mar 4, 2015
… for improvements to SQL blob storage] (@brodybits) from PR #170 thanks @aarononeal. NOTE: changes related to SQL blob binding are #ifdef'd out (TBD subjet to change).
brodycj
pushed a commit
that referenced
this pull request
Mar 4, 2015
…onditionally-included SQL binding code (ref: PR #170)
brodycj
pushed a commit
that referenced
this pull request
Mar 6, 2015
Small fix to handling of Blob to prepare for SQL blob improvements (ref: PR #170)
brodycj
pushed a commit
that referenced
this pull request
Mar 10, 2015
Fixes: - #193: workaround for Android db locking/closing issue - #144: convert array parameters to string to match Web SQL - #199: fix double-precision REAL values in result for iOS version - #150/#153: close Android db before removing from map - Fix truncation in iOS query result in case of UNICODE NULL (\0 or \u0000) (ref: PR #170) - Some fixes for error handling to be consistent with Web SQL (ref: PR #170) Testing ONLY: - #147: testing with UNICODE line separator - #195: Reproduce issue with double-precision REAL number on WP(8) ONLY
brodycj
pushed a commit
that referenced
this pull request
Mar 10, 2015
Fixes: - #193: workaround for Android db locking/closing issue - #144: convert array parameters to string to match Web SQL - #199: fix double-precision REAL values in result for iOS version - #150/#153: close Android db before removing from map - Fix truncation in iOS query result in case of UNICODE NULL (\0 or \u0000) (ref: PR #170) - Some fixes for error handling to be consistent with Web SQL (ref: PR #170) Testing ONLY: - #147: testing with UNICODE line separator - #195: Reproduce issue with double-precision REAL number on WP(8) ONLY
brodycj
pushed a commit
that referenced
this pull request
Mar 10, 2015
Related to the following fixes: - #193: workaround for Android db locking/closing issue - #144: convert array parameters to string to match Web SQL - #199: fix double-precision REAL values in result for iOS version - #150/#153: close Android db before removing from map - Fix truncation in iOS query result in case of UNICODE NULL (\0 or \u0000) (ref: PR #170) - Some fixes for error handling to be consistent with Web SQL (ref: PR #170)
This was referenced May 9, 2015
|
For general reference: all changes in this PR are included except for the BLOB enhancements. I plan to integrate the blob enhancements with the cordova-sqlite-ext version when I get a chance. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The last commit is dependent on some of the former so I'm batching all these together. I added detailed descriptions to each commit to make review easier.
Two of the more significant issues fixed include binary string truncation and blob support.