Skip to content

Do not limit nesting depth of parsed JSON #68

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

Merged
merged 1 commit into from
Jan 4, 2016
Merged

Conversation

wfleming
Copy link
Contributor

@wfleming wfleming commented Jan 3, 2016

I turned up some instances of files that killed the engine due to
deeply nested JSON results from the parser: the JSON gem by default puts
a limit on this.

👀 @codeclimate/review

@wfleming wfleming force-pushed the will/json-depth branch 2 times, most recently from e78bd73 to dd2aab1 Compare January 3, 2016 22:12
@brynary
Copy link
Member

brynary commented Jan 3, 2016

Historical context: In the past we've parsed with unlimited depth. Since this is in an engine, I think it might be worth doing that as the simplest possible thing (and not adding more exception handling, which can be troublesome) and seeing where that leaves us.

@wfleming
Copy link
Contributor Author

wfleming commented Jan 4, 2016

@brynary OK, I didn't realize we had an unlimited depth in classic analyzer. Will update.

@wfleming wfleming force-pushed the will/json-depth branch 2 times, most recently from 8f248f3 to e98ac61 Compare January 4, 2016 04:43
@wfleming wfleming changed the title Increase allowed nesting depth of JSON Do not limit nesting depth of parsed JSON Jan 4, 2016
@gdiggs
Copy link
Contributor

gdiggs commented Jan 4, 2016

LGTM

module Engine
module Analyzers
class ParserBase
protected
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be private. Ruby's method access is an odd-ball:

  • private - cannot be called with a target (even self. will raise)

  • protected - can only be called with a target if you're the same class or a subclass

    This is mostly (only?) useful for otherwise private attributes that need to be invoked with a target as part of an equality check:

    class Foo
      def ==(other)
        private_thing == other.private_thing
      end
    end

    This won't work if #private_thing is actually private, so it would be protected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I do this habitually more for how it reads than how it actually works. But good point, changing.

@pbrisbin
Copy link
Contributor

pbrisbin commented Jan 4, 2016

One nit. LGTM

I turned up some instances of files that killed the engine due to
deeply nested JSON results from the parser: the JSON gem by default puts
a limit on this.
wfleming added a commit that referenced this pull request Jan 4, 2016
Do not limit nesting depth of parsed JSON
@wfleming wfleming merged commit 7b5a477 into master Jan 4, 2016
@wfleming wfleming deleted the will/json-depth branch January 4, 2016 18:50
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.

4 participants