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

Add StreamWriteConstraints with a nesting depth check #1055

Merged
merged 2 commits into from Jul 8, 2023

Conversation

pjfanning
Copy link
Member

@pjfanning pjfanning commented Jun 18, 2023

  • relates to Add configurable processing limits for JSON generator (StreamWriteConstraints) #1048
  • open for discussion - still lots of work to do
  • equivalent of StreamReadConstraints
  • initially, only max nesting depth is tracked
  • the changes in this PR to get the Filtering Delegate Generator to check the depth are probably overkill - the underlying JsonGenerator (the one being delegated to) is probably tracking already
  • and tests, of course

@pjfanning
Copy link
Member Author

@cowtowncoder this code does not work yet because of a quirk in how JsonWriteContext works.

The nestingDepth is tracked if you create a JsonWriteContext with a parent context (inside the null check below).

But JsonWriteContext has the reset method too. Is it ok if I hack this to update the nestingDepth too? It is a pain because nestingDepth is final and that final marker will need to be removed. The reset will mutate the nestingDepth and not in a nice way - makes the code so much harder to test.

Is there any hope that this reset behaviour could be removed? So, we would instead create a new JsonWriteContext with a parent instead.

    public JsonWriteContext createChildArrayContext() {
        JsonWriteContext ctxt = _child;
        if (ctxt == null) {
            _child = ctxt = new JsonWriteContext(TYPE_ARRAY, this,
                    (_dups == null) ? null : _dups.child());
            return ctxt;
        }
        return ctxt.reset(TYPE_ARRAY);
    }

@cowtowncoder
Copy link
Member

+1 for working on this. I will not have time to engage on this for next 2 weeks or so (will try to send an email notification about my vacation), except for today and tomorrow.
But maybe we can get things started.

@pjfanning pjfanning marked this pull request as ready for review June 18, 2023 17:12
@pjfanning
Copy link
Member Author

@cowtowncoder I have the some tests working now and the issue I was hitting with JsonWriteContext was a bug in my code.

If I remove the changes in the Filtering Delegate Generator (because they are overkill), would it be possible to get this merged today/tomorrow? That would make it easier for me to test out the changes in jackson-databind and other jackson modules. Whatever way it falls out, we can complete the extra bits after your vacation.

@pjfanning pjfanning changed the title [DRAFT] add StreamWriteConstraints add StreamWriteConstraints with a nesting depth check Jun 18, 2023
@cowtowncoder
Copy link
Member

@cowtowncoder this code does not work yet because of a quirk in how JsonWriteContext works.

The nestingDepth is tracked if you create a JsonWriteContext with a parent context (inside the null check below).

But JsonWriteContext has the reset method too.

reset() is only called to reuse an instance at same nesting level, so shouldn't it be safe to in that case actually ignore check call altogether? (since that nesting level must have been validated once before).
Or if not, just copy nesting depth.

Reset is NOT used for general purpose reuse but specifically case of avoiding allocation for a sibling of earlier nesting level. This is noted in Javadoc comments.

I may be missing something here so want to make sure I understand the problem.

@pjfanning
Copy link
Member Author

@cowtowncoder this code does not work yet because of a quirk in how JsonWriteContext works.
The nestingDepth is tracked if you create a JsonWriteContext with a parent context (inside the null check below).
But JsonWriteContext has the reset method too.

reset() is only called to reuse an instance at same nesting level, so shouldn't it be safe to in that case actually ignore check call altogether? (since that nesting level must have been validated once before). Or if not, just copy nesting depth.

Reset is NOT used for general purpose reuse but specifically case of avoiding allocation for a sibling of earlier nesting level. This is noted in Javadoc comments.

I may be missing something here so want to make sure I understand the problem.

I was debugging an issue and I misunderstood what was going on. The context reset was not the issue and it does not need to change.

I have the PR working with a few basic tests now.

@cowtowncoder
Copy link
Member

@pjfanning Ok perfect. I just wanted to make sure you weren't blocked here. Plus role of reset() is not necessarily that clear without knowing the context (no pun intended)

@cowtowncoder
Copy link
Member

@pjfanning Oh, one other thing -- I know it's a pain, but once you have something ready, is there any chance of merging things incrementally, to help merge to master (3.0)? Starting with plumbing/scaffolding, then implementation, then tests?
I know it's a hassle so you can decide if it's worth it.

Update TokenFilterContext.java

make stream write constraints accessible

Update FilteringGeneratorDelegate.java

police nesting depth in json generators

Update JsonGeneratorImpl.java

add test that should fail

fix test

test issue

more tests

revert changes in FilteringGeneratorDelegate

Update FilteringGeneratorDelegate.java
@pjfanning
Copy link
Member Author

pjfanning commented Jun 19, 2023

@cowtowncoder I created #1056 for master.

@pjfanning
Copy link
Member Author

@cowtowncoder could you review this PR when you have time? There would be need of follow up PRs in jackson-databind and some of the other modules (XML, text formats, binary formats). I can do those after this is merged.

@cowtowncoder
Copy link
Member

@pjfanning Yes, trying to go through backlog now that I am back; will try to get this (and 1056) reviewed ASAP.

@cowtowncoder
Copy link
Member

Sorry, didn't get this done today; aiming for tomorrow.

@cowtowncoder cowtowncoder changed the title add StreamWriteConstraints with a nesting depth check Add StreamWriteConstraints with a nesting depth check Jul 8, 2023
@cowtowncoder cowtowncoder merged commit 833d87b into FasterXML:2.16 Jul 8, 2023
5 checks passed
@pjfanning pjfanning deleted the stream-write-constraints branch July 8, 2023 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.16 Issue planned (at earliest) for 2.16 processing-limits Issues related to limiting aspects of input/output that can be processed without exception
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants