codeant-ai-for-open-source[bot] commented on code in PR #39859: URL: https://github.com/apache/superset/pull/39859#discussion_r3507481193
########## tests/integration_tests/migrations/composite_pk_association_tables__tests.py: ########## @@ -0,0 +1,137 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Schema-shape assertion tests for the composite-PK association-tables +migration (revision 2bee73611e32). + +Builds the pre-migration shape against an isolated in-memory SQLite engine, +runs the migration's ``upgrade()``, and asserts the resulting shape: no +``id`` column, composite PK on the two FK columns, and no redundant +``UNIQUE(fk1, fk2)`` on the two tables that previously carried one. + +Continuum-restore verification is OUT OF SCOPE; that work lives in the +entity-versioning follow-up. Cross-backend verification (PostgreSQL, +MySQL) is handled by CI's test-postgres / test-mysql shards. +""" + +from importlib import import_module + +import pytest +import sqlalchemy as sa +from alembic.migration import MigrationContext +from alembic.operations import Operations +from sqlalchemy import inspect + +# Import the migration module under test. +_migration = import_module( + "superset.migrations.versions." + "2026-05-01_23-36_2bee73611e32_composite_pk_association_tables" +) +AFFECTED_TABLES = _migration.AFFECTED_TABLES +TABLES_WITH_PRE_EXISTING_UNIQUE = _migration.TABLES_WITH_PRE_EXISTING_UNIQUE +TABLES_WITH_NULLABLE_FKS = _migration.TABLES_WITH_NULLABLE_FKS + + [email protected](scope="module") +def post_upgrade_engine() -> sa.engine.Engine: + """An isolated in-memory SQLite engine with the migration applied to a + pre-migration-shaped seed schema. Used by the post-upgrade assertions + below. Module-scoped so the upgrade only runs once per module. + + FK columns are NULLABLE on the six tables that historically allowed + NULLs — with ``nullable=False`` here, ``test_fk_columns_not_null`` + would pass trivially rather than because the migration promoted + anything.""" + engine = sa.create_engine("sqlite:///:memory:") + md = sa.MetaData() + for t in AFFECTED_TABLES: + nullable = t.name in TABLES_WITH_NULLABLE_FKS + cols: list[sa.SchemaItem] = [ + sa.Column("id", sa.Integer, primary_key=True), + sa.Column(t.fk1, sa.Integer, nullable=nullable), + sa.Column(t.fk2, sa.Integer, nullable=nullable), + ] + constraints: list[sa.SchemaItem] = [] + if t.name in TABLES_WITH_PRE_EXISTING_UNIQUE: + constraints.append(sa.UniqueConstraint(t.fk1, t.fk2)) + sa.Table(t.name, md, *cols, *constraints) + md.create_all(engine) + + # Apply the migration's upgrade() against this engine via Alembic's + # MigrationContext, patching the migration module's ``op`` reference. + with engine.connect() as conn: + ctx = MigrationContext.configure(conn) + ops = Operations(ctx) + original_op = _migration.op + _migration.op = ops # type: ignore[attr-defined] + try: + _migration.upgrade() + finally: + _migration.op = original_op # type: ignore[attr-defined] + return engine + + [email protected]("t", AFFECTED_TABLES, ids=lambda t: t.name) +def test_no_id_column(post_upgrade_engine: sa.engine.Engine, t) -> None: Review Comment: **Suggestion:** Add an explicit type annotation for the `t` parameter in this test function (using the association-table type used by the migration module). [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The custom rule requires new or modified Python code to include type hints on functions and methods. This test function leaves `t` unannotated, so the suggestion identifies a real violation. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c1a851091de44f42bcae2e565d2a006d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c1a851091de44f42bcae2e565d2a006d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/integration_tests/migrations/composite_pk_association_tables__tests.py **Line:** 88:88 **Comment:** *Custom Rule: Add an explicit type annotation for the `t` parameter in this test function (using the association-table type used by the migration module). Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39859&comment_hash=b628113b4833c084f8f76afeeb62bad5b467804279af06fda56eb7a2526b3e53&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39859&comment_hash=b628113b4833c084f8f76afeeb62bad5b467804279af06fda56eb7a2526b3e53&reaction=dislike'>👎</a> ########## tests/integration_tests/migrations/composite_pk_association_tables__tests.py: ########## @@ -0,0 +1,137 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Schema-shape assertion tests for the composite-PK association-tables +migration (revision 2bee73611e32). + +Builds the pre-migration shape against an isolated in-memory SQLite engine, +runs the migration's ``upgrade()``, and asserts the resulting shape: no +``id`` column, composite PK on the two FK columns, and no redundant +``UNIQUE(fk1, fk2)`` on the two tables that previously carried one. + +Continuum-restore verification is OUT OF SCOPE; that work lives in the +entity-versioning follow-up. Cross-backend verification (PostgreSQL, +MySQL) is handled by CI's test-postgres / test-mysql shards. +""" + +from importlib import import_module + +import pytest +import sqlalchemy as sa +from alembic.migration import MigrationContext +from alembic.operations import Operations +from sqlalchemy import inspect + +# Import the migration module under test. +_migration = import_module( + "superset.migrations.versions." + "2026-05-01_23-36_2bee73611e32_composite_pk_association_tables" +) +AFFECTED_TABLES = _migration.AFFECTED_TABLES +TABLES_WITH_PRE_EXISTING_UNIQUE = _migration.TABLES_WITH_PRE_EXISTING_UNIQUE +TABLES_WITH_NULLABLE_FKS = _migration.TABLES_WITH_NULLABLE_FKS + + [email protected](scope="module") +def post_upgrade_engine() -> sa.engine.Engine: + """An isolated in-memory SQLite engine with the migration applied to a + pre-migration-shaped seed schema. Used by the post-upgrade assertions + below. Module-scoped so the upgrade only runs once per module. + + FK columns are NULLABLE on the six tables that historically allowed + NULLs — with ``nullable=False`` here, ``test_fk_columns_not_null`` + would pass trivially rather than because the migration promoted + anything.""" + engine = sa.create_engine("sqlite:///:memory:") + md = sa.MetaData() + for t in AFFECTED_TABLES: + nullable = t.name in TABLES_WITH_NULLABLE_FKS + cols: list[sa.SchemaItem] = [ + sa.Column("id", sa.Integer, primary_key=True), + sa.Column(t.fk1, sa.Integer, nullable=nullable), + sa.Column(t.fk2, sa.Integer, nullable=nullable), + ] + constraints: list[sa.SchemaItem] = [] + if t.name in TABLES_WITH_PRE_EXISTING_UNIQUE: + constraints.append(sa.UniqueConstraint(t.fk1, t.fk2)) + sa.Table(t.name, md, *cols, *constraints) + md.create_all(engine) + + # Apply the migration's upgrade() against this engine via Alembic's + # MigrationContext, patching the migration module's ``op`` reference. + with engine.connect() as conn: + ctx = MigrationContext.configure(conn) + ops = Operations(ctx) + original_op = _migration.op + _migration.op = ops # type: ignore[attr-defined] + try: + _migration.upgrade() + finally: + _migration.op = original_op # type: ignore[attr-defined] + return engine + + [email protected]("t", AFFECTED_TABLES, ids=lambda t: t.name) +def test_no_id_column(post_upgrade_engine: sa.engine.Engine, t) -> None: + """The synthetic ``id`` column is gone from each affected table.""" + insp = inspect(post_upgrade_engine) + column_names = {c["name"] for c in insp.get_columns(t.name)} + assert "id" not in column_names, ( + f"{t.name} still has an 'id' column after migration; " + f"composite-PK conversion incomplete" + ) + + [email protected]("t", AFFECTED_TABLES, ids=lambda t: t.name) +def test_primary_key_is_composite_fks(post_upgrade_engine: sa.engine.Engine, t) -> None: + """The primary key of each affected table is exactly ``(fk1, fk2)``.""" + insp = inspect(post_upgrade_engine) + pk_cols = set(insp.get_pk_constraint(t.name).get("constrained_columns", [])) + assert pk_cols == {t.fk1, t.fk2}, ( + f"{t.name} primary key is {pk_cols}, expected {{{t.fk1}, {t.fk2}}}" + ) + + [email protected]( + "t", + [t for t in AFFECTED_TABLES if t.name in TABLES_WITH_PRE_EXISTING_UNIQUE], + ids=lambda t: t.name, +) +def test_redundant_unique_dropped(post_upgrade_engine: sa.engine.Engine, t) -> None: Review Comment: **Suggestion:** Add a type hint for the `t` parameter in this test to satisfy the requirement that function signatures are fully annotated. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> This function signature omits a type hint for `t`, which violates the requirement that new or modified Python functions be fully annotated where possible. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=53a0d64507c448eaa04888e6756d8724&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=53a0d64507c448eaa04888e6756d8724&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/integration_tests/migrations/composite_pk_association_tables__tests.py **Line:** 113:113 **Comment:** *Custom Rule: Add a type hint for the `t` parameter in this test to satisfy the requirement that function signatures are fully annotated. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39859&comment_hash=14dc635d9b1f444a297c2af888866899dc3c365edc284edd22c15d9919956edf&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39859&comment_hash=14dc635d9b1f444a297c2af888866899dc3c365edc284edd22c15d9919956edf&reaction=dislike'>👎</a> ########## tests/integration_tests/migrations/composite_pk_association_tables__tests.py: ########## @@ -0,0 +1,137 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Schema-shape assertion tests for the composite-PK association-tables +migration (revision 2bee73611e32). + +Builds the pre-migration shape against an isolated in-memory SQLite engine, +runs the migration's ``upgrade()``, and asserts the resulting shape: no +``id`` column, composite PK on the two FK columns, and no redundant +``UNIQUE(fk1, fk2)`` on the two tables that previously carried one. + +Continuum-restore verification is OUT OF SCOPE; that work lives in the +entity-versioning follow-up. Cross-backend verification (PostgreSQL, +MySQL) is handled by CI's test-postgres / test-mysql shards. +""" + +from importlib import import_module + +import pytest +import sqlalchemy as sa +from alembic.migration import MigrationContext +from alembic.operations import Operations +from sqlalchemy import inspect + +# Import the migration module under test. +_migration = import_module( + "superset.migrations.versions." + "2026-05-01_23-36_2bee73611e32_composite_pk_association_tables" +) +AFFECTED_TABLES = _migration.AFFECTED_TABLES +TABLES_WITH_PRE_EXISTING_UNIQUE = _migration.TABLES_WITH_PRE_EXISTING_UNIQUE +TABLES_WITH_NULLABLE_FKS = _migration.TABLES_WITH_NULLABLE_FKS + + [email protected](scope="module") +def post_upgrade_engine() -> sa.engine.Engine: + """An isolated in-memory SQLite engine with the migration applied to a + pre-migration-shaped seed schema. Used by the post-upgrade assertions + below. Module-scoped so the upgrade only runs once per module. + + FK columns are NULLABLE on the six tables that historically allowed + NULLs — with ``nullable=False`` here, ``test_fk_columns_not_null`` + would pass trivially rather than because the migration promoted + anything.""" + engine = sa.create_engine("sqlite:///:memory:") + md = sa.MetaData() + for t in AFFECTED_TABLES: + nullable = t.name in TABLES_WITH_NULLABLE_FKS + cols: list[sa.SchemaItem] = [ + sa.Column("id", sa.Integer, primary_key=True), + sa.Column(t.fk1, sa.Integer, nullable=nullable), + sa.Column(t.fk2, sa.Integer, nullable=nullable), + ] + constraints: list[sa.SchemaItem] = [] + if t.name in TABLES_WITH_PRE_EXISTING_UNIQUE: + constraints.append(sa.UniqueConstraint(t.fk1, t.fk2)) + sa.Table(t.name, md, *cols, *constraints) + md.create_all(engine) + + # Apply the migration's upgrade() against this engine via Alembic's + # MigrationContext, patching the migration module's ``op`` reference. + with engine.connect() as conn: + ctx = MigrationContext.configure(conn) + ops = Operations(ctx) + original_op = _migration.op + _migration.op = ops # type: ignore[attr-defined] + try: + _migration.upgrade() + finally: + _migration.op = original_op # type: ignore[attr-defined] + return engine + + [email protected]("t", AFFECTED_TABLES, ids=lambda t: t.name) +def test_no_id_column(post_upgrade_engine: sa.engine.Engine, t) -> None: + """The synthetic ``id`` column is gone from each affected table.""" + insp = inspect(post_upgrade_engine) + column_names = {c["name"] for c in insp.get_columns(t.name)} + assert "id" not in column_names, ( + f"{t.name} still has an 'id' column after migration; " + f"composite-PK conversion incomplete" + ) + + [email protected]("t", AFFECTED_TABLES, ids=lambda t: t.name) +def test_primary_key_is_composite_fks(post_upgrade_engine: sa.engine.Engine, t) -> None: + """The primary key of each affected table is exactly ``(fk1, fk2)``.""" + insp = inspect(post_upgrade_engine) + pk_cols = set(insp.get_pk_constraint(t.name).get("constrained_columns", [])) + assert pk_cols == {t.fk1, t.fk2}, ( + f"{t.name} primary key is {pk_cols}, expected {{{t.fk1}, {t.fk2}}}" + ) + + [email protected]( + "t", + [t for t in AFFECTED_TABLES if t.name in TABLES_WITH_PRE_EXISTING_UNIQUE], + ids=lambda t: t.name, +) +def test_redundant_unique_dropped(post_upgrade_engine: sa.engine.Engine, t) -> None: + """For the two tables that previously carried a UNIQUE(fk1, fk2), that + constraint is now subsumed by the composite PK and must not appear + separately in the unique-constraint list.""" + insp = inspect(post_upgrade_engine) + redundant_pair = {t.fk1, t.fk2} + for uc in insp.get_unique_constraints(t.name): + cols = set(uc.get("column_names", [])) + assert cols != redundant_pair, ( + f"{t.name} still carries a redundant UniqueConstraint over " + f"{redundant_pair} (name={uc.get('name')!r}); " + f"composite-PK conversion incomplete" + ) + + [email protected]("t", AFFECTED_TABLES, ids=lambda t: t.name) +def test_fk_columns_not_null(post_upgrade_engine: sa.engine.Engine, t) -> None: Review Comment: **Suggestion:** Add an explicit annotation for the `t` parameter in this test function to make the signature fully typed. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The `t` parameter is not type-annotated in this new test function, so the custom type-hints rule is indeed violated. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2664f432fd9b46b29996ba9301e68e28&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=2664f432fd9b46b29996ba9301e68e28&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/integration_tests/migrations/composite_pk_association_tables__tests.py **Line:** 129:129 **Comment:** *Custom Rule: Add an explicit annotation for the `t` parameter in this test function to make the signature fully typed. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39859&comment_hash=64423825ef70d337efe5a43809350a3e2ec39378df83db0a44c6c18a1f03157b&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39859&comment_hash=64423825ef70d337efe5a43809350a3e2ec39378df83db0a44c6c18a1f03157b&reaction=dislike'>👎</a> ########## tests/integration_tests/migrations/composite_pk_round_trip__tests.py: ########## @@ -0,0 +1,200 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Schema round-trip tests for the composite-PK association-tables migration +(revision 2bee73611e32). Builds the pre-migration shape against an in-memory +SQLite engine, runs the migration's ``upgrade()``, asserts the post-upgrade +shape, runs ``downgrade()``, asserts the prior shape is restored (modulo the +documented FK NOT NULL asymmetry), and re-runs ``upgrade()`` to verify +idempotency. + +This is run against an isolated in-memory engine via Alembic's +``MigrationContext`` so the test does not perturb the project's test DB. + +Cross-backend (Postgres/MySQL) verification is handled by CI's +test-postgres / test-mysql shards running ``superset db upgrade``. This +file covers the SQLite slice. +""" + +from importlib import import_module +from typing import Any + +import sqlalchemy as sa +from alembic.migration import MigrationContext +from alembic.operations import Operations +from sqlalchemy import inspect + +# Import the migration module under test. +_migration = import_module( + "superset.migrations.versions." + "2026-05-01_23-36_2bee73611e32_composite_pk_association_tables" +) +AFFECTED_TABLES = _migration.AFFECTED_TABLES +TABLES_WITH_PRE_EXISTING_UNIQUE = _migration.TABLES_WITH_PRE_EXISTING_UNIQUE +TABLES_WITH_NULLABLE_FKS = _migration.TABLES_WITH_NULLABLE_FKS + + +def _build_pre_migration_schema(engine: sa.engine.Engine) -> None: + """Recreate the eight tables in their pre-migration shape (surrogate + ``id INTEGER PRIMARY KEY`` plus an optional ``UNIQUE(fk1, fk2)`` on the + two tables that previously carried one). FK columns are NULLABLE on + the six tables that historically allowed NULLs — fidelity matters: + with ``nullable=False`` here, the post-upgrade NOT NULL assertions + pass trivially rather than because the migration promoted anything, + and the NULL-row cleanup path can't be exercised. FKs to parent + tables are omitted to keep the test self-contained — we're testing + schema transformations, not FK enforcement.""" + md = sa.MetaData() + for t in AFFECTED_TABLES: + nullable = t.name in TABLES_WITH_NULLABLE_FKS + cols: list[sa.Column] = [ + sa.Column("id", sa.Integer, primary_key=True), + sa.Column(t.fk1, sa.Integer, nullable=nullable), + sa.Column(t.fk2, sa.Integer, nullable=nullable), + ] + constraints: list[sa.SchemaItem] = [] + if t.name in TABLES_WITH_PRE_EXISTING_UNIQUE: + constraints.append(sa.UniqueConstraint(t.fk1, t.fk2)) + sa.Table(t.name, md, *cols, *constraints) + md.create_all(engine) + + +def _shape(engine: sa.engine.Engine, table: str) -> dict[str, Any]: + """Return a structural summary for asserting equality across runs.""" + insp = inspect(engine) + pk = insp.get_pk_constraint(table).get("constrained_columns", []) + columns = sorted(c["name"] for c in insp.get_columns(table)) + uniques = sorted( + tuple(sorted(uc.get("column_names", []))) + for uc in insp.get_unique_constraints(table) + ) + return {"columns": columns, "pk": sorted(pk), "uniques": uniques} + + +def _run_with_alembic_context(engine: sa.engine.Engine, fn) -> None: Review Comment: **Suggestion:** Add an explicit callable type annotation for the `fn` parameter so the helper function has complete type hints. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The custom rule requires type hints on modified Python functions, and this helper leaves the `fn` parameter unannotated. That is a real type-hint omission in the current code. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=954c5ddb2b5a4b2eb82c29709d912e9a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=954c5ddb2b5a4b2eb82c29709d912e9a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/integration_tests/migrations/composite_pk_round_trip__tests.py **Line:** 87:87 **Comment:** *Custom Rule: Add an explicit callable type annotation for the `fn` parameter so the helper function has complete type hints. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39859&comment_hash=844870f5fbc0ba406755202dd89d2b667d0920ef6e349539b2c8829f99179b65&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39859&comment_hash=844870f5fbc0ba406755202dd89d2b667d0920ef6e349539b2c8829f99179b65&reaction=dislike'>👎</a> ########## tests/unit_tests/migrations/composite_pk_association_tables_test.py: ########## @@ -0,0 +1,144 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Unit tests for the composite-PK association-tables migration (revision +2bee73611e32). Verifies the post-migration constraint enforcement: duplicate +``(fk1, fk2)`` insertions fail with IntegrityError, distinct pairs succeed. + +Schema is built from the live ORM ``Table`` definitions via +``metadata.create_all(engine)`` against in-memory SQLite. This reflects the +post-T015–T018 ORM model state (composite-PK), independent of whether the +Alembic migration has run against the test DB. The two should agree. +""" + +import pytest +import sqlalchemy as sa +from sqlalchemy.exc import IntegrityError + +# (table_name, fk1_col, fk2_col, fk1_parent_table, fk2_parent_table) +# Parent-table names are needed to build the FK targets in the in-memory schema. +AFFECTED_TABLES = [ Review Comment: **Suggestion:** Add an explicit type annotation for the `AFFECTED_TABLES` constant (for example, a list of fixed-length string tuples) to satisfy the type-hint requirement for relevant variables. [custom_rule] **Severity Level:** Minor ⚠️ <details> <summary><b>Why it matters? 🤔 </b></summary> The module-level constant `AFFECTED_TABLES` is newly introduced without any type annotation, and it is a relevant variable that can be annotated under the Python type-hint rule. </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e9bedffb7fa249a18efde016426bf195&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=e9bedffb7fa249a18efde016426bf195&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/migrations/composite_pk_association_tables_test.py **Line:** 33:33 **Comment:** *Custom Rule: Add an explicit type annotation for the `AFFECTED_TABLES` constant (for example, a list of fixed-length string tuples) to satisfy the type-hint requirement for relevant variables. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39859&comment_hash=e9f826b77bb6bf0e1956617345ffc40e499df746c2959838957e26e66a3734b6&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39859&comment_hash=e9f826b77bb6bf0e1956617345ffc40e499df746c2959838957e26e66a3734b6&reaction=dislike'>👎</a> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
