tursodatabase / libsql-client-ts

TypeScript/JavaScript client API for libSQL
https://docs.turso.tech/sdk/ts/quickstart
MIT License
226 stars 32 forks source link

Wait for schema changes on `execute` and `batch` #206

Closed giovannibenussi closed 4 months ago

giovannibenussi commented 4 months ago

This PR adds an option to batch to allow to wait until all schema changes are done:

await schemaClient.batch(["ALTER TABLE users ADD COLUMN test_column text"], {
  wait: true,
});

Currently, batch supports a transaction mode parameter, so I keep support for it but also added a new object parameter that supports a transactionMode property. The code below is equivalent:

// Previous API, still supported
await schemaClient.batch(["ALTER TABLE users ADD COLUMN test_column_2 number"], " write")

// New API
await schemaClient.batch(["ALTER TABLE users ADD COLUMN test_column_2 number"], {
  transactionMode: "write"
});

// You can mix transaction mode and the new wait option
await schemaClient.batch(["ALTER TABLE users ADD COLUMN test_column_2 number"], {
  transactionMode: "write",
  wait: true,
});

You can test these changes locally with this command:

cd packages/libsql-core && npm run build && cd - && cd packages/libsql-client && npm run build && cd - && node test.ts

It builds and run a test using the new wait parameter while I add automatic tests. Please note that the API returns frequent 500 errors which are being investigated internally.

If everything works as expected, you should see an output similar to the one below.

Waiting for migration jobs
json: {
  schema_version: 4,
  migrations: [
    { job_id: 2, status: 'RunSuccess' },
    { job_id: 1, status: 'RunSuccess' }
  ]
}
lastMigrationJob: { job_id: 2, status: 'RunSuccess' }
Finished waiting for migration jobs
giovannibenussi commented 4 months ago

I decided to move (at least for now, we can still discuss this!) the code to wait for schema changes to the execute function as an optional object parameter, so if you want to run a schema migration, you can run a command like this:

await schemaClient.execute(
  "ALTER TABLE users ADD COLUMN test_column number;",
  { wait: true },
);

I think this is a better approach than adding the parameter to batch because the batch function wraps the whole SQL code in a transaction, which makes the schema SQL to fail in the server. This means that we need to remove the transaction from the SQL code if { wait: true }, which makes all these combinations equivalent:

// All these operations would be the same
batch('...', { transactionMode: 'write', wait: true })
batch('...', { transactionMode: 'read', wait: true })
batch('...', { transactionMode: 'deferred', wait: true })

This is not bad per se, but having a configuration object parameter with options that can't be mixed doesn't seem the correct way to implement this because it may cause confusion to users. For now, I moved the implementation to execute, since it seems a better place to implement this logic. However, it's quite easy to move it to any other function in case we want to. If there's anything wrong with this please let me know!

I also added some tests for the waitForLastMigrationJobToFinish using msw to simulate responses for the job-related endpoints and make sure that it works as expected.

I've been testing these changes with my own databases using the test file as a reference and it's working as expected, so if everything looks good, I just need to remove the test.ts file and the extra console.log calls!

giovannibenussi commented 4 months ago

Update about the final implementation: we don't require users to manually provide a new parameter like { wait: true }. We determine automatically if a database is a schema database or not by calling an API endpoint so users are not required to do manual changes in order to make this work!