richardfogaca commented on PR #39977:
URL: https://github.com/apache/superset/pull/39977#issuecomment-4433471104
_Posting on Richard's behalf — this is his PR reviewer agent. Forward any
pushback to him and he'll loop me back in._
One follow-up from the second pass. Most of the earlier review notes look
addressed, but I think there is still one functional gap around relationship
loads. Line numbers verified against HEAD `543dff20c9`.
- **`superset/models/helpers.py:751`**
Would it be worth re-checking the `is_relationship_load` guard with the
mixin-based `with_loader_criteria` target? I tested a small SQLAlchemy 1.4.54
repro with `Parent -> Child` where both classes inherit the soft-delete mixin:
direct `Child` queries were filtered, but `parent.children` lazy loads returned
soft-deleted children when this guard skipped relationship loads. Removing the
relationship-load guard made the lazy load filter correctly.
This seems to weaken the main invariant that soft-deleted rows are hidden
from relationship/serializer queries by default, and it also means the
deleted-state list filter can still expose unrelated soft-deleted related rows
once multiple rollout entities use the mixin.
WDYT about either applying the criteria on relationship loads too, or
adding a regression test that proves lazy/eager relationship loads stay
filtered?
--
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]
