refactor: Repeated boilerplate code between upload to database forms by exemplary-citizen · Pull Request #16756 · apache/superset

exemplary-citizen · 2021-09-21T01:40:50Z

SUMMARY

class CsvToDatabaseForm(DynamicForm):
    # pylint: disable=E0211
    def csv_allowed_dbs() -> List[Database]:  # type: ignore
        csv_enabled_dbs = (
            db.session.query(Database).filter_by(allow_csv_upload=True).all()
        )
        return [
            csv_enabled_db
            for csv_enabled_db in csv_enabled_dbs
            if CsvToDatabaseForm.at_least_one_schema_is_allowed(csv_enabled_db)
        ]

There's unnecessary duplication here between CSV and other forms.

Instead we can abstract this into a class UploadToDatabaseForm(DynamicForm) with all the repeated boilerplate and then extend those into class CsvToDatabaseForm(UploadToDatabaseForm), class ExcelToDatabaseForm(UploadToDatabaseForm) etc.

TESTING INSTRUCTIONS

Watch CI and any suggestions from reviewers

ADDITIONAL INFORMATION

Fixes #16046

Has associated issue: Repeated boilerplate code between upload to database forms #16046
Required feature flags:
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

cc: @villebro

github-actions · 2021-09-22T22:10:35Z

⚠️ @exemplary-citizen Your base branch master has just also updated superset/migrations.

❗ Please consider rebasing your branch to avoid db migration conflicts.

github-actions · 2021-09-23T08:29:20Z

⚠️ @exemplary-citizen Your base branch master has just also updated superset/migrations.

❗ Please consider rebasing your branch to avoid db migration conflicts.

…erplate_and_rename

codecov · 2021-09-24T04:16:41Z

Codecov Report

Merging #16756 (227f8ee) into master (37944e1) will decrease coverage by 0.21%.
The diff coverage is 78.37%.

@@            Coverage Diff             @@
##           master   #16756      +/-   ##
==========================================
- Coverage   76.89%   76.68%   -0.22%     
==========================================
  Files        1038     1038              
  Lines       55515    55494      -21     
  Branches     7564     7564              
==========================================
- Hits        42690    42556     -134     
- Misses      12575    12688     +113     
  Partials      250      250

Flag	Coverage Δ
hive	`?`
javascript	`70.90% <37.50%> (ø)`
mysql	`81.91% <89.65%> (+0.01%)`	⬆️
postgres	`81.92% <89.65%> (+0.01%)`	⬆️
presto	`?`
python	`82.00% <89.65%> (-0.41%)`	⬇️
sqlite	`81.59% <89.65%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
...c/views/CRUD/data/database/DatabaseModal/index.tsx	`43.57% <0.00%> (ø)`
...set-frontend/src/views/CRUD/data/database/types.ts	`100.00% <ø> (ø)`
superset/config.py	`91.47% <ø> (ø)`
superset/databases/api.py	`93.00% <ø> (ø)`
superset/views/database/mixins.py	`81.03% <ø> (-1.73%)`	⬇️
superset/databases/commands/export.py	`90.47% <66.66%> (ø)`
superset/views/database/forms.py	`94.44% <85.71%> (+7.34%)`	⬆️
superset/databases/schemas.py	`98.40% <87.50%> (ø)`
...tend/src/views/CRUD/data/database/DatabaseList.tsx	`78.07% <100.00%> (ø)`
.../CRUD/data/database/DatabaseModal/ExtraOptions.tsx	`93.18% <100.00%> (ø)`
... and 12 more

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 37944e1...227f8ee. Read the comment docs.

exemplary-citizen · 2021-09-24T05:22:32Z

@villebro do you mind taking a look when you get a chance?

github-actions · 2021-09-27T08:26:42Z

⚠️ @exemplary-citizen Your base branch master has just also updated superset/migrations.

❗ Please consider rebasing your branch to avoid db migration conflicts.

villebro

One question on the db migration (also, this needs a rebase). I think it may be safer to leave the column name in dbs as-is and only change the rest of the codebase so that we can avoid a database migration.

villebro · 2021-09-29T06:24:37Z

+def upgrade():
+    with op.batch_alter_table("dbs") as batch_op:
+        batch_op.add_column(
+            sa.Column(
+                "allow_file_upload",
+                sa.Boolean(),
+                nullable=False,
+                server_default=expression.true(),
+            )
+        )
+        batch_op.drop_column("allow_csv_upload")


Wouldn't this remove all existing allow_csv_upload values and set the new allow_file_upload to TRUE?

Instead of adding and dropping the columns, I think we can go with this renaming pattern instead:

Suggested change

def upgrade():

with op.batch_alter_table("dbs") as batch_op:

batch_op.add_column(

sa.Column(

"allow_file_upload",

sa.Boolean(),

nullable=False,

server_default=expression.true(),

)

)

batch_op.drop_column("allow_csv_upload")

def upgrade():

with op.batch_alter_table("dbs") as batch_op:

batch_op.alter_column("allow_csv_upload", new_column_name="allow_file_upload")

…erplate_and_rename

exemplary-citizen · 2021-10-01T04:24:36Z

-class ExcelToDatabaseForm(DynamicForm):
-    # pylint: disable=E0211
-    def excel_allowed_dbs() -> List[Database]:  # type: ignore
-        # TODO: change allow_csv_upload to allow_file_upload


This is a longstanding TODO so I do think we should go ahead with the db migration.

exemplary-citizen · 2021-10-01T04:28:15Z

+def upgrade():
+    with op.batch_alter_table("dbs") as batch_op:
+        batch_op.add_column(
+            sa.Column(
+                "allow_file_upload",
+                sa.Boolean(),
+                nullable=False,
+                server_default=expression.true(),
+            )
+        )
+        batch_op.drop_column("allow_csv_upload")


Instead of adding and dropping the columns, I think we can go with this renaming pattern instead:

Suggested change

def upgrade():

with op.batch_alter_table("dbs") as batch_op:

batch_op.add_column(

sa.Column(

"allow_file_upload",

sa.Boolean(),

nullable=False,

server_default=expression.true(),

)

)

batch_op.drop_column("allow_csv_upload")

def upgrade():

with op.batch_alter_table("dbs") as batch_op:

batch_op.alter_column("allow_csv_upload", new_column_name="allow_file_upload")

github-actions · 2021-10-13T04:41:29Z

⚠️ @exemplary-citizen Your base branch master has just also updated superset/migrations.

❗ Please consider rebasing your branch to avoid db migration conflicts.

…erplate_and_rename

…/superset into boilerplate_and_rename

exemplary-citizen · 2021-10-19T04:14:49Z

@villebro can you take another look?

villebro · 2021-10-20T20:28:37Z

Thanks @exemplary-citizen, sorry for the delay, I'll give this a new review + test as soon as I have some spare time (hoping to do it this week).

villebro

This is great work @exemplary-citizen ! LGTM and tested to work well!

betodealmeida · 2021-10-28T21:41:38Z

This migration is failing on MySQL for me, for some reason:

sqlalchemy.exc.OperationalError: (MySQLdb._exceptions.OperationalError) (3959, "Check constraint 'dbs_chk_9' uses column 'allow_csv_upload', hence column cannot be dropped or renamed.")

Looks like it happens in MySQL 8: sqlalchemy/alembic#699

john-bodley · 2021-10-29T22:19:29Z

@betodealmeida @exemplary-citizen @villebro it seems like there should be an entry in UPDATING.md for this as the DDL operation may require downtime.

john-bodley · 2021-10-29T23:08:18Z

Also @betodealmeida @exemplary-citizen @villebro the current migration is not suffice. The dbs.extra JSON encoded field needs to be updated given that the schemas_allowed_for_csv_upload field was renamed to schemas_allowed_for_file_upload.

exemplary-citizen · 2021-10-29T23:19:15Z

@john-bodley just opened #17294. feel free to assign to me. I'll start working on a fix

john-bodley · 2021-10-29T23:31:19Z

Thanks @exemplary-citizen. I've assigned the issue to you. Feel free to add me as a reviewer and/or ping me in Slack—given that most of my GitHub mentions seem to get lost in the aether.

Co-authored-by: John Bodley <4567245+john-bodley@users.noreply.github.com>

betodealmeida · 2021-11-02T23:46:46Z

Another problem with this PR that I missed: it changed a versioned schema (ImportV1DatabaseExtraSchema), breaking import/export.

(Not complaining or assigning blame, just calling it out so we catch these things in the future.)

junlincc · 2021-11-10T23:57:11Z

Sorry for being late the the discussion. @betodealmeida @john-bodley thanks for calling out.
Looks like this PR involves DB migration? We should definitely come up with a guideline for reviewing PRs that requires DB migration as they have higher risk. At least add the risk label while reviewing the PR.
@villebro

Co-authored-by: John Bodley <4567245+john-bodley@users.noreply.github.com>

exemplary-citizen and others added 7 commits September 18, 2021 16:53

abstract boilerplate code into class and rename csv to file

fe3af4c

add db migration

f863b4a

fix some stuff

182dfc8

more renaming of csv to file

f536de4

rename in translations

aeac4ea

Merge branch 'apache:master' into boilerplate_and_rename

5e7f65f

update down revision

1a9c649

exemplary-citizen requested a review from a team as a code owner September 21, 2021 01:40

superset-github-bot Bot added the dropbox Dropbox related label Sep 21, 2021

pull-request-size Bot added the size/L label Sep 21, 2021

exemplary-citizen added 2 commits September 23, 2021 20:32

Merge branch 'master' of https://github.com/apache/superset into boil…

0d374c9

…erplate_and_rename

update down revision

3d961bb

betodealmeida self-requested a review September 24, 2021 04:11

bump chart version

2e92eb8

villebro reviewed Sep 29, 2021

View reviewed changes

exemplary-citizen added 2 commits September 30, 2021 20:58

Merge branch 'master' of https://github.com/apache/superset into boil…

ec59479

…erplate_and_rename

switch to alter column name approach in db migration

6694407

exemplary-citizen commented Oct 1, 2021

View reviewed changes

exemplary-citizen and others added 2 commits September 30, 2021 22:09

fix db migration for MySQL

17d3e4b

Merge branch 'apache:master' into boilerplate_and_rename

61dbe8b

exemplary-citizen and others added 4 commits October 14, 2021 21:14

Merge branch 'master' of https://github.com/apache/superset into boil…

d0ab550

…erplate_and_rename

db migration conflict

ddc6336

Merge branch 'boilerplate_and_rename' of github.com:exemplary-citizen…

a1c1ddb

…/superset into boilerplate_and_rename

Merge branch 'apache:master' into boilerplate_and_rename

227f8ee

villebro self-requested a review October 21, 2021 11:57

villebro approved these changes Oct 25, 2021

View reviewed changes

villebro merged commit ef3afbd into apache:master Oct 25, 2021

exemplary-citizen mentioned this pull request Oct 28, 2021

fix: handle constraints in db migration for MySQL 8 #17274

Closed

betodealmeida mentioned this pull request Oct 29, 2021

fix: drop check constraint on dbs to rename column #17275

Merged

9 tasks

exemplary-citizen added a commit to exemplary-citizen/superset that referenced this pull request Oct 29, 2021

docs(UPDATING): Adding downtime for apache#16756

5ed9260

exemplary-citizen mentioned this pull request Oct 29, 2021

docs(UPDATING): Adding downtime for #16756 #17293

Merged

exemplary-citizen mentioned this pull request Oct 29, 2021

insufficient db migration in #16756 #17294

Closed

nytai pushed a commit that referenced this pull request Oct 30, 2021

docs(UPDATING): Adding downtime for #16756 (#17293)

79ed0ae

Co-authored-by: John Bodley <4567245+john-bodley@users.noreply.github.com>

exemplary-citizen mentioned this pull request Nov 2, 2021

fix: rename to schemas_allowed_for_file_upload in dbs.extra #17323

Merged

9 tasks

betodealmeida mentioned this pull request Nov 3, 2021

fix: import should accept old keys #17330

Merged

9 tasks

This was referenced Nov 15, 2021

[WIP] refactor: replace "csv" with "file" used in common (excel) upload cases #13805

Closed

fix: error with Upload Excel form #15867

Closed

Error while uploading Excel without all_database_access #15866

Closed

AAfghahi pushed a commit that referenced this pull request Jan 10, 2022

docs(UPDATING): Adding downtime for #16756 (#17293)

403cf79

Co-authored-by: John Bodley <4567245+john-bodley@users.noreply.github.com>

EBoisseauSierra mentioned this pull request Mar 26, 2022

Database error when upgrading from 1.4.1 to master #19100

Closed

3 tasks

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

Conversation

exemplary-citizen commented Sep 21, 2021

SUMMARY

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

github-actions Bot commented Sep 22, 2021

Uh oh!

github-actions Bot commented Sep 23, 2021

Uh oh!

codecov Bot commented Sep 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

exemplary-citizen commented Sep 24, 2021

Uh oh!

github-actions Bot commented Sep 27, 2021

Uh oh!

villebro left a comment

Choose a reason for hiding this comment

Uh oh!

villebro Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

exemplary-citizen Oct 1, 2021

Choose a reason for hiding this comment

Uh oh!

exemplary-citizen Oct 1, 2021

Choose a reason for hiding this comment

Uh oh!

exemplary-citizen Oct 1, 2021

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Oct 13, 2021

Uh oh!

exemplary-citizen commented Oct 19, 2021

Uh oh!

villebro commented Oct 20, 2021

Uh oh!

villebro left a comment

Choose a reason for hiding this comment

Uh oh!

betodealmeida commented Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

john-bodley commented Oct 29, 2021

Uh oh!

john-bodley commented Oct 29, 2021

Uh oh!

exemplary-citizen commented Oct 29, 2021

Uh oh!

john-bodley commented Oct 29, 2021

Uh oh!

betodealmeida commented Nov 2, 2021

Uh oh!

junlincc commented Nov 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov Bot commented Sep 24, 2021 •

edited

Loading

betodealmeida commented Oct 28, 2021 •

edited

Loading