Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Feb 25, 2022

Conversation

villebro
Copy link
Member

@villebro villebro commented Feb 25, 2022

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
Copy link

codecov bot commented Feb 25, 2022

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

Impacted file tree graph

@@            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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for this fix.

Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@villebro villebro merged commit 2bacedd into apache:master Feb 25, 2022
villebro added a commit that referenced this pull request Apr 3, 2022
@mistercrunch 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels lts-v1 preset-io size/XS 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants