sqlalchemy icon indicating copy to clipboard operation
sqlalchemy copied to clipboard

`NoReferencedColumnError` when using `referred_column_0[[_]N]_name` in FK naming convention

Open thomas-mckay opened this issue 5 years ago • 5 comments

I've read https://github.com/sqlalchemy/sqlalchemy/issues/3989 but this seems unrelated.

When defining a ForeignKey on a SQLAlchemy model, if the referenced column is given as a string ({table_name}.{column_name}) and the Base.metadata has been setup with a naming convention that uses any of thereferred_column_0[[_]N]_name tokens for the foreign key convention, then an sqlalchemy.exc.NoReferencedColumnError is raised when the model class is being declared/mapped. Declaring the ForeignKey by passing a Column object works, though.

This sample code triggers the bug:

import sqlalchemy as sa
from sqlalchemy import MetaData
from sqlalchemy.ext.declarative import declarative_base


metadata = MetaData(naming_convention={
    "fk": "fk__%(referred_table_name)s__%(referred_column_0_name)s",
})
Base = declarative_base(metadata=metadata)

class Parent(Base):
    __tablename__ = 'parent'
    id = sa.Column(sa.Integer, autoincrement=True, primary_key=True)
    parent_id = sa.Column(sa.Integer, sa.ForeignKey('parent.id'))

If I change the last line to

    parent_id = sa.Column(sa.Integer, sa.ForeignKey(id))

it works as expected. But referencing the column directly is not always possible. Although, in those cases, declaring the ForeignKeyConstraint separately via __table_args__ works like a charm.

Error

