cordova driver using unsupported sql transaction statements

[jacobg]

Issue type:

[ ] question
[X ] bug report
[ ] feature request
[ ] documentation issue

Database system/driver:

[X ] cordova
[ ] mongodb
[ ] mssql
[ ] mysql / mariadb
[ ] oracle
[ ] postgres
[ ] cockroachdb
[ ] sqlite
[ ] sqljs
[ ] react-native
[ ] expo

TypeORM version:

[X ] latest
[ ] @next
[ ] 0.x.x (or put your version here)

Steps to reproduce or a small repository showing the problem:

The cordova driver's CordovaQueryRunner, which extends AbstractSqliteQueryRunner, uses sql statements to begin, commit and rollback transactions. But the cordova sqlite plugin documents that such transactions are unsupported:

User-defined savepoints are not supported and not expected to be compatible with the transaction locking mechanism used by this plugin. In addition, the use of BEGIN/COMMIT/ROLLBACK statements is not supported.
https://github.com/xpbrew/cordova-sqlite-storage#other-limitations

Rather the plugin documents that the WebSql db.transaction() api should be used. Notably, it may be difficult, if even possible, to use the db.transaction() api in the cordova driver, as the driver semantics rely on asynchronous and undefined transaction boundaries, whereas the transaction api is limited to more synchronous boundaries sort of like a message queue that automatically closes when it's empty. Also there is no support for explicit rollback in the websql api.

I've opened an issue in the plugin's repo to more explicitly document the behavior of transaction statements, particularly if used serially. If they do work serially, then perhaps this driver could enforce that, or the plugin should:
storesafe/cordova-sqlite-storage#863

I've also opened issues in the plugin repo to create a new api to allow for better transaction support and concurrency:
storesafe/cordova-sqlite-storage#862
storesafe/cordova-sqlite-storage#864

Related issue here: #1884

@brodybits

[brodycj]

This sounds pretty bad for reasons I already stated in storesafe/cordova-sqlite-storage#865.

[imnotjames]

Digging through this - it doesn't seem like we can actually support transactions as they work for other databases - it's just not a match in the API. We cannot use the current API in the plugin as it's not really compatible (as noted in the original message) with how we are making queries. It effectively is a serial queue & doesn't seem to apply them until after the function completes. (Please correct me if I'm wrong here.)

There are three options I see here:

• Close this as WONTFIX & add a comment in any docs for cordova driver that it does nothing (optimal)
• Throw errors on transactions in cordova - not great because there's gonna be lots of side effects
• Remove the cordova driver entirely as @brodybits it looks like you're starting a brand new plugin to address the issues here.

If it's possible to somehow...

• START A TRANSACTION (somehow)
• take that transaction handle or the connection handle & execute queries against it
• COMMIT or ROLLBACK the transaction

... then PLEASE let me know. I spent about an hour looking through the API & source of the upstream project, experimenting with the API, and poking at it all - and could not find a mechanism to do so. The way it's written just does not support async transactions.

[jacobg]

The cordova driver works fine when it's wrapped in a serialization queue (i.e., concurrency 1). I feel the performance is also acceptable for a mobile app. Regarding that new cordova API you referred to, I don't think it really went anywhere, so there's not another cordova solution at this time. I would highly urge the core devs of typeorm to leave the cordova driver in the project, and perhaps reduce concurrency to 1.

Here is a gist of code that accomplishes this:
https://gist.github.com/jacobg/e077230d22eef04fc97d0eddc4f4b2c7

[imnotjames]

The concurrency is already effectively 1 but we are using no transactions - they are essentially no-ops as I'm seeing. That's not good because users - at this point in time - are trying to use those and are finding that they do not work or work unexpectedly during runtime.

That's the crux of the issue that you've opened here, right?

---

https://gist.github.com/jacobg/e077230d22eef04fc97d0eddc4f4b2c7

This is a bit difficult for me to follow. I'm reading this and understanding this as - "as long as you don't use typeorm like you would with every other driver, you won't have any problems" - which, sure, that's correct.

The code in the gist does not make the API we have for typeorm work - it's a bunch that wraps the API in a new API and then prevents queries from actually hitting the database when you expect them to.

That feels like a foot-gun to me.

That's why I chose not to implement it like that in the driver earlier today, eg, when you start a transaction we queue up the queries and then fire them off once you commit - it just won't work for our current execution model.

---

Regardless, the "removal" would be marking the driver as deprecated and not doing any work on improving it - with the actual removal coming later. Ideally when third-party drivers are possible.

The reasoning for that suggestion is that it seems like this cordova plugin won't see features that allow it to work within our execution paradigms without a lot of undue effort, and there are other alternatives I'd personally prefer we put more effort into - such as sql.js or websql or etc.

We also - because it's so different from every other driver - cannot run tests for it in our current CI. (Same goes for aurora, expo, and nativescript but hey, those I've got my eyes on, too, on what to do with 'em. 😆)

---

Again, though - I'm very open to making this happen - including transactions - but it seems like it just is not going to be possible to actually implement transactions with this driver.

[imnotjames]

I think option 2 seems good.

Throw errors on transactions in cordova - not great because there's gonna be lots of side effects

There's lots of side effects but it's the safest route as well as the clearest.

[imnotjames]

That's the route I've taken in #7771 - thoughts from anyone?

In particular, @brodybits

[jacobg]

@imnotjames Thanks for your work on typeorm. My concern about throwing exceptions is that it will break functionality in typeorm such as migrations. There may be many other features as well which using transactions in such a way automatically. I would prefer there be an option to just ignore the transaction boundaries. I have separate code in my application that prevents concurrency when using the driver.

Be well.

[imnotjames]

@imnotjames Thanks for your work on typeorm. My concern about throwing exceptions is that it will break functionality in typeorm such as migrations. There may be many other features as well which using transactions in such a way automatically.

Migrations use transactions, yes. There's an option to disable that.

The only other code that was using transactions implicitly seemed to be the synchronize, which I've updated to follow the migration transaction setting.

Per storesafe/cordova-sqlite-storage#865 the path forward with the Cordova sqlite storage plugin is to also block transactions.

I would prefer there be an option to just ignore the transaction boundaries. I have separate code in my application that prevents concurrency when using the driver.

Concurrency is not the issue here, though. The issue is that rollbacks don't actually rollback.

Adding an option like that is "add an option which breaks features silently", isn't it?

[jacobg]

@imnotjames Ah, so setting migrationsTransactionMode: 'none' keep everything working. Ok, sounds good. Thanks!

[insel-maz]

We also had to set transaction to false in SaveOptions or RemoveOptions to use Repository.save() or Repository.remove().