sqlacodegen icon indicating copy to clipboard operation
sqlacodegen copied to clipboard

Add constraint naming convention configuration option

Open leonarduschen opened this issue 2 years ago • 3 comments

Moving the discussion here from #120


One feature I think would be great to have is to allow customization of constraint naming conventions. We already have utils.uses_default_name, but it's currently not super useful because sqlalchemy.MetaData.naming_convention is always {'ix': 'ix_%(column_0_label)s'} by default. (Reference: https://docs.sqlalchemy.org/en/14/core/constraints.html#the-default-naming-convention)

We can pass the naming conventions as command line args like

sqlacodegen --conventions '"ix": "ix_%(column_0_label)s", "uq": "uq_%(table_name)s_%(column_0_name)s", "pk": "pk_%(table_name)s"'

or maybe have it read from a JSON file for easier formatting.

The implementation would be pretty straightforward. We just need to modify the MetaData in cli.main.

We will also need to make utils.uses_default_name check MetaData.naming_convention properly; right now it just checks for abbreviations: "fk", "pk", "ix", "ck", "uq" but the actual Constraint classes (e.g., PrimaryKeyConstraint) are valid as well. (Reference: https://docs.sqlalchemy.org/en/14/core/constraints.html#configuring-a-naming-convention-for-a-metadata-collection)

EDIT: formatting, typo

Originally posted by @leonarduschen in https://github.com/agronholm/sqlacodegen/issues/120#issuecomment-1087870235


@leonarduschen sounds good on the general level. I do question naming conventions being passed on the command line. That rabbit hole goes pretty deep when everybody wants to customize every aspect of the code generation step. A YAML or TOML based configuration file would be better. I'm looking forward to seeing what you come up with.

My intent with the next release is to create a system where you could have your own generator that just gets passed elements to be formatted (tables, classes, whatnot). If would then generate the code, optionally falling back to the default behavior.

Originally posted by @agronholm in https://github.com/agronholm/sqlacodegen/issues/120#issuecomment-1092788085

leonarduschen avatar Apr 16 '22 05:04 leonarduschen

I found 2 additional issues related to naming conventions:

  1. A bug from SQLAlchemy: https://github.com/sqlalchemy/sqlalchemy/issues/7958

    There's a bug when using referrenced table's columns in naming convention (e.g., %(referred_column_0_name)s", %(referred_column_0_N_name)s") together with ForeignKey. The tokens work fine using the ForeignKeyConstraint though.

    The bug fix will be released in 1.4.36, but this means we'll have a check of SQLAlchemy version:

    • For versions <1.4.36, we should use a nameless ForeignKeyConstraint instead of ForeignKey directly in Column when uses_default_name is True
    • For versions >=1.4.36 we should have the expeced behavior of using ForeignKey directly in Column when uses_default_name is True
  2. The naming convention token %(constraint_name)s

    We're not currently handling the token uses_default_name correctly. It's best to illustrate this with an example:

    Consider the following test case (copied from #197):

    def test_constraint_name_token(self, generator: CodeGenerator) -> None:
        generator.metadata.naming_convention = {
            "ck": "ck_%(table_name)s_%(constraint_name)s",
            "pk": "pk_%(table_name)s",
        }
    
        Table(
            "simple",
            generator.metadata,
            Column("id", INTEGER),
            Column("number", INTEGER),
            PrimaryKeyConstraint("id", name="pk_simple"),
            CheckConstraint("id > 0", name=conv("ck_simple_idcheck")),
            CheckConstraint("number > 0", name=conv("non_default_name")),
        )
    

    Current output:

    from sqlalchemy import CheckConstraint, Column, Integer
    from sqlalchemy.orm import declarative_base
    
    Base = declarative_base()
    Base.metadata.naming_convention = {'ck': 'ck_%(table_name)s_%(constraint_name)s', 'pk': 'pk_%(table_name)s'}
    
    
    class Simple(Base):
        __tablename__ = 'simple'
        __table_args__ = (
            CheckConstraint('id > 0', name='ck_simple_idcheck'),
            CheckConstraint('number > 0', name='non_default_name')
        )
    
        id = Column(Integer, primary_key=True)
        number = Column(Integer
    

    Expected output is:

    from sqlalchemy import CheckConstraint, Column, Integer, MetaData
    from sqlalchemy.orm import declarative_base
    from sqlalchemy.sql.elements import conv
    
    Base = declarative_base()
    Base.metadata.naming_convention = {'ck': 'ck_%(table_name)s_%(constraint_name)s', 'pk': 'pk_%(table_name)s'}
    
    
    class Simple(Base):
        __tablename__ = 'simple'
        __table_args__ = (
            CheckConstraint('id > 0', name='idcheck'),
            CheckConstraint('number > 0', name=conv('non_default_name'))
        )
    
        id = Column(Integer, primary_key=True)
        number = Column(Integer)
    

    There are 2 things to do here:

    • uses_default_name must be fixed to be able to handle constraint_name correctly
    • For contraints whose naming convention contains %(constraint_name)s, if the name of that constraint does not fit the naming convention (i.e., uses_default_name is False), it must be marked with sqlalchemy.sql.elements.conv

leonarduschen avatar Apr 26 '22 08:04 leonarduschen

Since we already require SQLAlchemy 1.4 in the latest release, we can just bump the minimum version requirement to 1.4.36 and not have to add any workarounds.

agronholm avatar Apr 26 '22 09:04 agronholm

SQLAlchemy 1.4.36 was released an hour ago so I've pushed changes to set the minimum sqla requirement to that now. Should address the first issue you described.

agronholm avatar Apr 26 '22 22:04 agronholm