superset icon indicating copy to clipboard operation
superset copied to clipboard

fix: set template_params to null on empty string

Open castodius opened this issue 7 months ago • 2 comments

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. GUI before

Network tab for the PUT operation. before_network

Network tab for the PUT operation with the change. after_network

TESTING INSTRUCTIONS

  1. Edit a dataset belonging to a Dashboard
  2. Add template parameters
  3. Remove them
  4. Export the Dashboard
  5. Import the Dashboard

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.

  • [x] Has associated issue: Maybe https://github.com/apache/superset/issues/27027
  • [ ] Required feature flags:
  • [ ] Changes UI
  • [ ] Includes DB Migration (follow approval process in SIP-59)
    • [ ] Migration is atomic, supports rollback & is backwards-compatible
    • [ ] Confirm DB migration upgrade and downgrade tested
    • [ ] Runtime estimates and downtime expectations provided
  • [ ] Introduces new feature or API
  • [ ] Removes existing feature or API

castodius avatar May 21 '25 08:05 castodius

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.

superset_app | 2025-05-27 06:40:02,296:WARNING:superset.commands.importers.v1:Import Error: {'template_params': ['Not a valid mapping type.']} superset_app | 2025-05-27 06:40:02,296:WARNING:superset.commands.importers.v1:Import Error: {'dashboards/FCC_New_Coder_Survey_2018_15.yaml': 'Dashboard already exists and overwrite=true was not passed'} superset_app | 2025-05-27 06:40:02,296 INFO sqlalchemy.engine.Engine ROLLBACK superset_app | 2025-05-27 06:40:02,296:INFO:sqlalchemy.engine.Engine:ROLLBACK superset_app | 2025-05-27 06:40:02,296:INFO:superset.commands.dashboard.importers.dispatcher:Command failed validation superset_app | 2025-05-27 06:40:02,296:WARNING:superset.views.error_handling:CommandException superset_app | Traceback (most recent call last): superset_app | File "/app/.venv/lib/python3.11/site-packages/flask/app.py", line 1484, in full_dispatch_request superset_app | rv = self.dispatch_request() superset_app | ^^^^^^^^^^^^^^^^^^^^^^^ superset_app | File "/app/.venv/lib/python3.11/site-packages/flask/app.py", line 1469, in dispatch_request superset_app | return self.ensure_sync(self.view_functions[rule.endpoint])(**view_args) superset_app | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ superset_app | File "/app/.venv/lib/python3.11/site-packages/flask_appbuilder/security/decorators.py", line 109, in wraps superset_app | return f(self, *args, **kwargs) superset_app | ^^^^^^^^^^^^^^^^^^^^^^^^ superset_app | File "/app/superset/views/base_api.py", line 120, in wraps superset_app | duration, response = time_function(f, self, *args, **kwargs) superset_app | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ superset_app | File "/app/superset/utils/core.py", line 1371, in time_function superset_app | response = func(*args, **kwargs) superset_app | ^^^^^^^^^^^^^^^^^^^^^ superset_app | File "/app/superset/utils/log.py", line 304, in wrapper superset_app | value = f(*args, **kwargs) superset_app | ^^^^^^^^^^^^^^^^^^ superset_app | File "/app/superset/views/base_api.py", line 107, in wraps superset_app | return f(self, *args, **kwargs) superset_app | ^^^^^^^^^^^^^^^^^^^^^^^^ superset_app | File "/app/superset/dashboards/api.py", line 1589, in import_ superset_app | command.run() superset_app | File "/app/superset/commands/dashboard/importers/dispatcher.py", line 57, in run superset_app | command.run() superset_app | File "/app/superset/utils/decorators.py", line 271, in wrapped superset_app | return on_error(ex) superset_app | ^^^^^^^^^^^^ superset_app | File "/app/superset/utils/decorators.py", line 236, in on_error superset_app | raise ex superset_app | File "/app/superset/utils/decorators.py", line 264, in wrapped superset_app | result = func(*args, **kwargs) superset_app | ^^^^^^^^^^^^^^^^^^^^^ superset_app | File "/app/superset/commands/importers/v1/init.py", line 84, in run superset_app | self.validate() superset_app | File "/app/superset/commands/importers/v1/init.py", line 120, in validate superset_app | raise CommandInvalidError( superset_app | superset.commands.exceptions.CommandInvalidError: Error importing dashboard superset_app | 2025-05-27 06:40:02,302:INFO:werkzeug:192.168.65.1 - - [27/May/2025 06:40:02] "POST /api/v1/dashboard/import/ HTTP/1.1" 422 -

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.

castodius avatar May 27 '25 06:05 castodius

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:

pip3 install -r requirements/development.txt
pre-commit install

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:

pre-commit run --all-files

rusackas avatar Jun 10 '25 20:06 rusackas

Hi @castodius 👋

This PR has been inactive for about 6 months. The fix looks valuable for the import/export workflow.

Current blockers:

  • 🔀 Merge conflicts need to be resolved
  • 🔧 Pre-commit checks need to pass (as noted in the previous comment)

Are you still interested in completing this PR? If so, please:

  1. Rebase on master to resolve conflicts
  2. Run pre-commit run --all-files and commit any fixes

If you're no longer able to work on this, please let us know and we can either close this or have someone else pick it up.

Thanks for your contribution! 🙏

rusackas avatar Dec 11 '25 22:12 rusackas

Converting to draft due to merge conflicts and pending pre-commit fixes. Please mark as ready for review once resolved.

rusackas avatar Dec 11 '25 22:12 rusackas