fix(dataset-import): support empty strings for extra fields #24663
Conversation
* fix(dataset-import):support empty strings for extra fields * Adding unit test * black update
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Codecov Report
@@ Coverage Diff @@
## master #24663 +/- ##
==========================================
- Coverage 69.05% 68.97% -0.08%
==========================================
Files 1907 1902 -5
Lines 74151 74011 -140
Branches 8182 8186 +4
==========================================
- Hits 51204 51049 -155
- Misses 20824 20841 +17
+ Partials 2123 2121 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
superset/datasets/schemas.py
Outdated
| if extra.strip(): | ||
| data["extra"] = json.loads(extra) | ||
| else: | ||
| data["extra"] = {} |
There was a problem hiding this comment.
You may want to try the EAFP approach here.. it could simplify this logic. Something like:
| data["extra"] = {} | |
| if isinstance(data.get("extra"), str): | |
| try: | |
| data["extra"] = json.loads(data["extra"]) | |
| except JSONDecodeError: | |
| data["extra"] = {} |
There was a problem hiding this comment.
@john-bodley had also suggested that we could try to fix the underlying code in order to make the data structures consistent. Unless you're feeling ambitious in this PR we could also fix the default in the import to be the correct format in a subsequent PR. I assume it should be an empty dictionary?
There was a problem hiding this comment.
Of course @betodealmeida's solution with the ternary is even shorter. :)
There was a problem hiding this comment.
I ended up using try/except, because if we use the walrus operator and the extra field is an empty string the if block wouldn't be executed and the import would still fail. Also defaulted the extra value to None since the schema actually accepts it.
I 100% agree this PR only fixes the "symptom" but doesn't address the cause. I can try to work on that in a future PR. Some thoughts to consider:
- Currently the dataset modification modal doesn't validate the text added to the
extrafield, so users can add strings. - The string would be successfully exported, but then the import would fail until this PR. Once this gets merged, if the
extrais set to a string in the YAML, its value would be discarded andextrawould be set toNoneduring import.
We could either:
- Apply validation in the dataset modification form so that it only accepts json/dict data in the
extrafield: I think it's a stable solution and prevents future data type variations. However, users that currently have a string set could struggle to update the datasets in case the error message is not clear. - Modify the dataset schema so that the
extrafield accepts either a dictionary or a string: not really helpful in ensuring a data type, but should allow users with strings to import their existing datasets and reflect the information.
@betodealmeida @eschutho @john-bodley any thoughts? Also if you could approve the tests, that would be awesome! Thanks!
* fix(dataset-import):support empty strings for extra fields * Adding unit tests * black update * Simplifying logic * Updating tests
|
This is great, thanks @Vitor-Avila! |
michael-s-molina
added
the
v3.0
(cherry picked from commit 65fb8e1)
mistercrunch
added
🍒 3.0.0
🍒 3.0.1
🍒 3.0.2
🍒 3.0.3
🍒 3.0.4
🏷️ bot
SUMMARY
During the import process, the
extravalue for datasets is loaded as a dictionary. This operation fails in case theextrakey has an empty string (extra: "").BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
extravalue (in the UI).extra: "".ADDITIONAL INFORMATION