Initial attempt to implement transactions

[kevinresol]

My rough idea is to introduce Connection#isolate which will:

1. on pooled connections, request a connection from the pool and disable auto-release
2. on single connection, just return the connection itself

Normal queries will just execute as before, i.e. using pooled connections + auto-release when available.

On the other hand, transactions will call isolate under the hood and release the connection back to the pool (if any) after the transaction is finished.

~Regarding point 2 in the beginning of this text, since isolate does actually nothing right now, so it is only safe on sync targets. We need some more thoughts on that. Perhaps we need some sort of local lock so that when that single connection is isolated, other requests that lands on the same connection has to wait until the lock is released.~

Only Node MySQL for now... my head aches...

[kevinresol]

Currently the API is used like so:

db.transaction(trx -> {
  // do business with trx, where $type(trx) == $type(db)
  return Promise.NOISE;
})

One problem is that trx.transaction() is a thing and if used it will fail nastily. I am thinking if it is better to let user define an interface instead of a class, and change Database to extend a generic-built Transaction class, so that one cannot call transaction on a Transaction itself:

interface Db {
  @:table var user:User;
  @:table var post:Post;
}

@:genericBuild(currentDatabaseBuilder())
class Transaction<Db> {} // becomes "class Transaction0 implements Db {...}"

class Database<Db> extends Transaction<Db> {
  public function transaction<T>(run:Transaction<Db>->Promise<TransactionEnd<T>>):Promise<TransactionEnd<T>>;
}

var db = new Database<Db>(name, driver);

[kevinresol]

I am thinking if it is better to let user define an interface instead of a class, and change Database to extend a generic-built Transaction class

The said change is now in place. There are a few (small) breaking changes.

1. tink.sql.Database is now generically built and cannot be extended directly.

     // before
     class Db extends Database {
       @:table var User:User;
     }
   
     // after
     interface Def extends DatabaseDefinition {
       @:table var User:User;
     }
     typedef Db = Database<Def>;

2. Because of the above change, extra fields has to be declared in a class extending the generically built database class

     // before
     class Db extends Database {
       function customFunc():Int;
     }
   
     // after
     interface Def extends DatabaseDefinition {
       @:table var User:User;
     }
     class MyDb extends Database<Def> {
       function customFunc():Int;
     }

   ~3. @:tables meta are no longer supported, one should now use @:table meta manually. Reason: since the generically built class cannot access private types specified in @:tables~

[benmerckx]

One problem is that trx.transaction() is a thing and if used it will fail nastily.

so that one cannot call transaction on a Transaction itself

Don't think you would want to pursue in this effort, but one other way of "going deeper" is using savepoints. They allow you to do "transactions in transactions".

[kevinresol]

Currently this is kind of solved by doing so:

1. User defines a database definition.
   • Before this PR, it is done by, for example, @:tables(User) class Db extends Database {}.
   • After this PR, it is done by @:tables(Users) interface Db extends DatabaseDefinition {}
2. Initialize the actual database instance:
   • Before this PR, new Db(...)
   • After this PR, new Database<Db>(...). Under the hood, the class is generated as:

     class Database0 implements Db {
     public final User:Table<User>;
     public function transaction(run:Db->Promise<...>);
     }

So, when inside a transaction, user will not be able to call transaction again.

[back2dos]

Hmm. I can see why you would want to try hiding the transaction method, but what's to stop the user from something like this?

var db = new Database<Db>();
db.transaction(_ -> db.transaction(...));

[kevinresol]

The inner transaction will be performed in another isolated connection if there is a pool, or wait until the outer one finishes and releases the connection if there is no pool.

[back2dos]

Hmm, ok, I think I understand. I would suggest a slightly different approach then:

• have a @:genericBuild class Transactional<Db> {} (or maybe you can think of a better name) that takes a database and produces a type with everything in Db except the transaction method, which is most easily generated as an abstract over the specific database class that @:forwards everything except the transaction method (and perhaps even add a @:forwardStatics, although the statics can be access through the original class)
• make Database work as before and generate transaction as Transactional<CurrentDb>->Promise<...>

This would avoid the breaking change and still address your concerns, right? If not, disregard that and feel free to merge ;)

[kevinresol]

That will work. But there is a subtle change in the implementation detail I didn't mention. It is about the

// defintion:
interface Def extends DatabaseDefinition {}

// instance: Database<Def> generates the following
class Transaction0 implements Def {
  function new(cnx:Connection);
}
class Database0 extends Transaction0 {
  final pool:ConnectionPool; // note that ConnectionPool extends Connection and provides an additional `isolate()` method
  function new(driver)
    super(pool = driver.open());

  public function transaction(run:Db->Promise<...>)
    // create a new Transaction object and do stuff with it
    new Transaction0(pool.isolate()); // the actual code regarding isolate() is slightly more complex 
}

If we want to stick with the old approach, then the constructor of the built Database instance has to take Driver, so as the proposed Transactional abstract. But it actually just wants a Connection, in that case we can work around it by declaring a dummy Driver that will just return a constant Connection upon calling open(). But I think that is not so "clean" just to avoid a rather easy-to-migrate (IMO) breaking change.

[back2dos]

But I think that is not so "clean" just to avoid a rather easy-to-migrate (IMO) breaking change.

Hmm, I'd be inclined to use an abstract over Connection that has a @:from Driver calling open(), but unfortunately I'm quite uninvolved in this lib, so I'll leave it for you to judge ;)