breaking: refactor(transaction): split logic into new classes

[callmehiphop]

• Tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

---

Previously all transaction logic was housed in a single class. This refactor breaks the logic up into multiple classes:

• Snapshot - read only transactions
• Transaction - read/write transactions
• PartitionedDml - partitioned dml capable transaction

Additionally it decouples the runTransaction logic from the Transaction class itself by introducing a new class TransactionRunner.

You might be asking yourself, "But why, Dave?". This will (IMO) clean up the code base and help us align more closely with the other client libs. It seems we did some stuff differently without good reason.

While I admit the diff on this PR is kind of insane, the breaking changes are actually pretty minimal, as highlighted below.

Running a read only transaction

Snapshots should never give back an ABORTED error, so really there is no reason to run them via runTransaction which contains ABORTED/UNKNOWN retry logic. It might also be worth noting that we are the only client library that ever has allowed snapshots to be ran this way.

Before

const options = {
  readOnly: true,
  strong: true,
};

database.runTransaction(options, (err, transaction) => {
  // do stuff with transaction
});

After

const options = {
  strong: true,
};

database.getSnapshot(options, (err, transaction) => {
  // do stuff with transaction
});

Timestamp bounds now expect milliseconds (instead of seconds)

This one seems kind of obvious, not sure why we did it this way, but these should allow for as much precision as we can provide.

Before

const bounds = {
  exactStaleness: 5,
};

After

const bounds = {
  exactStaleness: 5000,
};

Transaction#end is now synchronous

This mainly applies to read only transactions, as commit and rollback have always called end under the hood.

Before

transaction.end(callback);

After

transaction.end();
callback();

Session#beginTransaction was removed in favor of transaction factories

This is more of an implementation detail change. Users typically won't need to use this API because the lib currently tries to hide sessions as much as possible. They are still accessible, but mostly discouraged.

Before

interface TransactionOptions {
  readOnly?: boolean;
  readWrite?: boolean;
  pdml?: boolean;
}

const options: TransactionOptions = {pdml: true};
session.beginTransaction(options, (err, transaction) => {});

After

const snapshot = session.snapshot(timestampBounds);
const transaction = session.transaction();
const pdml = session.partitionedDml();

// all transactions begin the same way
transaction.begin(err => {});

[crwilcox]

You mention this will bring us more in line with other client libs. Which other languages are you referencing in particular?

[callmehiphop]

@crwilcox I think all of them. AFAIK we were the other lib to treat snapshots and r/w transactions the same way. You could call runTransaction against a snapshot, which as far as I know is overkill since a snapshot will never send back an ABORTED error.

[JustinBeckwith]

LGTM. Just make sure to do a release relatively soon after with good release notes :)

[callmehiphop]

Yeah, I'm hoping @crwilcox has time to take a run through :)

[crwilcox]

This is a real doozy. It is just large enough it is hard to really accurately review. While I looked it over I am not confident I will have found anything useful :(

That said, here are some thoughts.

1. If I previously got a readOnly=false transaction how do I do that now? Before I could infer this type of transaction from the database.runTransaction that became getSnapshot. This seems like something to include in migration docs or samples. https://github.com/googleapis/nodejs-spanner/pull/506/files#diff-467f0acd6b075beac5a17d10d9b1c549L41

2. Was the timestamp change related to the split. It seem to me these things are disconnected?

3. I noticed things like https://github.com/googleapis/nodejs-spanner/pull/506/files#diff-0441714de31e4a2a2fd4f1a10d2f0c4bR171 where we are now marking elements as possibly undefined. What caused this? Were we incorrect before in the d.ts?

[callmehiphop]

@crwilcox

1. If I previously got a readOnly=false transaction how do I do that now? Before I could infer this type of transaction from the database.runTransaction that became getSnapshot. This seems like something to include in migration docs or samples.

runTransaction for read/write and getSnapshot for read only. Usually when we make breaking changes we include them in the release notes and changelog with before/after examples.

2. Was the timestamp change related to the split. It seem to me these things are disconnected?

Yeah, thats a fail on my part, I should probably have used a more descriptive commit message. The gist is that this is a refactor to Transactions as a whole with the biggest piece being the split. I was already in there touching that code so I figured I might as well improve the timestamp bound support.

3. I noticed things like https://github.com/googleapis/nodejs-spanner/pull/506/files#diff-0441714de31e4a2a2fd4f1a10d2f0c4bR171 where we are now marking elements as possibly undefined. What caused this? Were we incorrect before in the d.ts?

Yes, they were incorrect. I hand wrote that file to provide types for the gapics and help cut back on duplicate interfaces declared across the handwritten files. Sometimes it isn't entirely clear from the documentation whether or not a property is optional (at least not to me) and there isn't any official TypeScript support for gapics (yet).

[crwilcox]

Thanks for the response @callmehiphop , re: my first bit about a sample of read/write, I suppose I was asking if you could make sure your notes for the change are reflected? Your current description for this PR seems to lack that so I wanted to be sure we help users migrate.