dyedgreen / deno-sqlite

Deno SQLite module
https://deno.land/x/sqlite
MIT License
409 stars 36 forks source link

Add transaction properties to DB client #81

Closed tksilicon closed 4 years ago

tksilicon commented 4 years ago

I opened this issue #80 which requires a mechanism to run transactions in DSO ORM . What I did was to use a modified local class: .

Here are examples of its usage.

clientTestSQLITE(async function testTransactionSuccess() {
  let topicId: number | undefined;
  let userId: number | undefined;
  const result = await dso.transactionSqlite<boolean>(async (trans) => {
    const userModel = trans.getModel(UserModel);
    const topicModel = trans.getModel(TopicModel);
    userId = await userModel.insert(
      { nickName: "foo", password: "bar" },
    );
    topicId = await topicModel.insert({ title: "zoo", userId });
    let user = await userModel.findById(userId!);
    assert(!!user);
    return true;
  });
  const user = await userModel.findById(userId!);
  const topic = await userModel.findById(topicId!);
  assertEquals(result, true);
  assert(!!topic);
  assert(!!user);
});

clientTestSQLITE(async function testTransactionFail() {
  let userId: number | undefined;
  let topicId: number | undefined;
  await assertThrowsAsync(async () => {
    await dso.transactionSqlite<boolean>(async (trans) => {
      const userModel = trans.getModel(UserModel);
      const topicModel = trans.getModel(TopicModel);
      userId = await userModel.insert({ nickName: "foo", password: "bar" });
      topicId = await topicModel.insert({ title: "zoo", userId });
      let user = await userModel.findById(userId!);
      assert(!!user);
      await userModel.query(new Query().table("notexists").select("*"));
      return true;
    });
  });

I had to localise the client, deps and other boilerplate code. It would be good to have this capability in-built in the client, straight from this library.

dyedgreen commented 4 years ago

Thank you so much for your contribution @tksilicon, I really appreciate your effort in writing and submitting this PR! However, I feel like the proposed changes do not go well with the overall technical direction of the library.

Generally, I don't want to wrap SQL commands or provide higher-level abstractions that go beyond running SQL queries and returning results from them.

I am excited that you want to use this library for DSO ORM and would be very happy to help you with writing a compatible client layer for your library that interfaces with this libraries lower-level APIs. However, I strongly feel that such an interface should be part of the ORM, since each ORM will have unique opinions about their architecture and interface requirements.

tksilicon commented 4 years ago

Where does that leave DSO ORM? Means we have to make use of a localized adapted class because the ORM needs a way to run transactions at a high level using callbacks with the SQL abstracted from the user.

dyedgreen commented 4 years ago

I would recommend to write a client that uses an underlying open DB connection. If you can link me to the interface you have to implement, I'd be happy to provide you with an example of how to go about implementing something like that. But generally the following would be the pattern (note that I don't know what interface you have to implement exactly, but the general idea is to write a wrapping client around the database connection, see also https://github.com/denolib/typeorm/tree/master/src/driver/sqlite for a further example of how to accomplish this):

import {DB} from 'https://deno.land/x/sqlite/mod.ts';

class Client {

  construct(path) {
    this.db = new DB(path);
  }

  async query(sql) {
    return [...this.db.query(sql)];
  }

  async transaction(fn) {
    this.db.query('begin');
    try {
      await fn(this);
      this.db.query('commit');
    } catch () {
      this.db.query('rollback');
    }
  }

}

// use like
const cli = new Client(':memory:');
await cli.transaction(async (cli) => {
  await cli.query('...');
});
tksilicon commented 4 years ago

Will try this approach. Hopefully it fits into our existing design.