gabotorresruiz commented on code in PR #45287:
URL: https://github.com/apache/superset/pull/45287#discussion_r4288823641
##########
superset/commands/semantic_layer/create.py:
##########
@@ -108,7 +111,41 @@ 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 for layer %s, view %s: %s",
+ layer_uuid,
+ name,
+ type(ex).__name__,
+ )
+ view = None
+ if view is None:
Review Comment:
Not a blocker, and this could ride along with the follow-up you already have
planned. The warning above only covers the raising half: a provider that
handles its own connection error and returns `None` lands here instead, and
neither the command nor the `SemanticViewInvalidError` handler at `api.py:399`
logs anything on that path, so the `422` goes out with no line naming the
layer. Worth a matching `logger.info` here, or do you see the two cases as
different enough that the silence is right?
##########
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:
Confirmed, thanks. On `d93a8e4c65` the line comes out as `Semantic view
lookup failed for layer <uuid>, view orders: ConnectionError`, where
`b4f6d7048e` gave only `Semantic view lookup failed: ConnectionError`. The
exact equality assertion on `caplog.messages` is the right shape too: swapping
`type(ex).__name__` for `ex` fails the test, so the secret free form is pinned
rather than just sampled.
##########
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:
That reasoning holds for me, in particular that a host side wait would not
necessarily cancel the provider's work or release its resources. Happy to take
the `views` bound and an enforceable lookup timeout as their 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:
Reads right to me now. I checked the paragraph against both shapes again and
"the API reports" is accurate: the partial success `201` body carries
`result.errors[].error` per item, and the all failed case returns the same
string in a `422`. `parseResponse.ts:42` still rejects before the modal can
read either, so no UI surface renders it. Good with the modal change being its
own 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: [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]
