john-bodley commented on code in PR #28341:
URL: https://github.com/apache/superset/pull/28341#discussion_r1589870410
##########
superset/commands/dataset/create.py:
##########
@@ -61,17 +61,16 @@ def run(self) -> Model:
def validate(self) -> None:
exceptions: list[ValidationError] = []
database_id = self._properties["database"]
- table_name = self._properties["table_name"]
schema = self._properties.get("schema")
catalog = self._properties.get("catalog")
sql = self._properties.get("sql")
owner_ids: Optional[list[int]] = self._properties.get("owners")
- table = Table(table_name, schema, catalog)
+ table = Table(self._properties["table_name"], schema, catalog)
# Validate uniqueness
if not DatasetDAO.validate_uniqueness(database_id, table):
- exceptions.append(DatasetExistsValidationError(table_name))
+ exceptions.append(DatasetExistsValidationError(table))
Review Comment:
We should be using a `Table` object rather than the table name which is too
terse, i.e., doesn't contain the schema or catalog.
##########
superset/commands/dataset/update.py:
##########
@@ -86,15 +86,21 @@ def validate(self) -> None:
except SupersetSecurityException as ex:
raise DatasetForbiddenError() from ex
- database_id = self._properties.get("database", None)
- table_name = self._properties.get("table_name", None)
+ database_id = self._properties.get("database")
+
+ table = Table(
+ self._properties.get("table_name"),
+ self._properties.get("schema"),
+ self._model.catalog,
+ )
+
# Validate uniqueness
if not DatasetDAO.validate_update_uniqueness(
self._model.database_id,
- Table(table_name, self._model.schema, self._model.catalog),
Review Comment:
Here's the bug. The schema should be the schema defined in the properties
and not the schema associated with the model.
##########
tests/unit_tests/commands/dataset/test_update.py:
##########
@@ -0,0 +1,43 @@
+from unittest.mock import MagicMock
Review Comment:
I was _blown away_ that there were [zero unit or integration
tests](https://github.com/search?q=repo%3Aapache%2Fsuperset+UpdateDatasetCommand+path%3A%2F%5Etests%5C%2F%2F&type=code)
for the `UpdateDatasetCommand`.
Though this test isn't pretty, it's a start.
##########
tests/unit_tests/dao/dataset_test.py:
##########
@@ -51,7 +51,7 @@ def test_validate_update_uniqueness(session: Session) -> None:
db.session.add_all([database, dataset1, dataset2])
db.session.flush()
- # same table name, different schema
Review Comment:
These comments were ordered wrongly and thus misleading. It's probably best
to not have comments if the logic is clear from the code.
##########
tests/unit_tests/commands/dataset/test_update.py:
##########
@@ -0,0 +1,43 @@
+from unittest.mock import MagicMock
Review Comment:
I was _shocked that there were [zero unit or integration
tests](https://github.com/search?q=repo%3Aapache%2Fsuperset+UpdateDatasetCommand+path%3A%2F%5Etests%5C%2F%2F&type=code)
for the `UpdateDatasetCommand`.
Though this test isn't pretty, it's a start.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
