feat(android): implement async Ti.Database.DB methods

[garymathews]

• Implement asynchronous Ti.Database.DB methods for executing SQL queries.
  • Ti.Database.DB.executeAsync(query, parameters[], callback)
  • Ti.Database.DB.executeAllAsync(queries[], callback)
• Implement method for executing multiple queries in one shot. (Useful for transactions)

JIRA Ticket

[build]

	Messages
📖	💾 Here's the generated SDK zipfile.
📖	✅ All tests are passing Nice one! All 29 tests are passing. (There are 6 tests skipped)

Generated by 🚫 dangerJS against 2a9924d

[sgtcoolguy]

So in JS do we actually have to pass in the callback as the first argument here, like so?

const db = Ti.Database.open('whatever');
db.executeAsync(result => { Ti.API.info(result); }, 'SELECT * FROM TABLE WHERE name = ?', myName);

Because if so, that is very odd and we should avoid it if possible. I'm assuming the use of the varargs for parameters here is why the callback needed to come first? Can we re-arrange it so the callback is the last JS arg? If we can't "natively" with our binding layer, we could add JS code to basically do the swap under the hood in the same way we do some hacking of say Ti.App.Properties here: https://github.com/appcelerator/titanium_mobile/blob/master/android/modules/app/src/js/properties.js

[sgtcoolguy]

Second, it's also not idiomatic to have the async variant have the extra name suffix.

Assuming we can hack around the arguments in JS or whatever, why not just use the same method name and if the last argument is a callback function then treat it as async, otherwise sync?

I suppose that boxes us in if we want to return a Promise...

Maybe introduce a db.executeSync, db.executeAsync where they are explicitly sync/async, have the existing method do either sync or async based on if callback is passed in, and spit out a deprecation warning if no callback is passed to it (basically saying, hey in the future this will return a Promise - so if you want sync behavior use db.executeSync)? I think at some point we want to remove the default sync nature of the method and push people towards async naturally - and eventually support Promises.

[garymathews]

I've modified the parameters so the callback is the last parameter, we need to keep Ti.Database.execute() to not cause breaking changes.

Ti.Database.DB.executeAsync(query, parameters[], result => { ... });

results[] = Ti.Database.DB.executeAll(queries[]);
Ti.Database.DB.executeAllAsync(queries[], results[] => { ... });

I'd prefer to introduce as few API changes possible until we make the jump to Promise based APIs in the near future. I didn't intend to refactor all of our Ti.Database.DB APIs with this PR.

I think these APIs allow the flexibility to do the primary things, including transactions if the user chooses to do so. We should create a ticket to refactor the methods in 9.0.0?

[sgtcoolguy]

We should look to something like this node sqlite3 package to consider how they've approached their API: https://github.com/mapbox/node-sqlite3/wiki/API#database

Maybe not introduce prepare/Statements, but having explicit methods to say whether you want to run something without caring about the result, or wanting the first result, or calling the callback with the batch results once vs calling for each row seems useful to me.

Note that there is also this package which does explicitly run sync intentionally for performance: https://github.com/JoshuaWise/better-sqlite3/blob/master/docs/api.md

I recall @jquick-axway mentioning usage of transactions helped performance significantly. Would we wrap this batch of queries in a transaction for the user? Would we expect them to include their own transaction management statements in this batch?

[build]

• ⚠️ tests/Resources/ti.database.2.addontest.js line 13 – Unexpected exclusive mocha test. (mocha/no-exclusive-tests)

[sgtcoolguy]

LGTM.

My only remaining feedback here is that the batch execute methods only take strings and there's no way to do the string + params style you'd do in a single query. I think that'd be a fairly common annoyance here, and you may want to consider being more expansive on how to handle elements in the queries array so it's a Object[] and could hold either a single String in each entry, or another Object[] with first element the sql String, the remaining the varargs params.

Something like:

const queries = [
  'DELETE FROM testTable',
  ['INSERT INTO testTable (text, number) VALUES (?, ?)', testName, testNumber]
]

[lokeshchdhry]

FR Passed.

executeAsync, executeAll & executeAllAsync work as expected.

Studio Ver: 5.1.3.201906102126
SDK Ver: 8.2.0 local build
OS Ver: 10.14.5
Xcode Ver: Xcode 10.2.1
Appc NPM: 4.2.13
Appc CLI: 7.0.12
Daemon Ver: 1.1.3
Ti CLI Ver: 5.1.1
Alloy Ver: 1.13.10
Node Ver: 8.15.1
NPM Ver: 6.4.1
Java Ver: 10.0.2
Devices: ⇨ google Pixel (Android 9)

[lokeshchdhry]

@garymathews , Its complaining about unit tests. Does it need any ?