search

Electron100 / butane

An ORM for Rust with a focus on simplicity and on writing Rust, not SQL
Apache License 2.0
91 stars 13 forks source link

Old types and tables left in .butane migrations directory #44

Closed jayvdb closed 1 year ago

jayvdb commented 1 year ago

When I commented out "StringMap" from my playground for https://github.com/Electron100/butane/issues/32#issuecomment-1417087644 , and I run the tests again, butane/.butane/migrations/current/types.json still has the entry from CT:StringMap.

Likewise if I add a new #[model] to the tests, and then comment it out, the butane/.butane/migrations/current/Foo.table still exists.

Electron100 commented 1 year ago

This is a good segue into a significant design change I've been wanting opinions on. One of the marquee features of Butane is the ability to have fully automatic migrations, but I unfortunately made a key architectural error in the design and implementation of this. All of the metadata to support the migration is written out by a proc-macro run during compilation. This has two major downsides:

  1. Since proc macros see only the AST and not the type system, all of the ToSql/FromSql implementations are worthless here, we're essentially maintaining two separate systems of knowledge for how to map between Rust types and SQL types -- one in the type system and the other in the proc macro. This makes butane_type a necessary kludge when also implementing the to/from conversions and requires adding (error-prone) type name recognition to the proc macro each time we add a new core type.
  2. The proc macro runs independently on each #[model], and Cargo does not appear to provide any environment variables with a unique identifier for the build. Thus each .table file is generated independently and there isn't a great way to reap dead ones (that I've identified).

I'd like to solve the first problem and part of the second by changing when the migration metadata happens, at the cost of slightly more work for the user (developer using Butane). Rather than creating the migration metadata (ADB) at build time from the #[model proc macro, the proc macro would just create model implementations capable of generating migration metadata (each model could generate its corresponding ATable). The proc macro would also generate a program: butane_migration_helper.rs designed to be invoked by butane_cli. We'd probably need the the user to add this as a [[bin]] to their Cargo.toml. We could automate that with butane_cli using toml_edit, but would probably still want to prompt the user before making automatic changes to their project file.

We'd probably also need to introduce a new proc macro where the user has to explicitly list all of their models to fully solve the dead-model problem and make generation of butane_migration_helpers.rs easier otherwise we'd have no canonical point for creating it.

I don't love the two extra steps for the user, but I'd really love to fix the current lack of type system information when generating the schema, as well as the dead types issue here.

Thoughts?

jayvdb commented 1 year ago

A quick fix for the reaping problem would be to have helper that deletes all of these generated files, which can be run from a build.rs, and then the existing ones get re-created like already happens.

Electron100 commented 1 year ago

Fair, is PR #46 along the lines you were thinking?

I'd still be interested in any opinions you might have on a future pivot for schema generation to occur "at runtime" in a helper with type system support rather than the current type-system-ignorant proc macro. I've started playing around with it and would like to take it forward, but if your reaction is "I would hate that", it would be good feedback.

jayvdb commented 1 year ago

My initial take is that the current approach is really nice, and I think it is worth keeping as a mode of operation if possible.

But a more advanced mode that gives developers more control/features would be good.

Cargo does not appear to provide any environment variables with a unique identifier for the build

A build.rs could set one, that butane then uses only if set, otherwise it falls back to the current behaviour.

This makes butane_type a necessary kludge ...

IMO it would be worthwhile brainstorming all the ways that butane_type can be made unnecessary, except for the most complicated cases. e.g. find a really simple syntax for https://github.com/Electron100/butane/issues/20

Perhaps the user can add more info to #[model], or a macro on the member level within the struct to say this member should be stored as a string.

jayvdb commented 1 year ago

IMO this can be closed now.