-
-
Notifications
You must be signed in to change notification settings - Fork 799
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
Add StreamReadConstraints.maxNestingDepth()
to constraint max nesting depth (default: 1000)
#943
Conversation
Yes; this is along lines of what I was originally thinking. I think there are some benefits in avoiding possibility of accidentally forgotten "closing" of nesting levels, to avoid bugs, |
public void validateNestingDepth(int depth) throws StreamConstraintsException | ||
{ | ||
if (depth > _maxNestingDepth) { | ||
throw new StreamConstraintsException(String.format("Depth (%d) exceeds the maximum allowed nesting depth (%d)", |
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.
Not related to this PR specifically, but one thing we may want to do afterwards is to add something to note that StreamReadConstraints
settings control this -- just add some more verbiage in exception message.
But I think that makes sense as a separate PR once we have nesting depth checks covered.
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.
StreamConstraintsException has Javadoc that describes what the exception is about. I'm not sure there is much to be gained by making the message more verbose.
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.
My perspective is that when someone sees a stack trace in log files, it is nice to have slightly more information on context to give Ops people bit more clue on what might be happening.
But I guess minimum level certainly is to have information on exception class itself, pointing to constraints settings. I'll see if I can add something to relevant JAva class(es) in jackson-core.
@pjfanning Let me know if and when you are content with the base -- I think this is the last "must-have" feature before 2.15 release candidates. |
@cowtowncoder I'm happy to get this merged more or less as it is. There is uptake needed in numerous other Jackson modules so it would be good to make a start on that. |
@pjfanning Merged into 2.15, up to master -- will make some smaller changes (increase test coverage, add that one constructor). Note: |
StreamReadConstraints.maxNestingDepth()
to constraint max nesting depth (default: 1000)
Basically #926 but depth is tracked on the JsonStreamContext instead. I have built a jackson-databind enhancement based on this.