googleapis / nodejs-spanner

Node.js client for Google Cloud Spanner: the world’s first fully managed relational database service to offer both strong consistency and horizontal scalability.
https://cloud.google.com/spanner/
Apache License 2.0
136 stars 101 forks source link

Unable to rollback transaction with no activity or with only queued mutations #2103

Open BobDickinson opened 2 months ago

BobDickinson commented 2 months ago

Environment details

Steps to reproduce

  1. Create a transaction - I use getTransaction()
  2. Attempt transaction.rollback()

Or alternatively, execute any number of insert, upsert, delete, etc operations on the transaction and then attempt rollback().

When attempting to rollback() a transaction that hasn't had any activity, or that has only had queued mutation activity, calling rollback() on that transaction throws an error: 'Transaction ID is unknown, nothing to rollback.'

From here: https://github.com/googleapis/nodejs-spanner/blob/992baaa5e7c953938225a0d9f16bc2e5a5c0a6f8/src/transaction.ts#L2315

Expected behavior

Any transaction in a state where it can be successfully committed should also be able to be rolled back without error.

The notion of the transaction id being set when the transaction has actually begun with Spanner (and there is something to roll back), and the idea of queued mutations, are internal to the library (not explained anywhere in the docs that I could find), so requiring the user to understand that they cannot call rollback() under these certain (only known to the library authors) conditions does not seem correct.

The workaround is pretty straightforward, but required me to spend a couple of hours with the library source code to ensure that it was correct (essentially, if id is not set, call end() instead of rollback() to abort the transaction).

The correct behavior would be for the transaction.rollback() to succeed (not throw an error) in this case (no id set, and potentially outstanding queue mutations) - it's a logically valid usage and the result is a noop, maybe with an internal call to end(), so why you would throw an error in that case is beyond me, especially given that the semantics the SDK user would need to understand to avoid the error are not documented AFAICT.

Just trying to save anyone else the 2 hours I spend on this issue. Thanks.