[Explore view] Use POST method for charting requests #3993

graceguo-supercat · 2017-12-03T01:06:06Z

kkalyan · 2017-12-03T05:16:04Z

@graceguo-supercat can please we have an option to disable this? GET requests have some benefits like parameterizable embedded iframes.

graceguo-supercat · 2017-12-03T16:44:21Z

@kkalyan this feature should be backward-compatible with previous work. Note we build post body for explore_json endpoint, but query parameters(GET) are given precedence than the POST body.

Explore view should support:

accessed by slice id,
accessed with full form_data parameter
accessed by shared url id, like /r/20
parameterizable embedded iframes, see the iframe generator icon in the chart header
download data in .csv, .json format

if you see any use cases are broken by this feature, please let me know. Thank you!

kkalyan · 2017-12-03T20:30:17Z

Thanks, @graceguo-supercat! I'll test and report if there are any issues.

fabianmenges · 2017-12-04T20:00:04Z

superset/views/core.py

@@ -1056,13 +1056,14 @@ def get_query_string_response(self, viz_obj):

    @log_this
    @has_access_api
-    @expose('/explore_json/<datasource_type>/<datasource_id>/')
+    @expose('/explore_json/<datasource_type>/<datasource_id>/', methods=['POST'])


While you are in here... I would love to get rid of the trailing / everywhere
basically add another line
@expose('/explore_json/<datasource_type>/<datasource_id>', methods=['POST'])

fabianmenges · 2017-12-04T20:03:40Z

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

