Fix form data issue switching viz types #5100

michellethomas · 2018-05-29T22:24:45Z

We've been having an issue with extra params getting into the form data (that are not valid controls for a particular viz type), this breaks the query and there's no way to remove it from the controls. This happens when you switch from a time series with a group by to a big number. The group by in the rawFormData will get added to the form_data and create a query that's invalid for a big number chart (that doesn't have a group by control).

It looks like this was introduced here so I'm assuming that the only extra form data param is our url params section. If this is not true let me know.

@graceguo-supercat @mistercrunch

codecov-io · 2018-05-29T23:06:30Z

Codecov Report

Merging #5100 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #5100      +/-   ##
==========================================
+ Coverage   63.49%   63.51%   +0.02%     
==========================================
  Files         360      360              
  Lines       22894    22888       -6     
  Branches     2551     2549       -2     
==========================================
+ Hits        14537    14538       +1     
+ Misses       8342     8335       -7     
  Partials       15       15

Impacted Files	Coverage Δ
superset/assets/src/explore/store.js	`23.07% <ø> (+1.12%)`	⬆️
superset/assets/src/explore/controls.jsx	`45.92% <ø> (ø)`	⬆️
superset/assets/src/explore/visTypes.jsx	`57.14% <ø> (ø)`	⬆️
...ts/src/explore/components/ExploreViewContainer.jsx	`0% <ø> (ø)`	⬆️
superset/views/core.py	`73.99% <100%> (+0.01%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e3b2e7...f7c48da. Read the comment docs.

michellethomas · 2018-05-29T23:49:55Z

Actually this seems to solve part of the problem but not all of it. This is still a WIP.

michellethomas · 2018-05-30T19:05:58Z

Ok got it working now, sorry I had a few changes I thought were maybe not necessary but they were. This is my initial thought on how to solve this problem but I'm not sure if there is a better place to do this?

mistercrunch · 2018-06-01T05:26:00Z

superset/assets/src/explore/components/ExploreViewContainer.jsx

@@ -299,14 +300,32 @@ class ExploreViewContainer extends React.Component {
 ExploreViewContainer.propTypes = propTypes;

 function mapStateToProps({ explore, charts, impressionId }) {
-  const form_data = getFormDataFromControls(explore.controls);
+  const rawFormDataControls = getControlsState(explore, explore.rawFormData);


is this constant used anywhere?

mistercrunch · 2018-06-01T05:27:46Z

superset/assets/src/explore/components/ExploreViewContainer.jsx

-  const form_data = getFormDataFromControls(explore.controls);
+  const rawFormDataControls = getControlsState(explore, explore.rawFormData);
+  const controls = Object.assign({}, explore.controls);
+  const formData = getFormDataFromControls(controls);


I think you can just getFormDataFromControls(explore.controls); here and remove the previous line

mistercrunch · 2018-06-01T05:38:10Z

It's becoming really hard to reason about how formData is flowing around the app, and I feel like this is adding yet another layer of complexity. I'd love to think of ways to fix the issue by removing a layer of complexity instead of adding one.

I'm thinking we almost need to take the time and whiteboard how formData is flowing and think how to simplify things. Also ExploreViewContainer probably isn't the right place to accumulate all this logic.

Maybe we can take some time to talk about this at tomorrow's meeting.

michellethomas · 2018-06-15T02:18:40Z

@mistercrunch I've started to document where we are changing form data. Before getting this in, I may think about how we handle formData on the front end and if there are any ways to clean it up. I don't have a clear picture of it yet, but we can discuss more at the meeting tomorrow.

michellethomas · 2018-06-20T23:30:09Z

@mistercrunch

I was experimenting with making some changes to how formData is stored/ flows around, but I found a simpler way to solve this problem. I'm making url_params a hidden control so that it will get stored in the explore controls and we don't need to change anything about the formData.

On the python side, there's a problem where we were adding slice params to the form data slice_form_data.update(form_data) then form_data = slice_form_data, which would override any params that still applied to this viz type but would persist extra params that no longer applied. There's no way to validate that params are valid for the viz type on the backend, so we should be careful when adding slice params to the form_data.

It looks like this change was added here. And in some of the tests in that PR (and in PRs since) we've added the ability to add in specific form_data overrides to explore_json. Ideally it seems the data flow should be to pull the slice info on explore, then let the front end validate the controls from the raw form data, and always send the full validated form_data to explore_json (and not add much to form_data at that point). But in some of these tests like test_slice_url_overrides we pass slice_id and specific form data overrides to explore_json expecting get_form_data to pull the full slice params and override a specific param. Would we ever make a call to explore_json with form data like {'slice_id': 51, 'groupby': ['state']} or do we need to fix the tests to pass through full form_data?

john-bodley · 2018-06-21T19:08:51Z

superset/views/core.py

-        if slice_id:
+        # Include the slice_form_data if form_data contains no datasource
+        datasource_info = form_data.get('datasource') or datasource_id
+        if slice_id and (use_slice_data or not datasource_info):


datasource_info is the name of a local method and thus you probably should use a different name. Does form_data.get('datasource') contain the datasource ID? I'm asking to try to understand why/when the form-data would reference a different datasource.

michellethomas · 2018-06-29T20:31:35Z

After our conversation I'm deprecating the undocumented feature for using overrides in explore_json. Explore_json should take in the full form_data and if someone in the future wants that feature they should add another endpoint that's similar to explore_json but calls get_form_data(use_slice_data=True).

I tried to only use the request.url when deciding whether to include the slice params but that breaks saving because the url is /explore/ but use_slice_data should be False. This didn't break the tests so I added an additional check to make sure assert slc.viz.form_data == form_data.

I updated the tests so they don't use overrides with explore_json calls, and instead make calls with full form_data. In your change Max, slc.form_data adds datasource to the form_data before returning it, but slc.viz.form_data does not contain datasource because we just set it to the slice params. So also changed tests to use slc.form_data.

michellethomas · 2018-07-10T23:37:50Z

@mistercrunch can you take a look?

mistercrunch · 2018-07-18T16:54:45Z

superset/assets/src/explore/store.js

@@ -104,13 +104,6 @@ export function applyDefaultFormData(form_data) {
      formData[k] = form_data[k];
    }
  });
-  // fill in additional params stored in form_data but not used by control
-  Object.keys(form_data)


Doing forensics here: @graceguo-supercat added this as part of [Explore] Save custom url parameters when user save slices (#4578)

mistercrunch · 2018-07-18T16:56:52Z

superset/assets/src/explore/components/ExploreViewContainer.jsx

@@ -292,12 +292,6 @@ ExploreViewContainer.propTypes = propTypes;

 function mapStateToProps({ explore, charts, impressionId }) {
  const form_data = getFormDataFromControls(explore.controls);
-  // fill in additional params stored in form_data but not used by control


@graceguo-supercat added this as part of 9edbd64c5

mistercrunch · 2018-07-18T16:59:16Z

I think this is fixing issues introduced by #4578, so deferring to @graceguo-supercat .

I've hit the bugs quite a few times and we should get this through quickly

michellethomas · 2018-07-20T17:32:00Z

@graceguo-supercat after our discussion I made the change so explore_json can accept form data with only slice id.

Slice params now get added on explore, slice, or explore_json where form_data only contains slice id. This is so that components can make calls outside of explore view (to just get the slice data). But we are still not allowing calls to explore_json with any overrides along with slice_id.

* Fixing issue with extra params in formData * Pass in param use_slice_data to decide whether to use slice data * Fixing core_tests to not use explore_json overrides (cherry picked from commit bf0afef)

* Fixing issue with extra params in formData * Pass in param use_slice_data to decide whether to use slice data * Fixing core_tests to not use explore_json overrides

michellethomas force-pushed the fix_fd_issue_switching_viz branch from 67b332f to 455a6aa Compare May 30, 2018 00:33

mistercrunch reviewed Jun 1, 2018

View reviewed changes

michellethomas force-pushed the fix_fd_issue_switching_viz branch 4 times, most recently from b616958 to 3ad80e6 Compare June 20, 2018 22:55

john-bodley reviewed Jun 21, 2018

View reviewed changes

michellethomas force-pushed the fix_fd_issue_switching_viz branch 2 times, most recently from 67209ac to fa60f9a Compare June 29, 2018 19:55

michellethomas force-pushed the fix_fd_issue_switching_viz branch from fa60f9a to c835031 Compare June 29, 2018 20:33

mistercrunch reviewed Jul 18, 2018

View reviewed changes

michellethomas force-pushed the fix_fd_issue_switching_viz branch 2 times, most recently from 1e21ddf to 7081b4c Compare July 20, 2018 01:29

michellethomas force-pushed the fix_fd_issue_switching_viz branch from 7081b4c to c02b65b Compare August 9, 2018 18:06

michellethomas added 3 commits August 15, 2018 10:15

Fixing issue with extra params in formData

05eed44

Pass in param use_slice_data to decide whether to use slice data

ffe9202

Fixing core_tests to not use explore_json overrides

f7c48da

michellethomas force-pushed the fix_fd_issue_switching_viz branch from c02b65b to f7c48da Compare August 15, 2018 17:16

hajdbo mentioned this pull request Sep 17, 2018

[explore] fix the 'altered' tag false positive #5636

Closed

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix form data issue switching viz types #5100

Fix form data issue switching viz types #5100

michellethomas commented May 29, 2018 •

edited

Loading

codecov-io commented May 29, 2018 •

edited

Loading

michellethomas commented May 29, 2018

michellethomas commented May 30, 2018

mistercrunch Jun 1, 2018

mistercrunch Jun 1, 2018

mistercrunch commented Jun 1, 2018

michellethomas commented Jun 15, 2018

michellethomas commented Jun 20, 2018

john-bodley Jun 21, 2018 •

edited

Loading

michellethomas commented Jun 29, 2018

michellethomas commented Jul 10, 2018

mistercrunch Jul 18, 2018

mistercrunch Jul 18, 2018

mistercrunch commented Jul 18, 2018

michellethomas commented Jul 20, 2018

Fix form data issue switching viz types #5100

Fix form data issue switching viz types #5100

Conversation

michellethomas commented May 29, 2018 • edited Loading

codecov-io commented May 29, 2018 • edited Loading

Codecov Report

michellethomas commented May 29, 2018

michellethomas commented May 30, 2018

mistercrunch Jun 1, 2018

Choose a reason for hiding this comment

mistercrunch Jun 1, 2018

Choose a reason for hiding this comment

mistercrunch commented Jun 1, 2018

michellethomas commented Jun 15, 2018

michellethomas commented Jun 20, 2018

john-bodley Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

michellethomas commented Jun 29, 2018

michellethomas commented Jul 10, 2018

mistercrunch Jul 18, 2018

Choose a reason for hiding this comment

mistercrunch Jul 18, 2018

Choose a reason for hiding this comment

mistercrunch commented Jul 18, 2018

michellethomas commented Jul 20, 2018

michellethomas commented May 29, 2018 •

edited

Loading

codecov-io commented May 29, 2018 •

edited

Loading

john-bodley Jun 21, 2018 •

edited

Loading