joshbuddy / jsonpath

Ruby implementation of http://goessner.net/articles/JsonPath/
MIT License
447 stars 72 forks source link

Factorize `dig` logic ; add a few options ; more tests ; fix README #137

Closed blambeau closed 3 years ago

blambeau commented 3 years ago

@Skarlso I restarted my branch with another strategy, but with same goal (support for symbolized keys).

Maybe you need to look at commits in order, but basically:

  1. I added some additional tests
  2. I refactored some dig logic without breaking anything
  3. Then and only then I added support for more dig scenario and the symbols option
  4. I also gave some love to the README (available options) and added unit tests for it

WDYT?

Skarlso commented 3 years ago

I'll take a look and try making sense of the code. :)) It's been a while since I last touched Ruby. :D

Skarlso commented 3 years ago

@blambeau Please ping me when you think this is done and I can take a look.

blambeau commented 3 years ago

@Skarlso yep, sorry, I was on a roll :-) I'm done now.

I'm 99% confident that it is backward compatible, I took the time to add test coverage before refactoring. The branch also documents supported features not explained in README.

In particular, I introduced a :allow_send option that defaults to true. I would suggest putting it to false in 2.0, for security reasons. Scenarios where it is probably used (e.g. Struct) are covered by a safer :dig strategy.

Skarlso commented 3 years ago

@blambeau I'm not sure I remember correctly, but isn't send quasi secure already?

But I might remember incorrectly so... ¯_(ツ)_/¯ BTW, thanks for all the love, I didn't really have time anymore since I started working to give this gem the love it deserves. :)

I also wanted to replace the regex parser with proper AST support. But that's also in the bag now-a-days. :)

Ruby 3.0 has some promising features as well. :)

blambeau commented 3 years ago

@Skarlso I wouldn’t say that send is secure. It’s good that the library does not allow calling Kernel methods.

Yet if paths are received from an external user (in my use case that’s a possibility), the library allows calling any public method (having no args or args with default values) on the context object or a traversed child. That’s a security issue if e.g. people use the library to query ActiveRecord instances with such paths.

I also wonder whether using parslet + an AST would not be better for the parsing stage... but that’s a very big change indeed.

I can keep giving some love here if that helps. Maybe starting with some triage on the open issues.

Skarlso commented 3 years ago

@blambeau so.... can I review now? :D

blambeau commented 3 years ago

@Skarlso Yes you can. That's what I meant by "I'm done now", that was probably not that clear, sorry.

Skarlso commented 3 years ago

This is some very nice change. Good work! Thanks! Can I merge and release this as is with the current version support?

blambeau commented 3 years ago

WDYM by "with the current version support".

Given the branch introduces new features, I would suggest a 1.1 (SemVer).

Skarlso commented 3 years ago

I meant the current Ruby versions in the list of ci. I guess it's okay, since all versions seem to pass. I was just making sure, since you said 99% sure.. :))

blambeau commented 3 years ago

I think the current Travis matrix is great. We should probably add ruby 3.0 in the near future, but that can be after a release.

Skarlso commented 3 years ago

Done and released. Thank you for your contribution! :)

blambeau commented 3 years ago

:) thanks.