codeant-ai-for-open-source[bot] commented on code in PR #40129: URL: https://github.com/apache/superset/pull/40129#discussion_r3510309536
########## tests/unit_tests/migrations/test_add_deleted_at_to_slices.py: ########## @@ -0,0 +1,186 @@ +# 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. +"""Tests for migration ``7c4a8d09ca37_add_deleted_at_to_slices``. + +Runs the migration's ``upgrade()`` and ``downgrade()`` against an +in-memory SQLite engine with a real Alembic ``Operations`` context. +The behaviour being pinned is the operator-facing contract documented +in ``UPDATING.md``: ``downgrade()`` reverses the schema but does not +hard-delete or otherwise mutate rows that were soft-deleted before +the migration was reversed — those rows survive the downgrade and +become visible to any code path that no longer applies the +soft-delete visibility filter. +""" + +from __future__ import annotations + +from datetime import datetime +from importlib import import_module + +import pytest +from alembic.migration import MigrationContext +from alembic.operations import Operations +from sqlalchemy import ( + Column, + create_engine, + insert, + inspect, + Integer, + MetaData, + select, + String, + Table, +) +from sqlalchemy.engine import Engine + +migration = import_module( + "superset.migrations.versions." + "2026-05-08_12-00_7c4a8d09ca37_add_deleted_at_to_slices" +) Review Comment: ✅ **Customized review instruction saved!** **Instruction:** > Do not require explicit type annotations for dynamically imported modules in test-scaffolding or test-only files when the type is already clear from context. **Applied to:** - `**/test/**` - `**/tests/**` - `**/*test*.py` --- 💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* ########## superset/daos/dashboard.py: ########## @@ -276,11 +277,30 @@ def set_dash_metadata( if isinstance(value, dict) ] - current_slices = ( - db.session.query(Slice).filter(Slice.id.in_(slice_ids)).all() - ) - - dashboard.slices = current_slices + # Bypass the soft-delete visibility filter when resolving the + # incoming chart ids: a dashboard's ``position_json`` may still + # reference a chart that is currently soft-deleted, and this + # assignment REBUILDS ``dashboard.slices`` wholesale. With the + # filter active, the hidden member would be silently dropped — + # deleting its ``dashboard_slices`` junction row (breaking the + # documented restore-reattach contract) and writing + # ``uuid: None`` into its position slot via ``uuid_map`` below. + # + # The bypass must be session-scoped and cover the ASSIGNMENT, + # not just the resolution query: assigning to + # ``dashboard.slices`` makes the unit of work diff the new + # collection against the existing one, which it lazy-loads at + # that moment. A filtered baseline load would exclude the + # trashed member, so the diff would treat it as net-new and + # INSERT a ``dashboard_slices`` row that still exists (soft + # delete never removes junction rows) — an IntegrityError on + # the composite primary key on every save of a dashboard + # containing a trashed chart. + with skip_visibility_filter(db.session, Slice): + current_slices = ( + db.session.query(Slice).filter(Slice.id.in_(slice_ids)).all() + ) Review Comment: ✅ **Customized review instruction saved!** **Instruction:** > Do not require explicit type annotations for unambiguous local variables in this DAO file; follow the surrounding file's existing style when the type is clear from context. **Applied to:** - `superset/daos/**` --- 💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)* -- 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]
