EnxDev commented on code in PR #36880:
URL: https://github.com/apache/superset/pull/36880#discussion_r3194746632
##########
superset/commands/database/validate.py:
##########
@@ -138,6 +154,101 @@ def run(self) -> None:
),
)
- def validate(self) -> None:
+ def _load_model(self) -> None:
+ """Load the existing database model if updating."""
if (database_id := self._properties.get("id")) is not None:
self._model = DatabaseDAO.find_by_id(database_id)
+
+ def _get_database_name_error(self) -> Optional[SupersetError]:
+ """Check for duplicate database name and return error if found."""
+ database_id = self._properties.get("id")
+
+ if database_name := self._properties.get("database_name"):
+ is_unique = (
+ DatabaseDAO.validate_update_uniqueness(database_id,
database_name)
+ if database_id is not None
+ else DatabaseDAO.validate_uniqueness(database_name)
+ )
+ if not is_unique:
+ return SupersetError(
+ message=__("A database with the same name already
exists."),
+ error_type=SupersetErrorType.INVALID_PAYLOAD_SCHEMA_ERROR,
+ level=ErrorLevel.ERROR,
+ extra={"invalid": ["database_name"]},
+ )
+ return None
+
+ def _validate_database_name(self) -> None:
+ """Check for duplicate database name and raise if found."""
+ if error := self._get_database_name_error():
+ raise InvalidParametersError([error])
+
+ def validate(self) -> None:
+ """Load the model and validate SSH tunnel if enabled."""
+ self._load_model()
+ self._validate_ssh_tunnel()
+
+ def _validate_ssh_tunnel(self) -> None:
+ """Validate SSH tunnel configuration if enabled."""
+ ssh_tunnel = self._properties.get("ssh_tunnel")
+ if ssh_tunnel:
+ if not is_feature_enabled("SSH_TUNNELING"):
+ raise SSHTunnelingNotEnabledError()
Review Comment:
Addressed in f846e10. `_validate_ssh_tunnel` now treats SSH as enabled when
**either** the `ssh_tunnel` payload is non-empty **or** `parameters.ssh` is
true, and runs the feature-flag + db-port guards in both cases.
##########
superset/commands/database/validate.py:
##########
@@ -138,6 +154,101 @@ def run(self) -> None:
),
)
- def validate(self) -> None:
+ def _load_model(self) -> None:
+ """Load the existing database model if updating."""
if (database_id := self._properties.get("id")) is not None:
self._model = DatabaseDAO.find_by_id(database_id)
+
+ def _get_database_name_error(self) -> Optional[SupersetError]:
+ """Check for duplicate database name and return error if found."""
+ database_id = self._properties.get("id")
+
+ if database_name := self._properties.get("database_name"):
+ is_unique = (
+ DatabaseDAO.validate_update_uniqueness(database_id,
database_name)
+ if database_id is not None
+ else DatabaseDAO.validate_uniqueness(database_name)
+ )
+ if not is_unique:
+ return SupersetError(
+ message=__("A database with the same name already
exists."),
+ error_type=SupersetErrorType.INVALID_PAYLOAD_SCHEMA_ERROR,
+ level=ErrorLevel.ERROR,
+ extra={"invalid": ["database_name"]},
+ )
+ return None
+
+ def _validate_database_name(self) -> None:
+ """Check for duplicate database name and raise if found."""
+ if error := self._get_database_name_error():
+ raise InvalidParametersError([error])
+
+ def validate(self) -> None:
+ """Load the model and validate SSH tunnel if enabled."""
+ self._load_model()
+ self._validate_ssh_tunnel()
+
+ def _validate_ssh_tunnel(self) -> None:
+ """Validate SSH tunnel configuration if enabled."""
+ ssh_tunnel = self._properties.get("ssh_tunnel")
+ if ssh_tunnel:
+ if not is_feature_enabled("SSH_TUNNELING"):
+ raise SSHTunnelingNotEnabledError()
+ # Check if port is provided (required for SSH tunneling)
+ parameters = self._properties.get("parameters", {})
+ if not parameters.get("port"):
+ raise SSHTunnelDatabasePortError()
+
+ def _get_ssh_tunnel_errors(self) -> list[SupersetError]:
+ """Validate SSH tunnel fields and return list of errors."""
+ errors: list[SupersetError] = []
+ ssh_tunnel = self._properties.get("ssh_tunnel") or {}
+ parameters = self._properties.get("parameters", {})
+
+ # Check if SSH is enabled via parameters.ssh flag
+ ssh_enabled = parameters.get("ssh", False)
+
+ # Only validate SSH tunnel if SSH is enabled or ssh_tunnel is provided
+ if not ssh_enabled and not ssh_tunnel:
+ return errors
+
+ # Required fields
+ required_fields = ["server_address", "server_port", "username"]
+ missing = [f for f in required_fields if not ssh_tunnel.get(f)]
+
+ if missing:
+ errors.append(
+ SupersetError(
+ message=__("One or more parameters are missing:
%(missing)s"),
Review Comment:
Addressed in f846e10. The `%(missing)s` placeholder is now passed to
`gettext` (`__(..., missing=", ".join(missing))`), so the response message
contains the actual missing fields.
##########
superset/commands/database/validate.py:
##########
@@ -42,14 +47,16 @@ def __init__(self, properties: dict[str, Any]):
self._properties = properties.copy()
self._model: Optional[Database] = None
- def run(self) -> None:
+ def run(self) -> None: # noqa: C901
self.validate()
engine = self._properties["engine"]
driver = self._properties.get("driver")
if engine in BYPASS_VALIDATION_ENGINES:
# Skip engines that are only validated onCreate
+ # But still validate database_name uniqueness
+ self._validate_database_name()
return
Review Comment:
Addressed in f846e10. The bypass-engine branch now also runs
`_get_ssh_tunnel_errors()` (and `_get_database_name_error()`), so SSH tunnel
field errors are surfaced during progressive validation for `bigquery` /
`datastore` / `snowflake` too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
