feat: nested transactions

[wschurman]

Why

There's a bug in the Expo server of the following nature:

await viewerContext.runInTransactionForDatabaseAdaptorFlavorAsync(
  'postgres',
  async (queryContext) => {
    const testEntity = await TestEntity.loader(viewerContext, queryContext)
      .enforcing()
      .loadByIDAsync(id);
    const testEntity2 = await TestEntity.loader(viewerContext, queryContext)
      .enforcing()
      .loadByIDAsync(id2);
    try {
      await TestEntity.updater(testEntity, queryContext)
        .setField('unique_field', 'non-unique-value')
        .enforceUpdateAsync();
    } catch {}

    // this never runs since the transaction was aborted, even though the catch caught the exception
    await TestEntity.updater(testEntity2).setField('blah', 'wat').enforceUpdateAsync();
  }
);

The exact text of the error is current transaction is aborted (for postgres database adapter).

To fix this, nested transactions are needed so that only the inner transaction rolls back on a postgres error and the outer transaction is allowed to continue.

How

With this PR, the code becomes:

await viewerContext.runInTransactionForDatabaseAdaptorFlavorAsync(
  'postgres',
  async (queryContext) => {
    const testEntity = await TestEntity.loader(viewerContext, queryContext)
      .enforcing()
      .loadByIDAsync(id);
    const testEntity2 = await TestEntity.loader(viewerContext, queryContext)
      .enforcing()
      .loadByIDAsync(id2);
    try {
      await queryContext.runInNestedTransactionAsync(async (innerQueryContext) => {
        await TestEntity.updater(testEntity, innerQueryContext)
          .setField('unique_field', 'non-unique-value')
          .enforceUpdateAsync();
      });
    } catch {}

    // this succeeds now
    await TestEntity.updater(testEntity2).setField('blah', 'wat').enforceUpdateAsync();
  }
);

Test Plan

Run the new tests.

[codecov]

Codecov Report

Merging #194 (13c6301) into main (acbad82) will decrease coverage by 0.12%.
The diff coverage is 89.18%.

@@            Coverage Diff             @@
##             main     #194      +/-   ##
==========================================
- Coverage   96.30%   96.17%   -0.13%     
==========================================
  Files          81       81              
  Lines        2029     2065      +36     
  Branches      269      269              
==========================================
+ Hits         1954     1986      +32     
- Misses         70       74       +4     
  Partials        5        5              

Flag	Coverage Δ
integration	96.17% <89.18%> (-0.13%)	⬇️
unittest	96.17% <89.18%> (-0.13%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ample/src/adapters/InMemoryQueryContextProvider.ts	75.00% <0.00%> (-25.00%)	⬇️
packages/entity/src/EntityQueryContext.ts	96.82% <90.47%> (-3.18%)	⬇️
...ter-knex/src/PostgresEntityQueryContextProvider.ts	100.00% <100.00%> (ø)
packages/entity/src/EntityQueryContextProvider.ts	100.00% <100.00%> (ø)
...tity/src/utils/testing/StubQueryContextProvider.ts	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[ide]

Left some minor comments but I think the big thing is that Postgres doesn't support nested transactions:

Issuing BEGIN when already inside a transaction block will provoke a warning message. The state of the transaction is not affected. To nest transactions within a transaction block, use savepoints (see SAVEPOINT).

https://www.postgresql.org/docs/current/sql-begin.html

I worry that what's happening is a sequence of commands like this:

BEGIN
statements intended for outer transaction
BEGIN ; warning but ultimately a no-op
statements intended for inner transaction
error occurs
ROLLBACK ; rolls back the outer transaction because Postgres does not support nested txns
COMMIT ; intended to commit outer transaction, but actually a no-op with a warning

That said, I could be wrong about this and need to look at Knex more because it does have some savepoint support (https://github.com/knex/knex/blob/dc6dbbf900c863a9992352ae097040eb6c8d16bd/lib/execution/transaction.js#L110-L120).

[ide]

This test's filename has a typo in "Postgres"

[ide]

tangent: we should upgrade the tests to use Postgres 14 and use gen_random_uuid

[ide]

I dug into this some more and am more convinced Knex does the right thing and uses SAVEPOINT and ROLLBACK TO for nested transactions.

• Savepoints: https://github.com/knex/knex/blob/dc6dbbf900c863a9992352ae097040eb6c8d16bd/lib/execution/transaction.js#L201-L203.
• Rollbacks: https://github.com/knex/knex/blob/dc6dbbf900c863a9992352ae097040eb6c8d16bd/lib/execution/transaction.js#L310-L313

I haven't verified this by actually running the code, only reading it, but I'm optimistic.

[wschurman]

Yep, Knex uses savepoints. I verified this by running tests with DEBUG=knex:query,knex:tx and seeing savepoints and rollbacks to savepoints.

Also the integration test tests for the case you specify above by doing nested rollbacks and commits in the parent transaction after the nested rollback.

[wschurman]

For the supports nested transactions test, this is the transaction output:

knex:query CREATE EXTENSION IF NOT EXISTS "uuid-ossp" undefined +0ms
  knex:query select * from information_schema.tables where table_name = $1 and table_schema = current_schema() undefined +2ms
  knex:query create table "postgres_test_entities" ("id" uuid default uuid_generate_v4(), "name" varchar(255)) undefined +7ms
  knex:query alter table "postgres_test_entities" add constraint "postgres_test_entities_pkey" primary key ("id") undefined +3ms
  knex:query alter table "postgres_test_entities" add constraint "postgres_test_entities_name_unique" unique ("name") undefined +5ms
  knex:query truncate "postgres_test_entities" restart identity undefined +4ms
  knex:tx trx2: Starting top level transaction +0ms
  knex:query BEGIN; trx2 +7ms
  knex:query insert into "postgres_test_entities" ("name") values ($1) returning * trx2 +3ms
  knex:query select * from "postgres_test_entities" where "id" = ANY($1) trx2 +5ms
  knex:query COMMIT; trx2 +2ms
  knex:tx trx2: releasing connection +12ms
  knex:tx trx3: Starting top level transaction +1ms
  knex:query BEGIN; trx3 +3ms
  knex:query insert into "postgres_test_entities" ("name") values ($1) returning * trx3 +1ms
  knex:query select * from "postgres_test_entities" where "id" = ANY($1) trx3 +3ms
  knex:query COMMIT; trx3 +2ms
  knex:tx trx3: releasing connection +7ms
  knex:tx trx4: Starting top level transaction +0ms
  knex:query BEGIN; trx4 +1ms
  knex:query select * from "postgres_test_entities" where "id" = ANY($1) trx4 +2ms
  knex:query update "postgres_test_entities" set "name" = $1 where "id" = $2 returning * trx4 +3ms
  knex:query select * from "postgres_test_entities" where "id" = ANY($1) trx4 +1ms
  knex:tx trx5: Starting nested transaction +8ms
  knex:query SAVEPOINT trx5; trx5 +3ms
  knex:query select * from "postgres_test_entities" where "id" = ANY($1) trx5 +1ms
  knex:query update "postgres_test_entities" set "name" = $1 where "id" = $2 returning * trx5 +3ms
postgres_1  | 2022-09-13T16:13:00.715440700Z ERROR:  duplicate key value violates unique constraint "postgres_test_entities_name_unique"
postgres_1  | 2022-09-13T16:13:00.715520500Z DETAIL:  Key (name)=(unique) already exists.
  knex:query ROLLBACK TO SAVEPOINT trx5 trx5 +10msMENT:  update "postgres_test_entities" set "name" = $1 where "
  knex:tx trx5: releasing connection +16ms
  knex:query select * from "postgres_test_entities" where "id" = ANY($1) trx5 +2ms
  knex:query update "postgres_test_entities" set "name" = $1 where "id" = $2 returning * trx5 +3ms
  knex:query select * from "postgres_test_entities" where "id" = ANY($1) trx5 +2ms
  knex:query COMMIT; trx5 +2ms
  knex:tx trx4: releasing connection +10ms
  knex:query select * from "postgres_test_entities" where "id" = ANY($1) trx5 +3ms
  knex:query select * from information_schema.tables where table_name = $1 and table_schema = current_schema() trx5 +3ms
  knex:query drop table "postgres_test_entities" trx5 +3ms