korbit-ai[bot] commented on code in PR #33384:
URL: https://github.com/apache/superset/pull/33384#discussion_r2078805435
##########
superset/commands/dataset/update.py:
##########
@@ -128,15 +106,68 @@
except ValidationError as ex:
exceptions.append(ex)
+ self._validate_dataset_source(exceptions)
self._validate_semantics(exceptions)
if exceptions:
raise DatasetInvalidError(exceptions=exceptions)
- def _validate_semantics(self, exceptions: list[ValidationError]) -> None:
+ def _validate_dataset_source(self, exceptions: list[ValidationError]) ->
None:
# we know we have a valid model
self._model = cast(SqlaTable, self._model)
Review Comment:
### Unclear method name and redundant type casting comment <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The comment 'we know we have a valid model' is redundant with the type
casting, and the method name doesn't clearly indicate it handles catalog and
database configuration.
###### Why this matters
Unclear method naming and redundant comments make the code harder to
understand at a glance.
###### Suggested change ∙ *Feature Preview*
Rename method to be more specific and remove redundant comment:
```python
def _validate_database_and_catalog_config(self, exceptions:
list[ValidationError]) -> None:
self._model = cast(SqlaTable, self._model)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/aec1573a-4769-4026-af64-7ed2824a7fd7/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/aec1573a-4769-4026-af64-7ed2824a7fd7?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/aec1573a-4769-4026-af64-7ed2824a7fd7?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/aec1573a-4769-4026-af64-7ed2824a7fd7?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/aec1573a-4769-4026-af64-7ed2824a7fd7)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6c9f03c2-f4cd-492e-a789-15e65473862a -->
[](6c9f03c2-f4cd-492e-a789-15e65473862a)
##########
superset/commands/dataset/update.py:
##########
@@ -128,15 +106,68 @@ def validate(self) -> None:
except ValidationError as ex:
exceptions.append(ex)
+ self._validate_dataset_source(exceptions)
self._validate_semantics(exceptions)
if exceptions:
raise DatasetInvalidError(exceptions=exceptions)
- def _validate_semantics(self, exceptions: list[ValidationError]) -> None:
+ def _validate_dataset_source(self, exceptions: list[ValidationError]) ->
None:
# we know we have a valid model
self._model = cast(SqlaTable, self._model)
+ database_id = self._properties.pop("database_id", None)
+ catalog = self._properties.get("catalog")
+ new_db_connection: Database | None = None
+
+ if database_id and database_id != self._model.database.id:
+ if new_db_connection := DatasetDAO.get_database_by_id(database_id):
+ self._properties["database"] = new_db_connection
+ else:
+ exceptions.append(DatabaseNotFoundValidationError())
+ db = new_db_connection or self._model.database
+ default_catalog = db.get_default_catalog()
+
+ # If multi-catalog is disabled, and catalog provided is not
+ # the default one, fail
+ if (
+ "catalog" in self._properties
+ and catalog != default_catalog
+ and not db.allow_multi_catalog
+ ):
+ exceptions.append(MultiCatalogDisabledValidationError())
+
+ # If the DB connection does not support multi-catalog,
+ # use the default catalog
+ elif not db.allow_multi_catalog:
+ catalog = self._properties["catalog"] = default_catalog
+
+ # Fallback to using the previous value if not provided
+ elif "catalog" not in self._properties:
+ catalog = self._model.catalog
Review Comment:
### Missing Catalog Persistence <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The catalog value is assigned but not stored in self._properties, causing
potential inconsistency between the local variable and the properties dict.
###### Why this matters
This could lead to the catalog value not being properly persisted during the
update operation, which contradicts the intended functionality.
###### Suggested change ∙ *Feature Preview*
Store the catalog value in properties:
```python
elif "catalog" not in self._properties:
catalog = self._properties["catalog"] = self._model.catalog
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/268f8793-a7d0-45eb-b6e6-b9854e1f1bab/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/268f8793-a7d0-45eb-b6e6-b9854e1f1bab?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/268f8793-a7d0-45eb-b6e6-b9854e1f1bab?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/268f8793-a7d0-45eb-b6e6-b9854e1f1bab?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/268f8793-a7d0-45eb-b6e6-b9854e1f1bab)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:357699ca-06c8-4f74-9ac1-ef011a10150d -->
[](357699ca-06c8-4f74-9ac1-ef011a10150d)
##########
superset/commands/dataset/exceptions.py:
##########
@@ -33,6 +33,18 @@ def get_dataset_exist_error_msg(table: Table) -> str:
return _("Dataset %(table)s already exists", table=table)
+class MultiCatalogDisabledValidationError(ValidationError):
+ """
+ Validation error for using a non-default catalog when multi-catalog is
disabled
+ """
Review Comment:
### Incomplete ValidationError Documentation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The docstring doesn't explain when this error is raised or what conditions
trigger it.
###### Why this matters
Without context about when this error occurs, developers may have difficulty
troubleshooting or properly handling this exception.
###### Suggested change ∙ *Feature Preview*
class MultiCatalogDisabledValidationError(ValidationError):
"""
Validation error raised when attempting to use a non-default catalog
in a database connection where multi-catalog support is disabled.
"""
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d5ea4df9-fdf4-4865-95b6-c1be7d1dd8c3/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d5ea4df9-fdf4-4865-95b6-c1be7d1dd8c3?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d5ea4df9-fdf4-4865-95b6-c1be7d1dd8c3?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d5ea4df9-fdf4-4865-95b6-c1be7d1dd8c3?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d5ea4df9-fdf4-4865-95b6-c1be7d1dd8c3)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6474a029-cd2c-4b59-b12a-f2e7c5dd4b16 -->
[](6474a029-cd2c-4b59-b12a-f2e7c5dd4b16)
##########
superset/commands/dataset/exceptions.py:
##########
@@ -33,6 +33,18 @@
return _("Dataset %(table)s already exists", table=table)
+class MultiCatalogDisabledValidationError(ValidationError):
Review Comment:
### Unclear Error Message for Catalog Selection <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The error message 'Only the default catalog is supported for this
connection' may be too vague for users to understand what action they need to
take.
###### Why this matters
Users may not understand what a 'default catalog' is or how to correct their
selection, leading to confusion and potential support tickets.
###### Suggested change ∙ *Feature Preview*
Enhance the error message to be more specific and actionable:
```python
class MultiCatalogDisabledValidationError(ValidationError):
def __init__(self) -> None:
super().__init__(
[_('Multi-catalog support is disabled. Please use the default
catalog or enable multi-catalog support in your database configuration.')],
field_name='catalog',
)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c5727bd4-1c6f-470a-b04b-3d35fdc04621/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c5727bd4-1c6f-470a-b04b-3d35fdc04621?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c5727bd4-1c6f-470a-b04b-3d35fdc04621?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c5727bd4-1c6f-470a-b04b-3d35fdc04621?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c5727bd4-1c6f-470a-b04b-3d35fdc04621)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f82026d2-d4c6-4294-8d67-31fae16f2f2f -->
[](f82026d2-d4c6-4294-8d67-31fae16f2f2f)
--
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
