fix(dataset): handle missing database in migration #18948

villebro · 2022-02-25T15:47:32Z

SUMMARY

The recent PR #18935 added a migration that fails if a dataset is referencing a database that has been removed. This changes the migration logic to return early if the database is missing.

BEFORE

When performing a migration on metadata containing datasets that reference a missing database, the following happens:

$ superset db upgrade
Loaded your LOCAL configuration at [/Users/ville/superset/superset_config.py]
logging was configured successfully
2022-02-25 17:44:27,115:INFO:superset.utils.logging_configurator:logging was configured successfully
2022-02-25 17:44:27,121:INFO:root:Configured event logger of type <class 'superset.utils.log.DBEventLogger'>
WARNI [alembic.env] SQLite Database support for metadata databases will         be removed in a future version of Superset.
INFO  [alembic.runtime.migration] Will assume transactional DDL.
INFO  [alembic.runtime.migration] Running upgrade 5afbb1a5849b -> b8d3a24d9131, New dataset models
Traceback (most recent call last):
  File "/Users/ville/src/superset/venv/bin/superset", line 33, in <module>
    sys.exit(load_entry_point('apache-superset', 'console_scripts', 'superset')())
  File "/Users/ville/src/superset/venv/lib/python3.8/site-packages/click/core.py", line 829, in __call__
    return self.main(*args, **kwargs)
  File "/Users/ville/src/superset/venv/lib/python3.8/site-packages/flask/cli.py", line 586, in main
    return super(FlaskGroup, self).main(*args, **kwargs)
  File "/Users/ville/src/superset/venv/lib/python3.8/site-packages/click/core.py", line 782, in main
    rv = self.invoke(ctx)
  File "/Users/ville/src/superset/venv/lib/python3.8/site-packages/click/core.py", line 1259, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/ville/src/superset/venv/lib/python3.8/site-packages/click/core.py", line 1259, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/ville/src/superset/venv/lib/python3.8/site-packages/click/core.py", line 1066, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/ville/src/superset/venv/lib/python3.8/site-packages/click/core.py", line 610, in invoke
    return callback(*args, **kwargs)
  File "/Users/ville/src/superset/venv/lib/python3.8/site-packages/click/decorators.py", line 21, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/Users/ville/src/superset/venv/lib/python3.8/site-packages/flask/cli.py", line 426, in decorator
    return __ctx.invoke(f, *args, **kwargs)
  File "/Users/ville/src/superset/venv/lib/python3.8/site-packages/click/core.py", line 610, in invoke
    return callback(*args, **kwargs)
  File "/Users/ville/src/superset/venv/lib/python3.8/site-packages/flask_migrate/cli.py", line 149, in upgrade
    _upgrade(directory, revision, sql, tag, x_arg)
  File "/Users/ville/src/superset/venv/lib/python3.8/site-packages/flask_migrate/__init__.py", line 98, in wrapped
    f(*args, **kwargs)
  File "/Users/ville/src/superset/venv/lib/python3.8/site-packages/flask_migrate/__init__.py", line 185, in upgrade
    command.upgrade(config, revision, sql=sql, tag=tag)
  File "/Users/ville/src/superset/venv/lib/python3.8/site-packages/alembic/command.py", line 294, in upgrade
    script.run_env()
  File "/Users/ville/src/superset/venv/lib/python3.8/site-packages/alembic/script/base.py", line 490, in run_env
    util.load_python_file(self.dir, "env.py")
  File "/Users/ville/src/superset/venv/lib/python3.8/site-packages/alembic/util/pyfiles.py", line 97, in load_python_file
    module = load_module_py(module_id, path)
  File "/Users/ville/src/superset/venv/lib/python3.8/site-packages/alembic/util/compat.py", line 184, in load_module_py
    spec.loader.exec_module(module)
  File "<frozen importlib._bootstrap_external>", line 843, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/Users/ville/src/superset/superset/migrations/env.py", line 126, in <module>
    run_migrations_online()
  File "/Users/ville/src/superset/superset/migrations/env.py", line 118, in run_migrations_online
    context.run_migrations()
  File "<string>", line 8, in run_migrations
  File "/Users/ville/src/superset/venv/lib/python3.8/site-packages/alembic/runtime/environment.py", line 813, in run_migrations
    self.get_context().run_migrations(**kw)
  File "/Users/ville/src/superset/venv/lib/python3.8/site-packages/alembic/runtime/migration.py", line 561, in run_migrations
    step.migration_fn(**kw)
  File "/Users/ville/src/superset/superset/migrations/versions/b8d3a24d9131_new_dataset_models.py", line 589, in upgrade
    after_insert(target=dataset)
  File "/Users/ville/src/superset/superset/migrations/versions/b8d3a24d9131_new_dataset_models.py", line 324, in after_insert
    or session.query(Database).filter_by(id=target.database_id).one()
  File "/Users/ville/src/superset/venv/lib/python3.8/site-packages/sqlalchemy/orm/query.py", line 3500, in one
    raise orm_exc.NoResultFound("No row was found for one()")
sqlalchemy.orm.exc.NoResultFound: No row was found for one()
$

AFTER

After the change the migration completes successfully:

