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/02/12 20:12:06 UTC
[GitHub] [incubator-superset] dpgaspar opened a new pull request #9129:
[datasets] new, API using command pattern
dpgaspar opened a new pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129
### CATEGORY
Choose one
- [ ] Bug Fix
- [X] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
<!--- What steps should be taken to 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:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r379123750
##########
File path: tests/dataset_api_tests.py
##########
@@ -0,0 +1,445 @@
+# 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.
+"""Unit tests for Superset"""
+import json
+from typing import List
+from unittest.mock import patch
+
+import prison
+
+from superset import db, security_manager
+from superset.connectors.sqla.models import SqlaTable
+from superset.models.core import Database
+from superset.utils.core import get_example_database
+
+from .base_tests import SupersetTestCase
+
+
+class DatasetApiTests(SupersetTestCase):
Review comment:
I like the idea of partial integration tests at this layer, but I wonder if this is the best testing approach? We could easily segment out the testing such that we test the commands, DAOs, and endpoints in isolation, as they all have public interfaces. I suspect that approach would make the separation of concerns clearer and lead to more comprehensive unit tests.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r379384670
##########
File path: superset/datasets/commands/create.py
##########
@@ -0,0 +1,82 @@
+# 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.
+from flask_appbuilder.security.sqla.models import User
+from marshmallow import UnmarshalResult, ValidationError
+
+from superset.commands.base import BaseCommand, CommandValidateReturn
+from superset.datasets.commands.base import populate_owners
+from superset.datasets.commands.exceptions import (
+ DatabaseNotFoundValidationError,
+ DatasetCreateFailedError,
+ DatasetExistsValidationError,
+ DatasetInvalidError,
+ TableNotFoundValidationError,
+)
+from superset.datasets.dao import DatasetDAO
+
+
+class CreateDatasetCommand(BaseCommand):
+ def __init__(self, user: User, unmarshal: UnmarshalResult):
+ self._actor = user
+ self._properties = unmarshal.data.copy()
+ self._errors = unmarshal.errors
+
+ def run(self):
+ is_valid, exceptions = self.validate()
+ if not is_valid:
+ for exception in exceptions:
+ self._errors.update(exception.normalized_messages())
+ raise DatasetInvalidError()
+
+ dataset = DatasetDAO.create(self._properties)
+
+ if not dataset:
+ raise DatasetCreateFailedError()
+ return dataset
+
+ def validate(self) -> CommandValidateReturn:
+ is_valid, exceptions = super().validate()
Review comment:
Agree
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r379533085
##########
File path: superset/commands/base.py
##########
@@ -0,0 +1,44 @@
+# 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.
+from abc import ABC, abstractmethod
+from typing import List, Tuple
+
+from marshmallow.validate import ValidationError
+
+CommandValidateReturn = Tuple[bool, List[ValidationError]]
+
+
+class BaseCommand(ABC):
+ """
+ Base class for all Command like Superset Logic objects
+ """
+
+ @abstractmethod
+ def run(self):
+ """
+ Run executes the command
+ :return:
+ """
+ pass
+
+ @abstractmethod
+ def validate(self) -> CommandValidateReturn:
Review comment:
I'd prefer to keep it a single word. It can be understood that `validate` may run queries if necessary. I would prefer not to be too prescriptive on the internal workings of a command. They should be generic interfaces that allow a great deal of flexibility.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r379414952
##########
File path: superset/datasets/commands/delete.py
##########
@@ -0,0 +1,57 @@
+# 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.
+from typing import Optional, List
+
+from flask_appbuilder.security.sqla.models import User
+
+from superset.commands.base import BaseCommand, CommandValidateReturn
+from superset.datasets.commands.exceptions import (
+ DatasetDeleteFailedError,
+ DatasetForbiddenError,
+ DatasetNotFoundError,
+)
+from superset.connectors.sqla.models import SqlaTable
+from superset.datasets.dao import DatasetDAO
+from superset.exceptions import SupersetSecurityException
+from superset.views.base import check_ownership
+
+
+class DeleteDatasetCommand(BaseCommand):
+ def __init__(self, user: User, model_id: int):
+ self._actor = user
+ self._model_id = model_id
+ self._model: Optional[SqlaTable] = None
+
+ def run(self):
+ self.validate()
+ dataset = DatasetDAO.delete(self._model)
+
+ if not dataset:
+ raise DatasetDeleteFailedError()
+ return dataset
+
+ def validate(self) -> CommandValidateReturn:
+ # Validate/populate model exists
Review comment:
Added `model` get, since `validate` should run independently of `run`
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r379115012
##########
File path: superset/datasets/commands/create.py
##########
@@ -0,0 +1,82 @@
+# 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.
+from flask_appbuilder.security.sqla.models import User
+from marshmallow import UnmarshalResult, ValidationError
+
+from superset.commands.base import BaseCommand, CommandValidateReturn
+from superset.datasets.commands.base import populate_owners
+from superset.datasets.commands.exceptions import (
+ DatabaseNotFoundValidationError,
+ DatasetCreateFailedError,
+ DatasetExistsValidationError,
+ DatasetInvalidError,
+ TableNotFoundValidationError,
+)
+from superset.datasets.dao import DatasetDAO
+
+
+class CreateDatasetCommand(BaseCommand):
+ def __init__(self, user: User, unmarshal: UnmarshalResult):
+ self._actor = user
+ self._properties = unmarshal.data.copy()
+ self._errors = unmarshal.errors
+
+ def run(self):
+ is_valid, exceptions = self.validate()
+ if not is_valid:
+ for exception in exceptions:
+ self._errors.update(exception.normalized_messages())
+ raise DatasetInvalidError()
+
+ dataset = DatasetDAO.create(self._properties)
+
+ if not dataset:
+ raise DatasetCreateFailedError()
+ return dataset
+
+ def validate(self) -> CommandValidateReturn:
+ is_valid, exceptions = super().validate()
Review comment:
There's no need to call the parent abstract method here - it's returning types but this implies shared functionality that does not exist.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r379416111
##########
File path: superset/datasets/commands/update.py
##########
@@ -0,0 +1,83 @@
+# 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.
+from flask_appbuilder.security.sqla.models import User
+from marshmallow import UnmarshalResult, ValidationError
+
+from superset.commands.base import BaseCommand, CommandValidateReturn
+from superset.datasets.commands.base import populate_owners
+from superset.datasets.commands.exceptions import (
+ DatabaseChangeValidationError,
+ DatasetExistsValidationError,
+ DatasetInvalidError,
+ DatasetNotFoundError,
+ DatasetUpdateFailedError,
+)
+from superset.datasets.dao import DatasetDAO
+from superset.views.base import check_ownership
+
+
+class UpdateDatasetCommand(BaseCommand):
+ def __init__(self, user: User, model_id: int, unmarshal: UnmarshalResult):
+ self._actor = user
+ self._model_id = model_id
+ self._properties = unmarshal.data.copy()
+ self._errors = unmarshal.errors
+ self._model = None
+
+ def run(self):
+ self._model = DatasetDAO.find_by_id(self._model_id)
+ if not self._model:
+ raise DatasetNotFoundError()
+ valid, exceptions = self.validate()
+ if not valid:
+ for exception in exceptions:
+ self._errors.update(exception.normalized_messages())
+ raise DatasetInvalidError()
+ dataset = DatasetDAO.update(self._model, self._properties)
+
+ if not dataset:
+ raise DatasetUpdateFailedError()
+ return dataset
+
+ def validate(self) -> CommandValidateReturn:
+ is_valid, exceptions = super().validate()
+ if not self._model:
Review comment:
Good point. Moved it to `validate` since I think it was a mistake to check it on `run` and making `validate` depend on it.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda commented on a change in pull
request #9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r386546425
##########
File path: superset/datasets/dao.py
##########
@@ -0,0 +1,119 @@
+# 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 Dict, Optional
+
+from flask import current_app
+from flask_appbuilder.models.sqla.interface import SQLAInterface
+from sqlalchemy.exc import SQLAlchemyError
+
+from superset import db
+from superset.connectors.sqla.models import SqlaTable
+from superset.models.core import Database
+from superset.views.base import DatasourceFilter
+
+logger = logging.getLogger(__name__)
+
+
+class DatasetDAO:
+ @staticmethod
+ def get_owner_by_id(owner_id: int) -> Optional[object]:
+ return (
+ db.session.query(current_app.appbuilder.sm.user_model)
+ .filter_by(id=owner_id)
+ .one_or_none()
+ )
+
+ @staticmethod
+ def get_database_by_id(database_id) -> Optional[Database]:
+ try:
+ return db.session.query(Database).filter_by(id=database_id).one_or_none()
+ except SQLAlchemyError as e: # pragma: no cover
+ logger.error(f"Could not get database by id: {e}")
+ return None
+
+ @staticmethod
+ def validate_table_exists(database: Database, table_name: str, schema: str) -> bool:
+ try:
+ database.get_table(table_name, schema=schema)
+ return True
+ except SQLAlchemyError as e: # pragma: no cover
+ logger.error(f"Got an error {e} validating table: {table_name}")
+ return False
+
+ @staticmethod
+ def validate_uniqueness(database_id: int, name: str) -> bool:
+ dataset_query = db.session.query(SqlaTable).filter(
+ SqlaTable.table_name == name, SqlaTable.database_id == database_id
+ )
+ return not db.session.query(dataset_query.exists()).scalar()
+
+ @staticmethod
+ def validate_update_uniqueness(
+ database_id: int, dataset_id: int, name: str
+ ) -> bool:
+ dataset_query = db.session.query(SqlaTable).filter(
+ SqlaTable.table_name == name,
+ SqlaTable.database_id == database_id,
+ SqlaTable.id != dataset_id,
+ )
+ return not db.session.query(dataset_query.exists()).scalar()
+
+ @staticmethod
+ def find_by_id(model_id: int) -> SqlaTable:
+ data_model = SQLAInterface(SqlaTable, db.session)
+ query = db.session.query(SqlaTable)
+ query = DatasourceFilter("id", data_model).apply(query, None)
+ return query.filter_by(id=model_id).one_or_none()
+
+ @staticmethod
+ def create(properties: Dict) -> Optional[SqlaTable]:
+ model = SqlaTable()
+ for key, value in properties.items():
+ setattr(model, key, value)
+ try:
+ db.session.add(model)
+ db.session.commit()
+ except SQLAlchemyError as e: # pragma: no cover
+ logger.error(f"Failed to create dataset: {e}")
+ db.session.rollback()
+ return None
+ return model
+
+ @staticmethod
+ def update(model: SqlaTable, properties: Dict) -> Optional[SqlaTable]:
+ for key, value in properties.items():
+ setattr(model, key, value)
+ try:
+ db.session.merge(model)
+ db.session.commit()
Review comment:
Not sure we want TX to finish in these methods. What happens when you want to do a few things as part of a single atomic unit of work? (I.e. update a few items via a DAO, and one fails)
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r379121654
##########
File path: tests/dataset_api_tests.py
##########
@@ -0,0 +1,445 @@
+# 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.
+"""Unit tests for Superset"""
+import json
+from typing import List
+from unittest.mock import patch
+
+import prison
+
+from superset import db, security_manager
+from superset.connectors.sqla.models import SqlaTable
+from superset.models.core import Database
+from superset.utils.core import get_example_database
+
+from .base_tests import SupersetTestCase
+
+
+class DatasetApiTests(SupersetTestCase):
+ @staticmethod
+ def insert_dataset(
+ table_name: str, schema: str, owners: List[int], database: Database
+ ) -> SqlaTable:
+ obj_owners = list()
+ for owner in owners:
+ user = db.session.query(security_manager.user_model).get(owner)
+ obj_owners.append(user)
+ table = SqlaTable(
+ table_name=table_name, schema=schema, owners=obj_owners, database=database
+ )
+ db.session.add(table)
+ db.session.commit()
+ return table
+
+ def test_get_dataset_list(self):
+ """
+ Dataset API: Test get dataset list
+ """
+ example_db = get_example_database()
+ self.login(username="admin")
+ arguments = {
+ "filters": [
+ {"col": "database", "opr": "rel_o_m", "value": f"{example_db.id}"},
+ {"col": "table_name", "opr": "eq", "value": f"birth_names"},
+ ]
+ }
+ uri = f"api/v1/dataset/?q={prison.dumps(arguments)}"
+ rv = self.client.get(uri)
+ self.assertEqual(rv.status_code, 200)
+ response = json.loads(rv.data.decode("utf-8"))
+ self.assertEqual(response["count"], 1)
+ expected_columns = [
+ "changed_by",
+ "changed_on",
+ "database_name",
+ "schema",
+ "table_name",
+ ]
+ self.assertEqual(sorted(list(response["result"][0].keys())), expected_columns)
+
+ def test_get_dataset_list_gamma(self):
+ """
+ Dataset API: Test get dataset list gamma
+ """
+ example_db = get_example_database()
+ self.login(username="gamma")
+ uri = "api/v1/dataset/"
+ rv = self.client.get(uri)
+ self.assertEqual(rv.status_code, 200)
+ response = json.loads(rv.data.decode("utf-8"))
+ self.assertEqual(response["result"], [])
+
+ def test_get_dataset_related_database_gamma(self):
+ """
+ Dataset API: Test get dataset related databases gamma
+ """
+ example_db = get_example_database()
+ self.login(username="gamma")
+ uri = "api/v1/dataset/related/database"
+ rv = self.client.get(uri)
+ self.assertEqual(rv.status_code, 200)
+ response = json.loads(rv.data.decode("utf-8"))
+ self.assertEqual(response["count"], 0)
+ self.assertEqual(response["result"], [])
+
+ def test_get_dataset_item(self):
+ """
+ Dataset API: Test get dataset item
+ """
+ example_db = get_example_database()
+ table = (
+ db.session.query(SqlaTable)
+ .filter_by(database=example_db, table_name="birth_names")
+ .one()
+ )
+ self.login(username="admin")
+ uri = f"api/v1/dataset/{table.id}"
+ rv = self.client.get(uri)
+ self.assertEqual(rv.status_code, 200)
+ response = json.loads(rv.data.decode("utf-8"))
+ expected_result = {
+ "cache_timeout": None,
+ "database": {"database_name": "examples", "id": 1},
+ "default_endpoint": None,
+ "description": None,
+ "fetch_values_predicate": None,
+ "filter_select_enabled": True,
+ "is_sqllab_view": False,
+ "main_dttm_col": "ds",
+ "offset": 0,
+ "owners": [],
+ "schema": None,
+ "sql": None,
+ "table_name": "birth_names",
+ "template_params": None,
+ }
+ self.assertEqual(response["result"], expected_result)
+
+ def test_get_dataset_info(self):
+ """
+ Dataset API: Test get dataset info
+ """
+ self.login(username="admin")
+ uri = "api/v1/dataset/_info"
+ rv = self.client.get(uri)
+ self.assertEqual(rv.status_code, 200)
+
+ def test_create_dataset_item(self):
+ """
+ Dataset API: Test create dataset item
+ """
+ example_db = get_example_database()
+ self.login(username="admin")
+ table_data = {
+ "database": example_db.id,
+ "schema": "",
+ "table_name": "ab_permission",
+ }
+ uri = "api/v1/dataset/"
+ rv = self.client.post(uri, json=table_data)
+ self.assertEqual(rv.status_code, 201)
+ data = json.loads(rv.data.decode("utf-8"))
+ model = db.session.query(SqlaTable).get(data.get("id"))
+ self.assertEqual(model.table_name, table_data["table_name"])
+ self.assertEqual(model.database_id, table_data["database"])
+ db.session.delete(model)
+ db.session.commit()
+
+ def test_create_dataset_item_gamma(self):
+ """
+ Dataset API: Test create dataset item gamma
+ """
+ self.login(username="gamma")
+ example_db = get_example_database()
+ table_data = {
+ "database": example_db.id,
+ "schema": "",
+ "table_name": "ab_permission",
+ }
+ uri = "api/v1/dataset/"
+ rv = self.client.post(uri, json=table_data)
+ self.assertEqual(rv.status_code, 401)
+
+ def test_create_dataset_item_owner(self):
+ """
+ Dataset API: Test create item owner
+ """
+ example_db = get_example_database()
+ self.login(username="alpha")
+ admin = self.get_user("admin")
+ alpha = self.get_user("alpha")
+
+ table_data = {
+ "database": example_db.id,
+ "schema": "",
+ "table_name": "ab_permission",
+ "owners": [admin.id],
+ }
+ uri = "api/v1/dataset/"
+ rv = self.client.post(uri, json=table_data)
+ self.assertEqual(rv.status_code, 201)
+ data = json.loads(rv.data.decode("utf-8"))
+ model = db.session.query(SqlaTable).get(data.get("id"))
+ self.assertIn(admin, model.owners)
+ self.assertIn(alpha, model.owners)
+ db.session.delete(model)
+ db.session.commit()
+
+ def test_create_dataset_item_owners_invalid(self):
+ """
+ Dataset API: Test create dataset item owner invalid
+ """
+ admin = self.get_user("admin")
+ example_db = get_example_database()
+ self.login(username="admin")
+ table_data = {
+ "database": example_db.id,
+ "schema": "",
+ "table_name": "ab_permission",
+ "owners": [admin.id, 1000],
+ }
+ uri = f"api/v1/dataset/"
+ rv = self.client.post(uri, json=table_data)
+ self.assertEqual(rv.status_code, 422)
+ data = json.loads(rv.data.decode("utf-8"))
+ expected_result = {"message": {"owners": ["Owners are invalid"]}}
+ self.assertEqual(data, expected_result)
+
+ def test_create_dataset_validate_uniqueness(self):
+ """
+ Dataset API: Test create dataset validate table uniqueness
+ """
+ example_db = get_example_database()
+ self.login(username="admin")
+ table_data = {
+ "database": example_db.id,
+ "schema": "",
+ "table_name": "birth_names",
+ }
+ uri = "api/v1/dataset/"
+ rv = self.client.post(uri, json=table_data)
+ self.assertEqual(rv.status_code, 422)
+ data = json.loads(rv.data.decode("utf-8"))
+ self.assertEqual(
+ data, {"message": {"table_name": ["Datasource birth_names already exists"]}}
+ )
+
+ def test_create_dataset_validate_database(self):
+ """
+ Dataset API: Test create dataset validate database exists
+ """
+ self.login(username="admin")
+ table_data = {"database": 1000, "schema": "", "table_name": "birth_names"}
+ uri = "api/v1/dataset/"
+ rv = self.client.post(uri, json=table_data)
+ self.assertEqual(rv.status_code, 422)
+ data = json.loads(rv.data.decode("utf-8"))
+ self.assertEqual(data, {"message": {"database": ["Database does not exist"]}})
+
+ def test_create_dataset_validate_tables_exists(self):
+ """
+ Dataset API: Test create dataset validate table exists
+ """
+ example_db = get_example_database()
+ self.login(username="admin")
+ table_data = {
+ "database": example_db.id,
+ "schema": "",
+ "table_name": "does_not_exist",
+ }
+ uri = "api/v1/dataset/"
+ rv = self.client.post(uri, json=table_data)
+ self.assertEqual(rv.status_code, 422)
+
+ @patch("superset.datasets.dao.DatasetDAO.create")
Review comment:
I think this test might be implemented at the wrong level - we're patching an item 2 levels deep (view -> command -> DAO) - it might be preferable to have 2 tests, one checking that the Command throws the proper error in the situation, and another ensuring that the error is handled correctly.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] aboganas commented on issue #9129: [datasets]
new, API using command pattern
Posted by GitBox <gi...@apache.org>.
aboganas commented on issue #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#issuecomment-586851072
This is code poetry. I enjoyed reading every line of it.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io commented on issue #9129:
[datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#issuecomment-594213989
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9129?src=pr&el=h1) Report
> Merging [#9129](https://codecov.io/gh/apache/incubator-superset/pull/9129?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/30b7064e309c8f6d080e1c81ab3684b4a949a496?src=pr&el=desc) will **decrease** coverage by `0.16%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9129?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9129 +/- ##
==========================================
- Coverage 59.1% 58.93% -0.17%
==========================================
Files 372 373 +1
Lines 11922 12014 +92
Branches 2919 2945 +26
==========================================
+ Hits 7046 7080 +34
- Misses 4694 4755 +61
+ Partials 182 179 -3
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9129?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/components/Loading.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9129/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTG9hZGluZy5qc3g=) | `55.55% <0%> (-44.45%)` | :arrow_down: |
| [...frontend/src/views/dashboardList/DashboardList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9129/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2Rhc2hib2FyZExpc3QvRGFzaGJvYXJkTGlzdC50c3g=) | `59.34% <0%> (-13.8%)` | :arrow_down: |
| [...uperset-frontend/src/views/chartList/ChartList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9129/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2NoYXJ0TGlzdC9DaGFydExpc3QudHN4) | `63.39% <0%> (-11.36%)` | :arrow_down: |
| [...rontend/src/SqlLab/components/ShareSqlLabQuery.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9129/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NoYXJlU3FsTGFiUXVlcnkuanN4) | `93.54% <0%> (-6.46%)` | :arrow_down: |
| [...rset-frontend/src/components/ListView/ListView.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9129/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvTGlzdFZpZXcudHN4) | `86.02% <0%> (-6.19%)` | :arrow_down: |
| [superset-frontend/src/components/ListView/utils.ts](https://codecov.io/gh/apache/incubator-superset/pull/9129/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvdXRpbHMudHM=) | `94.44% <0%> (-4.02%)` | :arrow_down: |
| [...ontend/src/components/ListView/TableCollection.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9129/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvVGFibGVDb2xsZWN0aW9uLnRzeA==) | `90% <0%> (-2.6%)` | :arrow_down: |
| [...erset-frontend/src/SqlLab/components/ResultSet.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9129/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC5qc3g=) | `75.7% <0%> (-1.86%)` | :arrow_down: |
| [...et-frontend/src/dashboard/components/Dashboard.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9129/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0Rhc2hib2FyZC5qc3g=) | `84.37% <0%> (-0.63%)` | :arrow_down: |
| [superset-frontend/src/setup/setupClient.js](https://codecov.io/gh/apache/incubator-superset/pull/9129/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwQ2xpZW50Lmpz) | `0% <0%> (ø)` | :arrow_up: |
| ... and [11 more](https://codecov.io/gh/apache/incubator-superset/pull/9129/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9129?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9129?src=pr&el=footer). Last update [30b7064...4978689](https://codecov.io/gh/apache/incubator-superset/pull/9129?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on issue #9129: [datasets]
new, API using command pattern
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on issue #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#issuecomment-586287905
@willbarrett,
Agree, it's looking far better, I also see great potential for the use of commands outside of the REST API. Very promising idea! +1
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r379117969
##########
File path: superset/datasets/commands/update.py
##########
@@ -0,0 +1,83 @@
+# 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.
+from flask_appbuilder.security.sqla.models import User
+from marshmallow import UnmarshalResult, ValidationError
+
+from superset.commands.base import BaseCommand, CommandValidateReturn
+from superset.datasets.commands.base import populate_owners
+from superset.datasets.commands.exceptions import (
+ DatabaseChangeValidationError,
+ DatasetExistsValidationError,
+ DatasetInvalidError,
+ DatasetNotFoundError,
+ DatasetUpdateFailedError,
+)
+from superset.datasets.dao import DatasetDAO
+from superset.views.base import check_ownership
+
+
+class UpdateDatasetCommand(BaseCommand):
+ def __init__(self, user: User, model_id: int, unmarshal: UnmarshalResult):
+ self._actor = user
+ self._model_id = model_id
+ self._properties = unmarshal.data.copy()
+ self._errors = unmarshal.errors
+ self._model = None
+
+ def run(self):
+ self._model = DatasetDAO.find_by_id(self._model_id)
+ if not self._model:
+ raise DatasetNotFoundError()
+ valid, exceptions = self.validate()
+ if not valid:
+ for exception in exceptions:
+ self._errors.update(exception.normalized_messages())
+ raise DatasetInvalidError()
+ dataset = DatasetDAO.update(self._model, self._properties)
+
+ if not dataset:
+ raise DatasetUpdateFailedError()
+ return dataset
+
+ def validate(self) -> CommandValidateReturn:
+ is_valid, exceptions = super().validate()
+ if not self._model:
Review comment:
This is an interesting special case. We're checking for model presence in the `run` method of the command, and again here. I'd say the model lookup should either move inside of the `validate` call and raise, or this should return a meaningful error message, or we should keep the check in `run` and remove it from here. I'm not sure which one, but returning `False` with an empty array does not give the caller any meaningful information.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r379114268
##########
File path: superset/datasets/commands/delete.py
##########
@@ -0,0 +1,49 @@
+# 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.
+
+from flask_appbuilder.security.sqla.models import User
+
+from superset.commands.base import BaseCommand, CommandValidateReturn
+from superset.datasets.commands.exceptions import (
+ DatasetDeleteFailedError,
+ DatasetNotFoundError,
+)
+from superset.datasets.dao import DatasetDAO
+from superset.views.base import check_ownership
+
+
+class DeleteDatasetCommand(BaseCommand):
+ def __init__(self, user: User, model_id: int):
+ self._actor = user
+ self._model_id = model_id
+ self._model = None
+
+ def run(self):
+ self._model = DatasetDAO.find_by_id(self._model_id)
+ if not self._model:
+ raise DatasetNotFoundError()
+ self.validate()
+ dataset = DatasetDAO.delete(self._model)
+
+ if not dataset:
+ raise DatasetDeleteFailedError()
+ return dataset
+
+ def validate(self) -> CommandValidateReturn:
+ is_valid, exceptions = super().validate()
+ check_ownership(self._model)
Review comment:
I assume this will raise on failure? Should we transform that error into an error message and return false? We're mixing paradigms for control flow 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r379430544
##########
File path: superset/datasets/commands/delete.py
##########
@@ -0,0 +1,49 @@
+# 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.
+
+from flask_appbuilder.security.sqla.models import User
+
+from superset.commands.base import BaseCommand, CommandValidateReturn
+from superset.datasets.commands.exceptions import (
+ DatasetDeleteFailedError,
+ DatasetNotFoundError,
+)
+from superset.datasets.dao import DatasetDAO
+from superset.views.base import check_ownership
+
+
+class DeleteDatasetCommand(BaseCommand):
+ def __init__(self, user: User, model_id: int):
+ self._actor = user
+ self._model_id = model_id
+ self._model = None
+
+ def run(self):
+ self._model = DatasetDAO.find_by_id(self._model_id)
+ if not self._model:
+ raise DatasetNotFoundError()
+ self.validate()
+ dataset = DatasetDAO.delete(self._model)
+
+ if not dataset:
+ raise DatasetDeleteFailedError()
+ return dataset
+
+ def validate(self) -> CommandValidateReturn:
+ is_valid, exceptions = super().validate()
+ check_ownership(self._model)
Review comment:
Changed it. But don't want to change `check_ownership` just yet, it's being used on lot's of places
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r379122658
##########
File path: superset/commands/base.py
##########
@@ -0,0 +1,44 @@
+# 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.
+from abc import ABC, abstractmethod
+from typing import List, Tuple
+
+from marshmallow.validate import ValidationError
+
+CommandValidateReturn = Tuple[bool, List[ValidationError]]
+
+
+class BaseCommand(ABC):
+ """
+ Base class for all Command like Superset Logic objects
+ """
+
+ @abstractmethod
+ def run(self):
+ """
+ Run executes the command
+ :return:
+ """
+ pass
+
+ @abstractmethod
+ def validate(self) -> CommandValidateReturn:
Review comment:
I like the idea of making `validate` an explicit external interface. I can see this functionality being useful down the line.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r379412720
##########
File path: superset/commands/base.py
##########
@@ -0,0 +1,44 @@
+# 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.
+from abc import ABC, abstractmethod
+from typing import List, Tuple
+
+from marshmallow.validate import ValidationError
+
+CommandValidateReturn = Tuple[bool, List[ValidationError]]
+
+
+class BaseCommand(ABC):
+ """
+ Base class for all Command like Superset Logic objects
+ """
+
+ @abstractmethod
+ def run(self):
+ """
+ Run executes the command
+ :return:
+ """
+ pass
+
+ @abstractmethod
+ def validate(self) -> CommandValidateReturn:
Review comment:
That's the idea, not totally happy with the name, since the validates and fetches/populates data. Maybe `fetch_validate`, thoughts?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r387822975
##########
File path: superset/commands/base.py
##########
@@ -0,0 +1,39 @@
+# 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.
+from abc import ABC, abstractmethod
+
+
+class BaseCommand(ABC):
+ """
+ Base class for all Command like Superset Logic objects
+ """
+
+ @abstractmethod
+ def run(self):
Review comment:
I disagree. I'd like for us to keep `run` abstract and not make assumptions about the resulting command implementation. So far commands have wrapped single database operations, but there's no guarantee that this will always be the case. Commands may not interact with the metadata database at all, or they may require multiple transactions. Not all commands may need a validate() step. I do not want the base command to be prescriptive.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda commented on a change in pull
request #9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r386543605
##########
File path: superset/datasets/commands/create.py
##########
@@ -0,0 +1,82 @@
+# 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.
+from flask_appbuilder.security.sqla.models import User
+from marshmallow import UnmarshalResult, ValidationError
+
+from superset.commands.base import BaseCommand, CommandValidateReturn
+from superset.datasets.commands.base import populate_owners
+from superset.datasets.commands.exceptions import (
+ DatabaseNotFoundValidationError,
+ DatasetCreateFailedError,
+ DatasetExistsValidationError,
+ DatasetInvalidError,
+ TableNotFoundValidationError,
+)
+from superset.datasets.dao import DatasetDAO
+
+
+class CreateDatasetCommand(BaseCommand):
+ def __init__(self, user: User, unmarshal: UnmarshalResult):
+ self._actor = user
+ self._properties = unmarshal.data.copy()
+ self._errors = unmarshal.errors
+
+ def run(self):
+ is_valid, exceptions = self.validate()
+ if not is_valid:
+ for exception in exceptions:
+ self._errors.update(exception.normalized_messages())
+ raise DatasetInvalidError()
+
+ dataset = DatasetDAO.create(self._properties)
+
+ if not dataset:
+ raise DatasetCreateFailedError()
+ return dataset
+
+ def validate(self) -> CommandValidateReturn:
+ is_valid, exceptions = True, list()
+ database_id = self._properties["database"]
+ table_name = self._properties["table_name"]
+ schema = self._properties.get("schema", "")
+
+ # Validate uniqueness
+ if not DatasetDAO.validate_uniqueness(database_id, table_name):
Review comment:
DAO stuff should be a little more generic here. Instead, just have something like `dataset_dao.find_by_tablename()` which then returns a dataset. In the command, we would check for a result, and raise if a dataset already exists. This allows the DAO to be a bit more low-level and pulls "business logic" 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r379425525
##########
File path: tests/dataset_api_tests.py
##########
@@ -0,0 +1,445 @@
+# 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.
+"""Unit tests for Superset"""
+import json
+from typing import List
+from unittest.mock import patch
+
+import prison
+
+from superset import db, security_manager
+from superset.connectors.sqla.models import SqlaTable
+from superset.models.core import Database
+from superset.utils.core import get_example_database
+
+from .base_tests import SupersetTestCase
+
+
+class DatasetApiTests(SupersetTestCase):
+ @staticmethod
+ def insert_dataset(
+ table_name: str, schema: str, owners: List[int], database: Database
+ ) -> SqlaTable:
+ obj_owners = list()
+ for owner in owners:
+ user = db.session.query(security_manager.user_model).get(owner)
+ obj_owners.append(user)
+ table = SqlaTable(
+ table_name=table_name, schema=schema, owners=obj_owners, database=database
+ )
+ db.session.add(table)
+ db.session.commit()
+ return table
+
+ def test_get_dataset_list(self):
+ """
+ Dataset API: Test get dataset list
+ """
+ example_db = get_example_database()
+ self.login(username="admin")
+ arguments = {
+ "filters": [
+ {"col": "database", "opr": "rel_o_m", "value": f"{example_db.id}"},
+ {"col": "table_name", "opr": "eq", "value": f"birth_names"},
+ ]
+ }
+ uri = f"api/v1/dataset/?q={prison.dumps(arguments)}"
+ rv = self.client.get(uri)
+ self.assertEqual(rv.status_code, 200)
+ response = json.loads(rv.data.decode("utf-8"))
+ self.assertEqual(response["count"], 1)
+ expected_columns = [
+ "changed_by",
+ "changed_on",
+ "database_name",
+ "schema",
+ "table_name",
+ ]
+ self.assertEqual(sorted(list(response["result"][0].keys())), expected_columns)
+
+ def test_get_dataset_list_gamma(self):
+ """
+ Dataset API: Test get dataset list gamma
+ """
+ example_db = get_example_database()
+ self.login(username="gamma")
+ uri = "api/v1/dataset/"
+ rv = self.client.get(uri)
+ self.assertEqual(rv.status_code, 200)
+ response = json.loads(rv.data.decode("utf-8"))
+ self.assertEqual(response["result"], [])
+
+ def test_get_dataset_related_database_gamma(self):
+ """
+ Dataset API: Test get dataset related databases gamma
+ """
+ example_db = get_example_database()
+ self.login(username="gamma")
+ uri = "api/v1/dataset/related/database"
+ rv = self.client.get(uri)
+ self.assertEqual(rv.status_code, 200)
+ response = json.loads(rv.data.decode("utf-8"))
+ self.assertEqual(response["count"], 0)
+ self.assertEqual(response["result"], [])
+
+ def test_get_dataset_item(self):
+ """
+ Dataset API: Test get dataset item
+ """
+ example_db = get_example_database()
+ table = (
+ db.session.query(SqlaTable)
+ .filter_by(database=example_db, table_name="birth_names")
+ .one()
+ )
+ self.login(username="admin")
+ uri = f"api/v1/dataset/{table.id}"
+ rv = self.client.get(uri)
+ self.assertEqual(rv.status_code, 200)
+ response = json.loads(rv.data.decode("utf-8"))
+ expected_result = {
+ "cache_timeout": None,
+ "database": {"database_name": "examples", "id": 1},
+ "default_endpoint": None,
+ "description": None,
+ "fetch_values_predicate": None,
+ "filter_select_enabled": True,
+ "is_sqllab_view": False,
+ "main_dttm_col": "ds",
+ "offset": 0,
+ "owners": [],
+ "schema": None,
+ "sql": None,
+ "table_name": "birth_names",
+ "template_params": None,
+ }
+ self.assertEqual(response["result"], expected_result)
+
+ def test_get_dataset_info(self):
+ """
+ Dataset API: Test get dataset info
+ """
+ self.login(username="admin")
+ uri = "api/v1/dataset/_info"
+ rv = self.client.get(uri)
+ self.assertEqual(rv.status_code, 200)
+
+ def test_create_dataset_item(self):
+ """
+ Dataset API: Test create dataset item
+ """
+ example_db = get_example_database()
+ self.login(username="admin")
+ table_data = {
+ "database": example_db.id,
+ "schema": "",
+ "table_name": "ab_permission",
+ }
+ uri = "api/v1/dataset/"
+ rv = self.client.post(uri, json=table_data)
+ self.assertEqual(rv.status_code, 201)
+ data = json.loads(rv.data.decode("utf-8"))
+ model = db.session.query(SqlaTable).get(data.get("id"))
+ self.assertEqual(model.table_name, table_data["table_name"])
+ self.assertEqual(model.database_id, table_data["database"])
+ db.session.delete(model)
+ db.session.commit()
+
+ def test_create_dataset_item_gamma(self):
+ """
+ Dataset API: Test create dataset item gamma
+ """
+ self.login(username="gamma")
+ example_db = get_example_database()
+ table_data = {
+ "database": example_db.id,
+ "schema": "",
+ "table_name": "ab_permission",
+ }
+ uri = "api/v1/dataset/"
+ rv = self.client.post(uri, json=table_data)
+ self.assertEqual(rv.status_code, 401)
+
+ def test_create_dataset_item_owner(self):
+ """
+ Dataset API: Test create item owner
+ """
+ example_db = get_example_database()
+ self.login(username="alpha")
+ admin = self.get_user("admin")
+ alpha = self.get_user("alpha")
+
+ table_data = {
+ "database": example_db.id,
+ "schema": "",
+ "table_name": "ab_permission",
+ "owners": [admin.id],
+ }
+ uri = "api/v1/dataset/"
+ rv = self.client.post(uri, json=table_data)
+ self.assertEqual(rv.status_code, 201)
+ data = json.loads(rv.data.decode("utf-8"))
+ model = db.session.query(SqlaTable).get(data.get("id"))
+ self.assertIn(admin, model.owners)
+ self.assertIn(alpha, model.owners)
+ db.session.delete(model)
+ db.session.commit()
+
+ def test_create_dataset_item_owners_invalid(self):
+ """
+ Dataset API: Test create dataset item owner invalid
+ """
+ admin = self.get_user("admin")
+ example_db = get_example_database()
+ self.login(username="admin")
+ table_data = {
+ "database": example_db.id,
+ "schema": "",
+ "table_name": "ab_permission",
+ "owners": [admin.id, 1000],
+ }
+ uri = f"api/v1/dataset/"
+ rv = self.client.post(uri, json=table_data)
+ self.assertEqual(rv.status_code, 422)
+ data = json.loads(rv.data.decode("utf-8"))
+ expected_result = {"message": {"owners": ["Owners are invalid"]}}
+ self.assertEqual(data, expected_result)
+
+ def test_create_dataset_validate_uniqueness(self):
+ """
+ Dataset API: Test create dataset validate table uniqueness
+ """
+ example_db = get_example_database()
+ self.login(username="admin")
+ table_data = {
+ "database": example_db.id,
+ "schema": "",
+ "table_name": "birth_names",
+ }
+ uri = "api/v1/dataset/"
+ rv = self.client.post(uri, json=table_data)
+ self.assertEqual(rv.status_code, 422)
+ data = json.loads(rv.data.decode("utf-8"))
+ self.assertEqual(
+ data, {"message": {"table_name": ["Datasource birth_names already exists"]}}
+ )
+
+ def test_create_dataset_validate_database(self):
+ """
+ Dataset API: Test create dataset validate database exists
+ """
+ self.login(username="admin")
+ table_data = {"database": 1000, "schema": "", "table_name": "birth_names"}
+ uri = "api/v1/dataset/"
+ rv = self.client.post(uri, json=table_data)
+ self.assertEqual(rv.status_code, 422)
+ data = json.loads(rv.data.decode("utf-8"))
+ self.assertEqual(data, {"message": {"database": ["Database does not exist"]}})
+
+ def test_create_dataset_validate_tables_exists(self):
+ """
+ Dataset API: Test create dataset validate table exists
+ """
+ example_db = get_example_database()
+ self.login(username="admin")
+ table_data = {
+ "database": example_db.id,
+ "schema": "",
+ "table_name": "does_not_exist",
+ }
+ uri = "api/v1/dataset/"
+ rv = self.client.post(uri, json=table_data)
+ self.assertEqual(rv.status_code, 422)
+
+ @patch("superset.datasets.dao.DatasetDAO.create")
Review comment:
I'm ensuring that low level (like backend) errors are handled correctly by the API, and get really high coverage levels.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda commented on a change in pull
request #9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r387810919
##########
File path: superset/datasets/commands/exceptions.py
##########
@@ -0,0 +1,103 @@
+# 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.
+from flask_babel import lazy_gettext as _
+from marshmallow.validate import ValidationError
+
+from superset.commands.exceptions import (
+ CommandException,
+ CommandInvalidError,
+ CreateFailedError,
+ DeleteFailedError,
+ ForbiddenError,
+ UpdateFailedError,
+)
+from superset.views.base import get_datasource_exist_error_msg
+
+
+class DatabaseNotFoundValidationError(ValidationError):
+ """
+ Marshmallow validation error for database does not exist
+ """
+
+ def __init__(self):
+ super().__init__(_("Database does not exist"), field_names=["database"])
+
+
+class DatabaseChangeValidationError(ValidationError):
+ """
+ Marshmallow validation error database changes are not allowed on update
+ """
+
+ def __init__(self):
+ super().__init__(_("Database not allowed to change"), field_names=["database"])
+
+
+class DatasetExistsValidationError(ValidationError):
+ """
+ Marshmallow validation error for dataset already exists
Review comment:
These aren't Marshmallow exceptions :)
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda commented on a change in pull
request #9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r387164922
##########
File path: superset/datasets/dao.py
##########
@@ -0,0 +1,119 @@
+# 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 Dict, Optional
+
+from flask import current_app
+from flask_appbuilder.models.sqla.interface import SQLAInterface
+from sqlalchemy.exc import SQLAlchemyError
+
+from superset import db
+from superset.connectors.sqla.models import SqlaTable
+from superset.models.core import Database
+from superset.views.base import DatasourceFilter
+
+logger = logging.getLogger(__name__)
+
+
+class DatasetDAO:
+ @staticmethod
+ def get_owner_by_id(owner_id: int) -> Optional[object]:
+ return (
+ db.session.query(current_app.appbuilder.sm.user_model)
+ .filter_by(id=owner_id)
+ .one_or_none()
+ )
+
+ @staticmethod
+ def get_database_by_id(database_id) -> Optional[Database]:
+ try:
+ return db.session.query(Database).filter_by(id=database_id).one_or_none()
+ except SQLAlchemyError as e: # pragma: no cover
+ logger.error(f"Could not get database by id: {e}")
+ return None
+
+ @staticmethod
+ def validate_table_exists(database: Database, table_name: str, schema: str) -> bool:
+ try:
+ database.get_table(table_name, schema=schema)
+ return True
+ except SQLAlchemyError as e: # pragma: no cover
+ logger.error(f"Got an error {e} validating table: {table_name}")
+ return False
+
+ @staticmethod
+ def validate_uniqueness(database_id: int, name: str) -> bool:
+ dataset_query = db.session.query(SqlaTable).filter(
+ SqlaTable.table_name == name, SqlaTable.database_id == database_id
+ )
+ return not db.session.query(dataset_query.exists()).scalar()
+
+ @staticmethod
+ def validate_update_uniqueness(
+ database_id: int, dataset_id: int, name: str
+ ) -> bool:
+ dataset_query = db.session.query(SqlaTable).filter(
+ SqlaTable.table_name == name,
+ SqlaTable.database_id == database_id,
+ SqlaTable.id != dataset_id,
+ )
+ return not db.session.query(dataset_query.exists()).scalar()
+
+ @staticmethod
+ def find_by_id(model_id: int) -> SqlaTable:
+ data_model = SQLAInterface(SqlaTable, db.session)
+ query = db.session.query(SqlaTable)
+ query = DatasourceFilter("id", data_model).apply(query, None)
+ return query.filter_by(id=model_id).one_or_none()
+
+ @staticmethod
+ def create(properties: Dict) -> Optional[SqlaTable]:
+ model = SqlaTable()
+ for key, value in properties.items():
+ setattr(model, key, value)
+ try:
+ db.session.add(model)
+ db.session.commit()
+ except SQLAlchemyError as e: # pragma: no cover
+ logger.error(f"Failed to create dataset: {e}")
+ db.session.rollback()
+ return None
+ return model
+
+ @staticmethod
+ def update(model: SqlaTable, properties: Dict) -> Optional[SqlaTable]:
+ for key, value in properties.items():
+ setattr(model, key, value)
+ try:
+ db.session.merge(model)
+ db.session.commit()
Review comment:
That works. I would also try and solve it as an aspect, maybe with a quick decorator? Don't want to push this off too much - we can always add the decorator later...
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda commented on a change in pull
request #9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r387810585
##########
File path: superset/datasets/dao.py
##########
@@ -0,0 +1,122 @@
+# 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 Dict, Optional
+
+from flask import current_app
+from flask_appbuilder.models.sqla.interface import SQLAInterface
+from sqlalchemy.exc import SQLAlchemyError
+
+from superset.connectors.sqla.models import SqlaTable
+from superset.extensions import db
+from superset.models.core import Database
+from superset.views.base import DatasourceFilter
+
+logger = logging.getLogger(__name__)
+
+
+class DatasetDAO:
+ @staticmethod
+ def get_owner_by_id(owner_id: int) -> Optional[object]:
+ return (
+ db.session.query(current_app.appbuilder.sm.user_model)
+ .filter_by(id=owner_id)
+ .one_or_none()
+ )
+
+ @staticmethod
+ def get_database_by_id(database_id) -> Optional[Database]:
+ try:
+ return db.session.query(Database).filter_by(id=database_id).one_or_none()
+ except SQLAlchemyError as e: # pragma: no cover
+ logger.error(f"Could not get database by id: {e}")
+ return None
+
+ @staticmethod
+ def validate_table_exists(database: Database, table_name: str, schema: str) -> bool:
+ try:
+ database.get_table(table_name, schema=schema)
+ return True
+ except SQLAlchemyError as e: # pragma: no cover
+ logger.error(f"Got an error {e} validating table: {table_name}")
+ return False
+
+ @staticmethod
+ def validate_uniqueness(database_id: int, name: str) -> bool:
+ dataset_query = db.session.query(SqlaTable).filter(
+ SqlaTable.table_name == name, SqlaTable.database_id == database_id
+ )
+ return not db.session.query(dataset_query.exists()).scalar()
+
+ @staticmethod
+ def validate_update_uniqueness(
+ database_id: int, dataset_id: int, name: str
+ ) -> bool:
+ dataset_query = db.session.query(SqlaTable).filter(
+ SqlaTable.table_name == name,
+ SqlaTable.database_id == database_id,
+ SqlaTable.id != dataset_id,
+ )
+ return not db.session.query(dataset_query.exists()).scalar()
+
+ @staticmethod
+ def find_by_id(model_id: int) -> SqlaTable:
+ data_model = SQLAInterface(SqlaTable, db.session)
+ query = db.session.query(SqlaTable)
+ query = DatasourceFilter("id", data_model).apply(query, None)
+ return query.filter_by(id=model_id).one_or_none()
+
+ @staticmethod
+ def create(properties: Dict, commit=True) -> Optional[SqlaTable]:
+ model = SqlaTable()
+ for key, value in properties.items():
+ setattr(model, key, value)
+ try:
+ db.session.add(model)
+ if commit:
+ db.session.commit()
+ except SQLAlchemyError as e: # pragma: no cover
+ logger.error(f"Failed to create dataset: {e}")
Review comment:
One thing here - we should not be catching/logging here. A better pattern is to do one of two things:
1. Catch -> rollback -> re-raise
2. Catch -> rollback -> re-raise a re-wrapped ex that is of a "superset data ex" type which we can then handle upstream.
Also, when logging exceptions, it's better to just use `logger.exception()`
The reason for this is that simply returning "None" doesn't allow the caller to do anything about the error case. Instead, something gets written to the log (which is good), but then the caller's control flow is not influenced in any way, not to mention, returning `None` here is error prone, as the caller needs to check the return for truthiness.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r379418836
##########
File path: superset/datasets/dao.py
##########
@@ -0,0 +1,119 @@
+# 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 Dict, Optional
+
+from flask import current_app
+from flask_appbuilder.models.sqla.interface import SQLAInterface
+from sqlalchemy.exc import SQLAlchemyError
+
+from superset import db
+from superset.connectors.sqla.models import SqlaTable
+from superset.models.core import Database
+from superset.views.base import DatasourceFilter
+
+logger = logging.getLogger(__name__)
+
+
+class DatasetDAO:
+ @staticmethod
+ def get_owner_by_id(owner_id: int) -> Optional[object]:
+ return (
+ db.session.query(current_app.appbuilder.sm.user_model)
+ .filter_by(id=owner_id)
+ .one_or_none()
+ )
+
+ @staticmethod
+ def get_database_by_id(database_id) -> Optional[Database]:
+ try:
+ return db.session.query(Database).filter_by(id=database_id).one_or_none()
+ except SQLAlchemyError as e: # pragma: no cover
+ logger.error(f"Could not get database by id: {e}")
+ return None
+
+ @staticmethod
+ def validate_table_exists(database: Database, table_name: str, schema: str) -> bool:
+ try:
+ database.get_table(table_name, schema=schema)
Review comment:
It would, but this is not SQLAlchemy query, it's going to fetch an actual SQLAlchemy table representation, and fail if the table does not exist on a engine
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda commented on a change in pull
request #9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r386545236
##########
File path: superset/datasets/dao.py
##########
@@ -0,0 +1,119 @@
+# 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 Dict, Optional
+
+from flask import current_app
+from flask_appbuilder.models.sqla.interface import SQLAInterface
+from sqlalchemy.exc import SQLAlchemyError
+
+from superset import db
Review comment:
This should be coming from `extensions`
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r387849449
##########
File path: superset/commands/base.py
##########
@@ -0,0 +1,39 @@
+# 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.
+from abc import ABC, abstractmethod
+
+
+class BaseCommand(ABC):
+ """
+ Base class for all Command like Superset Logic objects
+ """
+
+ @abstractmethod
+ def run(self):
Review comment:
I'm agnostic about `validate()` as a top-level entrypoint or not. Either way is fine by me.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r379412720
##########
File path: superset/commands/base.py
##########
@@ -0,0 +1,44 @@
+# 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.
+from abc import ABC, abstractmethod
+from typing import List, Tuple
+
+from marshmallow.validate import ValidationError
+
+CommandValidateReturn = Tuple[bool, List[ValidationError]]
+
+
+class BaseCommand(ABC):
+ """
+ Base class for all Command like Superset Logic objects
+ """
+
+ @abstractmethod
+ def run(self):
+ """
+ Run executes the command
+ :return:
+ """
+ pass
+
+ @abstractmethod
+ def validate(self) -> CommandValidateReturn:
Review comment:
That's the idea, not totally happy with the name, since it validates and fetches/populates data. Maybe `fetch_validate`, thoughts?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r379422828
##########
File path: tests/dataset_api_tests.py
##########
@@ -0,0 +1,445 @@
+# 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.
+"""Unit tests for Superset"""
+import json
+from typing import List
+from unittest.mock import patch
+
+import prison
+
+from superset import db, security_manager
+from superset.connectors.sqla.models import SqlaTable
+from superset.models.core import Database
+from superset.utils.core import get_example_database
+
+from .base_tests import SupersetTestCase
+
+
+class DatasetApiTests(SupersetTestCase):
Review comment:
Seems interesting, let's talk about it
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r387163227
##########
File path: superset/datasets/dao.py
##########
@@ -0,0 +1,119 @@
+# 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 Dict, Optional
+
+from flask import current_app
+from flask_appbuilder.models.sqla.interface import SQLAInterface
+from sqlalchemy.exc import SQLAlchemyError
+
+from superset import db
+from superset.connectors.sqla.models import SqlaTable
+from superset.models.core import Database
+from superset.views.base import DatasourceFilter
+
+logger = logging.getLogger(__name__)
+
+
+class DatasetDAO:
+ @staticmethod
+ def get_owner_by_id(owner_id: int) -> Optional[object]:
+ return (
+ db.session.query(current_app.appbuilder.sm.user_model)
+ .filter_by(id=owner_id)
+ .one_or_none()
+ )
+
+ @staticmethod
+ def get_database_by_id(database_id) -> Optional[Database]:
+ try:
+ return db.session.query(Database).filter_by(id=database_id).one_or_none()
+ except SQLAlchemyError as e: # pragma: no cover
+ logger.error(f"Could not get database by id: {e}")
+ return None
+
+ @staticmethod
+ def validate_table_exists(database: Database, table_name: str, schema: str) -> bool:
+ try:
+ database.get_table(table_name, schema=schema)
+ return True
+ except SQLAlchemyError as e: # pragma: no cover
+ logger.error(f"Got an error {e} validating table: {table_name}")
+ return False
+
+ @staticmethod
+ def validate_uniqueness(database_id: int, name: str) -> bool:
+ dataset_query = db.session.query(SqlaTable).filter(
+ SqlaTable.table_name == name, SqlaTable.database_id == database_id
+ )
+ return not db.session.query(dataset_query.exists()).scalar()
+
+ @staticmethod
+ def validate_update_uniqueness(
+ database_id: int, dataset_id: int, name: str
+ ) -> bool:
+ dataset_query = db.session.query(SqlaTable).filter(
+ SqlaTable.table_name == name,
+ SqlaTable.database_id == database_id,
+ SqlaTable.id != dataset_id,
+ )
+ return not db.session.query(dataset_query.exists()).scalar()
+
+ @staticmethod
+ def find_by_id(model_id: int) -> SqlaTable:
+ data_model = SQLAInterface(SqlaTable, db.session)
+ query = db.session.query(SqlaTable)
+ query = DatasourceFilter("id", data_model).apply(query, None)
+ return query.filter_by(id=model_id).one_or_none()
+
+ @staticmethod
+ def create(properties: Dict) -> Optional[SqlaTable]:
+ model = SqlaTable()
+ for key, value in properties.items():
+ setattr(model, key, value)
+ try:
+ db.session.add(model)
+ db.session.commit()
+ except SQLAlchemyError as e: # pragma: no cover
+ logger.error(f"Failed to create dataset: {e}")
+ db.session.rollback()
+ return None
+ return model
+
+ @staticmethod
+ def update(model: SqlaTable, properties: Dict) -> Optional[SqlaTable]:
+ for key, value in properties.items():
+ setattr(model, key, value)
+ try:
+ db.session.merge(model)
+ db.session.commit()
Review comment:
how about making an optional parameter for this? We can default to commit and on the cases (probably a minority) where we need complex transactions call it like `create(data, commit=False)` ?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda commented on a change in pull
request #9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r387839907
##########
File path: superset/commands/base.py
##########
@@ -0,0 +1,39 @@
+# 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.
+from abc import ABC, abstractmethod
+
+
+class BaseCommand(ABC):
+ """
+ Base class for all Command like Superset Logic objects
+ """
+
+ @abstractmethod
+ def run(self):
Review comment:
If they don't need a validate(), they can just not implement the abstract `validate()` from the base class, in which case validate() performs a no-op (which is currently the case). If we aren't calling `validate()` from outside the command, or from the base class, then there's no point in having it defined 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r379118740
##########
File path: superset/datasets/dao.py
##########
@@ -0,0 +1,119 @@
+# 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 Dict, Optional
+
+from flask import current_app
+from flask_appbuilder.models.sqla.interface import SQLAInterface
+from sqlalchemy.exc import SQLAlchemyError
+
+from superset import db
+from superset.connectors.sqla.models import SqlaTable
+from superset.models.core import Database
+from superset.views.base import DatasourceFilter
+
+logger = logging.getLogger(__name__)
+
+
+class DatasetDAO:
+ @staticmethod
+ def get_owner_by_id(owner_id: int) -> Optional[object]:
+ return (
+ db.session.query(current_app.appbuilder.sm.user_model)
+ .filter_by(id=owner_id)
+ .one_or_none()
+ )
+
+ @staticmethod
+ def get_database_by_id(database_id) -> Optional[Database]:
+ try:
+ return db.session.query(Database).filter_by(id=database_id).one_or_none()
+ except SQLAlchemyError as e: # pragma: no cover
+ logger.error(f"Could not get database by id: {e}")
+ return None
+
+ @staticmethod
+ def validate_table_exists(database: Database, table_name: str, schema: str) -> bool:
+ try:
+ database.get_table(table_name, schema=schema)
Review comment:
I think a `one_or_none` query here would be better - that would avoid the error handling.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r387161596
##########
File path: superset/datasets/commands/create.py
##########
@@ -0,0 +1,82 @@
+# 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.
+from flask_appbuilder.security.sqla.models import User
+from marshmallow import UnmarshalResult, ValidationError
+
+from superset.commands.base import BaseCommand, CommandValidateReturn
+from superset.datasets.commands.base import populate_owners
+from superset.datasets.commands.exceptions import (
+ DatabaseNotFoundValidationError,
+ DatasetCreateFailedError,
+ DatasetExistsValidationError,
+ DatasetInvalidError,
+ TableNotFoundValidationError,
+)
+from superset.datasets.dao import DatasetDAO
+
+
+class CreateDatasetCommand(BaseCommand):
+ def __init__(self, user: User, unmarshal: UnmarshalResult):
+ self._actor = user
+ self._properties = unmarshal.data.copy()
+ self._errors = unmarshal.errors
+
+ def run(self):
+ is_valid, exceptions = self.validate()
+ if not is_valid:
+ for exception in exceptions:
+ self._errors.update(exception.normalized_messages())
+ raise DatasetInvalidError()
+
+ dataset = DatasetDAO.create(self._properties)
+
+ if not dataset:
+ raise DatasetCreateFailedError()
+ return dataset
+
+ def validate(self) -> CommandValidateReturn:
+ is_valid, exceptions = True, list()
+ database_id = self._properties["database"]
+ table_name = self._properties["table_name"]
+ schema = self._properties.get("schema", "")
+
+ # Validate uniqueness
+ if not DatasetDAO.validate_uniqueness(database_id, table_name):
Review comment:
Makes sense, yet `validate_update_uniqueness` and `validate_uniqueness` make a lighter query since they are using `exists` and `scalar`.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda commented on a change in pull
request #9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r387813692
##########
File path: superset/commands/base.py
##########
@@ -0,0 +1,39 @@
+# 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.
+from abc import ABC, abstractmethod
+
+
+class BaseCommand(ABC):
+ """
+ Base class for all Command like Superset Logic objects
+ """
+
+ @abstractmethod
+ def run(self):
Review comment:
Slight tweak here:
1. Make `run()` non-abstract
2. In `run()`, call `validate()`
3. Define / implement a method named something like `do_run()` which is abstract, which subclasses will implement.
This ensures that validate() is always called, and allows us to place some boiler-plate logic in `run()` (think transaction handling)
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda commented on a change in pull
request #9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r386540449
##########
File path: superset/datasets/commands/create.py
##########
@@ -0,0 +1,82 @@
+# 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.
+from flask_appbuilder.security.sqla.models import User
+from marshmallow import UnmarshalResult, ValidationError
+
+from superset.commands.base import BaseCommand, CommandValidateReturn
+from superset.datasets.commands.base import populate_owners
+from superset.datasets.commands.exceptions import (
+ DatabaseNotFoundValidationError,
+ DatasetCreateFailedError,
+ DatasetExistsValidationError,
+ DatasetInvalidError,
+ TableNotFoundValidationError,
+)
+from superset.datasets.dao import DatasetDAO
+
+
+class CreateDatasetCommand(BaseCommand):
+ def __init__(self, user: User, unmarshal: UnmarshalResult):
+ self._actor = user
+ self._properties = unmarshal.data.copy()
+ self._errors = unmarshal.errors
+
+ def run(self):
+ is_valid, exceptions = self.validate()
+ if not is_valid:
+ for exception in exceptions:
+ self._errors.update(exception.normalized_messages())
+ raise DatasetInvalidError()
+
+ dataset = DatasetDAO.create(self._properties)
+
+ if not dataset:
+ raise DatasetCreateFailedError()
+ return dataset
+
+ def validate(self) -> CommandValidateReturn:
+ is_valid, exceptions = True, list()
Review comment:
do we need `is_valid` here? Can't we just rely on the emptiness of the `exceptions` list to imply success?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda commented on a change in pull
request #9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r386547819
##########
File path: superset/datasets/api.py
##########
@@ -0,0 +1,258 @@
+#
+# 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 flask import g, request, Response
+from flask_appbuilder.api import expose, protect, safe
+from flask_appbuilder.models.sqla.interface import SQLAInterface
+
+from superset.connectors.sqla.models import SqlaTable
+from superset.constants import RouteMethod
+from superset.datasets.commands.create import CreateDatasetCommand
+from superset.datasets.commands.delete import DeleteDatasetCommand
+from superset.datasets.commands.exceptions import (
+ DatasetCreateFailedError,
+ DatasetDeleteFailedError,
+ DatasetForbiddenError,
+ DatasetInvalidError,
+ DatasetNotFoundError,
+ DatasetUpdateFailedError,
+)
+from superset.datasets.commands.update import UpdateDatasetCommand
+from superset.datasets.schemas import DatasetPostSchema, DatasetPutSchema
+from superset.views.base import DatasourceFilter
+from superset.views.base_api import BaseOwnedModelRestApi
+from superset.views.database.filters import DatabaseFilter
+
+logger = logging.getLogger(__name__)
+
+
+class DatasetRestApi(BaseOwnedModelRestApi):
+ datamodel = SQLAInterface(SqlaTable)
+ base_filters = [["id", DatasourceFilter, lambda: []]]
+
+ resource_name = "dataset"
+ allow_browser_login = True
+
+ class_permission_name = "TableModelView"
+ include_route_methods = RouteMethod.REST_MODEL_VIEW_CRUD_SET | {RouteMethod.RELATED}
+
+ list_columns = [
+ "database_name",
+ "changed_by.username",
+ "changed_on",
+ "table_name",
+ "schema",
+ ]
+ show_columns = [
+ "database.database_name",
+ "database.id",
+ "table_name",
+ "sql",
+ "filter_select_enabled",
+ "fetch_values_predicate",
+ "schema",
+ "description",
+ "main_dttm_col",
+ "offset",
+ "default_endpoint",
+ "cache_timeout",
+ "is_sqllab_view",
+ "template_params",
+ "owners.id",
+ "owners.username",
+ ]
+ add_model_schema = DatasetPostSchema()
+ edit_model_schema = DatasetPutSchema()
+ add_columns = ["database", "schema", "table_name", "owners"]
+ edit_columns = [
+ "table_name",
+ "sql",
+ "filter_select_enabled",
+ "fetch_values_predicate",
+ "schema",
+ "description",
+ "main_dttm_col",
+ "offset",
+ "default_endpoint",
+ "cache_timeout",
+ "is_sqllab_view",
+ "template_params",
+ "owners",
+ ]
+ openapi_spec_tag = "Datasets"
+
+ filter_rel_fields_field = {"owners": "first_name", "database": "database_name"}
+ filter_rel_fields = {"database": [["id", DatabaseFilter, lambda: []]]}
+
+ @expose("/", methods=["POST"])
+ @protect()
+ @safe
+ def post(self) -> Response:
+ """Creates a new Dataset
+ ---
+ post:
+ description: >-
+ Create a new Dataset
+ requestBody:
+ description: Dataset schema
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/{{self.__class__.__name__}}.post'
+ responses:
+ 201:
+ description: Dataset added
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ id:
+ type: number
+ result:
+ $ref: '#/components/schemas/{{self.__class__.__name__}}.post'
+ 400:
+ $ref: '#/components/responses/400'
+ 401:
+ $ref: '#/components/responses/401'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ if not request.is_json:
+ return self.response_400(message="Request is not JSON")
+ item = self.add_model_schema.load(request.json)
+ # This validates custom Schema with custom validations
+ if item.errors:
+ return self.response_400(message=item.errors)
+ try:
+ new_model = CreateDatasetCommand(g.user, item).run()
+ return self.response(201, id=new_model.id, result=item.data)
+ except DatasetInvalidError:
+ return self.response_422(message=item.errors)
+ except DatasetCreateFailedError as e:
+ logger.error(f"Error creating model {self.__class__.__name__}: {e}")
+ return self.response_422(message=str(e))
+
+ @expose("/<pk>", methods=["PUT"])
+ @protect()
+ @safe
+ def put( # pylint: disable=too-many-return-statements, arguments-differ
+ self, pk: int
+ ) -> Response:
+ """Changes a Dataset
+ ---
+ put:
+ description: >-
+ Changes a Dataset
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ requestBody:
+ description: Dataset schema
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/{{self.__class__.__name__}}.put'
+ responses:
+ 200:
+ description: Dataset changed
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ id:
+ type: number
+ result:
+ $ref: '#/components/schemas/{{self.__class__.__name__}}.put'
+ 400:
+ $ref: '#/components/responses/400'
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ $ref: '#/components/responses/403'
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ if not request.is_json:
+ return self.response_400(message="Request is not JSON")
+ item = self.edit_model_schema.load(request.json)
+ # This validates custom Schema with custom validations
+ if item.errors:
+ return self.response_400(message=item.errors)
+ try:
+ changed_model = UpdateDatasetCommand(g.user, pk, item).run()
+ return self.response(200, id=changed_model.id, result=item.data)
+ except DatasetNotFoundError:
+ return self.response_404()
+ except DatasetForbiddenError:
+ return self.response_403()
+ except DatasetInvalidError:
+ return self.response_422(message=item.errors)
Review comment:
Rather than scabbing errors onto the item itself, why not just place the errors into the `InvalidError` being handled here. You can then just bubble that up and into the API response.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r379414508
##########
File path: superset/datasets/commands/delete.py
##########
@@ -0,0 +1,49 @@
+# 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.
+
+from flask_appbuilder.security.sqla.models import User
+
+from superset.commands.base import BaseCommand, CommandValidateReturn
+from superset.datasets.commands.exceptions import (
+ DatasetDeleteFailedError,
+ DatasetNotFoundError,
+)
+from superset.datasets.dao import DatasetDAO
+from superset.views.base import check_ownership
+
+
+class DeleteDatasetCommand(BaseCommand):
+ def __init__(self, user: User, model_id: int):
+ self._actor = user
+ self._model_id = model_id
+ self._model = None
+
+ def run(self):
+ self._model = DatasetDAO.find_by_id(self._model_id)
+ if not self._model:
+ raise DatasetNotFoundError()
+ self.validate()
Review comment:
True, it escapes the expected flow. Yet `validate` on delete will never return `False` since it has no data properties to validate, so it can only succeed or throw exceptions. Seems strange to process a condition that will never happen
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r387812429
##########
File path: superset/datasets/dao.py
##########
@@ -0,0 +1,119 @@
+# 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 Dict, Optional
+
+from flask import current_app
+from flask_appbuilder.models.sqla.interface import SQLAInterface
+from sqlalchemy.exc import SQLAlchemyError
+
+from superset import db
+from superset.connectors.sqla.models import SqlaTable
+from superset.models.core import Database
+from superset.views.base import DatasourceFilter
+
+logger = logging.getLogger(__name__)
+
+
+class DatasetDAO:
+ @staticmethod
+ def get_owner_by_id(owner_id: int) -> Optional[object]:
+ return (
+ db.session.query(current_app.appbuilder.sm.user_model)
+ .filter_by(id=owner_id)
+ .one_or_none()
+ )
+
+ @staticmethod
+ def get_database_by_id(database_id) -> Optional[Database]:
+ try:
+ return db.session.query(Database).filter_by(id=database_id).one_or_none()
+ except SQLAlchemyError as e: # pragma: no cover
+ logger.error(f"Could not get database by id: {e}")
+ return None
+
+ @staticmethod
+ def validate_table_exists(database: Database, table_name: str, schema: str) -> bool:
+ try:
+ database.get_table(table_name, schema=schema)
+ return True
+ except SQLAlchemyError as e: # pragma: no cover
+ logger.error(f"Got an error {e} validating table: {table_name}")
+ return False
+
+ @staticmethod
+ def validate_uniqueness(database_id: int, name: str) -> bool:
+ dataset_query = db.session.query(SqlaTable).filter(
+ SqlaTable.table_name == name, SqlaTable.database_id == database_id
+ )
+ return not db.session.query(dataset_query.exists()).scalar()
+
+ @staticmethod
+ def validate_update_uniqueness(
+ database_id: int, dataset_id: int, name: str
+ ) -> bool:
+ dataset_query = db.session.query(SqlaTable).filter(
+ SqlaTable.table_name == name,
+ SqlaTable.database_id == database_id,
+ SqlaTable.id != dataset_id,
+ )
+ return not db.session.query(dataset_query.exists()).scalar()
+
+ @staticmethod
+ def find_by_id(model_id: int) -> SqlaTable:
+ data_model = SQLAInterface(SqlaTable, db.session)
+ query = db.session.query(SqlaTable)
+ query = DatasourceFilter("id", data_model).apply(query, None)
+ return query.filter_by(id=model_id).one_or_none()
+
+ @staticmethod
+ def create(properties: Dict) -> Optional[SqlaTable]:
+ model = SqlaTable()
+ for key, value in properties.items():
+ setattr(model, key, value)
+ try:
+ db.session.add(model)
+ db.session.commit()
+ except SQLAlchemyError as e: # pragma: no cover
+ logger.error(f"Failed to create dataset: {e}")
+ db.session.rollback()
+ return None
+ return model
+
+ @staticmethod
+ def update(model: SqlaTable, properties: Dict) -> Optional[SqlaTable]:
+ for key, value in properties.items():
+ setattr(model, key, value)
+ try:
+ db.session.merge(model)
+ db.session.commit()
Review comment:
Not getting the idea of a decorator here, to switch commit's on and off?
I propose we talk about it and push it to the next PR for dataset's columns
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r379418836
##########
File path: superset/datasets/dao.py
##########
@@ -0,0 +1,119 @@
+# 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 Dict, Optional
+
+from flask import current_app
+from flask_appbuilder.models.sqla.interface import SQLAInterface
+from sqlalchemy.exc import SQLAlchemyError
+
+from superset import db
+from superset.connectors.sqla.models import SqlaTable
+from superset.models.core import Database
+from superset.views.base import DatasourceFilter
+
+logger = logging.getLogger(__name__)
+
+
+class DatasetDAO:
+ @staticmethod
+ def get_owner_by_id(owner_id: int) -> Optional[object]:
+ return (
+ db.session.query(current_app.appbuilder.sm.user_model)
+ .filter_by(id=owner_id)
+ .one_or_none()
+ )
+
+ @staticmethod
+ def get_database_by_id(database_id) -> Optional[Database]:
+ try:
+ return db.session.query(Database).filter_by(id=database_id).one_or_none()
+ except SQLAlchemyError as e: # pragma: no cover
+ logger.error(f"Could not get database by id: {e}")
+ return None
+
+ @staticmethod
+ def validate_table_exists(database: Database, table_name: str, schema: str) -> bool:
+ try:
+ database.get_table(table_name, schema=schema)
Review comment:
It would, but this is not a SQLAlchemy query, it's going to fetch an actual SQLAlchemy table representation, and fail if the table does not exist on a engine
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar merged pull request #9129: [datasets]
new, API using command pattern
Posted by GitBox <gi...@apache.org>.
dpgaspar merged pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #9129: [datasets] new, API using command pattern
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #9129: [datasets] new, API using command pattern
URL: https://github.com/apache/incubator-superset/pull/9129#discussion_r379113758
##########
File path: superset/datasets/commands/delete.py
##########
@@ -0,0 +1,49 @@
+# 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.
+
+from flask_appbuilder.security.sqla.models import User
+
+from superset.commands.base import BaseCommand, CommandValidateReturn
+from superset.datasets.commands.exceptions import (
+ DatasetDeleteFailedError,
+ DatasetNotFoundError,
+)
+from superset.datasets.dao import DatasetDAO
+from superset.views.base import check_ownership
+
+
+class DeleteDatasetCommand(BaseCommand):
+ def __init__(self, user: User, model_id: int):
+ self._actor = user
+ self._model_id = model_id
+ self._model = None
+
+ def run(self):
+ self._model = DatasetDAO.find_by_id(self._model_id)
+ if not self._model:
+ raise DatasetNotFoundError()
+ self.validate()
Review comment:
Here you're using `validate` like you expect it to raise or not, but it could return `False` without affecting control flow.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org