mikebridge commented on code in PR #45287:
URL: https://github.com/apache/superset/pull/45287#discussion_r4287846996
##########
docs/docs/using-superset/semantic-layer-connections.mdx:
##########
@@ -61,3 +61,9 @@ keeps dependent choices unavailable until that source is
filled. Clearing an
optional dependency can load choices for the empty state instead. Semantic
views listed for an earlier dependency are discarded, and views cannot be added
while dependent choices are loading.
+
+When adding a semantic view, Superset asks the provider to look up its name
with
+the submitted configuration before saving it. If lookup fails or returns no
view,
+Superset reports that the view may not exist or the semantic layer may be
+unreachable, and does not add it. Other valid views in the same request can
still
Review Comment:
Updated in d93a8e4c65: the docs now explicitly say “the API reports”. The
modal still uses a generic toast for a 422 and names only on partial success.
I’ve proposed a separate follow-up to surface `result.errors[].error` in both
paths; no UI behavior changes in this PR.
##########
superset/commands/semantic_layer/create.py:
##########
@@ -108,7 +111,36 @@ def validate(self) -> None:
raise SemanticViewForbiddenError()
name: str = self._properties.get("name", "")
- configuration: dict[str, Any] = self._properties.get("configuration")
or {}
+ raw_configuration: dict[str, Any] | str = (
+ self._properties.get("configuration") or {}
+ )
+ try:
+ configuration: Any = (
+ json.loads(raw_configuration)
+ if isinstance(raw_configuration, str)
+ else raw_configuration
+ )
+ except ValueError:
+ configuration = None
+ if not isinstance(configuration, dict):
+ raise SemanticViewInvalidError(
+ _("The view configuration must be a JSON object.")
+ )
+ try:
+ view: SemanticViewABC | None =
layer.implementation.get_semantic_view(
+ name, configuration
+ )
+ except Exception as ex: # pylint: disable=broad-except
+ logger.warning("Semantic view lookup failed: %s",
type(ex).__name__)
Review Comment:
Added the layer UUID and view name in d93a8e4c65, while keeping only the
exception type and no provider traceback. The regression test asserts the exact
warning and retains the checks that provider exception text does not appear in
logs or responses.
##########
superset/commands/semantic_layer/create.py:
##########
@@ -108,7 +111,36 @@ def validate(self) -> None:
raise SemanticViewForbiddenError()
name: str = self._properties.get("name", "")
- configuration: dict[str, Any] = self._properties.get("configuration")
or {}
+ raw_configuration: dict[str, Any] | str = (
+ self._properties.get("configuration") or {}
+ )
+ try:
+ configuration: Any = (
+ json.loads(raw_configuration)
+ if isinstance(raw_configuration, str)
+ else raw_configuration
+ )
+ except ValueError:
+ configuration = None
+ if not isinstance(configuration, dict):
+ raise SemanticViewInvalidError(
+ _("The view configuration must be a JSON object.")
+ )
+ try:
+ view: SemanticViewABC | None =
layer.implementation.get_semantic_view(
Review Comment:
Agreed: this is one lookup per item, so the discovery-endpoint comparison
does not capture the bulk multiplier. Lookup stays before uniqueness to avoid
putting a remote round trip between the uniqueness check and insert; an
already-added view therefore also pays that lookup. This PR accepts that
latency and metadata-connection hold. I’ve proposed a separate follow-up to
bound `views` and define an enforceable provider lookup timeout, including
retry/partial-success behavior. A host-side wait timeout alone would not
necessarily cancel the provider operation or release its resources, so I’m
leaving request handling unchanged here.
##########
docs/docs/using-superset/semantic-layer-connections.mdx:
##########
@@ -61,3 +61,9 @@ keeps dependent choices unavailable until that source is
filled. Clearing an
optional dependency can load choices for the empty state instead. Semantic
views listed for an earlier dependency are discarded, and views cannot be added
while dependent choices are loading.
+
+When adding a semantic view, Superset asks the provider to look up its name
with
+the submitted configuration before saving it. If lookup fails or returns no
view,
+Superset reports that the view may not exist or the semantic layer may be
Review Comment:
Thanks for tracing both paths. The docs are limited to the API response in
d93a8e4c65. I’ve proposed a follow-up for the modal to render
`result.errors[].error` for both all-failed 422 responses and partial-success
responses, with a fallback when the response has no usable per-item error. That
follow-up is a local proposal, not an additional change in this PR.
--
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]
