EnxDev commented on code in PR #44902:
URL: https://github.com/apache/superset/pull/44902#discussion_r4170002750
##########
superset/semantic_layers/models.py:
##########
@@ -193,9 +193,12 @@ class SemanticLayer(AuditMixinNullable, Model):
perm = Column(String(1000), nullable=True)
# Semantic views relationship
+ # SQLAlchemy 2 needs an explicit collection class for this forward
annotation.
semantic_views: list[SemanticView] = relationship(
"SemanticView",
back_populates="semantic_layer",
+ uselist=True,
+ collection_class=list,
cascade="all, delete-orphan",
passive_deletes=True,
Review Comment:
With the collection mapped properly, `passive_deletes=True` is what still
skips the per-view hooks. `DeleteSemanticLayerCommand` loads the layer through
`find_by_uuid` and never touches `semantic_views`, so it always takes the
unloaded path. I probed it: `semantic_view_after_delete` fires twice when the
views are loaded and zero times when they aren't. Each view's
`[layer].[view](id:N)` `datasource_access` PVM stays behind after a layer
delete.
That's not a regression from this PR, and you called it out. But dropping
`passive_deletes` (or loading the views in the command) would close it now that
the cascade really iterates. Fine as a separate PR if you'd rather keep this
one to the mapping fix.
##########
tests/unit_tests/semantic_layers/models_test.py:
##########
@@ -1223,6 +1226,54 @@ def test_semantic_view_get_compatible_dimensions(
# =============================================================================
+def test_semantic_layer_loads_all_semantic_views(session: Session) -> None:
+ """A reloaded layer exposes every stored view as a collection."""
+ assert inspect(SemanticLayer).relationships.semantic_views.uselist is True
+ SemanticView.metadata.create_all(session.get_bind())
+ layer: SemanticLayer = SemanticLayer(
+ uuid=uuid.uuid4(), name="Orders", type="test", configuration="{}"
+ )
+ views: list[SemanticView] = [
+ SemanticView(name=name, semantic_layer=layer, configuration="{}")
+ for name in ("Daily", "Monthly")
+ ]
+ session.add_all([layer, *views])
+ session.flush()
+ session.expire(layer, ["semantic_views"])
+
+ assert {view.name for view in layer.semantic_views} == {"Daily", "Monthly"}
+
+
[email protected]("load_before_delete", [True, False])
+def test_semantic_layer_delete_removes_multiple_views(
+ session: Session, load_before_delete: bool
+) -> None:
+ """Loaded and unloaded relationships delete all persisted child rows."""
+ engine: Engine = cast(Engine, session.get_bind())
+ connection: Connection
+ with engine.connect() as connection:
Review Comment:
Nit, take it or leave it. I confirmed the unloaded case really depends on
this PRAGMA (without it you get `[1, 2] == []`). It only reaches the session
because `sqlite://` hands back the same pooled connection.
`conftest.py` lets a module override `session_engine`. A `session_engine`
fixture with an `event.listen(engine, "connect", ...)` that turns on foreign
keys would make that explicit and drop the `cast` here.
--
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]
