You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "mikebridge (via GitHub)" <gi...@apache.org> on 2026/10/20 19:38:56 UTC
[PR] fix(semantic-layers): report provider discovery faults as backend errors [superset]
mikebridge opened a new pull request, #45537:
URL: https://github.com/apache/superset/pull/45537
### SUMMARY
When a semantic layer provider fails while listing its dimensions or metrics, Superset should report a server error. It should not tell the user their request was wrong, and it should not show the provider's internal error text. This PR fixes both. It also makes the semantic query mapper say exactly which of its errors are user mistakes.
Before this change:
- If the provider failed in `get_dimensions()` while a chart-data request was being parsed, the API's generic `ValueError` handler returned **400 "Request is incorrect"** with the provider's exception text in the body.
- If it failed in `get_metrics()`, the request returned 500. The generic error handler still put the provider's exception text in the JSON body.
- The mapper used built-in `ValueError` for real input refusals (unknown columns, bad filter values, bad time ranges). It caught every `ValueError` in `get_results` / `validate_query_object` and turned it into a 400, so a bug that happened to raise `ValueError` was reported as a user error.
Changes:
- `discover_semantic_metadata()` (next to `execute_semantic_query()` in `superset/semantic_layers/exceptions.py`) wraps provider metadata reads on the chart-data path. Host signals (validation, security, OAuth, cancellation, existing semantic errors) pass through unchanged. Any other provider exception is logged and becomes `SemanticLayerExecutionError`: a 500 with the generic "An error occurred while fetching the data." message. The two helpers now share one list of passthrough exceptions.
- Every `SemanticView` read of provider metadata goes through it (`metrics`, `columns`, `data`, time grains, the drill-by access check, compatibility lookups). So do the mapper's per-query metadata snapshot and the semantic filter validation in `QueryObject.validate`. The wrapper logs only the exception type, which matches the existing dashboard datasets policy, because provider messages can contain credentials or URLs.
- The mapper raises `SemanticQueryInputError` for input refusals. It subclasses `ValueError`, so existing direct callers that catch `ValueError` (MCP chart helpers, `QueryObject.validate`) behave as before. A small adapter turns `ValueError`s from the date/time-range parsers (for example, "From date cannot be larger than to date") into this type. `get_results` and `validate_query_object` now convert only `SemanticQueryInputError` to `QueryObjectValidationError` (400). Any other exception propagates as a 500.
User-facing 400 messages for real input mistakes are unchanged.
Not changed here: the semantic layer REST endpoints and the MCP semantic view tool read provider metadata under their own error handling. Provider construction and the compatibility endpoints' `get_compatible_*` calls also stay outside the classifier. Over HTTP, Superset's standard `SupersetErrorException` handler still logs the chained exception, the same as for query execution faults.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (API behavior)
| Provider/mapper failure | Before | After |
|---|---|---|
| `get_dimensions()` raises during chart data | 400, provider text in body | 500, generic message |
| `get_metrics()` raises during chart data | 500, provider text in body | 500, generic message |
| Unexpected `ValueError` inside the mapper | 400 | 500 |
| Unknown column / bad filter value / reversed time range | 400 | 400 (same message) |
### TESTING INSTRUCTIONS
```bash
pytest tests/unit_tests/semantic_layers/query_errors_test.py
pytest tests/unit_tests/semantic_layers
```
The new tests in `query_errors_test.py` cover:
- both discovery methods, with and without filters, over HTTP: status, generic message, provider text not exposed, one error log entry, and no query executed;
- each `SemanticView` metadata read, plus the dashboard datasets path, with a real provider fault: no provider text in the log;
- each date/time parser adapter;
- an injected `ValueError` during member lookup and during mapping: HTTP 500, and the original exception from `get_results`;
- invalid selections and time ranges: still 400, typed;
- the helper contract: `SemanticQueryInputError` is a `ValueError`.
### ADDITIONAL INFORMATION
Follow-up to #45292.
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(semantic-layers): report provider discovery faults as backend errors [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi merged PR #45537:
URL: https://github.com/apache/superset/pull/45537
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(semantic-layers): report provider discovery faults as backend errors [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45537:
URL: https://github.com/apache/superset/pull/45537#discussion_r4331625936
##########
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:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `cbe2b47`.
Metadata discovery errors are raised as `SemanticLayerExecutionError() from None`, suppressing the provider exception cause and traceback.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
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:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `cbe2b47`.
The provider access is inside a lambda passed to `discover_semantic_metadata`, so resolving `self.implementation` and calling `get_dimensions()` occur within the wrapper’s error-classification boundary.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(semantic-layers): report provider discovery faults as backend errors [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45537:
URL: https://github.com/apache/superset/pull/45537#discussion_r4380747196
##########
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:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `b3df014`.
The provider implementation is now passed as a lambda to the discovery wrapper, deferring `self.implementation` evaluation until the wrapper invokes it.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(semantic-layers): report provider discovery faults as backend errors [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45537:
URL: https://github.com/apache/superset/pull/45537#discussion_r4361469418
##########
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:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `f9152f7`.
The implementation lookup is now deferred inside `lambda: self.implementation`, allowing the discovery wrapper to classify errors raised while binding the provider.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(semantic-layers): report provider discovery faults as backend errors [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45537:
URL: https://github.com/apache/superset/pull/45537#discussion_r4331464832
##########
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:
**Suggestion:** Python evaluates `self.implementation` before calling the wrapper, so provider binding or `get_semantic_view` failures bypass classification and can still surface as raw errors on metadata reads.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Rarely` · 🏷️ `Incomplete implementation`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=29cc4fb0d6fe4b67b79677bb6b4546a6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=29cc4fb0d6fe4b67b79677bb6b4546a6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/semantic_layers/models.py
**Line:** 612:616
**Comment:**
*Incomplete Implementation: Python evaluates `self.implementation` before calling the wrapper, so provider binding or `get_semantic_view` failures bypass classification and can still surface as raw errors on metadata reads.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45537&comment_hash=26b886c1b4d43521b6366b7a97ae879bab5c9939c2e7441fbc5caa58ae742ee7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45537&comment_hash=26b886c1b4d43521b6366b7a97ae879bab5c9939c2e7441fbc5caa58ae742ee7&reaction=dislike'>👎</a>
##########
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:
**Suggestion:** The chained cause retains the provider traceback; when Superset logs this `SupersetErrorException` with `exc_info=True`, credentials or provider URLs reappear in server logs.
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Rarely` · 🏷️ `Security`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d95610185db04e80bf745175b5133f5c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d95610185db04e80bf745175b5133f5c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/semantic_layers/exceptions.py
**Line:** 139:139
**Comment:**
*Security: The chained cause retains the provider traceback; when Superset logs this `SupersetErrorException` with `exc_info=True`, credentials or provider URLs reappear in server logs.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45537&comment_hash=9841f057687932447f0d9e86077d8526722f567ac7ed65a79b0dc4cdf2512635&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F45537&comment_hash=9841f057687932447f0d9e86077d8526722f567ac7ed65a79b0dc4cdf2512635&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(semantic-layers): report provider discovery faults as backend errors [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45537:
URL: https://github.com/apache/superset/pull/45537#discussion_r4339484692
##########
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:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `cee77a3`.
The implementation lookup is now deferred inside `lambda: self.implementation`, allowing the discovery wrapper to classify errors raised while binding the provider.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(semantic-layers): report provider discovery faults as backend errors [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #45537:
URL: https://github.com/apache/superset/pull/45537#issuecomment-6302338114
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | e8c25f29ecceb01995b6bc28a9e2560c55240188 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6ad7e0de73d84326fd63bf9e |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-45537--superset-docs-preview.netlify.app](https://deploy-preview-45537--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpBCoAgEADAv-w5odS0-kHPUHelwFK0Aon-nrc5zAspxyNdsAB0YLM53dZcHBu4lnJkuBcXH8qVeXOHq7SFlEKsK7anDGrqkbTASQquPCph_Uzw_Q) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-45537--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(semantic-layers): report provider discovery faults as backend errors [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #45537:
URL: https://github.com/apache/superset/pull/45537#issuecomment-6304223022
## CodeAnt PR Risk: Medium Risk
- The PR needs attention before merging.
- Chaining provider exceptions preserves their tracebacks, which can expose sensitive provider details if downstream error handling logs them.
- Evaluating self.implementation.get_metrics or get_dimensions happens before the wrapper runs, so failures during provider resolution bypass classification.
**Assessed commit:** `2dc6c73354ec`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(semantic-layers): report provider discovery faults as backend errors [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #45537:
URL: https://github.com/apache/superset/pull/45537#discussion_r4365123960
##########
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:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `0705ea0`.
The provider implementation is now passed as a lambda to the discovery wrapper, deferring `self.implementation` evaluation until the wrapper invokes it.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(semantic-layers): report provider discovery faults as backend errors [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on PR #45537:
URL: https://github.com/apache/superset/pull/45537#issuecomment-6321453932
@fitzee thanks, both logging points were right. Fixed in cee77a383a6ae4f3bd4aef6bcfc0e34b1ab1f6d1. Status codes and response text haven't changed: every case below still returns the same generic 500, with no chained cause.
**1. The discovery log now identifies the view.** Each fault is logged as one record with `operation`, `view_id`, `layer_uuid` and the real exception type, followed by the file, line and function of the calling frames and of the fault's own frames. So a dashboard failure now names the view and the `ValueError` on the same line, instead of `error=SemanticLayerExecutionError` with the real type on a separate line. For example:
```
Semantic provider metadata discovery failed: operation=get_metrics view_id=7 layer_uuid=8765…8765 error=ValueError
Called from:
File ".../superset/semantic_layers/models.py", line 713, in data
...
Traceback (exception text and source omitted):
File ".../provider/views.py", line 42, in get_metrics
```
I left out source lines as well as the message. `format_tb` prints each frame's source line, and a source line can hold a string literal, so the record keeps only frame locations. `test_dashboard_datasets_provider_discovery_fault_logs_no_detail` now asserts that the operation, view id, layer uuid and type are all on the same record.
**2. Superset faults are logged separately from provider faults.** I kept binding inside the wrapper, so provider construction faults stay classified. The calls that actually enter provider code (`from_configuration`, `get_semantic_view`, `get_metrics`, `get_dimensions`) are now marked, and only exceptions raised from those are logged as provider faults with frames only. Anything else in that path is logged as `Semantic view metadata discovery failed in Superset, not the provider: …` with its full traceback. That covers the metadata-DB lazy load, the registry lookup, `json.loads` of the stored configuration, and the generation capture. The traceback is built without the exception chain, so a provider exception that Superset code was handling when it failed is still not printed.
There are tests for a corrupt stored configuration (`JSONDecodeError`), an unregistered layer type (`KeyError`), each provider binding stage, and a Superset fault raised while handling a provider exception. If a provider exception refuses the marker attribute, it propagates unchanged.
**Consistency with #45531.** With (1) in place, the MCP handlers there still get the generic error from these reads. The server log now has the view, layer, operation, type and stack, but still not the provider's message. That is the remaining difference from "detailed diagnostics remain in server logs", and query execution (`execute_semantic_query`) still logs the full provider exception. I'd rather settle which of those two policies wins in one place than drift in both PRs. If you're happy with frames-without-message for discovery, I'll word #45531 to match.
I also corrected the PR description. The MCP and structure-endpoint reads you pointed out don't go through these helpers. The description now says so instead of claiming they run "under their own error handling" as if they were unaffected.
Your test findings (the four surviving mutants) and the nits aren't in this push. I'll pick them up separately.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(semantic-layers): report provider discovery faults as backend errors [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #45537:
URL: https://github.com/apache/superset/pull/45537#issuecomment-6300279324
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/45537?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `22.05882%` with `53 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 64.90%. Comparing base ([`293f159`](https://app.codecov.io/gh/apache/superset/commit/293f159deaf63c20d08ada41af77c2925b89a0a0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`7e1d2e3`](https://app.codecov.io/gh/apache/superset/commit/7e1d2e3650259eec69ad3a122f392820ff479594?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/45537?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/semantic\_layers/mapper.py](https://app.codecov.io/gh/apache/superset/pull/45537?src=pr&el=tree&filepath=superset%2Fsemantic_layers%2Fmapper.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvc2VtYW50aWNfbGF5ZXJzL21hcHBlci5weQ==) | 12.50% | [42 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/45537?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/semantic\_layers/exceptions.py](https://app.codecov.io/gh/apache/superset/pull/45537?src=pr&el=tree&filepath=superset%2Fsemantic_layers%2Fexceptions.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvc2VtYW50aWNfbGF5ZXJzL2V4Y2VwdGlvbnMucHk=) | 50.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/45537?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/semantic\_layers/models.py](https://app.codecov.io/gh/apache/superset/pull/45537?src=pr&el=tree&filepath=superset%2Fsemantic_layers%2Fmodels.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvc2VtYW50aWNfbGF5ZXJzL21vZGVscy5weQ==) | 42.85% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/45537?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/common/query\_object.py](https://app.codecov.io/gh/apache/superset/pull/45537?src=pr&el=tree&filepath=superset%2Fcommon%2Fquery_object.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbW9uL3F1ZXJ5X29iamVjdC5weQ==) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/45537?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #45537 +/- ##
==========================================
+ Coverage 56.19% 64.90% +8.71%
==========================================
Files 3053 3053
Lines 200807 200830 +23
Branches 46867 46867
==========================================
+ Hits 112842 130356 +17514
+ Misses 86880 67522 -19358
- Partials 1085 2952 +1867
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/45537/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/45537/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.90% <17.64%> (+<0.01%)` | :arrow_up: |
| [presto](https://app.codecov.io/gh/apache/superset/pull/45537/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `36.65% <17.64%> (+<0.01%)` | :arrow_up: |
| [python](https://app.codecov.io/gh/apache/superset/pull/45537/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `52.73% <22.05%> (+16.03%)` | :arrow_up: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/45537/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `52.61% <22.05%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/45537?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(semantic-layers): report provider discovery faults as backend errors [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(semantic-layers): report provider discovery faults as backend errors [superset]
Posted by "mikebridge (via GitHub)" <gi...@apache.org>.
mikebridge commented on PR #45537:
URL: https://github.com/apache/superset/pull/45537#issuecomment-6373797825
_AI-assisted reply on Mike's behalf._
@fitzee: good catch. f9152f77ee logs `requested_operation=`, so the metadata request isn't mistaken for the stage that failed. The binding-fault regressions check that neither metadata method ran, and the full error-handling test file passes (97 tests).
I've also corrected the description:
- The MCP and `get_compatible_*` scope is now listed path by path.
- The mapper bullet says exactly which `ValueError`s become 400s.
- The testing section no longer claims every metadata read is covered.
#45531's summary now matches this PR's logging policy: frame locations without provider messages.
Still deferred, not claimed here:
- test gaps for the standalone `metrics` property and the `QueryObject.validate` dimension wrapper;
- the unreachable fallback and validation mutation sites.
The 5xx classification and the layer/view diagnostic context are unchanged. Query-execution logging is outside this follow-up.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org