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.

Sign up for GitHub

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

Jump to bottom

[fix] make names non-nullable #8332

Merged
merged 1 commit into from
Oct 4, 2019
Merged

[fix] make names non-nullable #8332

merged 1 commit into from
Oct 4, 2019

Conversation

serenajiang
Copy link
Contributor

@serenajiang serenajiang commented Oct 1, 2019 •
edited
Loading

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

If a user somehow creates a table with a null name, 503 errors occur whenever repr(table) is called because __repr__ returns a non-string type.

Fixed schema to make columns referenced by __repr__ non-nullable.

Before running the migration, you must manually fix any records with null clusters.cluster_name, dbs.database_name, or tables.table_name. To find problematic records, run a query like:

SELECT 
    *
FROM 
   tables
WHERE 
   table_name IS NULL

TEST PLAN

Confirmed that after upgrade, I get a warning if I try to insert a row with a null table_name.

Confirmed that after downgrade, I am able to insert a row with a null table_name with no problem.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

REVIEWERS

@graceguo-supercat @etr2460 @john-bodley

john-bodley
john-bodley requested changes Oct 1, 2019
View reviewed changes
Copy link
Member

@john-bodley john-bodley left a comment

Choose a reason for hiding this comment

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

@serenajiang would you mind adding the Alembic migration to the PR?

@codecov-io
Copy link

codecov-io commented Oct 1, 2019 •
edited
Loading

Codecov Report

Merging #8332 into master will increase coverage by 5.52%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8332      +/-   ##
==========================================
+ Coverage    67.7%   73.22%   +5.52%     
==========================================
  Files         447      115     -332     
  Lines       22652    12404   -10248     
  Branches     2363        0    -2363     
==========================================
- Hits        15336     9083    -6253     
+ Misses       7178     3321    -3857     
+ Partials      138        0     -138
Impacted Files Coverage Δ
superset/connectors/druid/models.py 82.41% <100%> (ø) ⬆️
superset/connectors/sqla/models.py 84.96% <100%> (ø) ⬆️
superset/models/core.py 81.29% <100%> (-0.64%) ⬇️
superset/db_engine_specs/mysql.py 43.13% <0%> (-49.02%) ⬇️
superset/db_engine_specs/sqlite.py 54.83% <0%> (-16.13%) ⬇️
superset/dataframe.py 92.12% <0%> (-2.37%) ⬇️
superset/db_engine_specs/base.py 84.38% <0%> (-1%) ⬇️
superset/views/core.py 69.95% <0%> (-0.21%) ⬇️
superset/utils/core.py 88.41% <0%> (-0.17%) ⬇️
... and 332 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 05b6767...acbbbd4. Read the comment docs.

@pull-request-size pull-request-size bot added size/M and removed size/XS labels Oct 2, 2019
@serenajiang serenajiang force-pushed the serena-null-datasource-names branch 3 times, most recently from e3153a0 to 2989c9e Compare October 2, 2019 03:30
john-bodley
john-bodley reviewed Oct 2, 2019
View reviewed changes
superset/migrations/versions/b6fa807eac07_make_datasource_names_non_nullable_py.py Outdated
bind = op.get_bind()
session = db.Session(bind=bind)

session.query(DruidCluster).filter_by(cluster_name=None).update({"cluster_name": "NO_NAME"})
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this as the migration should fail if there are NULL values. Instead please add a line item to UPDATING.md to indicate that this migration may fail (see other examples).

superset/migrations/versions/b6fa807eac07_make_datasource_names_non_nullable_py.py Outdated
session.commit()

op.alter_column('clusters', 'cluster_name',
existing_type=mysql.VARCHAR(length=256),
Copy link
Member

Choose a reason for hiding this comment

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

Please use the generic SQLAlchemy types as used in other migrations.

etr2460
etr2460 reviewed Oct 2, 2019
View reviewed changes
superset/migrations/versions/b6fa807eac07_make_datasource_names_non_nullable.py Outdated


def downgrade():

Copy link
Member

Choose a reason for hiding this comment

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

nit: no newline

@etr2460
Copy link
Member

etr2460 commented Oct 2, 2019

One other request, can you add sql snippets to the PR explaining how to find the bad rows if the migration fails? you can follow the format here as an example: #5451

@etr2460 etr2460 merged commit 65a05ca into apache:master Oct 4, 2019
serenajiang added a commit to serenajiang/incubator-superset that referenced this pull request Oct 9, 2019
@serenajiang
Revert "[fix] make datasource names non-nullable (apache#8332)"
0697f7e 
This reverts commit 65a05ca.
@serenajiang serenajiang mentioned this pull request Oct 9, 2019
Merged
etr2460 pushed a commit that referenced this pull request Oct 9, 2019
@serenajiang
Revert "[fix] make datasource names non-nullable (#8332)" (#8363)
7e7ea3d 
This reverts commit 65a05ca.
@serenajiang serenajiang mentioned this pull request Oct 10, 2019
Merged
12 tasks
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.35.0 labels Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etr2460 etr2460 etr2460 approved these changes

@john-bodley john-bodley john-bodley approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 0.35.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@serenajiang @codecov-io @etr2460 @john-bodley @mistercrunch