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

Allow TokenFIlter to skip last elements in arrays #882

Closed
pgomulka opened this issue Dec 29, 2022 · 5 comments · Fixed by #883
Closed

Allow TokenFIlter to skip last elements in arrays #882

pgomulka opened this issue Dec 29, 2022 · 5 comments · Fixed by #883
Labels
2.14 Issue planned (at earliest) for 2.14
Milestone

Comments

@pgomulka
Copy link
Contributor

When the last element in array is an array or object and that element is skipped, the FilteringParserDelegate will end up in a loop from which it cannot exit. This means that the rest of the input will be skipped too. This results in incorrect JSON.

This behaviour exists since 2.9+
I believe this is due to 7db467d#diff-f6642caef61e0c403f51a6150ecf45263034fca5002782fd02eacd01e53fe549L694 where the if (gotEnd) conditions where removed.
I think this should be added as currently the logic is:

 boolean gotEnd = (_headContext == buffRoot);
                    boolean returnEnd = gotEnd && _headContext.isStartHandled();

                    _headContext = _headContext.getParent();
                    _itemFilter = _headContext.getFilter();

                    if (returnEnd) {
                        return t;
                    }

and that means that it can only exit when _headContext.isStartHandled() is true. For skipped elements this is false.

This can be easily reproduced with this testcase

 @Test
    public void testCustomIncludesWithMultipleObjectsInArrayMissLast() throws Exception {
        var factory = new JsonFactory();
        var baseParser = factory.createParser("{\"foo\":[{\"bar\":\"baz\"},{\"bing\":\"boom\"}]}");
        var filteredParser = getFilteredParser(baseParser, filter("foo", "bar"));
        var writer = new StringWriter();
        var generator = factory.createGenerator(writer);
        Assertions.assertTrue(filteredParser.nextToken().isStructStart());
        generator.copyCurrentStructure(filteredParser);
        generator.flush();
        Assertions.assertEquals("{\"foo\":[{\"bar\":\"baz\"}]}", writer.toString());
        //Expected :{"foo":[{"bar":"baz"}]}
        //Actual   :{"foo":[{"bar":"baz"} 
    }

cc @tvernum who coauthored the fix and the testcases

@cowtowncoder
Copy link
Member

Sounds reasonable. I'd be happy to merge, but need CLA first: will add a note on PR.

@cowtowncoder cowtowncoder added 2.14 Issue planned (at earliest) for 2.14 and removed 2.15 Issue planned (at earliest) for 2.15 labels Jan 8, 2023
@cowtowncoder cowtowncoder added this to the 2.14.2 milestone Jan 8, 2023
cowtowncoder added a commit that referenced this issue Jan 8, 2023
@pgomulka
Copy link
Contributor Author

@cowtowncoder should we close this issue manually? or will the team close this once the release is done?

@cowtowncoder
Copy link
Member

No, I need to close it since it didn't automatically do so. Closing done when fix merged for specific branch.

Thanks!

@pgomulka
Copy link
Contributor Author

@cowtowncoder how frequent are jackson releases? I am wondering when the 2.14.2 might be released.
We want to fix the problem in Elasticsearch as well, upgrading to 2.14.2 would be easiest. If the 2.14.2 is not due to be released soon we will have to find some workaround.

@cowtowncoder
Copy link
Member

It's the usual "when it's ready". Looking at:

https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.14.2

it's bit short list.

But.... potentially I could release 2.14.2 in January, esp. since you have clear need and have provided patches.
I do try to work with users and developers in such cases.
There are couple of possibly related PRs I'd like to get in first, but I guess there is nothing specifically blocking release.

Oh and maybe I should make sure I get Woodstox 6.5.0 released too.

pgomulka added a commit to elastic/elasticsearch that referenced this issue Jan 18, 2023
while jackson 2.14.2 with FasterXML/jackson-core#882 is still not released
we want to patch the jackson-core used by x-content with the modified class that fixes the bug #92480

closes #92480
pgomulka added a commit to pgomulka/elasticsearch that referenced this issue Jan 18, 2023
while jackson 2.14.2 with FasterXML/jackson-core#882 is still not released
we want to patch the jackson-core used by x-content with the modified class that fixes the bug elastic#92480

closes elastic#92480
thomasdullien pushed a commit to thomasdullien/elasticsearch-profiling-experiments that referenced this issue Jan 18, 2023
while jackson 2.14.2 with FasterXML/jackson-core#882 is still not released
we want to patch the jackson-core used by x-content with the modified class that fixes the bug elastic#92480

closes elastic#92480
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.14 Issue planned (at earliest) for 2.14
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants