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

entc/gen: set foreign-key columns non-nullable for required edges #1703

Merged
merged 1 commit into from Jan 23, 2022

Conversation

a8m
Copy link
Member

@a8m a8m commented Jul 9, 2021

Note, this only applies to edges without circular references.
Fixed #1688 and #1374

@serbanmarti
Copy link

Is this still being worked on? This fix is really needed for my use case.

@ksthiele
Copy link

is this still active?

@edigaryev
Copy link

edigaryev commented Nov 8, 2021

We faced this issue at @cirruslabs while trying to ensure consistency and I've tried reproducing the failed migration test locally and got this:

=== RUN   TestMySQL/56
    enttest.go:67: sql/schema: alter table "groups": Error 1830: Column 'group_info' cannot be NOT NULL: needed in a foreign key constraint 'test/groups_group_infos_info' SET NULL
    --- FAIL: TestMySQL/56 (0.05s)

The previous migration created by master had a ON DELETE SET NULL constraint on a group_info column, but the new migration tries to change the group_info column type to NOT NULL, which makes no sense without changing the constraint too and triggers error 1830.

It seems that the current migration logic is somewhat append-only and won't change any constraints:

// Create creates all schema resources in the database. It works in an "append-only"
// mode, which means, it only create tables, append column to tables or modifying column type.
//
// Column can be modified by turning into a NULL from NOT NULL, or having a type conversion not
// resulting data altering. From example, changing varchar(255) to varchar(120) is invalid, but
// changing varchar(120) to varchar(255) is valid. For more info, see the convert function below.
//
// Note that SQLite dialect does not support (this moment) the "append-only" mode describe above,
// since it's used only for testing.

@a8m do you think adding a separate migration option that will enable schema modifications such as constraint updates would be a reasonable way solve this?

I've also read in some issues that a work on a new migration engine is being done, do you have any plans to de-compose this task into smaller chunks and make this effort more transparent overall, so that the community could help?

a8m added a commit to ariga/atlas that referenced this pull request Nov 12, 2021
a8m added a commit to ariga/atlas that referenced this pull request Nov 14, 2021
@iwata
Copy link
Contributor

iwata commented Jan 21, 2022

@a8m Any progress on this pull request?

@a8m
Copy link
Member Author

a8m commented Jan 21, 2022

Hey all, this PR actually waited a long time for the new migration engine, due to migration failures, and thanks to github.com/ariga/atlas, this issue is resolved now and I plan to merge it on Sunday morning. CI is green, and the only thing left is to update the documentation.

If you didn't have a chance to see our v0.10 yesterday, here's the announcement post. Thanks 🙏 ❤️

Note, this only applies to edges without circular references.
Fixed #1688 and #1374
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.

Edge field makes required field nullable in database
5 participants