Vitor-Avila commented on code in PR #24663:
URL: https://github.com/apache/superset/pull/24663#discussion_r1261958380
##########
superset/datasets/schemas.py:
##########
@@ -204,8 +204,13 @@ def fix_extra(self, data: dict[str, Any], **kwargs: Any)
-> dict[str, Any]:
"""
Fix for extra initially being exported as a string.
"""
- if isinstance(data.get("extra"), str):
- data["extra"] = json.loads(data["extra"])
+ if "extra" in data:
+ extra = data["extra"]
+ if isinstance(extra, str):
+ if extra.strip():
+ data["extra"] = json.loads(extra)
+ else:
+ data["extra"] = {}
Review 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 `extra` field, 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 `extra` is set to a string in the
YAML, its value would be discarded and `extra` would be set to `None` during
import.
We could either:
* Apply validation in the dataset modification form so that it only accepts
json/dict data in the `extra` field: 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 `extra` field 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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
