-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
[Explore view] Use POST method for charting requests #3993
[Explore view] Use POST method for charting requests #3993
Conversation
@graceguo-supercat can please we have an option to disable this? GET requests have some benefits like parameterizable embedded iframes. |
@kkalyan this feature should be backward-compatible with previous work. Note we build post body for Explore view should support:
if you see any use cases are broken by this feature, please let me know. Thank you! |
Thanks, @graceguo-supercat! I'll test and report if there are any issues. |
superset/views/core.py
Outdated
@@ -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']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'])
addHistory(isReplace = false) { | ||
const { url, payload } = createExplore(this.props.form_data); | ||
if (isReplace) { | ||
history.replaceState( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't figure out where history
is defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
history
is window.history
, html5 Web API:
https://developer.mozilla.org/en-US/docs/Web/API/Window/history
@@ -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']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing slashes are pretty common in the Django / Python world.
d55f84c
to
39a3ae4
Compare
@@ -101,13 +106,43 @@ class ExploreViewContainer extends React.Component { | |||
} | |||
} | |||
|
|||
addHistory(isReplace = false, title) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think object signatures are a little more readable + less error prone, thoughts? e.g., ({ isReplace = false, title })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
longUrl); | ||
} | ||
|
||
// it seems browsers don't support pushState title attribute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some do right? otherwise it wouldn't be an input to push/replaceState
? maybe caveat with "some browsers"?
const formData = history.state; | ||
if (formData && Object.keys(formData).length) { | ||
this.props.actions.setExploreControls(formData); | ||
this.props.actions.runQuery(formData, false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit -- I would do newlines for each var
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
handleResize() { | ||
clearTimeout(this.resizeTimer); | ||
this.resizeTimer = setTimeout(() => { | ||
this.setState({ height: this.getHeight(), width: this.getWidth() }); | ||
}, 250); | ||
} | ||
|
||
handleNavigate() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Navigate
is a little ambiguous, what do you think about mirroring the listener: handlePopstate
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
return uri.directory(directory).search(search).toString(); | ||
} | ||
|
||
export function createExplore(form_data, endpointType = 'base', force = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 = {} })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we make this method name more clear? like getExploreUrlAndPayload
it took me a while to understand what getExplore
meant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed. used object signatures for getExploreUrlAndPayload
method
@@ -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', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ....')
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
superset/views/core.py
Outdated
url_form_data.update(form_data) | ||
form_data = url_form_data | ||
|
||
slice_id = form_data.get('slice_id') | ||
slc = None | ||
if slice_id: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably not ever really an issue but should this technically be a elif
now / mutually exclusive with the shortened url?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
d4b74da
to
985969a
Compare
@@ -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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be called exploreChart
? not export
? if the default behavior is not to export the data, it seems like explore
is more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's actually an action. it exports (download) .csv file, or opens .json file in another tab.
@@ -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))}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed. now i use bound callbacks.
@@ -29,7 +30,7 @@ export default class EmbedCodeButton extends React.Component { | |||
generateEmbedHTML() { | |||
const srcLink = ( | |||
window.location.origin + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's strange to use string templates and concatenation. can we just use the template?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I use single line template, it will be too long and break lint rule.
so i have to make it multiple lines.
@@ -37,7 +38,7 @@ export default function ExploreActionButtons({ | |||
</a> | |||
|
|||
<a | |||
href={slice.data.csv_endpoint} | |||
onClick={() => { exportChart(latestQueryFormData, 'csv'); }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto bound functions but not a blocker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed. now use bound callback function.
.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 }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - template literals? there's inconsistent usage in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed. now use template.
if (isReplace) { | ||
history.replaceState( | ||
payload, | ||
document.title, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be title
instead of document.title
? or make the document.title = title;
call before these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be title
. fixed.
@@ -12,48 +13,124 @@ describe('utils', () => { | |||
expect(uri1.toString()).to.equal(uri2.toString()); | |||
} | |||
|
|||
it('getExploreUrl generates proper base url', () => { | |||
it('generates proper base url', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed. i add describe
section.
}); | ||
it('getExploreLongUrl generates proper base url with form_data', () => { | ||
compareURI( | ||
URI(getExploreLongUrl(formData, 'base', false, 'http://superset.com')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last 2 arguments are not used! fixed.
}; | ||
} | ||
|
||
export function exportChart(formData, endpointType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
985969a
to
bccb2ef
Compare
superset/views/core.py
Outdated
# get form data from url | ||
d = {} | ||
# Supporting POST | ||
if request.form.get('form_data'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it'd be better to not request.form.get('form_data')
many times. First affect to a variable and then use the var.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
superset/views/core.py
Outdated
form_data = '{}' | ||
|
||
d = json.loads(form_data) | ||
request_param = request.args.get('form_data') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as previous comment
Had a few minor notes, but otherwise LGTM. I'm assuming it's been extensively tested in your staging prior to merging. |
confirmed. i manually tested top 20+ dashboard and more slices in dev box with production data. |
Cool then, feel free to merge once the conflicts are resolved. |
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
* [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
fix #3795