fix: set template_params to null on empty string #33547
Conversation
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
I've completed my review and didn't find any issues.
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/src/components/Datasource/DatasourceModal.tsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| @@ -140,7 +140,7 @@ const DatasourceModal: FunctionComponent<DatasourceModalProps> = ({ | |||
| cache_timeout: | |||
| datasource.cache_timeout === '' ? null : datasource.cache_timeout, | |||
| is_sqllab_view: datasource.is_sqllab_view, | |||
| template_params: datasource.template_params, | |||
| template_params: datasource.template_params === '' ? null : datasource.template_params, | |||
There was a problem hiding this comment.
ideally we align the types and avoid the lazy Record<string, any> set above, seems the right type is here -> https://github.com/apache/superset/blob/master/superset-frontend/src/features/datasets/types.ts#L72 , let's use it at line 123
Now should template_params be a string? Unclear, maybe, maybe not, ideally the components where that came from always returns a proper JSON serializable, but that's probably more work.
Now if the backend is in charge of serializing the string, if feels like the fix should be done there ideally. Not again the quickfix here but should include a // TODO change type to {...} and push validation in the component.
Trying to figure out where in the backend the serialization is done ...do you have a stacktrace of the backend failing?
|
Sorry for the slow response, I was AFK for a couple of a days. When running things locally using the latest version of master I get the following error. The information about "overwrite=true" feels unimportant, the import fails before the frontend can provide with me with the option.
I believe the error occurs when marshmallow tries to validate the empty string as a dict. This would be the line: https://github.com/apache/superset/blob/master/superset/datasets/schemas.py#L285. |
|
Looks like pre-commit needs to be resolved at least... Superset uses Git pre-commit hooks courtesy of pre-commit. To install run the following: A series of checks will now run when you make a git commit. Alternatively it is possible to run pre-commit by running pre-commit manually: |
SUMMARY
When editing Template Parameters in the frontend the template_parameters key on a dataset gets set to empty string when saving empty data. This breaks export->import since the import validation expects a python dict or nothing. This PR only updates the frontend to not send empty strings to the backend.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
When attempting to upload a Dashboard with a dataset which has had its template parameters removed before exporting.
Network tab for the PUT operation.
Network tab for the PUT operation with the change.
TESTING INSTRUCTIONS
Before the fix this should fail, with the fix this should work.
ADDITIONAL INFORMATION
I wanted a proper fix in the backend, but the data gets read/written directly from the database from what I can tell. This is a dirty quick fix that should help some people.