-
Notifications
You must be signed in to change notification settings - Fork 14k
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: set correct schema on config import #16041
Conversation
Codecov Report
@@ Coverage Diff @@
## master #16041 +/- ##
==========================================
- Coverage 77.08% 76.95% -0.13%
==========================================
Files 1037 1037
Lines 55647 55690 +43
Branches 7608 7608
==========================================
- Hits 42898 42859 -39
- Misses 12499 12581 +82
Partials 250 250
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
e52fe12
to
836f7e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any way in the future to dry up some of the duplication?
@betodealmeida Hi Beto! |
d20ac3c
to
525fb5f
Compare
525fb5f
to
0ec0b45
Compare
6454c83
to
1e77343
Compare
1e77343
to
d9bd35b
Compare
83c131e
to
2e15b0f
Compare
2e15b0f
to
00cb584
Compare
83fff75
to
af4e3c8
Compare
af4e3c8
to
c8c5868
Compare
* fix: set correct schema on config import * Fix lint * Fix test * Fix tests * Fix another test * Fix another test * Fix base test * Add helper function * Fix examples * Fix test * Fix test * Fixing more tests (cherry picked from commit 1fbce88)
* fix: set correct schema on config import * Fix lint * Fix test * Fix tests * Fix another test * Fix another test * Fix base test * Add helper function * Fix examples * Fix test * Fix test * Fixing more tests
SUMMARY
When we import examples the schema is set to
null
, even in databases that support schemas and have a default schema. This is problematic, because users might create duplicate physical datasets, which are usually not allowed, resulting in, eg:examples.NULL.birth_names
(actually in the default "public" schema, for Postgres)examples.public.birth_names
These duplicate datasets have caused problems in the past, because they break assumptions about physical datasets.
This PR changes the
load-examples
command and the import mechanism to explicitly set the schema to the default one when one is not set.There's a tricky case that this PR needs to handle: when a user creates a dataset duplicating one of the new examples defined in YAML files. These datasets have fixed UUIDs, and the following happens:
superset load-examples
.users
is created withtable_name=users
,schema=null
anduuid=7195db6b-2d17-7619-b7c7-26b15378df8c
.users
. When adding a dataset we force the user to select a schema, so this dataset hastable_name=users
,schema=public
(in Postgres) and a random UUID. Normally you can't create duplicate physical datasets, but because the schema is technically different this works.superset load-examples
again, after merging this PR.users
dataset matches the one with NULL schema, based on the UUID. it tries to update that dataset, setting its schema to "public".database_id
,table_name
andschema
(the one created by the user).This is a rare case, but it can happen. I added a workaround so that when this happens the duplicate datasets are not updated. This could be potentially problematic in the future, if we add more columns to a dataset and have charts use those columns; then the charts would fail to render because the dataset wouldn't be updated.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TESTING INSTRUCTIONS
superset load-examples
onmaster
.superset load-examples
on this branch.Same for clean installations. I tested with Postgresql, MySQL, and SQLite.
ADDITIONAL INFORMATION