feat: Configuration Method and expanded parameters for Database Model #14451

AAfghahi · 2021-05-03T22:33:18Z

SUMMARY

We are creating an alternative way of editing or creating a database, in order for this to be feasible we want to add a configuration_method column to the database model.

This PR adds the necessary logic to the model, with validation, and tests to make sure that it is working.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TEST PLAN

Unit tests added.

ADDITIONAL INFORMATION

Has associated issue:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

eschutho · 2021-05-06T22:48:42Z

superset/models/core.py

@@ -99,6 +100,11 @@ class CssTemplate(Model, AuditMixinNullable):
    css = Column(Text, default="")


+class ConfigurationMethod(str, enum.Enum):
+    SQLALCHEMY_URI = "SQLALCHEMY_URI"
+    DYNAMIC_FORM = "DYNAMIC_FORM"


small nit, but for consistency, we can make these values lowercase

dpgaspar · 2021-05-07T09:17:33Z

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)


nit: add a small description here

# configuration_method is used on the frontend to inform the backend whether to explode parameters or to provide only a sqlalchemy_uri configuration_method = EnumField(ConfigurationMethod, by_value=True)

Does this look ok?

We have two choices here.. let's discuss. When saving the database, the return is a sqlalchemy_uri, regardless of how the database was created. The new form is a two step form, so you basically create and then update. My current plan (pr isn't up yet, but I can paste it here for reference) is to not provide the configuration_method in the put. There are no plans as of now to allow people to switch from one to the other once they have created the database. But @yousoph and @Steejay and I have been talking about this, so it could change. The other option is to return the parameters and the configuration_method value with the save db response, which would be a more flexible solution, but might take more time to implement, and as it looks now we don't need it, so it could wait for a v2.

@AAfghahi you should add the description to the field, this way it shows up in the Swagger interface:

configuration_method = EnumField(ConfigurationMethod, by_value=True, description="...")

tests/databases/commands_tests.py

eschutho · 2021-05-07T21:55:36Z

superset/databases/commands/update.py

        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 {


Have you tested this? I believe if the value is not in the enum you're going to get an error, and you'll have to catch the error instead.

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)

THis is the test I wrote for it.

cool. can you maybe assert the error message, too, to validate that you're getting the right error?

I agree with @eschutho, the 400 you're seeing in your test is probably a generic error:

>>> 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:

if configuration_method not in {method.value for method in ConfigurationMethod}:

superset/databases/commands/update.py

eschutho · 2021-05-07T22:06:31Z

superset/models/core.py

@@ -36,6 +36,7 @@
    Column,
    create_engine,
    DateTime,
+    Enum,


I think I forgot to remove this when I converted to a string.. @AAfghahi

Yeah, it was failing a lint test, so I removed this.

betodealmeida · 2021-05-11T20:06:25Z

superset/databases/commands/create.py

+                ConfigurationMethod.SQLALCHEMY_URI,
+                ConfigurationMethod.DYNAMIC_FORM,
+            }:
+                exceptions.append(DatabaseExistsValidationError())


We should use a different exception here, since DatabaseExistsValidationError is used when a database that is being created already exists.

would DatabaseRequiredFieldValidationError be better?

betodealmeida · 2021-05-11T20:10:21Z

superset/databases/commands/update.py

        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 {


I agree with @eschutho, the 400 you're seeing in your test is probably a generic error:

>>> 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:

if configuration_method not in {method.value for method in ConfigurationMethod}:

betodealmeida · 2021-05-11T20:15:04Z

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)


@AAfghahi you should add the description to the field, this way it shows up in the Swagger interface:

configuration_method = EnumField(ConfigurationMethod, by_value=True, description="...")

tests/databases/api_tests.py

codecov · 2021-05-11T21:00:53Z

Codecov Report

Merging #14451 (3d05a71) into master (b064cc1) will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #14451      +/-   ##
==========================================
+ Coverage   77.45%   77.54%   +0.09%     
==========================================
  Files         958      958              
  Lines       48533    48541       +8     
  Branches     5702     5702              
==========================================
+ Hits        37592    37642      +50     
+ Misses      10740    10698      -42     
  Partials      201      201

Flag	Coverage Δ
hive	`81.10% <100.00%> (+<0.01%)`	⬆️
mysql	`81.37% <100.00%> (+<0.01%)`	⬆️
postgres	`81.39% <100.00%> (+<0.01%)`	⬆️
presto	`81.09% <100.00%> (?)`
python	`81.92% <100.00%> (+0.16%)`	⬆️
sqlite	`81.01% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/databases/api.py	`92.41% <ø> (ø)`
superset/databases/commands/create.py	`92.15% <ø> (ø)`
superset/databases/schemas.py	`99.54% <100.00%> (+<0.01%)`	⬆️
superset/models/core.py	`89.78% <100.00%> (+0.38%)`	⬆️
superset/utils/cache.py	`74.25% <0.00%> (+0.99%)`	⬆️
superset/connectors/sqla/models.py	`90.09% <0.00%> (+1.44%)`	⬆️
superset/db_engine_specs/presto.py	`90.31% <0.00%> (+5.89%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b064cc1...3d05a71. Read the comment docs.

superset/databases/commands/create.py

eschutho

Other than the extra import, this LGTM. thanks @AAfghahi!

superset/databases/commands/update.py

superset/databases/schemas.py

superset/migrations/versions/453530256cea_add_save_option_column_to_db_model.py

superset/models/core.py

tests/databases/api_tests.py

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

sujeetpillai · 2021-05-17T15:27:48Z

This change has broken the superset-frontend though for master. configuration_method is not being passed from the database add modal. Works fine for edit since I'm guessing it's loaded from the api but when creating a new one it's giving an error like the attached.

My guess is superset-frontend/src/views/CRUD/data/database/types.ts needs to be updated, but I'm not too great at React to be sure.

AAfghahi · 2021-05-17T15:40:14Z

@sujeetpillai I think this will be fixed with: #14583

Though for the time being I am pushing up this change to make it optional; #14668

sujeetpillai · 2021-05-18T05:06:46Z

@AAfghahi Thanks for the quick fix. I can confirm it's now fixed.

jmistry · 2021-05-18T06:01:05Z

Thank you so much, I can confirm it's fixed as well

…

On Tue, May 18, 2021, 7:07 AM Sujeet Pillai ***@***.***> wrote: @AAfghahi <https://github.com/AAfghahi> Thanks for the quick fix. I can confirm it's now fixed. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#14451 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA3VSVXR7NJW2ZJBFXPHPSLTOHYXTANCNFSM44BTZ7MQ> .

AAfghahi · 2021-05-19T17:32:13Z

Nice! Glad to hear it.

…apache#14451) * db migration for dbs * first draft at logic * added unit tests * revisions * use strings for db values * lint and revisions to tests * changed test back * added revisions for testing * Update superset/databases/commands/update.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * Update superset/databases/schemas.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * Update superset/models/core.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * Update superset/databases/commands/update.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> * got rid of extra imports added new test * Update superset/databases/schemas.py Co-authored-by: Beto Dealmeida <roberto@dealmeida.net> Co-authored-by: Elizabeth Thompson <eschutho@gmail.com> Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

pull-request-size bot added size/M size/L and removed size/M labels May 3, 2021

AAfghahi marked this pull request as ready for review May 6, 2021 21:00

AAfghahi requested a review from a team as a code owner May 6, 2021 21:00

AAfghahi force-pushed the ch8325_DBModelUpdate branch 3 times, most recently from cc6c632 to 0ee9d78 Compare May 6, 2021 21:20

AAfghahi mentioned this pull request May 6, 2021

feat: Adding configuration_method column to Database Model #14433

Merged

8 tasks

eschutho reviewed May 6, 2021

View reviewed changes

dpgaspar reviewed May 7, 2021

View reviewed changes

hughhhh reviewed May 7, 2021

View reviewed changes

tests/databases/commands_tests.py Outdated Show resolved Hide resolved

eschutho reviewed May 7, 2021

View reviewed changes

superset/databases/commands/update.py Outdated Show resolved Hide resolved

eschutho reviewed May 7, 2021

View reviewed changes

AAfghahi force-pushed the ch8325_DBModelUpdate branch 5 times, most recently from 296dbce to 5e0a541 Compare May 11, 2021 20:02

betodealmeida requested changes May 11, 2021

View reviewed changes

AAfghahi force-pushed the ch8325_DBModelUpdate branch from 2219035 to 140ef90 Compare May 11, 2021 20:45

eschutho reviewed May 12, 2021

View reviewed changes

superset/databases/commands/create.py Outdated Show resolved Hide resolved

eschutho approved these changes May 12, 2021

View reviewed changes

betodealmeida requested changes May 12, 2021

View reviewed changes

AAfghahi force-pushed the ch8325_DBModelUpdate branch 4 times, most recently from 7f483ec to 213cb60 Compare May 14, 2021 21:34

AAfghahi and others added 13 commits May 14, 2021 17:57

db migration for dbs

172c42a

first draft at logic

0415073

added unit tests

4c2c77e

revisions

8855ff3

use strings for db values

ae52d8a

lint and revisions to tests

8aeda42

changed test back

3a6815b

added revisions for testing

7aacc07

Update superset/databases/commands/update.py

ec6fed6

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

Update superset/databases/schemas.py

842a286

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

Update superset/models/core.py

e4b6575

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

Update superset/databases/commands/update.py

7da586e

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

got rid of extra imports added new test

62038d6

AAfghahi force-pushed the ch8325_DBModelUpdate branch from 16a032d to 95b51b3 Compare May 14, 2021 23:59

Update superset/databases/schemas.py

3d05a71

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

AAfghahi force-pushed the ch8325_DBModelUpdate branch from 95b51b3 to 3d05a71 Compare May 15, 2021 00:01

betodealmeida approved these changes May 15, 2021

View reviewed changes

betodealmeida merged commit 4f5c537 into apache:master May 15, 2021

xch-jack mentioned this pull request May 20, 2021

Connection to postgres database as a source DB refused while working as a metadata DB #14735

Closed

3 tasks

eschutho deleted the ch8325_DBModelUpdate branch May 24, 2021 17:29

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Configuration Method and expanded parameters for Database Model #14451

feat: Configuration Method and expanded parameters for Database Model #14451

AAfghahi commented May 3, 2021 •

edited

Loading

eschutho May 6, 2021

dpgaspar May 7, 2021

AAfghahi May 7, 2021 •

edited

Loading

eschutho May 7, 2021

betodealmeida May 11, 2021

eschutho May 7, 2021

AAfghahi May 7, 2021

eschutho May 8, 2021

betodealmeida May 11, 2021

eschutho May 7, 2021

AAfghahi May 7, 2021

betodealmeida May 11, 2021

AAfghahi May 12, 2021

betodealmeida May 11, 2021

betodealmeida May 11, 2021

codecov bot commented May 11, 2021 •

edited

Loading

eschutho left a comment

sujeetpillai commented May 17, 2021

AAfghahi commented May 17, 2021 •

edited

Loading

sujeetpillai commented May 18, 2021

jmistry commented May 18, 2021 via email

AAfghahi commented May 19, 2021

feat: Configuration Method and expanded parameters for Database Model #14451

feat: Configuration Method and expanded parameters for Database Model #14451

Conversation

AAfghahi commented May 3, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AAfghahi May 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 11, 2021 • edited Loading

Codecov Report

eschutho left a comment

Choose a reason for hiding this comment

sujeetpillai commented May 17, 2021

AAfghahi commented May 17, 2021 • edited Loading

sujeetpillai commented May 18, 2021

jmistry commented May 18, 2021 via email

AAfghahi commented May 19, 2021

AAfghahi commented May 3, 2021 •

edited

Loading

AAfghahi May 7, 2021 •

edited

Loading

codecov bot commented May 11, 2021 •

edited

Loading

AAfghahi commented May 17, 2021 •

edited

Loading