mikebridge commented on PR #40130:
URL: https://github.com/apache/superset/pull/40130#issuecomment-4607456313
Follow-up on the CI failure: pushed c4560c1b39 to fix it. The honest read is
that I introduced a test whose premise the DB constraint contradicts.
**What was failing:** `test_restore_blocked_by_active_logical_duplicate` was
hitting `IntegrityError: UNIQUE constraint failed` (SQLite) /
`_customer_location_uc` violation (Postgres) at the `db.session.commit()` that
tried to seed the active twin while the original was soft-deleted.
**Root cause:** the `tables` table carries two DB-level unique constraints
on the logical identity:
1. `UniqueConstraint("database_id", "catalog", "schema", "table_name")`
declared in the `SqlaTable` model.
2. The legacy `_customer_location_uc` on `(database_id, schema, table_name)`
from the 2016 `b4456560d4f3` migration — still present in test DBs (the 2024
`df3d7e2eb9a4` removal migration runs only on a fresh upgrade chain).
Either constraint rejects the twin insert before the application-level
restore check can be exercised. The "two live datasets pointing at the same
physical table" scenario the test was written for is unreachable in practice —
the DB stops step 2 (the create) with an IntegrityError before step 3 (the
restore) ever runs.
**What this means for the original review concerns:**
@richardfogaca's `api.py:963` concern and @aminghadersohi's matching HIGH at
`restore.py:46` were about the *application layer* `validate_uniqueness` being
filtered by the SoftDeleteMixin listener — which they were, before the fix. The
DB-level constraint masks this in production today: the create returns an
opaque 500 IntegrityError instead of a clean 422.
The application-level fixes are kept as defense-in-depth and still pay off —
they convert the 500 to a clean 422:
- `DatasetDAO.validate_uniqueness` / `validate_update_uniqueness` use
`execution_options(SKIP_VISIBILITY_FILTER_CLASSES={SqlaTable})` so the create
path returns 422. Exercised end-to-end by the surviving
`test_create_blocked_by_soft_deleted_logical_duplicate`.
- `RestoreDatasetCommand._has_active_logical_duplicate` is retained for
symmetry — fires cleanly if the DB constraint is ever relaxed (it has been
removed once before via `df3d7e2eb9a4`, so it's not theoretical).
- Unit coverage of the restore-side check is preserved in
`tests/unit_tests/commands/dataset/restore_test.py::test_restore_dataset_logical_duplicate_raises`
(mocked).
The deleted integration test is replaced with a comment block at the same
location explaining why it's unreachable, so the next contributor doesn't
re-introduce it.
Sorry for the noise.
--
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]
