Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply appropriate type conversion for viz form_data #1334

Closed
wants to merge 0 commits into from

Conversation

leebailey88
Copy link

@leebailey88 leebailey88 commented Oct 13, 2016

Problem

On the Dashboard view, the form_data is passed in as a regular dict. This is then passed as **kwargs to WTForms which as the note below describes doesn't do proper type conversion. u'false' becomes True which is not what we want. This is not currently impacting users because slices always pull data from the json_endpoint but should still be fixed because the data inside the rendered html is inconsistent.

Solution

If passing just one parameter as form_data, the WTForms constructor will do proper type conversion as if the form_data came from a POST. This is why this bug doesn't surface when using the json_endpoint to get slice data, but it does on the dashboard page. The fix is to pass the form_data parameter rather than as **kwargs.

Example reproduction steps:

  1. Open Genders sample data slice in explore page.
  2. Save as a donut chart
  3. Save again as a pie chart
  4. Go to dashboard page, add Genders slice
  5. Inspect data-dashboard in the markup. Note that form_data.donut is true when it should be false.

You have to save as donut and then back to pie to reproduce because initially donut isn't in the params object. You'll note that the Genders slice still renders as a pie correctly without this fix, but only because internally slices render using data from json_endpoint in which form_data always follows the form = form_class(form_data) code path instead of being used as a **kwargs. This fix is still important because the form_data is out of sync with the real values and could be problematic in the future or in the present to people who are extending Caravel to use form_data now (which is how I discovered it).

Note Backing-store objects and kwargs are both expected to be provided with the values being already-coerced datatypes. WTForms does not check the types of incoming object-data or coerce them like it will for formdata as it is expected this data is defaults or data from a backing store which this form represents. See the section on using Forms for more information.

WTForms docs

@mistercrunch

@leebailey88 leebailey88 closed this Nov 2, 2016
zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 17, 2021
zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 24, 2021
zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 25, 2021
zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant