sadpandajoe commented on code in PR #43527:
URL: https://github.com/apache/superset/pull/43527#discussion_r3867645197


##########
superset/datasource/api.py:
##########
@@ -523,6 +556,255 @@ def compatible(self, datasource_type: str, datasource_id: 
int) -> FlaskResponse:
 
         return self.response(200, result=result)
 
+    def _resolve_for_query(
+        self, datasource_type: str, datasource_id: int, payload: dict[str, Any]
+    ) -> ResolvedExplorable:
+        """Resolve + authorize, translating DAO/security errors to HTTP."""
+        if DatasourceType(
+            datasource_type
+        ) == DatasourceType.SEMANTIC_VIEW and not 
is_feature_enabled("SEMANTIC_LAYERS"):
+            raise _HttpError(404, "Semantic views are not enabled.")
+        try:
+            return resolve_explorable(
+                datasource_type,
+                datasource_id,
+                time_column=payload.get("time_column"),
+                has_time_range=bool(payload.get("time_range")),
+            )
+        except DatasourceTypeNotSupportedError as ex:
+            raise _HttpError(400, ex.message) from ex
+        except DatasourceNotFound as ex:
+            raise _HttpError(404, ex.message) from ex
+        except SupersetSecurityException as ex:
+            raise _HttpError(403, ex.message) from ex
+        except TabularQueryValidationError as ex:
+            raise _HttpError(400, str(ex)) from ex
+
+    @expose("/<datasource_type>/<int:datasource_id>/query", methods=("POST",))
+    @protect()
+    @safe
+    @statsd_metrics
+    @event_logger.log_this_with_context(
+        action=lambda self, *args, **kwargs: 
f"{self.__class__.__name__}.query",
+        log_to_statsd=False,
+    )
+    def query(self, datasource_type: str, datasource_id: int) -> FlaskResponse:
+        """Query a datasource using metric and dimension names.
+        ---
+        post:
+          summary: Query a datasource by its semantic definitions
+          parameters:
+          - in: path
+            schema:
+              type: string
+            name: datasource_type
+          - in: path
+            schema:
+              type: integer
+            name: datasource_id
+          requestBody:
+            required: true
+            content:
+              application/json:
+                schema:
+                  $ref: '#/components/schemas/DatasourceQuerySchema'
+          responses:
+            200:
+              description: Query result
+              content:
+                application/json:
+                  schema:
+                    type: object
+                    properties:
+                      result:
+                        type: array
+                        items:
+                          type: object
+                application/vnd.apache.arrow.stream:
+                  schema:
+                    type: string
+                    format: binary
+            400:
+              $ref: '#/components/responses/400'
+            401:
+              $ref: '#/components/responses/401'
+            403:
+              $ref: '#/components/responses/403'
+            404:
+              $ref: '#/components/responses/404'
+        """
+        # No ValueError guard here: marshmallow raises ValidationError (not a
+        # ValueError subclass), and datasource_type comes from the path, not 
the
+        # body. DatasourceType() is converted in _resolve_for_query below.
+        try:
+            payload = DatasourceQuerySchema().load(request.json or {})
+        except ValidationError as ex:
+            return self.response_400(message=ex.messages)
+
+        try:
+            resolved = self._resolve_for_query(datasource_type, datasource_id, 
payload)
+        except _HttpError as ex:
+            return self.response(ex.status, message=ex.message)
+        except ValueError:
+            return self.response(
+                400, message=f"Invalid datasource type: {datasource_type}"
+            )
+
+        if errors := validate_query_names(
+            resolved.valid_metrics,
+            resolved.valid_dimensions,
+            metrics=payload["metrics"],
+            dimensions=payload["dimensions"],
+            filters=payload["filters"],
+            order_names=[term["column"] for term in payload["order"]],
+        ):
+            return self.response_400(message="; ".join(errors))
+
+        return self._execute_and_respond(resolved, payload)
+
+    def _execute_and_respond(
+        self, resolved: ResolvedExplorable, payload: dict[str, Any]
+    ) -> FlaskResponse:
+        """Run the query and render it in the requested result format."""
+        grain_column = resolved.resolve_grain_column(
+            payload["time_column"], payload["dimensions"]
+        )
+        if payload["time_grain"] and not grain_column:
+            # Silently dropping the grain would return unbucketed rows that 
look
+            # correct, so refuse instead.
+            return self.response_400(
+                message=(
+                    "time_grain requires a temporal column. Set time_column, "
+                    "include a datetime dimension, or provide time_range."
+                )
+            )
+
+        query_dict = build_query_dict(
+            time_column=resolved.time_column,
+            metrics=payload["metrics"],
+            dimensions=payload["dimensions"],
+            filters=payload["filters"],
+            time_range=payload["time_range"],
+            time_grain=payload["time_grain"],
+            grain_column=grain_column,
+            limit=payload["limit"],
+            offset=payload["offset"],
+            order=[(term["column"], term["descending"]) for term in 
payload["order"]],
+            # No wire field for this; follow the leading term's direction.
+            order_desc=(
+                payload["order"][0]["descending"] if payload["order"] else True
+            ),
+        )
+        result_format = payload["result_format"]
+
+        try:
+            result = execute_tabular_query(
+                int(resolved.explorable.id),
+                str(resolved.explorable.type),
+                query_dict,
+                result_format=result_format,
+                use_cache=payload["use_cache"],
+                force=payload["force"],
+                cache_timeout=payload["cache_timeout"],
+            )
+        except SupersetSecurityException as ex:
+            return self.response(403, message=ex.message)
+        except (TabularQueryValidationError, QueryObjectValidationError) as ex:
+            return self.response_400(message=str(ex))
+        except CommandException as ex:
+            return self.response_400(message=ex.message or str(ex))
+
+        queries = result.get("queries") or []
+        if result_format == ChartDataResultFormat.ARROW:
+            if len(queries) != 1:
+                return self.response_400(
+                    message="Arrow result format supports exactly one query."
+                )
+            return Response(
+                queries[0]["data"],
+                mimetype="application/vnd.apache.arrow.stream",
+            )
+        return self.response(200, result=queries)
+
+    @expose("/<datasource_type>/<int:datasource_id>", methods=("GET",))
+    @protect()
+    @safe
+    @statsd_metrics
+    @event_logger.log_this_with_context(
+        action=lambda self, *args, **kwargs: (
+            f"{self.__class__.__name__}.datasource_info"
+        ),
+        log_to_statsd=False,
+    )
+    def datasource_info(
+        self, datasource_type: str, datasource_id: int
+    ) -> FlaskResponse:
+        """Get datasource metadata and query capabilities.
+        ---
+        get:
+          summary: Get datasource metadata and capabilities
+          parameters:
+          - in: path
+            schema:
+              type: string
+            name: datasource_type
+          - in: path
+            schema:
+              type: integer
+            name: datasource_id
+          responses:
+            200:
+              description: Datasource metadata plus a capabilities block
+              content:
+                application/json:
+                  schema:
+                    type: object
+                    properties:
+                      result:
+                        type: object
+            400:
+              $ref: '#/components/responses/400'
+            401:
+              $ref: '#/components/responses/401'
+            403:
+              $ref: '#/components/responses/403'
+            404:
+              $ref: '#/components/responses/404'
+        """
+        try:
+            resolved = self._resolve_for_query(datasource_type, datasource_id, 
{})
+        except _HttpError as ex:
+            return self.response(ex.status, message=ex.message)
+        except ValueError:
+            return self.response(
+                400, message=f"Invalid datasource type: {datasource_type}"
+            )
+
+        datasource = resolved.explorable
+        features = sorted(
+            feature.value
+            for feature in getattr(
+                getattr(datasource, "implementation", None), "features", set()
+            )
+        )
+        result = dict(datasource.data)
+        result["capabilities"] = {
+            "is_rls_supported": datasource.is_rls_supported,
+            "query_language": datasource.query_language,
+            "supports_samples": getattr(datasource, "supports_samples", True),
+            "supports_drill_to_detail": getattr(
+                datasource, "supports_drill_to_detail", True
+            ),
+            # Datasets accept ad-hoc metrics; semantic views reject them in the
+            # mapper, since the provider owns metric definitions.
+            "supports_adhoc_metrics": DatasourceType(datasource_type)
+            != DatasourceType.SEMANTIC_VIEW,
+            "time_grains": datasource.get_time_grains(),
+            "supported_operators": [op.value for op in FilterOperator],

Review Comment:
   This advertises every global filter operator even though semantic views 
reject several of them in the mapper. A metadata-driven client can therefore 
send an advertised operator and receive a server error. Could the capabilities 
list be limited to operators supported by this datasource?



##########
superset/datasource/api.py:
##########
@@ -523,6 +556,255 @@ def compatible(self, datasource_type: str, datasource_id: 
int) -> FlaskResponse:
 
         return self.response(200, result=result)
 
+    def _resolve_for_query(
+        self, datasource_type: str, datasource_id: int, payload: dict[str, Any]
+    ) -> ResolvedExplorable:
+        """Resolve + authorize, translating DAO/security errors to HTTP."""
+        if DatasourceType(
+            datasource_type
+        ) == DatasourceType.SEMANTIC_VIEW and not 
is_feature_enabled("SEMANTIC_LAYERS"):
+            raise _HttpError(404, "Semantic views are not enabled.")
+        try:
+            return resolve_explorable(
+                datasource_type,
+                datasource_id,
+                time_column=payload.get("time_column"),
+                has_time_range=bool(payload.get("time_range")),
+            )
+        except DatasourceTypeNotSupportedError as ex:
+            raise _HttpError(400, ex.message) from ex
+        except DatasourceNotFound as ex:
+            raise _HttpError(404, ex.message) from ex
+        except SupersetSecurityException as ex:
+            raise _HttpError(403, ex.message) from ex
+        except TabularQueryValidationError as ex:
+            raise _HttpError(400, str(ex)) from ex
+
+    @expose("/<datasource_type>/<int:datasource_id>/query", methods=("POST",))

Review Comment:
   The committed OpenAPI snapshot does not include either new datasource 
endpoint or `DatasourceQuerySchema`, so generated clients and the published API 
reference cannot discover this API. Could the generated OpenAPI resource be 
refreshed with this change?



##########
superset/datasource/api.py:
##########
@@ -523,6 +556,255 @@ def compatible(self, datasource_type: str, datasource_id: 
int) -> FlaskResponse:
 
         return self.response(200, result=result)
 
+    def _resolve_for_query(
+        self, datasource_type: str, datasource_id: int, payload: dict[str, Any]
+    ) -> ResolvedExplorable:
+        """Resolve + authorize, translating DAO/security errors to HTTP."""
+        if DatasourceType(
+            datasource_type
+        ) == DatasourceType.SEMANTIC_VIEW and not 
is_feature_enabled("SEMANTIC_LAYERS"):
+            raise _HttpError(404, "Semantic views are not enabled.")
+        try:
+            return resolve_explorable(
+                datasource_type,
+                datasource_id,
+                time_column=payload.get("time_column"),
+                has_time_range=bool(payload.get("time_range")),
+            )
+        except DatasourceTypeNotSupportedError as ex:
+            raise _HttpError(400, ex.message) from ex
+        except DatasourceNotFound as ex:
+            raise _HttpError(404, ex.message) from ex
+        except SupersetSecurityException as ex:
+            raise _HttpError(403, ex.message) from ex
+        except TabularQueryValidationError as ex:
+            raise _HttpError(400, str(ex)) from ex
+
+    @expose("/<datasource_type>/<int:datasource_id>/query", methods=("POST",))
+    @protect()
+    @safe
+    @statsd_metrics
+    @event_logger.log_this_with_context(
+        action=lambda self, *args, **kwargs: 
f"{self.__class__.__name__}.query",
+        log_to_statsd=False,
+    )
+    def query(self, datasource_type: str, datasource_id: int) -> FlaskResponse:
+        """Query a datasource using metric and dimension names.
+        ---
+        post:
+          summary: Query a datasource by its semantic definitions
+          parameters:
+          - in: path
+            schema:
+              type: string
+            name: datasource_type
+          - in: path
+            schema:
+              type: integer
+            name: datasource_id
+          requestBody:
+            required: true
+            content:
+              application/json:
+                schema:
+                  $ref: '#/components/schemas/DatasourceQuerySchema'
+          responses:
+            200:
+              description: Query result
+              content:
+                application/json:
+                  schema:
+                    type: object
+                    properties:
+                      result:
+                        type: array
+                        items:
+                          type: object
+                application/vnd.apache.arrow.stream:
+                  schema:
+                    type: string
+                    format: binary
+            400:
+              $ref: '#/components/responses/400'
+            401:
+              $ref: '#/components/responses/401'
+            403:
+              $ref: '#/components/responses/403'
+            404:
+              $ref: '#/components/responses/404'
+        """
+        # No ValueError guard here: marshmallow raises ValidationError (not a
+        # ValueError subclass), and datasource_type comes from the path, not 
the
+        # body. DatasourceType() is converted in _resolve_for_query below.
+        try:
+            payload = DatasourceQuerySchema().load(request.json or {})
+        except ValidationError as ex:
+            return self.response_400(message=ex.messages)
+
+        try:
+            resolved = self._resolve_for_query(datasource_type, datasource_id, 
payload)
+        except _HttpError as ex:
+            return self.response(ex.status, message=ex.message)
+        except ValueError:
+            return self.response(
+                400, message=f"Invalid datasource type: {datasource_type}"
+            )
+
+        if errors := validate_query_names(
+            resolved.valid_metrics,
+            resolved.valid_dimensions,
+            metrics=payload["metrics"],
+            dimensions=payload["dimensions"],
+            filters=payload["filters"],
+            order_names=[term["column"] for term in payload["order"]],
+        ):
+            return self.response_400(message="; ".join(errors))
+
+        return self._execute_and_respond(resolved, payload)
+
+    def _execute_and_respond(
+        self, resolved: ResolvedExplorable, payload: dict[str, Any]
+    ) -> FlaskResponse:
+        """Run the query and render it in the requested result format."""
+        grain_column = resolved.resolve_grain_column(
+            payload["time_column"], payload["dimensions"]
+        )
+        if payload["time_grain"] and not grain_column:
+            # Silently dropping the grain would return unbucketed rows that 
look
+            # correct, so refuse instead.
+            return self.response_400(
+                message=(
+                    "time_grain requires a temporal column. Set time_column, "
+                    "include a datetime dimension, or provide time_range."
+                )
+            )
+
+        query_dict = build_query_dict(
+            time_column=resolved.time_column,
+            metrics=payload["metrics"],
+            dimensions=payload["dimensions"],
+            filters=payload["filters"],
+            time_range=payload["time_range"],
+            time_grain=payload["time_grain"],
+            grain_column=grain_column,
+            limit=payload["limit"],
+            offset=payload["offset"],
+            order=[(term["column"], term["descending"]) for term in 
payload["order"]],
+            # No wire field for this; follow the leading term's direction.
+            order_desc=(
+                payload["order"][0]["descending"] if payload["order"] else True
+            ),
+        )
+        result_format = payload["result_format"]
+
+        try:
+            result = execute_tabular_query(
+                int(resolved.explorable.id),
+                str(resolved.explorable.type),
+                query_dict,
+                result_format=result_format,
+                use_cache=payload["use_cache"],
+                force=payload["force"],
+                cache_timeout=payload["cache_timeout"],
+            )
+        except SupersetSecurityException as ex:
+            return self.response(403, message=ex.message)
+        except (TabularQueryValidationError, QueryObjectValidationError) as ex:

Review Comment:
   Semantic-view validation raises `ValueError` for request errors such as an 
ad-hoc metric, but this handler does not map that exception. The request then 
escapes through `@safe` as a 500 instead of the documented 400. Could this 
translate those validation errors before they reach the generic handler?



-- 
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]

Reply via email to