mikebridge commented on code in PR #39859: URL: https://github.com/apache/superset/pull/39859#discussion_r3507635791
########## 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: Removed this scaffolding from the PR — `examples-init.sql`, the `docker-compose-mysql.yml` override, and `seed_junction_load.py` were a local MySQL scale-eval harness for timing the migration, not product code, so the dev-only credentials never ship. Preserved outside the repo for local reuse. ########## 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: Fixed — the idempotency check now builds a per-table diff with `!=` on the shapes instead of set-differencing `.items()` (whose values are unhashable dicts), so a real failure shows a diagnosable per-table diff rather than raising `TypeError`. ########## 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: Fixed — the module docstring now states the schema is built synthetically from the hardcoded `AFFECTED_TABLES` list (via `_build_in_memory_schema`), not from the live ORM models. -- 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]
