mikebridge commented on code in PR #44902:
URL: https://github.com/apache/superset/pull/44902#discussion_r4198501930
##########
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:
Thanks for probing this. I kept this PR to the mapping fix and closed the
unloaded-delete gap separately in #44905: a `before_delete` hook on the layer
removes its child views' `datasource_access` permissions and role grants
whether or not the views are loaded, so `DeleteSemanticLayerCommand`'s unloaded
path is covered without relying on per-view hooks. It keeps a permission that
another live resource still uses.
##########
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:
Good catch on why it works. Agreed that a module-level `session_engine`
fixture with a `connect` listener would make the foreign-key requirement
explicit. I'll leave the test as is in this PR to keep it to the mapping fix,
and pick that up if we touch these tests again.
--
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]
