You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/10/14 11:15:25 UTC
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request #11229: feat: export databases as a ZIP bundle
dpgaspar commented on a change in pull request #11229:
URL: https://github.com/apache/incubator-superset/pull/11229#discussion_r504579667
##########
File path: superset/databases/commands/exceptions.py
##########
@@ -28,7 +28,7 @@
class DatabaseInvalidError(CommandInvalidError):
- message = _("Dashboard parameters are invalid.")
+ message = _("Database parameters are invalid.")
Review comment:
ups ;)
##########
File path: superset/databases/commands/export.py
##########
@@ -0,0 +1,89 @@
+# 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.
+# isort:skip_file
+
+import json
+import os.path
+from io import BytesIO
+from typing import Any, Dict
+from zipfile import ZipFile
+
+import yaml
+
+from superset.commands.base import BaseCommand
+from superset.databases.commands.exceptions import DatabaseNotFoundError
+from superset.databases.dao import DatabaseDAO
+from superset.utils.dict_import_export import IMPORT_EXPORT_VERSION, sanitize
+
+
+class ExportDatabaseCommand(BaseCommand):
+ def __init__(self, database_id: int, filename: str):
+ self.database_id = database_id
+ self.filename = filename
+
+ def run(self) -> BytesIO:
+ self.validate()
+
+ root = os.path.splitext(self.filename)[0]
+ database = DatabaseDAO.find_by_id(self.database_id)
+ name = sanitize(database.database_name)
+ database_filename = f"{root}/databases/{name}.yaml"
+
+ payload: Dict[Any, Any]
+ payload = database.export_to_dict(
+ recursive=False,
+ include_parent_ref=False,
+ include_defaults=True,
+ export_uuids=True,
+ )
+ if "extra" in payload:
+ try:
+ payload["extra"] = json.loads(payload["extra"])
+ except json.decoder.JSONDecodeError:
+ pass
+
+ payload["version"] = IMPORT_EXPORT_VERSION
+
+ buf = BytesIO()
+ with ZipFile(buf, "w") as bundle:
Review comment:
Assuming no file is ever created on the FS, is this correct?
##########
File path: tests/databases/api_tests.py
##########
@@ -801,3 +803,26 @@ def test_get_database_related_objects_not_found(self):
uri = f"api/v1/database/{database.id}/related_objects/"
rv = self.client.get(uri)
self.assertEqual(rv.status_code, 404)
+
+ def test_export_database(self):
+ """
+ Database API: Test export database
+ """
+ self.login(username="admin")
+ uri = "api/v1/database/1/export/"
+ rv = self.client.get(uri)
+
+ self.assertEqual(rv.status_code, 200)
+
+ buf = BytesIO(rv.data)
+ self.assertTrue(is_zipfile(buf))
+
+ def test_export_database_not_allowed(self):
+ """
+ Database API: Test export database
+ """
+ self.login(username="gamma")
+ uri = "api/v1/database/1/export/"
+ rv = self.client.get(uri)
+
+ self.assertEqual(rv.status_code, 401)
Review comment:
This should return a 404, right?
##########
File path: tests/databases/api_tests.py
##########
@@ -801,3 +803,26 @@ def test_get_database_related_objects_not_found(self):
uri = f"api/v1/database/{database.id}/related_objects/"
rv = self.client.get(uri)
self.assertEqual(rv.status_code, 404)
+
+ def test_export_database(self):
+ """
+ Database API: Test export database
+ """
+ self.login(username="admin")
+ uri = "api/v1/database/1/export/"
+ rv = self.client.get(uri)
+
+ self.assertEqual(rv.status_code, 200)
+
+ buf = BytesIO(rv.data)
+ self.assertTrue(is_zipfile(buf))
+
+ def test_export_database_not_allowed(self):
+ """
+ Database API: Test export database
Review comment:
nit: `Database API: Test export database not allowed`
##########
File path: tests/databases/api_tests.py
##########
@@ -801,3 +803,26 @@ def test_get_database_related_objects_not_found(self):
uri = f"api/v1/database/{database.id}/related_objects/"
rv = self.client.get(uri)
self.assertEqual(rv.status_code, 404)
+
+ def test_export_database(self):
+ """
+ Database API: Test export database
+ """
+ self.login(username="admin")
+ uri = "api/v1/database/1/export/"
Review comment:
Fetch a database `get_examples_database` then use the id from the model itself
##########
File path: tests/databases/api_tests.py
##########
@@ -801,3 +803,26 @@ def test_get_database_related_objects_not_found(self):
uri = f"api/v1/database/{database.id}/related_objects/"
rv = self.client.get(uri)
self.assertEqual(rv.status_code, 404)
+
+ def test_export_database(self):
+ """
+ Database API: Test export database
+ """
+ self.login(username="admin")
+ uri = "api/v1/database/1/export/"
+ rv = self.client.get(uri)
+
+ self.assertEqual(rv.status_code, 200)
+
+ buf = BytesIO(rv.data)
+ self.assertTrue(is_zipfile(buf))
+
+ def test_export_database_not_allowed(self):
+ """
+ Database API: Test export database
+ """
+ self.login(username="gamma")
+ uri = "api/v1/database/1/export/"
Review comment:
Fetch a database get_examples_database then use the id from the model itself
##########
File path: superset/databases/commands/export.py
##########
@@ -0,0 +1,89 @@
+# 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.
+# isort:skip_file
+
+import json
+import os.path
+from io import BytesIO
+from typing import Any, Dict
+from zipfile import ZipFile
+
+import yaml
+
+from superset.commands.base import BaseCommand
+from superset.databases.commands.exceptions import DatabaseNotFoundError
+from superset.databases.dao import DatabaseDAO
+from superset.utils.dict_import_export import IMPORT_EXPORT_VERSION, sanitize
+
+
+class ExportDatabaseCommand(BaseCommand):
+ def __init__(self, database_id: int, filename: str):
+ self.database_id = database_id
+ self.filename = filename
+
+ def run(self) -> BytesIO:
+ self.validate()
+
+ root = os.path.splitext(self.filename)[0]
+ database = DatabaseDAO.find_by_id(self.database_id)
+ name = sanitize(database.database_name)
+ database_filename = f"{root}/databases/{name}.yaml"
+
+ payload: Dict[Any, Any]
+ payload = database.export_to_dict(
+ recursive=False,
+ include_parent_ref=False,
+ include_defaults=True,
+ export_uuids=True,
+ )
+ if "extra" in payload:
Review comment:
Should this logic live inside `export_to_dict` ?
##########
File path: tests/databases/api_tests.py
##########
@@ -801,3 +803,26 @@ def test_get_database_related_objects_not_found(self):
uri = f"api/v1/database/{database.id}/related_objects/"
rv = self.client.get(uri)
self.assertEqual(rv.status_code, 404)
+
+ def test_export_database(self):
+ """
+ Database API: Test export database
+ """
+ self.login(username="admin")
+ uri = "api/v1/database/1/export/"
+ rv = self.client.get(uri)
+
+ self.assertEqual(rv.status_code, 200)
Review comment:
nit: assert rv.status_code == 200
##########
File path: superset/databases/commands/export.py
##########
@@ -0,0 +1,89 @@
+# 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.
+# isort:skip_file
+
+import json
+import os.path
+from io import BytesIO
+from typing import Any, Dict
+from zipfile import ZipFile
+
+import yaml
+
+from superset.commands.base import BaseCommand
+from superset.databases.commands.exceptions import DatabaseNotFoundError
+from superset.databases.dao import DatabaseDAO
+from superset.utils.dict_import_export import IMPORT_EXPORT_VERSION, sanitize
+
+
+class ExportDatabaseCommand(BaseCommand):
+ def __init__(self, database_id: int, filename: str):
+ self.database_id = database_id
+ self.filename = filename
+
+ def run(self) -> BytesIO:
+ self.validate()
+
+ root = os.path.splitext(self.filename)[0]
+ database = DatabaseDAO.find_by_id(self.database_id)
Review comment:
Avoid this extra fetch by using validate: https://github.com/apache/incubator-superset/blob/master/superset/databases/commands/update.py#L74
##########
File path: tests/databases/api_tests.py
##########
@@ -801,3 +803,26 @@ def test_get_database_related_objects_not_found(self):
uri = f"api/v1/database/{database.id}/related_objects/"
rv = self.client.get(uri)
self.assertEqual(rv.status_code, 404)
+
+ def test_export_database(self):
+ """
+ Database API: Test export database
+ """
+ self.login(username="admin")
+ uri = "api/v1/database/1/export/"
+ rv = self.client.get(uri)
+
+ self.assertEqual(rv.status_code, 200)
+
+ buf = BytesIO(rv.data)
+ self.assertTrue(is_zipfile(buf))
+
+ def test_export_database_not_allowed(self):
+ """
+ Database API: Test export database
+ """
+ self.login(username="gamma")
+ uri = "api/v1/database/1/export/"
+ rv = self.client.get(uri)
+
+ self.assertEqual(rv.status_code, 401)
Review comment:
nit: add one more test with a non existent database_id (use max then plus 1).
##########
File path: tests/datasets/api_tests.py
##########
@@ -189,10 +190,9 @@ def pg_test_query_parameter(query_parameter, expected_response):
"admin_database",
"information_schema",
"public",
- "superset",
]
expected_response = {
- "count": 5,
+ "count": 4,
Review comment:
This should be replaced by an actual db.query instead of the hardcoded value
##########
File path: tests/datasets/api_tests.py
##########
@@ -165,6 +165,7 @@ def test_get_dataset_distinct_schema(self):
"""
Dataset API: Test get dataset distinct schema
"""
+ self.maxDiff = None
Review comment:
remove (debug I know) :)
##########
File path: superset/databases/commands/export.py
##########
@@ -0,0 +1,89 @@
+# 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.
+# isort:skip_file
+
+import json
+import os.path
+from io import BytesIO
+from typing import Any, Dict
+from zipfile import ZipFile
+
+import yaml
+
+from superset.commands.base import BaseCommand
+from superset.databases.commands.exceptions import DatabaseNotFoundError
+from superset.databases.dao import DatabaseDAO
+from superset.utils.dict_import_export import IMPORT_EXPORT_VERSION, sanitize
+
+
+class ExportDatabaseCommand(BaseCommand):
+ def __init__(self, database_id: int, filename: str):
+ self.database_id = database_id
+ self.filename = filename
+
+ def run(self) -> BytesIO:
+ self.validate()
+
+ root = os.path.splitext(self.filename)[0]
+ database = DatabaseDAO.find_by_id(self.database_id)
+ name = sanitize(database.database_name)
+ database_filename = f"{root}/databases/{name}.yaml"
+
+ payload: Dict[Any, Any]
+ payload = database.export_to_dict(
+ recursive=False,
+ include_parent_ref=False,
+ include_defaults=True,
+ export_uuids=True,
+ )
+ if "extra" in payload:
+ try:
+ payload["extra"] = json.loads(payload["extra"])
+ except json.decoder.JSONDecodeError:
+ pass
+
+ payload["version"] = IMPORT_EXPORT_VERSION
+
+ buf = BytesIO()
+ with ZipFile(buf, "w") as bundle:
+ with bundle.open(database_filename, "w") as fp:
+ fp.write(yaml.safe_dump(payload, sort_keys=False).encode())
+
+ for dataset in database.tables:
+ name = sanitize(dataset.table_name)
+ dataset_filename = f"{root}/datasets/{name}.yaml"
+
+ # TODO (betodealmeida): reuse logic from ExportDatasetCommand
+ # once it's implemented
+ payload = dataset.export_to_dict(
+ recursive=True,
+ include_parent_ref=False,
+ include_defaults=True,
+ export_uuids=True,
+ )
+ payload["version"] = IMPORT_EXPORT_VERSION
Review comment:
Should `export_to_dict` have this logic?
##########
File path: tests/dashboards/api_tests.py
##########
@@ -955,12 +956,14 @@ def test_export(self):
self.login(username="admin")
argument = [1, 2]
uri = f"api/v1/dashboard/export/?q={prison.dumps(argument)}"
- rv = self.get_assert_metric(uri, "export")
+
+ # freeze time to ensure filename is deterministic
+ with freeze_time("2020-01-01T00:00:00Z"):
+ rv = self.get_assert_metric(uri, "export")
+ headers = generate_download_headers("json")["Content-Disposition"]
+
self.assertEqual(rv.status_code, 200)
- self.assertEqual(
- rv.headers["Content-Disposition"],
- generate_download_headers("json")["Content-Disposition"],
- )
+ self.assertEqual(rv.headers["Content-Disposition"], headers)
Review comment:
nit: We are currently preferring `assert rv.headers["Content-Disposition"] == headers`. More inline with pytest
----------------------------------------------------------------
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.
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