mikebridge commented on code in PR #44370:
URL: https://github.com/apache/superset/pull/44370#discussion_r4052568569
##########
superset/common/query_object.py:
##########
@@ -405,6 +406,38 @@ def validate(
) -> QueryObjectValidationError | None:
"""Validate query object"""
try:
+ if self.datasource and self.datasource.type == "semantic_view":
+ try:
+ cast(
+ "SemanticView", self.datasource
+ ).implementation.validate_selection_version(
+ self.extras.get("semantic_selection_version")
+ )
Review Comment:
Confirmed at 9a1ee5f2: resolving implementation inside this catch
misclassifies JSONDecodeError as a stale selection. I will narrow the catch and
add a red-first malformed-config regression. Implementation is paused on the
values-route contract decision in your third thread, per the coordinated
dispatch; no fix is pushed yet. @aminghadersohi I will request another look
once the verified fix is published.
##########
superset/mcp_service/semantic_layer/tool/get_table.py:
##########
@@ -182,17 +186,34 @@ def _resolve_external_view(
error_type="AccessDenied",
)
- display_name = view.name
- valid_columns = {c.column_name for c in view.columns}
- valid_dttm_columns = {c.column_name for c in view.columns if c.is_dttm}
- valid_metrics = {m.metric_name for m in view.metrics}
+ try:
+ view.implementation.validate_selection_version(
+ request.semantic_selection_version
+ )
Review Comment:
Confirmed. Hoisting implementation resolution narrows the version catch, but
the outer MCP error handler also interpolates the exception, so I will cover a
sanitized configuration-error response as well as stale-version
ValidationError. No fix pushed yet while the values-route contract is being
decided. @aminghadersohi I will request re-review with the verified patch.
##########
superset/datasource/api.py:
##########
@@ -751,6 +751,7 @@ def _build_query_dict(
time_range=payload["time_range"],
time_grain=payload["time_grain"],
grain_column=grain_column,
+
semantic_selection_version=payload.get("semantic_selection_version"),
Review Comment:
Confirmed: there is no version gate here, including on a cache hit.
Member-ID lookup alone cannot distinguish an old saved title that equals a
current ID. I recommend requiring semantic_selection_version for versioned
views before cache lookup, with AdhocFilter and Matrixify callers forwarding
saved/live selection provenance (never auto-stamping an old title from current
metadata). The alternative is disabling suggestions for versioned views until
that provenance is available. I have sent this contract choice to the conductor
and paused implementation as dispatched. @aminghadersohi please review this
proposed contract; the final code will be re-requested after verification.
--
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]
