fluent-kit icon indicating copy to clipboard operation
fluent-kit copied to clipboard

Scheme .references() and foreignKey() 's constraint naming strategies are not unified.

Open kevinzhow opened this issue 1 year ago • 4 comments

Describe the bug

Ref to Discord discussion Link

.field("star_id", .uuid, .required, .references("stars", "id")) will create a constraint with name suffixed with _fkey in postgresql database.

it is different from the constraint created with foreignKey() which prefixed with fk.

The behavior is not working as expected as the docment says the are the same

image

if we create a constraint with .references() and want to modify the constraint in the next migration, we will find it's hard to delete constraint with .deleteConstraint() method.

Expected behavior

.references() and foreignKey() should have the same naming strategy.

Environment

  • Vapor Framework version: 4.66.1
  • OS version: macOS 13

kevinzhow avatar Oct 25 '22 12:10 kevinzhow

Just ran into the same issue. It seems impossible to delete a constraint created with .references(...) w/o resorting to its name (whose naming strategy seems to be left to Postgres - or at least I wasn't able to find the corresponding code).

Also, the code in constraintIdentifier uses the altered schema as prefix for all (foreign) fields (see here). If the target (foreign) table name would be used, ~the generated name would actually be correct~ it would make much more sense.

ffried avatar Mar 22 '23 19:03 ffried

~~Btw. in Postgres, the foreign key does not have a _fkey suffix for me. Instead it begins with fk: (much like the generated constraintIdentifier). However, the issue really is the incorrect table prefix for foreign fields as I just described.~~

EDIT: Scratch that. Seems like the constraints I was seeing were from an older (pre Vapor 4) installation. Newly generated databases actually have the _fkey suffix as described by the OP. Which explains why I couldn't find any code generating that name. The _fkey suffix is created by postgres, as explained here.

ffried avatar Mar 22 '23 20:03 ffried

We're aware of this but I think that changing it would be considered a breaking change so will need to wait for Fluent 5

0xTim avatar Apr 11 '23 12:04 0xTim

I had intended to fix this by, as @ffried correctly suggests, using the foreign table name instead of the referencing table name, as this was technically never correct to begin with. Unfortunately, even if doing so wouldn't be breaking due to the sudden naming change (which it is), it would still require either a semver-major break or a massive amount of very painful refactoring and code duplication, thanks to design flaws in FluentKit (very short version: as things are now, there's no means of making the foreign table's name available in several of the places a constraint identifier needs to be computed).

I should also mention that while Postgres does semantically treat the column-constraint and table-constraint forms of a foreign key constraint more or less identically, neither MySQL not SQLite can say the same (MySQL silently ignores the column form, and SQLite applies different default semantics depending on which is used); ANSI SQL considers them to be distinct (though related) constructs, which means it's technically incorrect to expect them to be interchangeable.

While this is unfortunately not documented anywhere, the consequence of the above is that the .deleteConstraint() modifier should only be used for table constraints. To drop (or add!) a column constraint, use .fieldUpdate() instead. I will add this to the documentation (hopefully in a comprehensible fashion! 😅) when I get a chance.

(I'll also note that much of the oddity involved here has to do with the radically different SQL syntaxes for table alteration between Postgres and MySQL and the lack of any properly structured support inside Fluent for handling it. This in turn is mostly thanks to ALTER TABLE support having been tacked on well after the 4.0.0 release and too many things having been indiscriminately marked public...)

gwynne avatar Apr 11 '23 12:04 gwynne