$ superset db upgrade
Loaded your LOCAL configuration at [/Users/ville/superset/superset_config.py]
logging was configured successfully
2022-02-25 17:45:18,798:INFO:superset.utils.logging_configurator:logging was configured successfully
2022-02-25 17:45:18,805:INFO:root:Configured event logger of type <class 'superset.utils.log.DBEventLogger'>
WARNI [alembic.env] SQLite Database support for metadata databases will         be removed in a future version of Superset.
INFO  [alembic.runtime.migration] Will assume transactional DDL.
INFO  [alembic.runtime.migration] Running upgrade 5afbb1a5849b -> b8d3a24d9131, New dataset models
$

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
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

codecov · 2022-02-25T16:08:51Z

Codecov Report

Merging #18948 (abe31d3) into master (4923256) will decrease coverage by 0.07%.
The diff coverage is 90.00%.

❗ Current head abe31d3 differs from pull request most recent head 0b0b7f9. Consider uploading reports for the commit 0b0b7f9 to get more accurate results

@@            Coverage Diff             @@
##           master   #18948      +/-   ##
==========================================
- Coverage   66.39%   66.31%   -0.08%     
==========================================
  Files        1640     1640              
  Lines       63512    63512              
  Branches     6417     6417              
==========================================
- Hits        42170    42121      -49     
- Misses      19681    19730      +49     
  Partials     1661     1661

Flag	Coverage Δ
hive	`52.58% <ø> (ø)`
mysql	`?`
postgres	`81.86% <ø> (ø)`
presto	`?`
python	`82.13% <ø> (-0.16%)`	⬇️
sqlite	`81.55% <ø> (ø)`

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

Impacted Files	Coverage Δ
...ins/legacy-preset-chart-deckgl/src/Multi/Multi.jsx	`0.00% <0.00%> (ø)`
...c/SqlLab/components/RunQueryActionButton/index.tsx	`50.00% <ø> (ø)`
...d/src/SqlLab/components/SaveDatasetModal/index.tsx	`71.42% <ø> (ø)`
...frontend/src/SqlLab/components/SaveQuery/index.tsx	`57.89% <ø> (ø)`
...rc/SqlLab/components/ScheduleQueryButton/index.tsx	`19.23% <ø> (ø)`
...d/src/SqlLab/components/TabbedSqlEditors/index.jsx	`56.39% <ø> (ø)`
superset-frontend/src/common/components/index.tsx	`100.00% <ø> (ø)`
...rc/components/Datasource/ChangeDatasourceModal.tsx	`78.68% <ø> (ø)`
...rset-frontend/src/components/DeleteModal/index.tsx	`84.21% <ø> (ø)`
...uperset-frontend/src/components/Dropdown/index.tsx	`100.00% <ø> (ø)`
... and 85 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 4923256...0b0b7f9. Read the comment docs.

betodealmeida · 2022-02-25T16:12:35Z

superset/migrations/versions/b8d3a24d9131_new_dataset_models.py

+    if database:
+        engine = database.get_sqla_engine(schema=target.schema)
+    conditional_quote = (
+        engine.dialect.identifier_preparer.quote if engine else lambda x: x


I think we should just skip the dataset if it has no database... it seems that this is something that shouldn't happen, and a dataset without an associated database is useless, no? Or am I missing something?

zhaoyongjie

LGTM. Thanks for this fix.

betodealmeida

If there's no legit reason for a dataset without a database let's just return early in the function when that happens.

(cherry picked from commit 2bacedd)

fix(dataset): handle missing database in migration

ec145b8

villebro requested a review from a team as a code owner February 25, 2022 15:47

superset-github-bot bot added the preset-io label Feb 25, 2022

pull-request-size bot added the size/S label Feb 25, 2022

villebro requested a review from betodealmeida February 25, 2022 15:49

betodealmeida reviewed Feb 25, 2022

View reviewed changes

zhaoyongjie approved these changes Feb 25, 2022

View reviewed changes

betodealmeida requested changes Feb 25, 2022

View reviewed changes

return early if database is None

0b0b7f9

pull-request-size bot added size/XS and removed size/S labels Feb 25, 2022

villebro requested a review from betodealmeida February 25, 2022 17:04

betodealmeida approved these changes Feb 25, 2022

View reviewed changes

villebro merged commit 2bacedd into apache:master Feb 25, 2022

villebro added the lts-v1 label Mar 29, 2022

villebro added a commit that referenced this pull request Apr 3, 2022

fix(dataset): handle missing database in migration (#18948)

268ce0a

(cherry picked from commit 2bacedd)

mistercrunch added 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dataset): handle missing database in migration #18948

fix(dataset): handle missing database in migration #18948

villebro commented Feb 25, 2022 •

edited

Loading

codecov bot commented Feb 25, 2022 •

edited

Loading

betodealmeida Feb 25, 2022

zhaoyongjie left a comment

betodealmeida left a comment

fix(dataset): handle missing database in migration #18948

fix(dataset): handle missing database in migration #18948

Conversation

villebro commented Feb 25, 2022 • edited Loading

SUMMARY

BEFORE

AFTER

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Feb 25, 2022 • edited Loading

Codecov Report

betodealmeida Feb 25, 2022

Choose a reason for hiding this comment

zhaoyongjie left a comment

Choose a reason for hiding this comment

betodealmeida left a comment

Choose a reason for hiding this comment

villebro commented Feb 25, 2022 •

edited

Loading

codecov bot commented Feb 25, 2022 •

edited

Loading