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

Should hammer diffs account for "invalid" changes? #32

Open
tgolsson opened this issue Jan 19, 2022 · 1 comment
Open

Should hammer diffs account for "invalid" changes? #32

tgolsson opened this issue Jan 19, 2022 · 1 comment

Comments

@tgolsson
Copy link

tgolsson commented Jan 19, 2022

Hello!

This isn't quite a bug report or feature request, but could be either. I had the following table:

table Foos {
      BarID INT64 NOT NULL,

      CONSTRAINT FK_FoosBar FOREIGN KEY (BarID) REFERENCES Bars (ID),
}

I then wanted to remove the NOT NULL, i.e,

table Foo {
      BarID INT64,

      CONSTRAINT FK_FooBar FOREIGN KEY (BarID) REFERENCES Bars (ID),
}

And the diff said:

ALTER TABLE Foos ALTER COLUMN BarID INT64

However, this isn't a legal change! Since BarID is part of a foreign key it doesn't apply. Instead, the correct change is to drop the foreign key, change the table, and then readd the foreign key.

The question is: should hammer (a) refuse the diff, (b) warn about it, (c) generate a diff that does the correct drop/regen dance, or (d) ignore this and leave it up to the user to know?

Not saying either is wrong, or what I'd expect, but I think (d) is the least helpful (and works bad with "automation").

@daichirata
Copy link
Owner

Thank you for your report. I was able to reproduce it in my environment.

For this problem, as suggested in (c), it would be better to remove the foreign key constraint once and regenerate the foreign key constraint after changing the column.

I'll see if this method works.

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

No branches or pull requests

2 participants