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

create_foreign_key has wrong type annotations #1429

Closed
kasium opened this issue Feb 21, 2024 · 4 comments · Fixed by #1430
Closed

create_foreign_key has wrong type annotations #1429

kasium opened this issue Feb 21, 2024 · 4 comments · Fixed by #1430
Labels
bug Something isn't working pep 484 typing related issues

Comments

@kasium
Copy link
Contributor

kasium commented Feb 21, 2024

Describe the bug
BatchOperations.create_foreign_key states that the constraint_name should be a string, but it should be Optional[str]

Versions.

  • OS: Linux
  • Python: 3.12.0
  • Alembic: 1.12.1
  • SQLAlchemy: 1.4.51

Additional context
I'm happy to open a PR, but first wanted to create an issue

Have a nice day!

@kasium kasium added the requires triage New issue that requires categorization label Feb 21, 2024
@CaselIT
Copy link
Member

CaselIT commented Feb 21, 2024

Hi,

Indeed it seems to be a bug, since the normal create_fk has it optional:

def create_foreign_key(
cls,
operations: Operations,
constraint_name: Optional[str],
source_table: str,
referent_table: str,
local_cols: List[str],
remote_cols: List[str],
*,
onupdate: Optional[str] = None,
ondelete: Optional[str] = None,
deferrable: Optional[bool] = None,
initially: Optional[str] = None,
match: Optional[str] = None,
source_schema: Optional[str] = None,
referent_schema: Optional[str] = None,
**dialect_kw: Any,
) -> None:
"""Issue a "create foreign key" instruction using the
current migration context.
e.g.::
from alembic import op
op.create_foreign_key(
"fk_user_address",
"address",
"user",
["user_id"],
["id"],
)
This internally generates a :class:`~sqlalchemy.schema.Table` object
containing the necessary columns, then generates a new
:class:`~sqlalchemy.schema.ForeignKeyConstraint`
object which it then associates with the
:class:`~sqlalchemy.schema.Table`.
Any event listeners associated with this action will be fired
off normally. The :class:`~sqlalchemy.schema.AddConstraint`
construct is ultimately used to generate the ALTER statement.
:param constraint_name: Name of the foreign key constraint. The name
is necessary so that an ALTER statement can be emitted. For setups
that use an automated naming scheme such as that described at
:ref:`sqla:constraint_naming_conventions`,
``name`` here can be ``None``, as the event listener will
apply the name to the constraint object when it is associated
with the table.
:param source_table: String name of the source table.
:param referent_table: String name of the destination table.
:param local_cols: a list of string column names in the
source table.
:param remote_cols: a list of string column names in the
remote table.
:param onupdate: Optional string. If set, emit ON UPDATE <value> when
issuing DDL for this constraint. Typical values include CASCADE,
DELETE and RESTRICT.
:param ondelete: Optional string. If set, emit ON DELETE <value> when
issuing DDL for this constraint. Typical values include CASCADE,
DELETE and RESTRICT.
:param deferrable: optional bool. If set, emit DEFERRABLE or NOT
DEFERRABLE when issuing DDL for this constraint.
:param source_schema: Optional schema name of the source table.
:param referent_schema: Optional schema name of the destination table.
"""
op = cls(
constraint_name,
source_table,
referent_table,
local_cols,
remote_cols,
onupdate=onupdate,
ondelete=ondelete,
deferrable=deferrable,
source_schema=source_schema,
referent_schema=referent_schema,
initially=initially,
match=match,
**dialect_kw,
)
return operations.invoke(op)
@classmethod
def batch_create_foreign_key(
cls,
operations: BatchOperations,
constraint_name: str,
referent_table: str,
local_cols: List[str],
remote_cols: List[str],
*,
referent_schema: Optional[str] = None,
onupdate: Optional[str] = None,
ondelete: Optional[str] = None,
deferrable: Optional[bool] = None,
initially: Optional[str] = None,
match: Optional[str] = None,
**dialect_kw: Any,
) -> None:

A PR would be appreciated!

@CaselIT CaselIT added bug Something isn't working pep 484 typing related issues and removed requires triage New issue that requires categorization labels Feb 21, 2024
kasium added a commit to kasium/alembic that referenced this issue Feb 22, 2024
The constraint name parameter of create_foreign_key should be optional, but the batch function
defined it as str instead of Optional[str].

Closes sqlalchemy#1429
@kasium
Copy link
Contributor Author

kasium commented Feb 22, 2024

@CaselIT thanks for the confirmation. I'll open a PR

CaselIT pushed a commit that referenced this issue Feb 22, 2024
The constraint name parameter of create_foreign_key should be optional, but the batch function
defined it as str instead of Optional[str].

Closes #1429
@kasium
Copy link
Contributor Author

kasium commented Feb 24, 2024

Do you already have a plan when you want to release a new alembic version?

@CaselIT
Copy link
Member

CaselIT commented Feb 24, 2024

Not yet, but it shouldn't be too long

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pep 484 typing related issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants