Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AutoMigrate for Postgres dialect #926

Open
wants to merge 53 commits into
base: master
Choose a base branch
from

Conversation

bevzzz
Copy link
Collaborator

@bevzzz bevzzz commented Nov 1, 2023

This PR is my second attempt at implementing #456 :)

This time around I decided not to meddle with ALTER TABLE query and focus on the functionality we want to bring -- AutoMigrator. I took into account our discussions in #726 and left it up to the dialects to implement "alter table" operations, keeping it internal. My thinking was that, if auto migration is done well, the users wouldn't need to work with ALTER TABLE all that much.

AutoMigrator exposes a simple and minimal API: CreateSQLMigrations(ctx) and Migrate(ctx, options). These are similar to Migrator except that they do not require writing any SQL/Go migrations manually.

What AutoMigrator can do:

  • Create, drop, and rename tables
  • Create, drop, and rename column
  • Modify DEFAULT value
  • Modify NOT NULL and UNIQUE constraints
  • Modify primary key and foreign key constraints
  • Change column data type
  • Add or remove identity
  • Create SQL migration files with support for transactional migrations (.tx.up.sql/.tx.down.sql)

What it doesn't do:
AutoMigrator performs schema migrations and not data migrations. For example, when changing a primary key on the table, related foreign keys will not be updated -- they will continue referencing the old PK; to "re-link" them would require updating the referencing values themselves, which is beyond AutoMigrator's scope.

Presently, only pgdialect supports database inspection and migration, for which it implements two additional interfaces. AutoMigrator will return an informative error if used with a dialect that doesn't support this feature.

Other changes:

  • test.sh forwards additional arguments to go test
    (for example, now possible to run 1 test with ./test.sh -run=TestAutoMigrator_Run)
  • added Schema string field to differentiate tables in different schemas.
  • each Dialect now reports the name of the default schema
  • had to update commit message in 95c4a8e because it was upsetting the linter

  • In-code documentation
  • Documentation on the website
  • Example CLI to generate and run auto-migrations

@bevzzz
Copy link
Collaborator Author

bevzzz commented Nov 1, 2023

I had to update a lot of to the existing unit tests (~5-7 files) because I noticed that many of them weren't properly cleaning up the database, causing side-effects in the auto-migrate tests.
All of those changes come in a single commit and I could open a different PR for them to be merged separately.

UPD: opened #927 for this specific change. Should make it easier to review the current one once the other changes are merged.

@bevzzz bevzzz changed the title AutoMigrate: Rename table AutoMigrate: Rename/Create/Drop tables Nov 3, 2023
@bevzzz bevzzz force-pushed the feature/automigrate-rename-table branch 5 times, most recently from 3ef6553 to ce75416 Compare August 3, 2024 14:46
@bevzzz bevzzz force-pushed the feature/automigrate-rename-table branch 3 times, most recently from eeadd41 to 9faa55b Compare October 20, 2024 10:00
@bevzzz bevzzz force-pushed the feature/automigrate-rename-table branch from 85d0877 to e7e7c4f Compare October 27, 2024 11:18
Aoang and others added 18 commits October 27, 2024 12:24
…ce#1028)

* feat(schema): add support type for net/netip.Addr and net/netip.Prefix

* fix(schema): net.IPNet(not ptr) is not implement fmt.Stringer

Edit: updated commit message to comply with commitlint [subject-case] rule.
Original subject: "Add support type..."
- RawQuery
- CreateTableQuery
Additionally:
- allow custom FK names (limited applicability rn)
- implement GetReverse for AddFK and DropFK
- detect renamed foreign keys
- EqualSignature handles empty models (no columns)
This is a WIP commit.
- Do not implement AppendQuery on Operation level.
This is a leaky abstraction as queries are dialect-specific
and migrate package should not be concerned with how they are constructed.
- AlterTableQuery also is an unnecessary abstraction.
Now pgdialect will just build a simple string-query for each Operation.
- Moved operations.go to migrate/ package and deleted alt/ package.
- Minor clean-ups and documentation.

testChangeColumnType is commented out because the implementation is missing.
Bufixes and improvements:
- pgdialect.Inspector canonicalizes all default expressions (lowercase)
to make sure they are always comparable with the model definition.
- sqlschema.SchemaInspector canonicalizes all default expressions (lowercase)
- pgdialect and sqlschema now support type-equivalence, which prevents unnecessary
migrations like CHAR -> CHARACTER from being created.

Changing PRIMARY KEY and UNIQUE-ness are outside of this commit's scope, because
those constraints can span multiple columns.
- Add -Op suffix to all operations
- Replace Apply() method with AppendSQL() in Migrator interface
- Add snapshot tests for SQL generation
- Suspended support for renaming constraints

