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

Introduce Javascript controls #4076

Merged
merged 2 commits into from
Dec 21, 2017
Merged

Introduce Javascript controls #4076

merged 2 commits into from
Dec 21, 2017

Conversation

mistercrunch
Copy link
Member

@mistercrunch mistercrunch commented Dec 18, 2017

screen shot 2017-12-17 at 10 02 49 pm

screen shot 2017-12-17 at 10 04 10 pm

This allows power-users to perform intricate transformations on data and
objects using javascript code.

The operations allowed are "sanboxed" or limited using node's vm
runInNewContext
https://nodejs.org/api/vm.html#vm_vm_runinnewcontext_code_sandbox_options

For now I'm only enabling in the line chart visualization, but the plan
would be to go towards offering more power to people who can write some
JS moving forward.

@fabianmenges
Copy link
Contributor

fabianmenges commented Dec 19, 2017

I have 2 concerns:

  • We are exposing the internal data model to the users, thus making it difficult to ever change. Now they can implement complex transformations on the data, which we won't be able to automatically migrate.
  • What does the development story for these JS snippets look like? Is there a way for us to provide a debug console or something?

@mistercrunch
Copy link
Member Author

@fabianmenges thanks for the feedback. I agree that we should be extremely careful in what is exposed here, whatever goes in and out has to be treated like a public API, so no backwards incompatible changes should be allowed without release notes (if ever).

We do control exactly what is accessible in the sandbox. I pass the console object in the context by default so users can console.log all they want. For instance I also pass underscore and d3 in there since there's a lot of utilities users may want, though that means that if we ever want to go d3.v4 we would have to be careful in that regard somehow as we could break user reports.

After thinking this through I'm removing d3 from the context and keeping underscore.

@mistercrunch
Copy link
Member Author

So this PR adds the new control but the control is not used anywhere. I'd like to merge this independently from the use cases which will come in future PRs in the geospatial space.

Anytime this control will be used, we should consider the fact that it locks whatever it hooks (input and output) from backwards incompatible changes.

This allows power-users to perform intricate transformations on data and
objects using javascript code.

The operations allowed are "sanboxed" or limited using node's vm
`runInNewContext`
https://nodejs.org/api/vm.html#vm_vm_runinnewcontext_code_sandbox_options

For now I'm only enabling in the line chart visualization, but the plan
would be to go towards offering more power to people who can write some
JS moving forward.
@mistercrunch
Copy link
Member Author

@fabianmenges mind taking another look? Here the control is not actually being used by any visualization (planning to use it on DECK.GL viz eventually), and the console concern is taken care of.

Copy link
Contributor

@fabianmenges fabianmenges left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mistercrunch mistercrunch merged commit 69195f8 into apache:master Dec 21, 2017
michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
* Introduce Javascript controls

This allows power-users to perform intricate transformations on data and
objects using javascript code.

The operations allowed are "sanboxed" or limited using node's vm
`runInNewContext`
https://nodejs.org/api/vm.html#vm_vm_runinnewcontext_code_sandbox_options

For now I'm only enabling in the line chart visualization, but the plan
would be to go towards offering more power to people who can write some
JS moving forward.

* Not applied
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
* Introduce Javascript controls

This allows power-users to perform intricate transformations on data and
objects using javascript code.

The operations allowed are "sanboxed" or limited using node's vm
`runInNewContext`
https://nodejs.org/api/vm.html#vm_vm_runinnewcontext_code_sandbox_options

For now I'm only enabling in the line chart visualization, but the plan
would be to go towards offering more power to people who can write some
JS moving forward.

* Not applied
@mistercrunch 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.23.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants