codeant-ai-for-open-source[bot] commented on code in PR #39859: URL: https://github.com/apache/superset/pull/39859#discussion_r3507517633
########## docker/mysql-init/examples-init.sql: ########## @@ -0,0 +1,32 @@ +-- 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. + +-- MySQL counterpart to docker/docker-entrypoint-initdb.d/examples-init.sh. +-- Creates the analytics-examples database and user that Superset's +-- ``load-examples`` command writes to. Mounted by docker-compose-mysql.yml +-- at /docker-entrypoint-initdb.d/ so the MySQL image's first-boot +-- entrypoint runs it automatically. (The Postgres init scripts under +-- docker/docker-entrypoint-initdb.d/ are NOT mounted on the MySQL +-- service — they invoke psql, which doesn't exist in the MySQL image.) + +CREATE DATABASE IF NOT EXISTS examples + CHARACTER SET utf8mb4 + COLLATE utf8mb4_0900_ai_ci; + +CREATE USER IF NOT EXISTS 'examples'@'%' IDENTIFIED BY 'examples'; +GRANT ALL PRIVILEGES ON examples.* TO 'examples'@'%'; Review Comment: **Suggestion:** This creates a globally reachable MySQL account (`'%'` host wildcard) with a trivial hardcoded password and grants full privileges on the examples DB, which is an avoidable credential-exposure risk if this compose stack is reused outside strictly local environments. Restrict the host scope and avoid hardcoded plaintext credentials so access is not broadly open by default. [security] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx ⚠️ Docker MySQL dev stack exposes examples DB with trivial password. ⚠️ Weak default DB credentials may be reused beyond local setups. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open `docker/mysql-init/examples-init.sql` and observe at lines 26–31 that the script creates the `examples` database and the MySQL user `'examples'@'%'` with password `'examples'`, then grants `ALL PRIVILEGES` on the `examples` schema to that user. 2. Open `docker-compose-mysql.yml` and see at lines 51–61 that the Superset containers are configured to connect to this examples database using `EXAMPLES_USER=examples` and `EXAMPLES_PASSWORD=examples`, and at lines 64–76 that the MySQL service exposes port 3306 on the host as `127.0.0.1:${DATABASE_PORT_MYSQL:-13306}`. 3. Start the MySQL-backed dev stack as documented at `docker-compose-mysql.yml` lines 22–24 using `docker compose -f docker-compose.yml -f docker-compose-mysql.yml up`, which runs the MySQL container and executes `docker/mysql-init/examples-init.sql` on first boot. 4. From the same host, connect to MySQL with `mysql -h 127.0.0.1 -P13306 -u examples -pexamples` (using the port from step 2) and verify that the connection succeeds and that the `examples` user has full access to the `examples` database due to the `GRANT ALL PRIVILEGES` statement at `docker/mysql-init/examples-init.sql:31`. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c2a1cec3eaae4eb4b47ae81237871afc&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=c2a1cec3eaae4eb4b47ae81237871afc&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:** docker/mysql-init/examples-init.sql **Line:** 30:31 **Comment:** *Security: This creates a globally reachable MySQL account (`'%'` host wildcard) with a trivial hardcoded password and grants full privileges on the examples DB, which is an avoidable credential-exposure risk if this compose stack is reused outside strictly local environments. Restrict the host scope and avoid hardcoded plaintext credentials so access is not broadly open by default. 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=f6e39b496cc500e046d5f89a6f9462491bc6b1cf8d258dae110cc8ec0b52e0ea&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39859&comment_hash=f6e39b496cc500e046d5f89a6f9462491bc6b1cf8d258dae110cc8ec0b52e0ea&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: + """Run ``fn()`` (the migration's upgrade/downgrade body) inside a fresh + Alembic ``MigrationContext`` bound to ``engine``. Patches the + migration module's ``op`` to point at this context so its + ``op.get_bind()`` and ``op.batch_alter_table`` calls execute against + the in-memory engine.""" + with engine.connect() as conn: + ctx = MigrationContext.configure(conn) + ops = Operations(ctx) + original_op = _migration.op + _migration.op = ops # type: ignore[attr-defined] + try: + fn() + finally: + _migration.op = original_op # type: ignore[attr-defined] + + +def test_round_trip_against_in_memory_sqlite() -> None: + """Round-trip: pre-migration → upgrade → downgrade → upgrade again. + + Asserts: + - Post-upgrade shape: no ``id``, composite PK on (fk1, fk2), no + UNIQUE(fk1, fk2) on the two tables that previously carried one. + - Post-downgrade shape: ``id`` restored, PK back on (id), UNIQUE + re-added on the two tables. (FK columns remain NOT NULL — the + documented intentional asymmetry.) + - Post-re-upgrade idempotency: shape matches the first post-upgrade. + """ + engine = sa.create_engine("sqlite:///:memory:") + _build_pre_migration_schema(engine) + + _run_with_alembic_context(engine, _migration.upgrade) + + for t in AFFECTED_TABLES: + s = _shape(engine, t.name) + assert "id" not in s["columns"], f"{t.name}: id still present post-upgrade: {s}" + assert s["pk"] == sorted([t.fk1, t.fk2]), ( + f"{t.name}: PK is {s['pk']}, expected {sorted([t.fk1, t.fk2])}" + ) + assert tuple(sorted([t.fk1, t.fk2])) not in s["uniques"], ( + f"{t.name}: redundant UNIQUE not dropped post-upgrade: {s['uniques']}" + ) + + post_upgrade_shape = {t.name: _shape(engine, t.name) for t in AFFECTED_TABLES} + + _run_with_alembic_context(engine, _migration.downgrade) + + for t in AFFECTED_TABLES: + s = _shape(engine, t.name) + assert "id" in s["columns"], f"{t.name}: id not restored post-downgrade: {s}" + assert s["pk"] == ["id"], f"{t.name}: PK is {s['pk']}, expected ['id']" + if t.name in TABLES_WITH_PRE_EXISTING_UNIQUE: + assert tuple(sorted([t.fk1, t.fk2])) in s["uniques"], ( + f"{t.name}: UNIQUE not restored post-downgrade: {s['uniques']}" + ) + + _run_with_alembic_context(engine, _migration.upgrade) + + re_upgrade_shape = {t.name: _shape(engine, t.name) for t in AFFECTED_TABLES} + assert re_upgrade_shape == post_upgrade_shape, ( + "Re-upgrade shape differs from initial upgrade shape — " + "migration is not idempotent. " + f"diff: {set(re_upgrade_shape.items()) ^ set(post_upgrade_shape.items())}" + ) Review Comment: **Suggestion:** The failure-message diff computation converts `dict` values to sets, but each `.items()` entry contains an unhashable nested dict value; if this assertion ever fails, Python will raise `TypeError` while formatting the message and hide the real migration regression. Build the diff using a hashable representation (or plain pretty-printed dict comparison) so assertion failures remain diagnosable. [type error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx ⚠️ Idempotency regression surfaces as TypeError instead of clear diff. ⚠️ Debugging migration shape changes becomes slower and more difficult. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In `tests/integration_tests/migrations/composite_pk_round_trip__tests.py`, locate `_shape()` at lines 75–84, which returns a `dict` of the form `{"columns": columns, "pk": sorted(pk), "uniques": uniques}` where the values are lists and lists of tuples (unhashable when nested inside another dict). 2. Still in the same file, find `test_round_trip_against_in_memory_sqlite()` at lines 104–150, and note at lines 145–150 that `re_upgrade_shape` and `post_upgrade_shape` are dicts mapping table names to the `_shape()` dicts, and the assertion builds a diff in the f-string using `set(re_upgrade_shape.items()) ^ set(post_upgrade_shape.items())`. 3. Introduce an idempotency regression in the migration under test (for example by editing `superset/migrations/versions/2026-05-01_23-36_2bee73611e32_composite_pk_association_tables.py` so that the second `upgrade()` call creates a different schema), then run `pytest tests/integration_tests/migrations/composite_pk_round_trip__tests.py::test_round_trip_against_in_memory_sqlite`. 4. When the shapes differ, the condition `re_upgrade_shape == post_upgrade_shape` is false and Python evaluates the f-string at lines 146–150; constructing `set(re_upgrade_shape.items())` attempts to hash tuples whose second element is a dict, causing `TypeError: unhashable type: 'dict'` and turning the test into an error that obscures the underlying schema-difference regression. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6a565c9dd07b4a20aa03e18a35aae6d5&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=6a565c9dd07b4a20aa03e18a35aae6d5&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:** 146:150 **Comment:** *Type Error: The failure-message diff computation converts `dict` values to sets, but each `.items()` entry contains an unhashable nested dict value; if this assertion ever fails, Python will raise `TypeError` while formatting the message and hide the real migration regression. Build the diff using a hashable representation (or plain pretty-printed dict comparison) so assertion failures remain diagnosable. 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=3d9486edfe093af93986413e827a4d6017e3a9ae13c5a36248a92293979ddae5&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39859&comment_hash=3d9486edfe093af93986413e827a4d6017e3a9ae13c5a36248a92293979ddae5&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. Review Comment: **Suggestion:** The module docstring states the schema is built from live ORM `Table` definitions, but this test actually builds a synthetic schema from a hardcoded table list, so the documentation contradicts behavior and can mislead maintainers about what drift this test can detect. [docstring mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx ⚠️ Maintainers misled about ORM versus migration drift coverage. ⚠️ Future refactors may rely on nonexistent ORM-backed schema checks. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open `tests/unit_tests/migrations/composite_pk_association_tables_test.py` and read the module docstring at lines 17–25, which states that the schema is built from the live ORM `Table` definitions via `metadata.create_all(engine)` and that this reflects the post-migration ORM model state. 2. Scroll down to the implementation at lines 31–60, where `AFFECTED_TABLES` is defined as a hardcoded list of `(table_name, fk1_col, fk2_col, fk1_parent_table, fk2_parent_table)` tuples, with no imports of ORM models from `superset.models.dashboard`, `superset.models.slice`, or other production modules. 3. Inspect `_build_in_memory_schema()` at lines 63–99 and the parametrized tests `test_duplicate_insert_rejected` and `test_distinct_pairs_accepted` at lines 112–144; these functions construct minimal `sa.Table` objects from the literals in `AFFECTED_TABLES` and use `metadata.create_all(engine)` on this synthetic metadata, without referencing the live ORM `Table` definitions. 4. From these concrete code paths, confirm that the docstring claim about using the live ORM tables is inaccurate, which can mislead maintainers into believing this unit test will automatically catch drift between ORM models and Alembic migrations when in fact it only validates constraints on the manually specified schema. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=11315b28576e44f3b541e5dd9984cc85&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=11315b28576e44f3b541e5dd9984cc85&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:** 21:24 **Comment:** *Docstring Mismatch: The module docstring states the schema is built from live ORM `Table` definitions, but this test actually builds a synthetic schema from a hardcoded table list, so the documentation contradicts behavior and can mislead maintainers about what drift this test can detect. 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=968eb13488c65d01b28909713099a7e8bc0c286320d8925873530478f75cdbe6&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39859&comment_hash=968eb13488c65d01b28909713099a7e8bc0c286320d8925873530478f75cdbe6&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]