Included bugfixes:
- changeColumnType uses "type equivalence" to update varchar length if needed
- addColumn supports DEFAULT clause
- CreateSQLMigrations() writes up- and down- SQL to migration files in the migrations directory
- Migrate() generates migration files and applies them, creating a corresponding entry in the database
- Run() is deprecated. It was decided that AutoMigrator will not support in-place migrations for now
- no-op operations produce a comment in the down files if the an operation is not reversible
@bevzzz
Copy link
Collaborator Author

bevzzz commented Nov 4, 2024

Looks like we're now creating those up and down files!

image

This commit is aimed at making sqlschema models more extensible
and the migration-planning logic more readable. Key changes are:

- TableDefinition and Column definition both
    have Additional interface{} field to allow adding inspector-specific
    data which remains opaque to the general bulk of the code.
    For example, SchemaInspector passes (*Model)(nil) zero interface
    which is used to create a table.
- TableDefinitions is structured as a map, similarly to ColumnDefinitions.
    Map lookups are convenient and are more readable than the custom tableSet
    struct we used before. The latter is now retired.
- sqlschema exposes data structures for describing the databse schema and its
    interface should not be cluttered with utilities only used by Detector.
    Which is why both Signature and RefMap are moved to migrate package.
- Simplified refMap only has the essential methods. It allows detector to
    avoid re-creating FKs for tables/columns that were renamed.
- Refactored a whole bunch of DependsOn() methods for various operations to
    leverage the new unitility methods on sqlschema.ForeignKey and Columns.
- Deleted tests for refMap, as they are now an implementation detail and not
    part of the API.
- TableDefinition (base implementation) now uses FQN as a key for every table to be able to store tables from different schemas with the same name
This feature will be added in the follow-up pull requests.
@vmihailenco
Copy link
Member

vmihailenco commented Nov 9, 2024

@bevzzz few things that I've noticed:

  • You've added schema.FQN to support schema.table names. Do we really need it? It complicates things a lot and I don't think that we should support more than a single schema, e.g. do we really plan to support moving tables between schemas? Is that even possible?

    I suggest we drop schema.FQN and ignore the schema name althogether.

  • In every interface you have Tables []Table, but Tables map[string]Table in the implementation because it is handy. That will cause some issues, because maps don't preserve the order. I suggested we use ordered maps in all such places so we have both map and list, for example, https://github.com/wk8/go-ordered-map/v2

So we will have:

type Table interface {
  GetTables() *orderedmap.Map[string, Table]
}
  • I also added some clarification comment to BaseTable, BaseColumn etc so they must not be used outside dialects
// BaseTable is a base table definition.
//
// Dialects and only dialects can use it to implement the Table interface.
// Other packages must use the Table interface.

WDYT?

@bevzzz
Copy link
Collaborator Author

bevzzz commented Nov 9, 2024

do we really plan to support moving tables between schemas? Is that even possible?

No, I don't think moving schemas between tables is possible. Rather, I wanted to make sure that the tables in different schemas aren't shadowing each other names, i.e. I want to be able to rename books.authors -> writers without renaming public.authors or paintings.authors.

Most databases I've worked with had several schemas, so I thought this may make automigrations more resilient to name clashes.

UPD: I realize now that we can achieve the same by using the name from the bun:"table" tag directly. Will remove schema.FQN 👌

That will cause some issues, because maps don't preserve the order. I suggested we use ordered maps

Sure, I would even argue that GetTables may return []Table and have detector use orderedmap.Map internally, where it's most needed. This way dialects needn't bring orderedmap as a dependency.

Other than the different order in which Operations may be applied, is there any reason a lack of order in map may cause issues? I'm asking because the dependencies between operations are always resolved during migration planning and the order in which tables/columns are supplied does not break that.

@vmihailenco
Copy link
Member

is there any reason a lack of order in map may cause issues?

I assume that query generation won't be stable if 2 columns/tables are changed at once.

I also see that the tests rely on having a map[string]Table/Column. Also I had a similar task before and orderedmap worked well.

@vmihailenco
Copy link
Member

@bevzzz I am going to introduce orderedmap myself and leave FQN removal to you. Hopefully that's okay with you.

Regarding schemas, I think that we might need to accept a schema name via configuration and then only work with that schema. But that can be added later.

@vmihailenco
Copy link
Member

@bevzzz я все :)

Тесты я поправил но билд почему-то не проходит c непонятной ошибкой. Предлагаю пока это игнорировать.

AutoMigrator only supports 1 schema at a time.
Use WithSchemaName() option to configure it.
Defaults to the default schema in the dialect.
Now that AutoMigrator only works with one schema at a time, there's no need to keep
the code which was used ot differentiate tables between schemas
@bevzzz
Copy link
Collaborator Author

bevzzz commented Nov 11, 2024

Short update: we've decided to drop multi-schema support, so the user would instead configure AutoMigrator to work with a particular schema by passing migrate.WithSchemaName("my_schema") option.
By default, AutoMigrator will continue to use the dialect's default schema, e.g. public for Postgres.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants