Skip to content

Conversation

@mkarbo
Copy link

@mkarbo mkarbo commented Feb 24, 2022

Due to no name on foreign key objects, Sqlalchemy will complain when trying to drop tables (CircularDependencyError). This PR adds name attribute to foreign key objects created by SQLModel.

The error happens inside sqlalchemy.sql.ddl module at line 968, e.g., when using Postgres trying to drop tables.

@matsavage
Copy link

matsavage commented May 11, 2022

Is there any appetite to get this into SQLModel, I am also having this problem and using sa_column isn't really working for me either, nor can I find any other sensible way to name my foreign keys using SQLModel

@nomagick: did you find a workaround I can use in the mean-time?

@mkarbo
Copy link
Author

mkarbo commented May 11, 2022

Hi mat, I don\t know what workaround you are refering to - we ended up using a branch like this, and it solved our issues related to the problem I specified in the PR

@matsavage
Copy link

@mkarbo ah sorry, I was just meaning did you find another way to achieve this same result in the un-patched version of SQLModel

@mkarbo
Copy link
Author

mkarbo commented May 12, 2022

No, unfortunately not - if you do or you see an improvement to my fork/PR please do let me know :)

@matsavage
Copy link

matsavage commented May 13, 2022

I was able to do this in the unpatched with the following:

table_two_id: Optional[int] = Field(
    sa_column=Column(
        Integer,
        ForeignKey(
            "table_two.id", name="fk-table_one-table_two-id"
        ),
        default=None,
        nullable=False,
    )

However this does not work with the BaseModel relationships pattern https://sqlmodel.tiangolo.com/tutorial/fastapi/relationships/ as the SQLModel Relationship is not recognized by SQLAlchemy

I have also seen that SQLModel will name an index when you add one to a column, but why not an FK 👀

@mkarbo
Copy link
Author

mkarbo commented Jun 4, 2022

@matsavage the fact that indices but not foreign keys are named is what led to this PR, trying to mimick the logic from indices - I agree it would be great to have it part of a release

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.

2 participants