dpgaspar commented on code in PR #22501:
URL: https://github.com/apache/superset/pull/22501#discussion_r1055719155
##########
superset/databases/api.py:
##########
@@ -493,6 +500,76 @@ def schemas(self, pk: int, **kwargs: Any) -> FlaskResponse:
except SupersetException as ex:
return self.response(ex.status, message=ex.message)
+ @expose("/<int:pk>/tables/")
+ @protect()
+ @safe
+ @rison(database_tables_query_schema)
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs: f"{self.__class__.__name__}"
f".tables",
+ log_to_statsd=False,
+ )
+ def tables(self, pk: int, **kwargs: Any) -> FlaskResponse:
+ """Get a list of tables for given database
+ ---
+ get:
+ description: Get a list of tables for given database
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ description: The database id
+ - in: query
+ name: q
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/database_tables_query_schema'
+ responses:
+ 200:
+ description: Tables list
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ result:
+ description: >-
+ A List of tables for given database
+ type: object
+ properties:
+ tableLength:
+ type: integer
+ options:
+ type: array
+ items:
+ $ref:
'#/components/schemas/DatabaseTablesResponse'
+ 400:
+ $ref: '#/components/responses/400'
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ force = kwargs["rison"].get("force", False)
+ schema_name = kwargs["rison"].get("schema_name", "")
+ if not schema_name:
+ return self.response_422("Schema undefined")
+
+ try:
+ command = TablesDatabaseCommand(pk, schema_name, force)
+ payload = command.run()
+ return self.response(200, result=payload)
+ except DatabaseNotFoundError:
+ return self.response_404()
+ except SupersetException as ex:
+ return self.response(ex.status, message=ex.message)
+ except DatabaseTablesUnexpectedError as ex:
+ return self.response_500(ex.message)
Review Comment:
do we really want to respond an HTTP 500 on this case?
##########
tests/integration_tests/databases/api_tests.py:
##########
@@ -1381,6 +1381,56 @@ def test_database_schemas_invalid_query(self):
)
self.assertEqual(rv.status_code, 400)
+ def test_database_tables(self):
+ """
+ Database API: Test database tables
+ """
+ self.login(username="admin")
+ database =
db.session.query(Database).filter_by(database_name="examples").one()
+
+ schema_name = self.default_schema_backend_map[database.backend]
+ rv = self.client.get(
+
f"api/v1/database/{database.id}/tables/?q={prison.dumps({'schema_name':
schema_name})}"
+ )
+
+ self.assertEqual(rv.status_code, 200)
Review Comment:
Check the response payload also
##########
superset/databases/schemas.py:
##########
@@ -555,6 +563,12 @@ class SchemasResponseSchema(Schema):
result = fields.List(fields.String(description="A database schema name"))
+class DatabaseTablesResponse(Schema):
+ extra = fields.Dict(required=False)
Review Comment:
don't think that the require=False is necessary here
##########
superset/databases/commands/tables.py:
##########
@@ -0,0 +1,113 @@
+# 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.
+import logging
+from typing import Any, cast, Dict
+
+from superset.commands.base import BaseCommand
+from superset.connectors.sqla.models import SqlaTable
+from superset.databases.commands.exceptions import (
+ DatabaseNotFoundError,
+ DatabaseTablesUnexpectedError,
+)
+from superset.databases.dao import DatabaseDAO
+from superset.exceptions import SupersetException
+from superset.extensions import db, security_manager
+from superset.models.core import Database
+from superset.utils.core import DatasourceName
+
+logger = logging.getLogger(__name__)
+
+
+class TablesDatabaseCommand(BaseCommand):
+ _model: Database
+
+ def __init__(self, db_id: int, schema_name: str, force: bool):
+ self._db_id = db_id
+ self._schema_name = schema_name
+ self._force = force
+
+ def run(self) -> Dict[str, Any]:
+ self.validate()
+ try:
+ tables = security_manager.get_datasources_accessible_by_user(
+ database=self._model,
+ schema=self._schema_name,
+ datasource_names=sorted(
+ DatasourceName(*datasource_name)
+ for datasource_name in
self._model.get_all_table_names_in_schema(
+ schema=self._schema_name,
+ force=self._force,
+ cache=self._model.table_cache_enabled,
+ cache_timeout=self._model.table_cache_timeout,
+ )
+ ),
+ )
+
+ views = security_manager.get_datasources_accessible_by_user(
+ database=self._model,
+ schema=self._schema_name,
+ datasource_names=sorted(
+ DatasourceName(*datasource_name)
+ for datasource_name in
self._model.get_all_view_names_in_schema(
+ schema=self._schema_name,
+ force=self._force,
+ cache=self._model.table_cache_enabled,
+ cache_timeout=self._model.table_cache_timeout,
+ )
+ ),
+ )
+
+ extra_dict_by_name = {
+ table.name: table.extra_dict
+ for table in (
+ db.session.query(SqlaTable).filter(
+ SqlaTable.database_id == self._model.id,
+ SqlaTable.schema == self._schema_name,
+ )
+ ).all()
+ }
+
+ options = sorted(
+ [
+ {
+ "value": table.table,
+ "type": "table",
+ "extra": extra_dict_by_name.get(table.table, None),
+ }
+ for table in tables
+ ]
+ + [
+ {
+ "value": view.table,
+ "type": "view",
+ }
+ for view in views
+ ],
+ key=lambda item: item["value"],
+ )
+
+ payload = {"tableLength": len(tables) + len(views), "options":
options}
Review Comment:
If `tableLength` is a count of all records on the payload, we are using
`count` on all other API endpoints
##########
superset/databases/api.py:
##########
@@ -493,6 +500,76 @@ def schemas(self, pk: int, **kwargs: Any) -> FlaskResponse:
except SupersetException as ex:
return self.response(ex.status, message=ex.message)
+ @expose("/<int:pk>/tables/")
+ @protect()
+ @safe
+ @rison(database_tables_query_schema)
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs: f"{self.__class__.__name__}"
f".tables",
+ log_to_statsd=False,
+ )
+ def tables(self, pk: int, **kwargs: Any) -> FlaskResponse:
+ """Get a list of tables for given database
+ ---
+ get:
+ description: Get a list of tables for given database
Review Comment:
let's start using `summary:` instead of `description:`
##########
superset/databases/schemas.py:
##########
@@ -555,6 +563,12 @@ class SchemasResponseSchema(Schema):
result = fields.List(fields.String(description="A database schema name"))
+class DatabaseTablesResponse(Schema):
+ extra = fields.Dict(required=False)
+ type = fields.String()
+ value = fields.String()
Review Comment:
would be nice to add a description on these fields to improve out
documentation for the API
##########
superset/databases/api.py:
##########
@@ -493,6 +500,76 @@ def schemas(self, pk: int, **kwargs: Any) -> FlaskResponse:
except SupersetException as ex:
return self.response(ex.status, message=ex.message)
+ @expose("/<int:pk>/tables/")
+ @protect()
+ @safe
+ @rison(database_tables_query_schema)
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs: f"{self.__class__.__name__}"
f".tables",
+ log_to_statsd=False,
+ )
+ def tables(self, pk: int, **kwargs: Any) -> FlaskResponse:
+ """Get a list of tables for given database
+ ---
+ get:
+ description: Get a list of tables for given database
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ description: The database id
+ - in: query
+ name: q
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/database_tables_query_schema'
+ responses:
+ 200:
+ description: Tables list
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ result:
+ description: >-
+ A List of tables for given database
+ type: object
+ properties:
+ tableLength:
+ type: integer
+ options:
+ type: array
+ items:
+ $ref:
'#/components/schemas/DatabaseTablesResponse'
+ 400:
+ $ref: '#/components/responses/400'
+ 401:
+ $ref: '#/components/responses/401'
+ 404:
+ $ref: '#/components/responses/404'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ force = kwargs["rison"].get("force", False)
+ schema_name = kwargs["rison"].get("schema_name", "")
+ if not schema_name:
+ return self.response_422("Schema undefined")
Review Comment:
you can set the `schema` arg to be required on
`database_tables_query_schema` directly so you don't have to check it, it will
return HTTP 400. You can also set a default value there
--
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]
