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

Add constraint naming convention configuration option #193

Open
leonarduschen opened this issue Apr 16, 2022 · 3 comments · May be fixed by #197
Open

Add constraint naming convention configuration option #193

leonarduschen opened this issue Apr 16, 2022 · 3 comments · May be fixed by #197

Comments

@leonarduschen
Copy link
Contributor

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 #120 (comment)


@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 #120 (comment)

@leonarduschen
Copy link
Contributor Author

I found 2 additional issues related to naming conventions:

  1. A bug from SQLAlchemy: Using naming convention token referred_column_0_name raises sqlalchemy.exc.NoReferencedColumnError sqlalchemy/sqlalchemy#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 Add constraint naming convention configuration option #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

@agronholm
Copy link
Owner

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 added a commit that referenced this issue Apr 26, 2022
This should partially address #193.
@agronholm
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants