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

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

Open
kevinzhow opened this issue Oct 25, 2022 · 4 comments
Labels
bug Something isn't working

Comments

@kevinzhow
Copy link

kevinzhow commented Oct 25, 2022

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 kevinzhow added the bug Something isn't working label Oct 25, 2022
@ffried
Copy link
Contributor

ffried commented Mar 22, 2023

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
Copy link
Contributor

ffried commented Mar 22, 2023

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.

@0xTim
Copy link
Member

0xTim commented Apr 11, 2023

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

@gwynne
Copy link
Member

gwynne commented Apr 11, 2023

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...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants