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

Naming conventions in metadata sequences are ignored #1396

Open
iurisilvio opened this issue Jan 11, 2024 · 4 comments
Open

Naming conventions in metadata sequences are ignored #1396

iurisilvio opened this issue Jan 11, 2024 · 4 comments

Comments

@iurisilvio
Copy link
Contributor

Describe the bug

Metadata sequences were implemented in #38, only for auto generation, in 7d5f6ea.

The SchemaObjects.metadata() is older than the sequences implementation and don't check if it is a list. https://github.com/sqlalchemy/alembic/blob/main/alembic/operations/schemaobj.py#L193-L202

Expected behavior

Naming conventions should be respected with metadata sequences.

To Reproduce
Please try to provide a Minimal, Complete, and Verifiable example, with the migration script and/or the SQLAlchemy tables or models involved.
See also Reporting Bugs on the website.

from sqlalchemy import MetaData
from alembic.config import Config
from alembic.operations.schemaobj import SchemaObjects
from alembic.runtime.environment import EnvironmentContext
from alembic.script import ScriptDirectory

convention = {
    "ix": "foo_ix_%(table_name)s_%(column_0_name)s",
}

context = EnvironmentContext(Config(), ScriptDirectory("."))
context.configure(
    dialect_name="postgresql",
    target_metadata=[
        MetaData(naming_convention=convention),
        MetaData(),
    ],
)
# This is unexpectedly the default naming convention `immutabledict({'ix': 'ix_%(column_0_label)s'})`
print(SchemaObjects(context.get_context()).metadata().naming_convention)

context = EnvironmentContext(Config(), ScriptDirectory("."))
context.configure(
    dialect_name="postgresql",
    target_metadata=MetaData(naming_convention=convention),
)
# It works as expected
print(SchemaObjects(context.get_context()).metadata().naming_convention)

Error

There is no error, but it doesn't use my conventions.

Versions.

  • OS:
  • Python:
  • Alembic:
  • SQLAlchemy:
  • Database:
  • DBAPI:

Additional context

Have a nice day!

@iurisilvio iurisilvio added the requires triage New issue that requires categorization label Jan 11, 2024
@iurisilvio iurisilvio changed the title Naming conventions are not used with metadata sequences Naming conventions in metadata sequences are ignored Jan 11, 2024
@zzzeek zzzeek added bug Something isn't working autogenerate - rendering op directives and removed requires triage New issue that requires categorization autogenerate - rendering labels Jan 11, 2024
@zzzeek
Copy link
Member

zzzeek commented Jan 11, 2024

this is a little bit of a minor disaster and I think we might have to get away with doing something simple but not 100% of the cases, which is, "choose the naming convention from the first MetaData".

Expected behavior
Naming conventions should be respected with metadata sequences.

So this...is not really generalizable. what if you had a list with two MetaData objects where both have naming conventions, that are different? what is expected then?

the docs at https://alembic.sqlalchemy.org/en/latest/naming.html#integration-of-naming-conventions-into-operations-autogenerate definitely have to change to accommodate this. we might want to consider warnings if multiple metadatas are present and differing naming conventions are located.

For tests here we have to ensure autogenerate rendering takes this into account as we see in test_autogen_render->RenderNamingConventionTest, tested with two metadata objects with all combinations of naming conventions / ordering /present or not - the render should always be using the naming convention that applies to the object being rendered, which I think should be the case already right now.

then for the list issue here we would have to set up more tests in test_op_naming_convention.py

@CaselIT any ideas here

@CaselIT
Copy link
Member

CaselIT commented Jan 11, 2024

since it's something new I think it would make sense to either use the naming convention of the fist metadata or raise if the metadata have different non-empty naming conventions

@iurisilvio
Copy link
Contributor Author

I probably just don't understand the idea, but isn't it possible to match the table name with the metadata.tables? The AutogenContext has something like this.

@zzzeek
Copy link
Member

zzzeek commented Jan 11, 2024

I probably just don't understand the idea, but isn't it possible to match the table name with the metadata.tables? The AutogenContext has something like this.

that's interesting, I guess every op directive we have technically has a way to locate a table name which could then be searched in all the MetaData objects. If two of those MetaData objects have the same effective schema/table name, we'd assume we can just use the first one.

Would be a pretty big change all throughout how the op directive "impls" function since that context would need to be passed all the way through.

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

No branches or pull requests

3 participants