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

Non-eval version #4

Closed
wants to merge 9 commits into from
Closed

Conversation

erithmetic
Copy link

Just wanted to run this by you. I created a version of JSONPath that doesn't use eval(). The reason for this was to satisfy some security concerns that reviewers at Mozilla Addons had about a plugin I developed that made use of JSONPath. This implementation limits the operations that can be tested on a node to basic comparisons and arithmetic. However, it could be possible to add more features, possibly with some sort of JS parsing library.

For now, this version works well enough for many of the base cases and it covers the suite of integration tests you had defined. (N.B. I am using a different unit testing framework that has better output, run options, and test organization)

Thoughts?

@s3u
Copy link
Collaborator

s3u commented Oct 8, 2011

Thanks for doing this. I will check it out.

Question question - have you done any perf analysis if this change had any negative or positive impact on perf?

@againsley
Copy link

Any chance of getting this pulled in? the fact that the function is called eval breaks jshint. Very frustrating.

@kosmotaur
Copy link

+1 for pulling in ;)

@petermanser
Copy link

👍

@s3u
Copy link
Collaborator

s3u commented Dec 7, 2014

Unfortunately there are two issues with this PR

  • Does not merge cleanly
  • Once merged, most tests break as expected results are getting wrapped inside an array of size one.

@brettz9 brettz9 mentioned this pull request Dec 10, 2014
@brettz9
Copy link
Collaborator

brettz9 commented Dec 10, 2014

Any way the original poster is still interested to update this? If it is primarily an issue of removing wrapping when the config is set, I would think that shouldn't be too hard for the OP to fix.

@s3u
Copy link
Collaborator

s3u commented Jun 7, 2015

Can not be merged at present

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.

None yet

6 participants