rusackas commented on code in PR #42803:
URL: https://github.com/apache/superset/pull/42803#discussion_r3750826866
##########
superset/databases/utils.py:
##########
@@ -72,17 +73,37 @@ def get_table_metadata(database: Any, table: Table) ->
TableMetadataResponse:
:return: Dict table metadata ready for API response
"""
keys = []
- columns = database.get_columns(table)
- primary_key = database.get_pk_constraint(table)
+ table_missing = False
+ try:
+ columns = database.get_columns(table)
+ primary_key = database.get_pk_constraint(table)
+ foreign_keys = get_foreign_keys_metadata(database, table)
+ indexes = get_indexes_metadata(database, table)
+ table_comment = database.get_table_comment(table)
+ except NoSuchTableError:
+ # SQLAlchemy 2.0's sqlite dialect raises NoSuchTableError from
+ # reflection (get_columns/get_pk_constraint/etc.) for a table that
+ # doesn't exist - 1.4's sqlite dialect silently returned empty
+ # results instead, which this API has always relied on to answer
+ # with an empty-but-200 payload for sqlite specifically (other
+ # backends' dialects already raised on missing tables pre-2.0, so
+ # they're unaffected and still surface as the 422 below). Only
+ # sqlite gets the graceful fallback, matching that pre-existing,
+ # dialect-driven difference in behavior between backends.
+ if database.backend != "sqlite":
+ raise
+ table_missing = True
Review Comment:
Fair point, only `get_columns` needs the sqlite fallback since it always
fails first for a missing table. Narrowed the catch so a `NoSuchTableError`
from the fk/index/comment reflection calls propagates normally instead of being
read as a missing table.
##########
tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py:
##########
@@ -40,6 +40,7 @@
from superset.mcp_service.dashboard.tool.generate_dashboard import (
_generate_title_from_charts,
)
+from superset.models.dashboard import Dashboard as _RealDashboard
Review Comment:
Neither Dashboard nor Slice touches encrypted_field_factory at module or
class-definition time, and this file already imports several other
superset.mcp_service/models modules at module scope. CI's unit-tests job
collects and runs this file fine, so I don't think this applies here.
##########
tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py:
##########
@@ -167,6 +168,24 @@ def _setup_generate_dashboard_mocks(
mock_dashboard_cls.return_value = dashboard
mock_find_by_id.return_value = dashboard
+ # `generate_dashboard` builds its re-fetch eager-load options with
+ # `subqueryload(Dashboard.slices).subqueryload(Slice.editors)` etc.
+ # against this same patched `Dashboard` class. SQLAlchemy 2.0 validates
+ # loader-path arguments eagerly and raises `ArgumentError` ("Wildcard
+ # token cannot be followed by another entity") when given a plain
+ # MagicMock attribute instead of a real `InstrumentedAttribute` --
+ # SQLAlchemy 1.4 didn't validate this eagerly, so the same mock chain
+ # silently worked before. Copy over the real class-level relationship
+ # attributes (captured at module import time, before `Dashboard` gets
+ # patched, since `from ... import Dashboard` done here would just
+ # return the mock itself) so `subqueryload`/`joinedload` construction
+ # sees genuine mapped attributes while `Dashboard(...)` instantiation
+ # (used to create new dashboards) still returns the mocked `dashboard`
+ # object.
+ mock_dashboard_cls.slices = _RealDashboard.slices
+ mock_dashboard_cls.editors = _RealDashboard.editors
+ mock_dashboard_cls.tags = _RealDashboard.tags
+
# Prevent Subject DB queries during dashboard creation.
Review Comment:
That patch().start() call is pre-existing on master, not something this PR
touches, so it's out of scope 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]
