daichirata / hammer

🛠 hammer is a command-line tool to schema management for Google Cloud Spanner.
MIT License
83 stars 24 forks source link

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

Open tgolsson opened 2 years ago

tgolsson commented 2 years ago

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 commented 2 years ago

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.