gabotorresruiz commented on code in PR #45287:
URL: https://github.com/apache/superset/pull/45287#discussion_r4287082147
##########
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:
More a question about the bulk shape than a blocker. `views` is only checked
for non-empty at `superset/semantic_layers/api.py:379`, and each item now pays
its own provider round trip, so the request time scales with the item count. I
couldn't find a timeout on `get_semantic_view` anywhere under
`superset/semantic_layers/`, so a hung provider holds the worker and the
metadata connection for as long as the provider's own socket waits, once per
item. The known limitation note compares this to the discovery endpoint, but
that one is a single call per request.
Also, because the lookup deliberately runs before the uniqueness check,
re-submitting an already added view now pays the round trip too. Both of those
feel like the right trade for the fix, so mostly I'm asking whether `views` is
worth bounding here or whether that belongs in its own change.
##########
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:
Agreed with @aminghadersohi here, and one more data point for it: when
nothing is created the response is 422, `parseResponse` rejects on
`!response.ok`, and `AddSemanticViewModal.tsx:650` falls into the generic `An
error occurred while adding semantic views` toast. So this reason isn't
rendered in either shape today, not only on partial success where the toast
carries just the names. Worth surfacing `result.errors[].error` in the modal as
a follow-up so the new string reaches someone?
##########
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:
Not a blocker. The type does separate a `ConnectionError` from a
provider-side `AttributeError`, which is good, but there is no layer uuid and
no view name on this line, so a burst of these doesn't say which layer is down.
The same provider call on the discovery path
(`superset/semantic_layers/api.py:898`) logs `str(ex)` with `exc_info=True`, so
provider exception text already reaches the logs on the request the Add dialog
makes right before this one. Could we add the identifiers here? Both are
already echoed in the response, so neither is new exposure.
```suggestion
logger.warning(
"Semantic view lookup failed for layer %s, view %s: %s",
layer_uuid,
name,
type(ex).__name__,
)
```
--
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]
