fix: Fix Uniqueness check before update for Sqllab Overwrites #16859
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
9 tasks
5279de7 to
6103252
Compare
6103252 to
f6c30fc
Compare
AAfghahi
approved these changes
Sep 27, 2021
betodealmeida
requested changes
Sep 27, 2021
27c19df to
956b2e6
Compare
Codecov Report
@@ Coverage Diff @@
## master #16859 +/- ##
==========================================
+ Coverage 77.05% 77.16% +0.11%
==========================================
Files 1021 1021
Lines 54693 56341 +1648
Branches 7457 7679 +222
==========================================
+ Hits 42141 43475 +1334
- Misses 12307 12618 +311
- Partials 245 248 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
superset/connectors/sqla/models.py
Outdated
| @@ -1662,9 +1663,9 @@ def before_update( | |||
|
|
|||
| # Check whether the relevant attributes have changed. | |||
| state = db.inspect(target) # pylint: disable=no-member | |||
|
|
|||
| for attr in ["database_id", "schema", "table_name"]: | |||
| for attr in ["schema", "table_name"]: | |||
There was a problem hiding this comment.
We shouldn't remove database_id here, otherwise someone might create a dataset with the same database ID, schema and table name. We should figure out why database ID is coming as null when updating.
956b2e6 to
08d2a94
Compare
db851ed to
0fc7454
Compare
9c9dbd1 to
8f49122
Compare
8f49122 to
68c8d3c
Compare
betodealmeida
approved these changes
Sep 29, 2021
3 tasks
opus-42
pushed a commit
to opus-42/incubator-superset
that referenced
this pull request
Nov 14, 2021
…#16859) * add condition to make sure columns are available before throwing * fix * remove database_id from history changed * refactor update call to datasets * cleanup * oops * prettier
QAlexBall
pushed a commit
to QAlexBall/superset
that referenced
this pull request
Dec 28, 2021
…#16859) * add condition to make sure columns are available before throwing * fix * remove database_id from history changed * refactor update call to datasets * cleanup * oops * prettier
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
Inside the
before_updateevent listener whenever a user would overwrite a dataset from sqllab we'd get'committed_state': {'database_id': symbol('NO_VALUE')},. This is causing update (PUT) request to fail in sqllab to fix this we've updated the network call to not includedatabase_idsince it isn't mandatory.Also did a small refactor to remove
src/api/datasets.tsBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION