denoflare icon indicating copy to clipboard operation
denoflare copied to clipboard

add D1 migration support in denoflare cli

Open codehz opened this issue 2 years ago • 9 comments

It would be very useful for d1 application

codehz avatar Nov 06 '23 08:11 codehz

Can you be more specific? I'm not sure what you're asking

johnspurlock-skymethod avatar Nov 06 '23 16:11 johnspurlock-skymethod

https://developers.cloudflare.com/d1/platform/migrations/ it is possible with official wrangler, but it cannot be used standalone, I mean, the wrangler.toml file is required for this function, I think create a dummy wrangler.toml is not an ideal solution.

if denoflare cli provide a standalone d1 migrate subcommand, things will be easier

codehz avatar Nov 06 '23 16:11 codehz

ah ok that makes sense, thanks

johnspurlock-skymethod avatar Nov 06 '23 16:11 johnspurlock-skymethod

hmm, so it looks like "migrations" are just a local wrangler concept, there is no Cloudflare API surface for them. They just have you create a folder of sql files, and create a migrations table in your database as state.

Would you want denoflare to simply mimic the same logic that wrangler includes? Not sure its much easier than simply running sql files yourself, but do let me know what you think.

johnspurlock-skymethod avatar Nov 06 '23 17:11 johnspurlock-skymethod

hmm, so it looks like "migrations" are just a local wrangler concept, there is no Cloudflare API surface for them. They just have you create a folder of sql files, and create a migrations table in your database as state.

Would you want denoflare to simply mimic the same logic that wrangler includes? Not sure its much easier than simply running sql files yourself, but do let me know what you think.

yes, it is almost equals to "simply running sql files yourself", but

  1. denoflare d1 query can only run a single query from command line argument, it is not ideal, usually a "migration" contains many sql statement at once. the api does support multiple statements, but I got error in denoflare The request is malformed: params with multiple statements is not supported Running multiple statements means they will rollback when any of this failed, so you will not get a "partial applied" migration state
  2. need track the applied migration manually, so it is hard to automate (such as in a CI environment) It can be done by create a d1_migrations table like official tool

codehz avatar Nov 07 '23 02:11 codehz

I agree running multi-statement .sql files ought to be supported, I'll do that soon.

Now as far as tracking special named (and ordered?) migration sql files, how important is compatibility with the the wrangler local folder structure and their migrations state table?

In general, I try to avoid simply rewriting wrangler impl, and do things in a simpler way.

johnspurlock-skymethod avatar Nov 07 '23 03:11 johnspurlock-skymethod

as a note, the d1_migrations table is just

CREATE TABLE IF NOT EXISTS ${migrationsTableName}(
		id         INTEGER PRIMARY KEY AUTOINCREMENT,
		name       TEXT UNIQUE,
		applied_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP NOT NULL
);

I agree it can be improved (so not just rewriting), since this implementation cannot perverse the order of the migrations, you cannot easily rollback the migrations in reverse order(this problem happens in their official repo ^1, they have to manually add prefix to the migration files and hope the migration is execute in order), I want something like hasura's migration design [^2], aka have folder structure like

📂 migrations
└─ 📂 default
   └─ 📂 1654696186008_init
      └─ 📄 up.sql
   └─ 📂 1654698227514_create_table_public_address
      ├─ 📄 down.sql
      └─ 📄 up.sql

I suggest use the migrations state table like

CREATE TABLE IF NOT EXISTS denoflare_migrations(
  version INTEGER PRIMARY KEY, -- the 1654696186008 part, alias to rowid
  name TEXT, -- doesn't have to unique, only used for debugging!
  applied_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP NOT NULL
)

and provide command like

  • denoflare d1 migrate apply <database name> [--version version] [--type up|down] [--skip-exection]
  • denoflare d1 migrate status <database name> to get a status like
    VERSION NAME SOURCE STATUS DATABASE STATUS
    1654696186008 init Present(up) 2023-03-04 13:24:44
    1654696713921 create_table_public_address Present(up,down) Not Present
  • denoflare d1 migrate create <database-name> <name-of-migration> auto add version prefix

ps: I think you can just append the insert into denoflare_migrations(...)/delete from denoflare_migrations where version=... statement to the sql, so it will update the migration state automatically, and no race condition (it may happens in CI environment!)

[^2]: Hasura's document for Manage Migrations

codehz avatar Nov 07 '23 04:11 codehz

I like the up/down concept.

I like the idea of a numbered version, but not sure about timestamp and being able to apply migrations out of order. Why not a simple incrementing integer scheme, then with a single database applied version - and enforce strict ordering, which is typical anyway.

And default is the database name in this example? What is the probe logic, look for a migrations child directory in cwd and provide an option to override? Don't love the extra subdirectory for the db name.

johnspurlock-skymethod avatar Nov 07 '23 16:11 johnspurlock-skymethod

simple incrementing integer

Consider the project have more than one maintainer(like from PR), if the version is just a simple integer, merge changes from different development branch will become nightmare. yes, using timestamp as version won't magically solve ALL problems about database migrations, but I think it is good enough to avoid many common issues.

default is the database name

yes, but I'me not sure if it should map to d1's database (uuid or name) directly or a local alias in .denoflare files... I prefer local alias, since there is no "database namespace" concept in cloudflare, different project may have same name (or you just want to deploy same project twice in different name).

extra subdirectory

The thing is, you can have multiple database bindings in a single project, so it must be a way to distinguish between different database targets. Just like you can have multiple scripts in a single denoflare configuration.

And I don't think you need to scan the "migrations", since the command is always require a database name.

codehz avatar Nov 07 '23 18:11 codehz