mikebridge commented on code in PR #45537:
URL: https://github.com/apache/superset/pull/45537#discussion_r4331625794
##########
superset/semantic_layers/exceptions.py:
##########
@@ -100,6 +100,45 @@ def __init__(self) -> None:
)
+ProviderResult = TypeVar("ProviderResult")
+
+# Host signals that keep their own classification when raised by a provider.
+_HOST_SIGNALS: tuple[type[BaseException], ...] = (
+ QueryObjectValidationError,
+ SemanticLayerQueryRejectedError,
+ SemanticLayerExecutionError,
+ SupersetSecurityException,
+ OAuth2RedirectError,
+ OAuth2Error,
+ SupersetCancelQueryException,
+ SoftTimeLimitExceeded,
+)
+
+
+def discover_semantic_metadata(
+ discovery: Callable[[], ProviderResult],
+) -> ProviderResult:
+ """
+ Classify a provider metadata fault as a backend error.
+
+ Discovery takes no user input, so a provider exception here is never a
+ correctable request; it must not reach generic ``ValueError`` handlers
+ that answer 400, and its text must not reach the response. This helper
+ logs only the exception type: discovery also runs for dashboards and
+ metadata views, whose logs must not carry provider detail.
+ """
+ try:
+ return discovery()
+ except _HOST_SIGNALS:
+ raise
+ except Exception as ex: # pylint: disable=broad-except
+ # Provider exception text may contain credentials or provider URLs.
+ logger.error(
+ "Semantic provider metadata discovery failed: %s",
type(ex).__name__
+ )
+ raise SemanticLayerExecutionError() from ex
Review Comment:
Agreed, this was real. `show_superset_error` in
`superset/views/error_handling.py` logs every `SupersetErrorException` with
`exc_info=True`, so the chained provider exception's message and traceback
ended up in the server log, which is exactly what this helper's own contract
rules out.
Fixed in f021203db91afbd8b66faaf6092509c97636aaac:
`discover_semantic_metadata` now raises `SemanticLayerExecutionError() from
None`, so no provider cause is retained, and the helper still logs only the
exception type. The discovery tests that previously asserted the cause now
assert that there is none and that the provider text does not appear in the
formatted traceback. cbe2b47563deaea274bb47b32db7eeb5b24d1246 adds a missing
test annotation on top.
##########
superset/semantic_layers/models.py:
##########
@@ -606,6 +607,14 @@ def uid(self) -> str:
def type(self) -> str:
return "semantic_view"
+ def _provider_metrics(self) -> set[Metric]:
+ """Read the provider's metrics, classifying faults as backend
errors."""
+ return discover_semantic_metadata(self.implementation.get_metrics)
+
+ def _provider_dimensions(self) -> set[Dimension]:
+ """Read the provider's dimensions, classifying faults as backend
errors."""
+ return discover_semantic_metadata(self.implementation.get_dimensions)
Review Comment:
Agreed, this was real. `SemanticView.implementation` binds the layer and
calls `get_semantic_view`, so evaluating it as the argument ran provider code
before `discover_semantic_metadata` could classify a failure.
Fixed in f021203db91afbd8b66faaf6092509c97636aaac: `_provider_metrics` and
`_provider_dimensions` now pass `lambda: self.implementation.get_metrics()` /
`get_dimensions()`, so binding happens inside the wrapper. The same pattern in
`superset/semantic_layers/mapper.py` (the dimension read used when no metadata
snapshot is available) was fixed the same way. A new parametrized test,
`test_host_metadata_reads_classify_binding_faults`, makes binding fail and
checks that the time grain, drill-by and compatible metric and dimension reads
all report a backend error with no provider detail.
cbe2b47563deaea274bb47b32db7eeb5b24d1246 adds a missing annotation in that test.
--
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]
