🐛 Remove repeated column construction from `SQLModelMetaclass.__init__`

[daniil-berg]

Summary

There was a bug in the SQLModelMetaclass whereby the SQLAlchemy Column objects were constructed twice for each field defined on a table model: First in the meta class' __new__ method and then again in its __init__ method.

With these changes the get_column_from_field function is called only once for each field, namely in SQLModelMetaclass.__new__.

Example

Construct a table model with a foreign key explicitly constructed via the SQLAlchemy ForeignKey class:

from sqlmodel import Field, SQLModel, create_engine
from sqlalchemy.sql.schema import ForeignKey


class User(SQLModel, table=True):
    id: int = Field(primary_key=True)


class Post(SQLModel, table=True):
    id: int = Field(primary_key=True)
    user_id: int = Field(
        sa_column_args=(ForeignKey("user.id", ondelete="CASCADE"),)
    )

Without the proposed changes, even just running these class definitions causes an error:

Traceback (most recent call last):
  File "/home/daniil/coding/sqlmodel/experiments/__init__.py", line 12, in <module>
    class Post(SQLModel, table=True):
  File "/home/daniil/coding/sqlmodel/sqlmodel/main.py", line 329, in __init__
    dict_used[field_name] = get_column_from_field(field_value)
  File "/home/daniil/coding/sqlmodel/sqlmodel/main.py", line 457, in get_column_from_field
    return Column(sa_type, *args, **kwargs)  # type: ignore
  File "/home/daniil/.cache/pypoetry/virtualenvs/sqlmodel-TV15XYpK-py3.10/lib/python3.10/site-packages/sqlalchemy/sql/schema.py", line 1765, in __init__
    self._init_items(*args)
  File "/home/daniil/.cache/pypoetry/virtualenvs/sqlmodel-TV15XYpK-py3.10/lib/python3.10/site-packages/sqlalchemy/sql/schema.py", line 144, in _init_items
    spwd(self, **kw)
  File "/home/daniil/.cache/pypoetry/virtualenvs/sqlmodel-TV15XYpK-py3.10/lib/python3.10/site-packages/sqlalchemy/sql/base.py", line 1047, in _set_parent_with_dispatch
    self._set_parent(parent, **kw)
  File "/home/daniil/.cache/pypoetry/virtualenvs/sqlmodel-TV15XYpK-py3.10/lib/python3.10/site-packages/sqlalchemy/sql/schema.py", line 2559, in _set_parent
    raise exc.InvalidRequestError(
sqlalchemy.exc.InvalidRequestError: This ForeignKey already has a parent !

This is fixed with this PR.

Add the following to the example:

sqlite_url = "sqlite:///:memory:"
engine = create_engine(sqlite_url, echo=True)
SQLModel.metadata.create_all(engine)

There is no more error and the SQL statements are as expected:

CREATE TABLE user (
	id INTEGER NOT NULL, 
	PRIMARY KEY (id)
)

CREATE TABLE post (
	id INTEGER NOT NULL, 
	user_id INTEGER NOT NULL, 
	PRIMARY KEY (id), 
	FOREIGN KEY(user_id) REFERENCES user (id) ON DELETE CASCADE
)

PS

The codecov bot is weird... I really don't see how coverage could possibly have decreased. The statement below is also inconsistent with the report, if you click on it. Oh well.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (ea79c47) 98.49% compared to head (5ad7c6a) 97.76%. Report is 66 commits behind head on main.

:exclamation: Current head 5ad7c6a differs from pull request most recent head 50a3f76. Consider uploading reports for the commit 50a3f76 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #443      +/-   ##
==========================================
- Coverage   98.49%   97.76%   -0.74%     
==========================================
  Files         185      187       +2     
  Lines        5856     6266     +410     
==========================================
+ Hits         5768     6126     +358     
- Misses         88      140      +52     

Files	Coverage Δ
sqlmodel/main.py	84.83% <ø> (ø)

... and 1 file with indirect coverage changes

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[github-actions[bot]]

📝 Docs preview for commit 5ad7c6ad052dbc272f00c07c0633192f85d4e301 at: https://631b234f16d4cf27863512be--sqlmodel.netlify.app

[github-actions[bot]]

📝 Docs preview for commit eee8e80c137ab7d610a30c78421d0064052cdcdb at: https://639cdfc1a7c2ed00568a9bc6--sqlmodel.netlify.app

[Vanderhoof]

Hey is there a workaround for cascade FK definition now while this fix is being investigated? 😊

UPD, probably something like this:

from sqlmodel import Field, SQLModel, create_engine
from sqlalchemy import Column, Integer
from sqlalchemy.sql.schema import ForeignKey


class User(SQLModel, table=True):
    id: int = Field(primary_key=True)


class Post(SQLModel, table=True):
    id: int = Field(primary_key=True)
    user_id: int = Field(
        sa_column=Column("user_id", Integer, ForeignKey("user.id", ondelete="CASCADE")),)
    )

[github-actions[bot]]

📝 Docs preview for commit 50a3f7699409702d7f4a9c7b61cb492b43deab5c at: https://7b43a033.sqlmodel.pages.dev

[tiangolo]

Awesome, thanks a lot for the thorough explanation @daniil-berg! 🚀

I tweaked it and updated the internals a bit.

This will be available in the next version, 0.0.9. 🎉