+  addHistory(isReplace = false) {
+    const { url, payload } = createExplore(this.props.form_data);
+    if (isReplace) {
+      history.replaceState(


I can't figure out where history is defined.

history is window.history, html5 Web API:
https://developer.mozilla.org/en-US/docs/Web/API/Window/history

fabianmenges · 2017-12-04T20:07:42Z

superset/views/core.py

@@ -1128,18 +1129,32 @@ def explorev2(self, datasource_type, datasource_id):

    @log_this
    @has_access
-    @expose('/explore/<datasource_type>/<datasource_id>/')
+    @expose('/explore/<datasource_type>/<datasource_id>/', methods=['GET', 'POST'])


Is there a reason we have these trailing / everywhere? Can we (start) to get rid of them?
e.g. Add another line
@expose('/explore/<datasource_type>/<datasource_id>', methods=['GET', 'POST'])

I asked Max about this, he feel this is just flask url style. But because of backward compatibility, I can't remove the trailing /, otherwise users' saved explore url won't work.

Trailing slashes are pretty common in the Django / Python world.

williaster · 2017-12-12T19:14:02Z

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

@@ -101,13 +106,43 @@ class ExploreViewContainer extends React.Component {
    }
  }

+  addHistory(isReplace = false, title) {


I think object signatures are a little more readable + less error prone, thoughts? e.g., ({ isReplace = false, title })

williaster · 2017-12-12T19:15:03Z

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

+        longUrl);
+    }
+
+    // it seems browsers don't support pushState title attribute


some do right? otherwise it wouldn't be an input to push/replaceState? maybe caveat with "some browsers"?

williaster · 2017-12-12T19:17:40Z

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

+    const formData = history.state;
+    if (formData && Object.keys(formData).length) {
+      this.props.actions.setExploreControls(formData);
+      this.props.actions.runQuery(formData, false,


nit -- I would do newlines for each var here.

williaster · 2017-12-12T19:19:04Z

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

  handleResize() {
    clearTimeout(this.resizeTimer);
    this.resizeTimer = setTimeout(() => {
      this.setState({ height: this.getHeight(), width: this.getWidth() });
    }, 250);
  }

+  handleNavigate() {


Navigate is a little ambiguous, what do you think about mirroring the listener: handlePopstate?

williaster · 2017-12-12T19:24:14Z

superset/assets/javascripts/explore/exploreUtils.js

+  return uri.directory(directory).search(search).toString();
+}
+
+export function createExplore(form_data, endpointType = 'base', force = false,


same thoughts about an object signature (less error-prone and calls are more readable), also could we use camelCase? ({ formData, endpointType = 'base', force = false, currUrl = null, requestParams = {} })

could we make this method name more clear? like getExploreUrlAndPayload it took me a while to understand what getExplore meant.

fixed. used object signatures for getExploreUrlAndPayload method

williaster · 2017-12-12T19:33:47Z

superset/assets/spec/javascripts/explore/utils_spec.jsx

@@ -12,48 +13,84 @@ describe('utils', () => {
    expect(uri1.toString()).to.equal(uri2.toString());
  }

-  it('getExploreUrl generates proper base url', () => {
+  it('createExplore generates proper base url', () => {


prob outside the scope of this PR but just a thought for test organization, if you break out a describe('specific utils func...', () => { ... }) you wouldn't have to repeat the name for each test and could say expect('it ....').

williaster · 2017-12-12T19:38:47Z

superset/views/core.py

+                url_form_data.update(form_data)
+                form_data = url_form_data
+
+        slice_id = form_data.get('slice_id')
        slc = None
        if slice_id:


probably not ever really an issue but should this technically be a elif now / mutually exclusive with the shortened url?

I want to allow form_date in request override saved url.
for example, if request say /r=123&form_data={filter: {a:1,b:2}}, the request parameter can overwrite saved form_data.

williaster · 2018-01-17T00:57:12Z

superset/assets/javascripts/SqlLab/components/VisualizeModal.jsx

@@ -9,7 +9,7 @@ import { Alert, Button, Col, Modal } from 'react-bootstrap';
 import Select from 'react-select';
 import { Table } from 'reactable';
 import shortid from 'shortid';
-import { getExploreUrl } from '../../explore/exploreUtils';
+import { exportChart } from '../../explore/exploreUtils';


should this be called exploreChart? not export? if the default behavior is not to export the data, it seems like explore is more readable.

it's actually an action. it exports (download) .csv file, or opens .json file in another tab.

williaster · 2018-01-17T01:04:24Z

superset/assets/javascripts/dashboard/components/SliceHeader.jsx

@@ -129,7 +130,7 @@ class SliceHeader extends React.PureComponent {
                  <i className="fa fa-pencil" />
                </TooltipWrapper>
              </a>
-              <a className="exportCSV" href={this.props.exportCSVUrl}>
+              <a className="exportCSV" onClick={() => (this.props.exportCSV(slice))}>


ideally these would be bound to the component but I see there is a precedent in other calls to create a new function on every render.

fixed. now i use bound callbacks.

williaster · 2018-01-17T01:11:24Z

superset/assets/javascripts/explore/components/EmbedCodeButton.jsx

@@ -29,7 +30,7 @@ export default class EmbedCodeButton extends React.Component {
  generateEmbedHTML() {
    const srcLink = (
      window.location.origin +


it's strange to use string templates and concatenation. can we just use the template?

if I use single line template, it will be too long and break lint rule.
so i have to make it multiple lines.

williaster · 2018-01-17T01:13:12Z

superset/assets/javascripts/explore/components/ExploreActionButtons.jsx

@@ -37,7 +38,7 @@ export default function ExploreActionButtons({
        </a>

        <a
-          href={slice.data.csv_endpoint}
+          onClick={() => { exportChart(latestQueryFormData, 'csv'); }}


ditto bound functions but not a blocker

fixed. now use bound callback function.

williaster · 2018-01-17T01:14:20Z

superset/assets/javascripts/explore/components/ExploreChartHeader.jsx

      .then((data) => {
        if (isNewSlice) {
          this.props.actions.createNewSlice(
            data.can_add, data.can_download, data.can_overwrite,
            data.slice, data.form_data);
+          this.props.addHistory({ isReplace: true, title: '[slice] ' + data.slice.slice_name });


nit - template literals? there's inconsistent usage in this PR.

fixed. now use template.

williaster · 2018-01-17T01:39:34Z

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

+    if (isReplace) {
+      history.replaceState(
+        payload,
+        document.title,


should this be title instead of document.title? or make the document.title = title; call before these?

should be title. fixed.

williaster · 2018-01-17T01:44:54Z

superset/assets/spec/javascripts/explore/utils_spec.jsx

@@ -12,48 +13,124 @@ describe('utils', () => {
    expect(uri1.toString()).to.equal(uri2.toString());
  }

-  it('getExploreUrl generates proper base url', () => {
+  it('generates proper base url', () => {


do you think the tests should include the util func name? you do this inconsistently here and less readable if not. you could also create a dedicated describe block for a given func if you want to omit the name.

fixed. i add describe section.

williaster · 2018-01-17T01:48:02Z

superset/assets/spec/javascripts/explore/utils_spec.jsx

+  });
+  it('getExploreLongUrl generates proper base url with form_data', () => {
+    compareURI(
+      URI(getExploreLongUrl(formData, 'base', false, 'http://superset.com')),


are the last two arguments used by the getExploreLongUrl func? I think functions that take an object are much less error-prone than relying on positional arguments.

last 2 arguments are not used! fixed.

williaster · 2018-01-17T01:54:50Z

superset/assets/javascripts/explore/exploreUtils.js

+  };
+}
+
+export function exportChart(formData, endpointType) {


seems like a mix of positional + named in this PR, big fan of consistently using the latter (which is used in getExploreUrlAndPayload for instance) but not a blocker.

mistercrunch · 2018-01-20T01:50:40Z

superset/views/core.py

-        # get form data from url
+        d = {}
+        # Supporting POST
+        if request.form.get('form_data'):


nit: it'd be better to not request.form.get('form_data') many times. First affect to a variable and then use the var.

mistercrunch · 2018-01-20T01:52:54Z

superset/views/core.py

-            form_data = '{}'
-
-        d = json.loads(form_data)
+            request_param = request.args.get('form_data')


same as previous comment

mistercrunch · 2018-01-20T02:01:47Z

Had a few minor notes, but otherwise LGTM. I'm assuming it's been extensively tested in your staging prior to merging.

graceguo-supercat · 2018-01-24T19:54:35Z

confirmed. i manually tested top 20+ dashboard and more slices in dev box with production data.

mistercrunch · 2018-02-04T04:34:18Z

Cool then, feel free to merge once the conflicts are resolved.

…into gg-ShortenURL

graceguo-supercat · 2018-02-14T01:21:26Z

Merged.

* [Explore view] Use POST method for charting requests * fix per code review comments * more code review fixes * code review fix: remove duplicated calls for getting values from request * [Explore view] Use POST method for charting requests * fix per code review comments * more code review fixes * code review fix: remove duplicated calls for getting values from request

graceguo-supercat requested a review from mistercrunch December 3, 2017 01:06

fabianmenges reviewed Dec 4, 2017

View reviewed changes

graceguo-supercat force-pushed the gg-ShortenURL branch from d55f84c to 39a3ae4 Compare December 10, 2017 01:00

williaster reviewed Dec 12, 2017

View reviewed changes

graceguo-supercat force-pushed the gg-ShortenURL branch from d4b74da to 985969a Compare December 19, 2017 07:01

williaster reviewed Jan 17, 2018

View reviewed changes

Grace Guo added 3 commits January 18, 2018 01:07

[Explore view] Use POST method for charting requests

3496911

fix per code review comments

87bd7b8

more code review fixes

bccb2ef

graceguo-supercat force-pushed the gg-ShortenURL branch from 985969a to bccb2ef Compare January 18, 2018 09:07

mistercrunch reviewed Jan 20, 2018

View reviewed changes

Grace Guo added 5 commits January 22, 2018 16:41

code review fix: remove duplicated calls for getting values from request

2b3954a

[Explore view] Use POST method for charting requests

ea24c24

fix per code review comments

8cb9030

more code review fixes

ab504f2

code review fix: remove duplicated calls for getting values from request

f1e92e5

Grace Guo added 2 commits February 13, 2018 15:20

Merge branch 'master' of https://github.com/apache/incubator-superset …

46e105e

…into gg-ShortenURL

Merge remote-tracking branch 'supercat/gg-ShortenURL' into gg-ShortenURL

5697d8c

graceguo-supercat merged commit 342180b into apache:master Feb 14, 2018

graceguo-supercat mentioned this pull request Feb 14, 2018

Implement the column config and row config in pivot table #4394

Closed

graceguo-supercat deleted the gg-ShortenURL branch February 16, 2018 19:15

graceguo-supercat mentioned this pull request Nov 20, 2018

shortened URL didn't work for complicated queries #6414

Closed

3 tasks

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

[Explore view] Use POST method for charting requests #3993

[Explore view] Use POST method for charting requests #3993

Conversation

graceguo-supercat commented Dec 3, 2017

kkalyan commented Dec 3, 2017

graceguo-supercat commented Dec 3, 2017 • edited Loading

kkalyan commented Dec 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graceguo-supercat Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graceguo-supercat Jan 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistercrunch Jan 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistercrunch commented Jan 20, 2018

graceguo-supercat commented Jan 24, 2018 • edited Loading

mistercrunch commented Feb 4, 2018

graceguo-supercat commented Feb 14, 2018

graceguo-supercat commented Dec 3, 2017 •

edited

Loading

graceguo-supercat Jan 17, 2018 •

edited

Loading

graceguo-supercat Jan 18, 2018 •

edited

Loading

mistercrunch Jan 20, 2018 •

edited

Loading

graceguo-supercat commented Jan 24, 2018 •

edited

Loading