fitzee commented on code in PR #45531:
URL: https://github.com/apache/superset/pull/45531#discussion_r4330816986
##########
superset/mcp_service/semantic_layer/tool/get_compatible_metrics.py:
##########
@@ -283,8 +283,9 @@ async def _resolve_compatible_metrics(
type(exc).__name__,
str(exc),
)
- await ctx.error("Unexpected error: %s: %s" % (type(exc).__name__,
str(exc)))
+ message: str = "Unable to retrieve compatible metrics."
Review Comment:
**non-blocking**: This broad except also catches `AmbiguousDimensionError`
(a `QueryObjectValidationError`, documented as "a host-detected catalog
ambiguity safe to report to an authorized user"). It is raised by
`resolve_dimensions` via `view.columns` (used by `validate_selection_names`
above) and by `view.get_compatible_metrics` -> `_compat_selection`. Before this
change the agent saw the actionable text, e.g. "Semantic dimension 'x' has
ambiguous variants for grain ...". Now it gets `InternalError` / "Unable to
retrieve compatible metrics.", and the REST `/compatible` endpoint still
returns that error as a 400 with the message. Confirmed by running the new test
with `side_effect=AmbiguousDimensionError("ambiguous-dim-msg")`: the message is
dropped. Same applies to `get_compatible_dimensions.py:279`. Suggest handling
it before the broad except:
```python
except AmbiguousDimensionError as exc:
return SemanticLayerError.create(error=str(exc),
error_type="ValidationError")
```
##########
superset/semantic_layers/api.py:
##########
@@ -306,14 +306,14 @@ def structure(self, pk: int) -> Response:
]
except SupersetSecurityException as ex:
return self.response(403, message=ex.message,
errors=[ex.to_dict()])
- except Exception as ex: # pylint: disable=broad-except
- logger.error(
- "Error fetching structure for semantic view %d: %s",
+ except Exception: # pylint: disable=broad-except
Review Comment:
**non-blocking (pre-existing)**: Only `SupersetSecurityException` passes
through here. `OAuth2RedirectError` and `OAuth2Error` are
`SupersetErrorException` subclasses but not security exceptions, so they land
here and become a generic 422 with no `errors` payload. The frontend then
cannot start the OAuth flow, and each occurrence is logged at ERROR with a
traceback. The execution path (`execute_semantic_query`) explicitly re-raises
both. Before this PR the client at least got the error's own message. Now it
gets "Unable to fetch semantic view structure.". Consider a passthrough like
`except SupersetErrorException as ex: return self.response(ex.status,
message=ex.message, errors=[ex.to_dict()])` here and in `runtime_schema` (line
849).
--
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]
