You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "diegomedina248 (via GitHub)" <gi...@apache.org> on 2023/01/30 22:44:24 UTC
[GitHub] [superset] diegomedina248 opened a new pull request, #22913: chore: Migrate /superset/csv/ to API v1
diegomedina248 opened a new pull request, #22913:
URL: https://github.com/apache/superset/pull/22913
### SUMMARY
Continuing the effort on deprecating all /superset/ REST API endpoints
Deprecates `/superset/csv` for `/api/v1/sqllab/export/`
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] 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
[GitHub] [superset] diegomedina248 commented on a diff in pull request #22913: chore: Migrate /superset/csv/ to API v1
Posted by "diegomedina248 (via GitHub)" <gi...@apache.org>.
diegomedina248 commented on code in PR #22913:
URL: https://github.com/apache/superset/pull/22913#discussion_r1098149986
##########
tests/integration_tests/sql_lab/commands_tests.py:
##########
@@ -15,23 +15,259 @@
# specific language governing permissions and limitations
# under the License.
from unittest import mock, skip
-from unittest.mock import patch
+from unittest.mock import Mock, patch
+import pandas as pd
import pytest
from superset import db, sql_lab
from superset.common.db_query_status import QueryStatus
-from superset.errors import SupersetErrorType
-from superset.exceptions import SerializationError, SupersetErrorException
+from superset.errors import ErrorLevel, SupersetErrorType
+from superset.exceptions import (
+ SerializationError,
+ SupersetError,
+ SupersetErrorException,
+ SupersetSecurityException,
+)
from superset.models.core import Database
from superset.models.sql_lab import Query
-from superset.sqllab.commands import results
+from superset.sqllab.commands import export, results
+from superset.sqllab.limiting_factor import LimitingFactor
from superset.utils import core as utils
from tests.integration_tests.base_tests import SupersetTestCase
+class TestSqlResultExportCommand(SupersetTestCase):
+ def test_validation_query_not_found(self) -> None:
+ command = export.SqlResultExportCommand("asdf")
+
+ database = Database(database_name="my_database", sqlalchemy_uri="sqlite://")
+ query_obj = Query(
+ client_id="test1",
+ database=database,
+ tab_name="test_tab",
+ sql_editor_id="test_editor_id",
+ sql="select * from bar",
+ select_sql="select * from bar",
+ executed_sql="select * from bar",
+ limit=100,
+ select_as_cta=False,
+ rows=104,
+ error_message="none",
+ results_key="abc1",
+ )
+
+ db.session.add(database)
+ db.session.add(query_obj)
+ db.session.commit()
Review Comment:
seems like the fixtures (or if done directly, same effect), are not being cleared properly.
Do I need to do anything else here?
--
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
[GitHub] [superset] diegomedina248 commented on a diff in pull request #22913: chore: Migrate /superset/csv/ to API v1
Posted by "diegomedina248 (via GitHub)" <gi...@apache.org>.
diegomedina248 commented on code in PR #22913:
URL: https://github.com/apache/superset/pull/22913#discussion_r1098130602
##########
superset/sqllab/api.py:
##########
@@ -72,13 +75,81 @@ class SqlLabRestApi(BaseSupersetApi):
apispec_parameter_schemas = {
"sql_lab_get_results_schema": sql_lab_get_results_schema,
+ "sql_lab_export_csv_schema": sql_lab_export_csv_schema,
}
openapi_spec_tag = "SQL Lab"
openapi_spec_component_schemas = (
ExecutePayloadSchema,
QueryExecutionResponseSchema,
)
+ @expose("/export/")
+ @protect()
+ @statsd_metrics
+ @rison(sql_lab_export_csv_schema)
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs: f"{self.__class__.__name__}"
+ f".export_csv",
+ log_to_statsd=False,
+ )
+ def export_csv(self, **kwargs: Any) -> CsvResponse:
+ """Exports the SQL Query results to a CSV
+ ---
+ get:
+ summary: >-
+ Exports the SQL Query results to a CSV
+ parameters:
+ - in: query
+ name: q
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/sql_lab_export_csv_schema'
+ responses:
+ 200:
+ description: SQL query results
+ content:
+ text/csv:
+ schema:
+ type: string
+ 400:
+ $ref: '#/components/responses/400'
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ $ref: '#/components/responses/403'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ params = kwargs["rison"]
+ client_id = params.get("client_id")
+ result = SqlResultExportCommand(client_id=client_id).run()
+
+ query = result.get("query")
+ data = result.get("data")
+ row_count = result.get("row_count")
Review Comment:
yeah, nice one!
The type must be Any here I think, since it's dynamic depending on the query
--
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
[GitHub] [superset] dpgaspar commented on a diff in pull request #22913: chore: Migrate /superset/csv/ to API v1
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar commented on code in PR #22913:
URL: https://github.com/apache/superset/pull/22913#discussion_r1093088467
##########
superset/sqllab/api.py:
##########
@@ -72,13 +75,81 @@ class SqlLabRestApi(BaseSupersetApi):
apispec_parameter_schemas = {
"sql_lab_get_results_schema": sql_lab_get_results_schema,
+ "sql_lab_export_csv_schema": sql_lab_export_csv_schema,
}
openapi_spec_tag = "SQL Lab"
openapi_spec_component_schemas = (
ExecutePayloadSchema,
QueryExecutionResponseSchema,
)
+ @expose("/export/")
Review Comment:
why not: `@expose("/export/<int:client_id>")` ?
##########
superset/sqllab/api.py:
##########
@@ -72,13 +75,81 @@ class SqlLabRestApi(BaseSupersetApi):
apispec_parameter_schemas = {
"sql_lab_get_results_schema": sql_lab_get_results_schema,
+ "sql_lab_export_csv_schema": sql_lab_export_csv_schema,
}
openapi_spec_tag = "SQL Lab"
openapi_spec_component_schemas = (
ExecutePayloadSchema,
QueryExecutionResponseSchema,
)
+ @expose("/export/")
+ @protect()
+ @statsd_metrics
+ @rison(sql_lab_export_csv_schema)
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs: f"{self.__class__.__name__}"
+ f".export_csv",
+ log_to_statsd=False,
+ )
+ def export_csv(self, **kwargs: Any) -> CsvResponse:
+ """Exports the SQL Query results to a CSV
+ ---
+ get:
+ summary: >-
+ Exports the SQL Query results to a CSV
+ parameters:
+ - in: query
+ name: q
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/sql_lab_export_csv_schema'
+ responses:
+ 200:
+ description: SQL query results
+ content:
+ text/csv:
+ schema:
+ type: string
+ 400:
+ $ref: '#/components/responses/400'
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ $ref: '#/components/responses/403'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ params = kwargs["rison"]
+ client_id = params.get("client_id")
+ result = SqlResultExportCommand(client_id=client_id).run()
+
+ query = result.get("query")
+ data = result.get("data")
+ row_count = result.get("row_count")
Review Comment:
isn't it: `result.get("count")` ?
always nice to define this Dict struct with `TypedDict`:
``` python
class SqlExportResult(TypedDict):
query: Query
count: int
data: List[Any] <- Check this One!
```
##########
superset/sqllab/commands/export.py:
##########
@@ -0,0 +1,130 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# pylint: disable=too-few-public-methods, too-many-arguments
+from __future__ import annotations
+
+import logging
+from typing import Any, cast, Dict
+
+import pandas as pd
+from flask_babel import gettext as __, lazy_gettext as _
+
+from superset import app, db, results_backend, results_backend_use_msgpack
+from superset.commands.base import BaseCommand
+from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+from superset.exceptions import SupersetErrorException, SupersetSecurityException
+from superset.models.sql_lab import Query
+from superset.sql_parse import ParsedQuery
+from superset.sqllab.limiting_factor import LimitingFactor
+from superset.utils import core as utils, csv
+from superset.utils.dates import now_as_float
+from superset.views.utils import _deserialize_results_payload
+
+config = app.config
+
+logger = logging.getLogger(__name__)
+
+
+class SqlResultExportCommand(BaseCommand):
+ _client_id: str
+ _query: Query
+
+ def __init__(
+ self,
+ client_id: str,
+ ) -> None:
+ self._client_id = client_id
+
+ def validate(self) -> None:
+ self._query = (
+ db.session.query(Query).filter_by(client_id=self._client_id).one_or_none()
+ )
+ if self._query is None:
+ raise SupersetErrorException(
+ SupersetError(
+ message=__(
+ "The query associated with these results could not be found. "
+ "You need to re-run the original query."
+ ),
+ error_type=SupersetErrorType.RESULTS_BACKEND_ERROR,
+ level=ErrorLevel.ERROR,
+ ),
+ status=404,
+ )
Review Comment:
nice! this path did not exist earlier
##########
superset/sqllab/commands/export.py:
##########
@@ -0,0 +1,130 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# pylint: disable=too-few-public-methods, too-many-arguments
+from __future__ import annotations
+
+import logging
+from typing import Any, cast, Dict
+
+import pandas as pd
+from flask_babel import gettext as __, lazy_gettext as _
+
+from superset import app, db, results_backend, results_backend_use_msgpack
+from superset.commands.base import BaseCommand
+from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+from superset.exceptions import SupersetErrorException, SupersetSecurityException
+from superset.models.sql_lab import Query
+from superset.sql_parse import ParsedQuery
+from superset.sqllab.limiting_factor import LimitingFactor
+from superset.utils import core as utils, csv
+from superset.utils.dates import now_as_float
+from superset.views.utils import _deserialize_results_payload
+
+config = app.config
+
+logger = logging.getLogger(__name__)
+
+
+class SqlResultExportCommand(BaseCommand):
+ _client_id: str
+ _query: Query
+
+ def __init__(
+ self,
+ client_id: str,
+ ) -> None:
+ self._client_id = client_id
+
+ def validate(self) -> None:
+ self._query = (
+ db.session.query(Query).filter_by(client_id=self._client_id).one_or_none()
+ )
+ if self._query is None:
+ raise SupersetErrorException(
+ SupersetError(
+ message=__(
+ "The query associated with these results could not be found. "
+ "You need to re-run the original query."
+ ),
+ error_type=SupersetErrorType.RESULTS_BACKEND_ERROR,
+ level=ErrorLevel.ERROR,
+ ),
+ status=404,
+ )
+
+ try:
+ self._query.raise_for_access()
+ except SupersetSecurityException:
Review Comment:
why not just let `SupersetSecurityException` raise here?
https://github.com/apache/superset/blob/master/superset/exceptions.py#L165
##########
tests/integration_tests/sql_lab/commands_tests.py:
##########
@@ -15,23 +15,259 @@
# specific language governing permissions and limitations
# under the License.
from unittest import mock, skip
-from unittest.mock import patch
+from unittest.mock import Mock, patch
+import pandas as pd
import pytest
from superset import db, sql_lab
from superset.common.db_query_status import QueryStatus
-from superset.errors import SupersetErrorType
-from superset.exceptions import SerializationError, SupersetErrorException
+from superset.errors import ErrorLevel, SupersetErrorType
+from superset.exceptions import (
+ SerializationError,
+ SupersetError,
+ SupersetErrorException,
+ SupersetSecurityException,
+)
from superset.models.core import Database
from superset.models.sql_lab import Query
-from superset.sqllab.commands import results
+from superset.sqllab.commands import export, results
+from superset.sqllab.limiting_factor import LimitingFactor
from superset.utils import core as utils
from tests.integration_tests.base_tests import SupersetTestCase
+class TestSqlResultExportCommand(SupersetTestCase):
+ def test_validation_query_not_found(self) -> None:
+ command = export.SqlResultExportCommand("asdf")
+
+ database = Database(database_name="my_database", sqlalchemy_uri="sqlite://")
+ query_obj = Query(
+ client_id="test1",
+ database=database,
+ tab_name="test_tab",
+ sql_editor_id="test_editor_id",
+ sql="select * from bar",
+ select_sql="select * from bar",
+ executed_sql="select * from bar",
+ limit=100,
+ select_as_cta=False,
+ rows=104,
+ error_message="none",
+ results_key="abc1",
+ )
+
+ db.session.add(database)
+ db.session.add(query_obj)
+ db.session.commit()
+
+ with pytest.raises(SupersetErrorException) as ex_info:
+ command.run()
+ assert ex_info.value.error.error_type == SupersetErrorType.RESULTS_BACKEND_ERROR
+
+ def test_validation_invalid_access(self) -> None:
+ command = export.SqlResultExportCommand("test2")
+
+ database = Database(database_name="my_database2", sqlalchemy_uri="sqlite://")
+ query_obj = Query(
+ client_id="test2",
+ database=database,
+ tab_name="test_tab",
+ sql_editor_id="test_editor_id",
+ sql="select * from bar",
+ select_sql="select * from bar",
+ executed_sql="select * from bar",
+ limit=100,
+ select_as_cta=False,
+ rows=104,
+ error_message="none",
+ results_key="abc2",
+ )
+
+ db.session.add(database)
+ db.session.add(query_obj)
+ db.session.commit()
+
+ with mock.patch(
+ "superset.security_manager.raise_for_access",
+ side_effect=SupersetSecurityException(
+ SupersetError(
+ "dummy",
+ SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR,
+ ErrorLevel.ERROR,
+ )
+ ),
+ ):
+ with pytest.raises(SupersetErrorException) as ex_info:
+ command.run()
+ assert (
+ ex_info.value.error.error_type
+ == SupersetErrorType.QUERY_SECURITY_ACCESS_ERROR
+ )
+
+ @patch("superset.models.sql_lab.Query.raise_for_access", lambda _: None)
+ @patch("superset.models.core.Database.get_df")
+ def test_run_no_results_backend_select_sql(self, get_df_mock: Mock) -> None:
+ command = export.SqlResultExportCommand("test3")
+
+ database = Database(database_name="my_database3", sqlalchemy_uri="sqlite://")
+ query_obj = Query(
+ client_id="test3",
+ database=database,
+ tab_name="test_tab",
+ sql_editor_id="test_editor_id",
+ sql="select * from bar",
+ select_sql="select * from bar",
+ executed_sql="select * from bar",
+ limit=100,
+ select_as_cta=False,
+ rows=104,
+ error_message="none",
+ results_key="abc3",
+ )
+
+ db.session.add(database)
+ db.session.add(query_obj)
+ db.session.commit()
+
+ get_df_mock.return_value = pd.DataFrame({"foo": [1, 2, 3]})
+
+ result = command.run()
+
+ data = result.get("data")
+ count = result.get("count")
+ query = result.get("query")
+
+ assert data == "foo\n1\n2\n3\n"
+ assert count == 3
+ assert query.client_id == "test3"
+
+ @patch("superset.models.sql_lab.Query.raise_for_access", lambda _: None)
+ @patch("superset.models.core.Database.get_df")
+ def test_run_no_results_backend_executed_sql(self, get_df_mock: Mock) -> None:
+ command = export.SqlResultExportCommand("test4")
+
+ database = Database(database_name="my_database4", sqlalchemy_uri="sqlite://")
+ query_obj = Query(
+ client_id="test4",
+ database=database,
+ tab_name="test_tab",
+ sql_editor_id="test_editor_id",
+ sql="select * from bar",
+ select_sql=None,
+ executed_sql="select * from bar limit 2",
+ limit=100,
+ select_as_cta=False,
+ rows=104,
+ error_message="none",
+ results_key="abc4",
+ )
+
+ db.session.add(database)
+ db.session.add(query_obj)
+ db.session.commit()
+
+ get_df_mock.return_value = pd.DataFrame({"foo": [1, 2, 3]})
+
+ result = command.run()
+
+ data = result.get("data")
+ count = result.get("count")
+ query = result.get("query")
+
+ assert data == "foo\n1\n2\n"
+ assert count == 2
+ assert query.client_id == "test4"
Review Comment:
we probably need to delete `database` and `query_obj` here
##########
tests/integration_tests/sql_lab/commands_tests.py:
##########
@@ -15,23 +15,259 @@
# specific language governing permissions and limitations
# under the License.
from unittest import mock, skip
-from unittest.mock import patch
+from unittest.mock import Mock, patch
+import pandas as pd
import pytest
from superset import db, sql_lab
from superset.common.db_query_status import QueryStatus
-from superset.errors import SupersetErrorType
-from superset.exceptions import SerializationError, SupersetErrorException
+from superset.errors import ErrorLevel, SupersetErrorType
+from superset.exceptions import (
+ SerializationError,
+ SupersetError,
+ SupersetErrorException,
+ SupersetSecurityException,
+)
from superset.models.core import Database
from superset.models.sql_lab import Query
-from superset.sqllab.commands import results
+from superset.sqllab.commands import export, results
+from superset.sqllab.limiting_factor import LimitingFactor
from superset.utils import core as utils
from tests.integration_tests.base_tests import SupersetTestCase
+class TestSqlResultExportCommand(SupersetTestCase):
+ def test_validation_query_not_found(self) -> None:
+ command = export.SqlResultExportCommand("asdf")
+
+ database = Database(database_name="my_database", sqlalchemy_uri="sqlite://")
+ query_obj = Query(
+ client_id="test1",
+ database=database,
+ tab_name="test_tab",
+ sql_editor_id="test_editor_id",
+ sql="select * from bar",
+ select_sql="select * from bar",
+ executed_sql="select * from bar",
+ limit=100,
+ select_as_cta=False,
+ rows=104,
+ error_message="none",
+ results_key="abc1",
+ )
+
+ db.session.add(database)
+ db.session.add(query_obj)
+ db.session.commit()
+
+ with pytest.raises(SupersetErrorException) as ex_info:
+ command.run()
+ assert ex_info.value.error.error_type == SupersetErrorType.RESULTS_BACKEND_ERROR
+
+ def test_validation_invalid_access(self) -> None:
+ command = export.SqlResultExportCommand("test2")
+
+ database = Database(database_name="my_database2", sqlalchemy_uri="sqlite://")
+ query_obj = Query(
+ client_id="test2",
+ database=database,
+ tab_name="test_tab",
+ sql_editor_id="test_editor_id",
+ sql="select * from bar",
+ select_sql="select * from bar",
+ executed_sql="select * from bar",
+ limit=100,
+ select_as_cta=False,
+ rows=104,
+ error_message="none",
+ results_key="abc2",
+ )
+
+ db.session.add(database)
+ db.session.add(query_obj)
+ db.session.commit()
+
+ with mock.patch(
+ "superset.security_manager.raise_for_access",
+ side_effect=SupersetSecurityException(
+ SupersetError(
+ "dummy",
+ SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR,
+ ErrorLevel.ERROR,
+ )
+ ),
+ ):
+ with pytest.raises(SupersetErrorException) as ex_info:
+ command.run()
+ assert (
+ ex_info.value.error.error_type
+ == SupersetErrorType.QUERY_SECURITY_ACCESS_ERROR
+ )
Review Comment:
we probably need to delete `database` and `query_obj` here
##########
superset/sqllab/commands/export.py:
##########
@@ -0,0 +1,130 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# pylint: disable=too-few-public-methods, too-many-arguments
+from __future__ import annotations
+
+import logging
+from typing import Any, cast, Dict
+
+import pandas as pd
+from flask_babel import gettext as __, lazy_gettext as _
+
+from superset import app, db, results_backend, results_backend_use_msgpack
+from superset.commands.base import BaseCommand
+from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+from superset.exceptions import SupersetErrorException, SupersetSecurityException
+from superset.models.sql_lab import Query
+from superset.sql_parse import ParsedQuery
+from superset.sqllab.limiting_factor import LimitingFactor
+from superset.utils import core as utils, csv
+from superset.utils.dates import now_as_float
+from superset.views.utils import _deserialize_results_payload
+
+config = app.config
+
+logger = logging.getLogger(__name__)
+
+
+class SqlResultExportCommand(BaseCommand):
+ _client_id: str
+ _query: Query
+
+ def __init__(
+ self,
+ client_id: str,
+ ) -> None:
+ self._client_id = client_id
+
+ def validate(self) -> None:
+ self._query = (
+ db.session.query(Query).filter_by(client_id=self._client_id).one_or_none()
+ )
+ if self._query is None:
+ raise SupersetErrorException(
+ SupersetError(
Review Comment:
why not just use the existing `QueryNotFoundException`?
##########
superset/sqllab/schemas.py:
##########
@@ -24,6 +24,14 @@
"required": ["key"],
}
+sql_lab_export_csv_schema = {
+ "type": "object",
+ "properties": {
+ "client_id": {"type": "string"},
+ },
+ "required": ["client_id"],
+}
Review Comment:
we prob don't need this
##########
tests/integration_tests/sql_lab/commands_tests.py:
##########
@@ -15,23 +15,259 @@
# specific language governing permissions and limitations
# under the License.
from unittest import mock, skip
-from unittest.mock import patch
+from unittest.mock import Mock, patch
+import pandas as pd
import pytest
from superset import db, sql_lab
from superset.common.db_query_status import QueryStatus
-from superset.errors import SupersetErrorType
-from superset.exceptions import SerializationError, SupersetErrorException
+from superset.errors import ErrorLevel, SupersetErrorType
+from superset.exceptions import (
+ SerializationError,
+ SupersetError,
+ SupersetErrorException,
+ SupersetSecurityException,
+)
from superset.models.core import Database
from superset.models.sql_lab import Query
-from superset.sqllab.commands import results
+from superset.sqllab.commands import export, results
+from superset.sqllab.limiting_factor import LimitingFactor
from superset.utils import core as utils
from tests.integration_tests.base_tests import SupersetTestCase
+class TestSqlResultExportCommand(SupersetTestCase):
+ def test_validation_query_not_found(self) -> None:
+ command = export.SqlResultExportCommand("asdf")
+
+ database = Database(database_name="my_database", sqlalchemy_uri="sqlite://")
+ query_obj = Query(
+ client_id="test1",
+ database=database,
+ tab_name="test_tab",
+ sql_editor_id="test_editor_id",
+ sql="select * from bar",
+ select_sql="select * from bar",
+ executed_sql="select * from bar",
+ limit=100,
+ select_as_cta=False,
+ rows=104,
+ error_message="none",
+ results_key="abc1",
+ )
+
+ db.session.add(database)
+ db.session.add(query_obj)
+ db.session.commit()
+
+ with pytest.raises(SupersetErrorException) as ex_info:
+ command.run()
+ assert ex_info.value.error.error_type == SupersetErrorType.RESULTS_BACKEND_ERROR
+
+ def test_validation_invalid_access(self) -> None:
+ command = export.SqlResultExportCommand("test2")
+
+ database = Database(database_name="my_database2", sqlalchemy_uri="sqlite://")
+ query_obj = Query(
+ client_id="test2",
+ database=database,
+ tab_name="test_tab",
+ sql_editor_id="test_editor_id",
+ sql="select * from bar",
+ select_sql="select * from bar",
+ executed_sql="select * from bar",
+ limit=100,
+ select_as_cta=False,
+ rows=104,
+ error_message="none",
+ results_key="abc2",
+ )
+
+ db.session.add(database)
+ db.session.add(query_obj)
+ db.session.commit()
+
+ with mock.patch(
+ "superset.security_manager.raise_for_access",
+ side_effect=SupersetSecurityException(
+ SupersetError(
+ "dummy",
+ SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR,
+ ErrorLevel.ERROR,
+ )
+ ),
+ ):
+ with pytest.raises(SupersetErrorException) as ex_info:
+ command.run()
+ assert (
+ ex_info.value.error.error_type
+ == SupersetErrorType.QUERY_SECURITY_ACCESS_ERROR
+ )
+
+ @patch("superset.models.sql_lab.Query.raise_for_access", lambda _: None)
+ @patch("superset.models.core.Database.get_df")
+ def test_run_no_results_backend_select_sql(self, get_df_mock: Mock) -> None:
+ command = export.SqlResultExportCommand("test3")
+
+ database = Database(database_name="my_database3", sqlalchemy_uri="sqlite://")
+ query_obj = Query(
+ client_id="test3",
+ database=database,
+ tab_name="test_tab",
+ sql_editor_id="test_editor_id",
+ sql="select * from bar",
+ select_sql="select * from bar",
+ executed_sql="select * from bar",
+ limit=100,
+ select_as_cta=False,
+ rows=104,
+ error_message="none",
+ results_key="abc3",
+ )
+
+ db.session.add(database)
+ db.session.add(query_obj)
+ db.session.commit()
+
+ get_df_mock.return_value = pd.DataFrame({"foo": [1, 2, 3]})
+
+ result = command.run()
+
+ data = result.get("data")
+ count = result.get("count")
+ query = result.get("query")
+
+ assert data == "foo\n1\n2\n3\n"
+ assert count == 3
+ assert query.client_id == "test3"
Review Comment:
we probably need to delete `database` and `query_obj` here
##########
tests/integration_tests/sql_lab/commands_tests.py:
##########
@@ -15,23 +15,259 @@
# specific language governing permissions and limitations
# under the License.
from unittest import mock, skip
-from unittest.mock import patch
+from unittest.mock import Mock, patch
+import pandas as pd
import pytest
from superset import db, sql_lab
from superset.common.db_query_status import QueryStatus
-from superset.errors import SupersetErrorType
-from superset.exceptions import SerializationError, SupersetErrorException
+from superset.errors import ErrorLevel, SupersetErrorType
+from superset.exceptions import (
+ SerializationError,
+ SupersetError,
+ SupersetErrorException,
+ SupersetSecurityException,
+)
from superset.models.core import Database
from superset.models.sql_lab import Query
-from superset.sqllab.commands import results
+from superset.sqllab.commands import export, results
+from superset.sqllab.limiting_factor import LimitingFactor
from superset.utils import core as utils
from tests.integration_tests.base_tests import SupersetTestCase
+class TestSqlResultExportCommand(SupersetTestCase):
+ def test_validation_query_not_found(self) -> None:
+ command = export.SqlResultExportCommand("asdf")
+
+ database = Database(database_name="my_database", sqlalchemy_uri="sqlite://")
+ query_obj = Query(
+ client_id="test1",
+ database=database,
+ tab_name="test_tab",
+ sql_editor_id="test_editor_id",
+ sql="select * from bar",
+ select_sql="select * from bar",
+ executed_sql="select * from bar",
+ limit=100,
+ select_as_cta=False,
+ rows=104,
+ error_message="none",
+ results_key="abc1",
+ )
+
+ db.session.add(database)
+ db.session.add(query_obj)
+ db.session.commit()
Review Comment:
probably better to create a pytest fixture for this
--
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
[GitHub] [superset] diegomedina248 commented on a diff in pull request #22913: chore: Migrate /superset/csv/ to API v1
Posted by "diegomedina248 (via GitHub)" <gi...@apache.org>.
diegomedina248 commented on code in PR #22913:
URL: https://github.com/apache/superset/pull/22913#discussion_r1098104429
##########
superset/sqllab/commands/export.py:
##########
@@ -0,0 +1,130 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# pylint: disable=too-few-public-methods, too-many-arguments
+from __future__ import annotations
+
+import logging
+from typing import Any, cast, Dict
+
+import pandas as pd
+from flask_babel import gettext as __, lazy_gettext as _
+
+from superset import app, db, results_backend, results_backend_use_msgpack
+from superset.commands.base import BaseCommand
+from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+from superset.exceptions import SupersetErrorException, SupersetSecurityException
+from superset.models.sql_lab import Query
+from superset.sql_parse import ParsedQuery
+from superset.sqllab.limiting_factor import LimitingFactor
+from superset.utils import core as utils, csv
+from superset.utils.dates import now_as_float
+from superset.views.utils import _deserialize_results_payload
+
+config = app.config
+
+logger = logging.getLogger(__name__)
+
+
+class SqlResultExportCommand(BaseCommand):
+ _client_id: str
+ _query: Query
+
+ def __init__(
+ self,
+ client_id: str,
+ ) -> None:
+ self._client_id = client_id
+
+ def validate(self) -> None:
+ self._query = (
+ db.session.query(Query).filter_by(client_id=self._client_id).one_or_none()
+ )
+ if self._query is None:
+ raise SupersetErrorException(
+ SupersetError(
Review Comment:
Both this and the one in 79 are raised like this to add context (error type & message), which the originals do not have
--
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
[GitHub] [superset] codecov[bot] commented on pull request #22913: chore: Migrate /superset/csv/ to API v1
Posted by codecov.
codecov[bot] commented on PR #22913:
URL: https://github.com/apache/superset/pull/22913#issuecomment-1409479048
# [Codecov](https://codecov.io/gh/apache/superset/pull/22913?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#22913](https://codecov.io/gh/apache/superset/pull/22913?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8125292) into [master](https://codecov.io/gh/apache/superset/commit/b94052e438944b34ef30681e976bf1842ba4e0a5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b94052e) will **decrease** coverage by `14.37%`.
> The diff coverage is `46.91%`.
```diff
@@ Coverage Diff @@
## master #22913 +/- ##
===========================================
- Coverage 67.25% 52.88% -14.37%
===========================================
Files 1876 1877 +1
Lines 72066 72633 +567
Branches 7869 7869
===========================================
- Hits 48470 38415 -10055
- Misses 21578 32200 +10622
Partials 2018 2018
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| presto | `?` | |
| python | `51.96% <44.73%> (-29.88%)` | :arrow_down: |
| unit | `51.96% <44.73%> (+0.26%)` | :arrow_up: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/22913?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [superset/sqllab/api.py](https://codecov.io/gh/apache/superset/pull/22913?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsbGFiL2FwaS5weQ==) | `60.57% <42.85%> (-38.24%)` | :arrow_down: |
| [superset/sqllab/commands/export.py](https://codecov.io/gh/apache/superset/pull/22913?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsbGFiL2NvbW1hbmRzL2V4cG9ydC5weQ==) | `43.39% <43.39%> (ø)` | |
| [...frontend/src/SqlLab/components/ResultSet/index.tsx](https://codecov.io/gh/apache/superset/pull/22913?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC9pbmRleC50c3g=) | `62.17% <80.00%> (ø)` | |
| [superset/sqllab/schemas.py](https://codecov.io/gh/apache/superset/pull/22913?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsbGFiL3NjaGVtYXMucHk=) | `100.00% <100.00%> (ø)` | |
| [superset/views/core.py](https://codecov.io/gh/apache/superset/pull/22913?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `28.61% <100.00%> (-46.01%)` | :arrow_down: |
| [superset/examples/css\_templates.py](https://codecov.io/gh/apache/superset/pull/22913?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZXhhbXBsZXMvY3NzX3RlbXBsYXRlcy5weQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset/utils/dashboard\_import\_export.py](https://codecov.io/gh/apache/superset/pull/22913?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvZGFzaGJvYXJkX2ltcG9ydF9leHBvcnQucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset/migrations/shared/migrate\_viz/\_\_init\_\_.py](https://codecov.io/gh/apache/superset/pull/22913?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvbWlncmF0aW9ucy9zaGFyZWQvbWlncmF0ZV92aXovX19pbml0X18ucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...perset/migrations/shared/migrate\_viz/processors.py](https://codecov.io/gh/apache/superset/pull/22913?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvbWlncmF0aW9ucy9zaGFyZWQvbWlncmF0ZV92aXovcHJvY2Vzc29ycy5weQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset/reports/commands/alert.py](https://codecov.io/gh/apache/superset/pull/22913?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvcmVwb3J0cy9jb21tYW5kcy9hbGVydC5weQ==) | `0.00% <0.00%> (-96.74%)` | :arrow_down: |
| ... and [323 more](https://codecov.io/gh/apache/superset/pull/22913?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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
[GitHub] [superset] diegomedina248 commented on a diff in pull request #22913: chore: Migrate /superset/csv/ to API v1
Posted by "diegomedina248 (via GitHub)" <gi...@apache.org>.
diegomedina248 commented on code in PR #22913:
URL: https://github.com/apache/superset/pull/22913#discussion_r1098149986
##########
tests/integration_tests/sql_lab/commands_tests.py:
##########
@@ -15,23 +15,259 @@
# specific language governing permissions and limitations
# under the License.
from unittest import mock, skip
-from unittest.mock import patch
+from unittest.mock import Mock, patch
+import pandas as pd
import pytest
from superset import db, sql_lab
from superset.common.db_query_status import QueryStatus
-from superset.errors import SupersetErrorType
-from superset.exceptions import SerializationError, SupersetErrorException
+from superset.errors import ErrorLevel, SupersetErrorType
+from superset.exceptions import (
+ SerializationError,
+ SupersetError,
+ SupersetErrorException,
+ SupersetSecurityException,
+)
from superset.models.core import Database
from superset.models.sql_lab import Query
-from superset.sqllab.commands import results
+from superset.sqllab.commands import export, results
+from superset.sqllab.limiting_factor import LimitingFactor
from superset.utils import core as utils
from tests.integration_tests.base_tests import SupersetTestCase
+class TestSqlResultExportCommand(SupersetTestCase):
+ def test_validation_query_not_found(self) -> None:
+ command = export.SqlResultExportCommand("asdf")
+
+ database = Database(database_name="my_database", sqlalchemy_uri="sqlite://")
+ query_obj = Query(
+ client_id="test1",
+ database=database,
+ tab_name="test_tab",
+ sql_editor_id="test_editor_id",
+ sql="select * from bar",
+ select_sql="select * from bar",
+ executed_sql="select * from bar",
+ limit=100,
+ select_as_cta=False,
+ rows=104,
+ error_message="none",
+ results_key="abc1",
+ )
+
+ db.session.add(database)
+ db.session.add(query_obj)
+ db.session.commit()
Review Comment:
seems like the fixtures (or if done directly, same effect), are not being cleared properly.
Do I need to do anything else here?
--
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
[GitHub] [superset] dpgaspar merged pull request #22913: chore: Migrate /superset/csv/ to API v1
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar merged PR #22913:
URL: https://github.com/apache/superset/pull/22913
--
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