Traceback (most recent call last):
  File "/home/thomas/dev/python/sqlalchemy_test/naming.py", line 17, in <module>
    class Child(Base):
  File "/home/thomas/dev/python/sqlalchemy_test/.env/lib/python3.8/site-packages/sqlalchemy/ext/declarative/api.py", line 75, in __init__
    _as_declarative(cls, classname, cls.__dict__)
  File "/home/thomas/dev/python/sqlalchemy_test/.env/lib/python3.8/site-packages/sqlalchemy/ext/declarative/base.py", line 131, in _as_declarative
    _MapperConfig.setup_mapping(cls, classname, dict_)
  File "/home/thomas/dev/python/sqlalchemy_test/.env/lib/python3.8/site-packages/sqlalchemy/ext/declarative/base.py", line 160, in setup_mapping
    cfg_cls(cls_, classname, dict_)
  File "/home/thomas/dev/python/sqlalchemy_test/.env/lib/python3.8/site-packages/sqlalchemy/ext/declarative/base.py", line 190, in __init__
    self._setup_table()
  File "/home/thomas/dev/python/sqlalchemy_test/.env/lib/python3.8/site-packages/sqlalchemy/ext/declarative/base.py", line 534, in _setup_table
    cls.__table__ = table = table_cls(
  File "<string>", line 2, in __new__
  File "/home/thomas/dev/python/sqlalchemy_test/.env/lib/python3.8/site-packages/sqlalchemy/util/deprecations.py", line 128, in warned
    return fn(*args, **kwargs)
  File "/home/thomas/dev/python/sqlalchemy_test/.env/lib/python3.8/site-packages/sqlalchemy/sql/schema.py", line 507, in __new__
    metadata._remove_table(name, schema)
  File "/home/thomas/dev/python/sqlalchemy_test/.env/lib/python3.8/site-packages/sqlalchemy/util/langhelpers.py", line 68, in __exit__
    compat.raise_(
  File "/home/thomas/dev/python/sqlalchemy_test/.env/lib/python3.8/site-packages/sqlalchemy/util/compat.py", line 178, in raise_
    raise exception
  File "/home/thomas/dev/python/sqlalchemy_test/.env/lib/python3.8/site-packages/sqlalchemy/sql/schema.py", line 502, in __new__
    table._init(name, metadata, *args, **kw)
  File "/home/thomas/dev/python/sqlalchemy_test/.env/lib/python3.8/site-packages/sqlalchemy/sql/schema.py", line 601, in _init
    self._init_items(*args)
  File "/home/thomas/dev/python/sqlalchemy_test/.env/lib/python3.8/site-packages/sqlalchemy/sql/schema.py", line 118, in _init_items
    spwd(self)
  File "/home/thomas/dev/python/sqlalchemy_test/.env/lib/python3.8/site-packages/sqlalchemy/sql/base.py", line 460, in _set_parent_with_dispatch
    self.dispatch.after_parent_attach(self, parent)
  File "/home/thomas/dev/python/sqlalchemy_test/.env/lib/python3.8/site-packages/sqlalchemy/event/attr.py", line 322, in __call__
    fn(*args, **kw)
  File "/home/thomas/dev/python/sqlalchemy_test/.env/lib/python3.8/site-packages/sqlalchemy/sql/schema.py", line 2111, in _set_table
    self.constraint._set_parent_with_dispatch(table)
  File "/home/thomas/dev/python/sqlalchemy_test/.env/lib/python3.8/site-packages/sqlalchemy/sql/base.py", line 460, in _set_parent_with_dispatch
    self.dispatch.after_parent_attach(self, parent)
  File "/home/thomas/dev/python/sqlalchemy_test/.env/lib/python3.8/site-packages/sqlalchemy/event/attr.py", line 261, in __call__
    fn(*args, **kw)
  File "/home/thomas/dev/python/sqlalchemy_test/.env/lib/python3.8/site-packages/sqlalchemy/sql/naming.py", line 180, in _constraint_name
    newname = _constraint_name_for_table(const, table)
  File "/home/thomas/dev/python/sqlalchemy_test/.env/lib/python3.8/site-packages/sqlalchemy/sql/naming.py", line 157, in _constraint_name_for_table
    convention
  File "/home/thomas/dev/python/sqlalchemy_test/.env/lib/python3.8/site-packages/sqlalchemy/sql/naming.py", line 108, in __getitem__
    tokens.append(getattr(self, attr)(idx))
  File "/home/thomas/dev/python/sqlalchemy_test/.env/lib/python3.8/site-packages/sqlalchemy/sql/naming.py", line 86, in _key_referred_column_X_name
    return fk.column.name
  File "/home/thomas/dev/python/sqlalchemy_test/.env/lib/python3.8/site-packages/sqlalchemy/util/langhelpers.py", line 883, in __get__
    obj.__dict__[self.__name__] = result = self.fget(obj)
  File "/home/thomas/dev/python/sqlalchemy_test/.env/lib/python3.8/site-packages/sqlalchemy/sql/schema.py", line 2057, in column
    raise exc.NoReferencedColumnError(
sqlalchemy.exc.NoReferencedColumnError: Could not initialize target column for ForeignKey 'parent.id' on table 'child': table 'parent' has no column named 'id'

After some investigating, I realized the the naming convention was applied before the ForeignKey had been associated to its referenced Column so, as a test, I made this rather crude modification to delay the _set_parent_with_dispatch after the Column had been associated. It works both on 1.3.16 and master, but my knowledge of SQLAlchemy's internals is not good, and the fix seems to delay a huge chunk of logic (a lot more than just applying the naming convention). I'm posting it here in case it can help in finding an actual fix.

diff --git a/lib/sqlalchemy/sql/schema.py b/lib/sqlalchemy/sql/schema.py
index 689eda11d..4245a7f45 100644
--- a/lib/sqlalchemy/sql/schema.py
+++ b/lib/sqlalchemy/sql/schema.py
@@ -2088,6 +2088,7 @@ class ForeignKey(DialectKWArgs, SchemaItem):
         # standalone ForeignKey - create ForeignKeyConstraint
         # on the hosting Table when attached to the Table.
         assert isinstance(table, Table)
+        created = False
         if self.constraint is None:
             self.constraint = ForeignKeyConstraint(
                 [],
@@ -2102,7 +2103,8 @@ class ForeignKey(DialectKWArgs, SchemaItem):
                 **self._unvalidated_dialect_kw
             )
             self.constraint._append_element(column, self)
-            self.constraint._set_parent_with_dispatch(table)
+            created = True
+
         table.foreign_keys.add(self)
         # set up remote ".column" attribute, or a note to pick it
         # up when the other Table/Column shows up
@@ -2124,6 +2126,8 @@ class ForeignKey(DialectKWArgs, SchemaItem):
             _column = self._colspec
             self._set_target_column(_column)
 
+        if created:
+            self.constraint._set_parent_with_dispatch(table)
 
 class DefaultGenerator(SchemaItem):
     """Base class for column *default* values."""

Otherwise, a least-side-effect-possible solution would be to detect the missing column inside of sqlalchemy.sql.naming.ConventionDict._key_referred_column_X_name, and raise a clearer error. Something like this (pseudo-code, just to illustrate):

def _key_referred_column_X_name(self, idx):
    fk = self.const.elements[idx]
    try:
        return fk.column.name
    except NoReferencedColumnError as err:
        raise NamingConventionError(
            'Token column_0_name cannot be used with ForeignKey(str)'
        ) from err

Versions

  • OS: Manjaro Linux
  • Python: 3.8.2
  • SQLAlchemy: 1.3.16 + master (both for the bug and the ugly fix)
  • Database: None
  • DBAPI: None

thomas-mckay avatar May 24 '20 17:05 thomas-mckay

sure, naming convention stuff has a lot of issues like this. if your patch does not break any current use cases I would gladly evaluate a pull request that includes a test. any interest ?

zzzeek avatar May 24 '20 18:05 zzzeek

I'll look into it. I'll try to setup a proper dev env for SQLAlchemy and get back to you ;)

thomas-mckay avatar May 24 '20 19:05 thomas-mckay

this likely just needs a better exception message for now, as this is a pretty edge case with an easy workaround and im not expecting the naming convention mechanics to be easy to change here.

zzzeek avatar May 26 '21 12:05 zzzeek

@thomas-mckay Thanks for discovering the cause of this (referred_column_*_name). I ran into it a couple days ago and have been banging my head against a wall (mostly figuratively) until now. Most answers I found for NoReferencedColumnError talked about migrations, schema mismatch, dropping the existing table – not helpful when it throws at the declaration stage and I have no table yet…. At least now I can take out the problematic parameter and get this working.

(For the record, using SA 1.4.31 with Docker image python:3.9-bullseye.)

criptych avatar Feb 03 '22 19:02 criptych

Although, in those cases, declaring the ForeignKeyConstraint separately via __table_args__ works like a charm.

I'd note that, when I saw this, I though that it could solve #7706. But, I still get NoReferencedTableError, so I assume there is in fact no way to use referred_column, unless you construct all your ORM models in the right order?

chrisjsewell avatar Feb 15 '22 12:02 chrisjsewell