betodealmeida commented on a change in pull request #14451:
URL: https://github.com/apache/superset/pull/14451#discussion_r630504968
##########
File path: superset/databases/commands/create.py
##########
@@ -87,7 +90,12 @@ def validate(self) -> None:
# Check database_name uniqueness
if not DatabaseDAO.validate_uniqueness(database_name):
exceptions.append(DatabaseExistsValidationError())
-
+ if configuration_method:
+ if ConfigurationMethod(configuration_method) not in {
+ ConfigurationMethod.SQLALCHEMY_URI,
+ ConfigurationMethod.DYNAMIC_FORM,
+ }:
+ exceptions.append(DatabaseExistsValidationError())
Review comment:
We should use a different exception here, since
`DatabaseExistsValidationError` is used when a database that is being created
already exists.
##########
File path: tests/databases/api_tests.py
##########
@@ -534,6 +568,23 @@ def test_update_database_uri_validate(self):
db.session.delete(test_database)
db.session.commit()
+ def test_update_database_with_invalid_configuration_method(self):
+ """
+ Database API: Test update
+ """
+ example_db = get_example_database()
+ test_database = self.insert_database(
+ "test-database", example_db.sqlalchemy_uri_decrypted
+ )
+ self.login(username="admin")
+ database_data = {
+ "database_name": "test-database-updated",
+ "configuration_method": "BAD_FORM",
+ }
+ uri = f"api/v1/database/{test_database.id}"
+ rv = self.client.put(uri, json=database_data)
+ self.assertEqual(rv.status_code, 400)
Review comment:
Same here.
##########
File path: superset/databases/schemas.py
##########
@@ -327,6 +329,7 @@ class Meta: # pylint: disable=too-few-public-methods
description=cache_timeout_description, allow_none=True
)
expose_in_sqllab = fields.Boolean(description=expose_in_sqllab_description)
+ configuration_method = EnumField(ConfigurationMethod, by_value=True)
Review comment:
@AAfghahi you should add the description to the field, this way it shows
up in the Swagger interface:
```python
configuration_method = EnumField(ConfigurationMethod, by_value=True,
description="...")
```
##########
File path: tests/databases/api_tests.py
##########
@@ -241,6 +242,34 @@ def test_create_database(self):
db.session.delete(model)
db.session.commit()
+ def test_create_database_invalid_configuration_method(self):
+ """
+ Database API: Test create
+ """
+ extra = {
+ "metadata_params": {},
+ "engine_params": {},
+ "metadata_cache_timeout": {},
+ "schemas_allowed_for_csv_upload": [],
+ }
+
+ self.login(username="admin")
+ example_db = get_example_database()
+ if example_db.backend == "sqlite":
+ return
+ database_data = {
+ "database_name": "test-create-database",
+ "sqlalchemy_uri": example_db.sqlalchemy_uri_decrypted,
+ "configuration_method": "BAD_FORM",
+ "server_cert": None,
+ "extra": json.dumps(extra),
+ }
+
+ uri = "api/v1/database/"
+ rv = self.client.post(uri, json=database_data)
+ response = json.loads(rv.data.decode("utf-8"))
+ self.assertEqual(rv.status_code, 400)
Review comment:
With pytest we get better context by using plain asserts instead:
```suggestion
assert rv.status_code == 400
```
We should also add an assert on the value of `rv.response`.
##########
File path: superset/databases/commands/update.py
##########
@@ -75,12 +75,21 @@ def validate(self) -> None:
if not self._model:
raise DatabaseNotFoundError()
database_name: Optional[str] = self._properties.get("database_name")
+ configuration_method: Optional[str] = self._properties.get(
+ "configuration_method"
+ )
if database_name:
# Check database_name uniqueness
if not DatabaseDAO.validate_update_uniqueness(
self._model_id, database_name
):
exceptions.append(DatabaseExistsValidationError())
+ if configuration_method:
+ if ConfigurationMethod(configuration_method) not in {
Review comment:
I agree with @eschutho, the 400 you're seeing in your test is probably a
generic error:
```python
>>> from enum import Enum
>>> class Foo(Enum):
... A = "A"
...
>>> Foo("A")
<Foo.A: 'A'>
>>> Foo("B")
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/beto/.pyenv/versions/3.9.2/lib/python3.9/enum.py", line 360,
in __call__
return cls.__new__(cls, value)
File "/Users/beto/.pyenv/versions/3.9.2/lib/python3.9/enum.py", line 677,
in __new__
raise ve_exc
ValueError: 'B' is not a valid Foo
```
You should do something like this instead:
```python
if configuration_method not in {method.value for method in
ConfigurationMethod}:
```
--
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
