Bug in FilteringParserDelegate

[jhaber]

There seems to be a bug in FilteringParserDelegate that specifically manifests when trying to filter out arrays. Here's a test that reproduces the behavior (can be added to BasicParserFilteringTest):

    public void testExcludeArrays() throws Exception
    {
        class NoArraysFilter extends TokenFilter
        {
            @Override
            public TokenFilter filterStartArray() {
                return null;
            }
        }

        String jsonString = aposToQuotes("{'a':123,'array':[1,2]}");
        JsonParser p0 = JSON_F.createParser(jsonString);
        FilteringParserDelegate p = new FilteringParserDelegate(p0,
            new NoArraysFilter(),
            true, // includePath
            true // multipleMatches
        );
        String result = readAndWrite(JSON_F, p);
        assertEquals(aposToQuotes("{'a':123}"), result);
        assertEquals(1, p.getMatchCount());
    }

This test fails with:

junit.framework.ComparisonFailure: 
Expected :{"a":123}
Actual   :{"a"}

I discovered this bug while adding test coverage for #573. However, I confirmed that the bug exists in jackson-core versions 2.8.9, 2.9.10, 2.10.5, and 2.11.3 (and therefore wasn't introduced in #573).

No one has complained yet, and a quick GitHub search suggests there isn't much code in the wild (if any) doing filterStartArray() { return null; } so I don't think it's a high priority to fix this. I mainly just wanted to log this issue so that I can link to it (above the commented out unit test)

[vincent-simpson]

@jhaber I see you added this test case to BasicParserFilteringTest in the failing tests package, but I believe the assert statement is incorrect.
https://github.com/FasterXML/jackson-core/pull/650/files#diff-21191a4efccbbbe22296418ee9163a5a85ff776f4047d3f166916ec54299eaa1R34

The number of expected matches should be 1 here as you mentioned above. So this should be removed as a failing test since the assert is incorrect?

[jhaber]

When I opened this issue the test was failing even before it got to that assertion. The filter was somehow causing Jackson to generate invalid JSON (I was getting a JSON object with a key but no value):

junit.framework.ComparisonFailure: 
Expected :{"a":123}
Actual   :{"a"}

I just checked out the 2.14 branch and seems like it's now generating the expected JSON, so the underlying bug must've been fixed at some point 😄

Now the test is just failing because of the match count assert. Updating the assertion to 1 makes sense to me, it was probably just an oversight that it was originally asserting 0

[cowtowncoder]

Could this be due to #715 added to 2.14?

I'd be super happy to merge a PR to fix the test, move it from under failing/ to under src/test/java/com/fasterxml/jackson/core/filter if anyone has time?
Or if not, I'll eventually circle back here as well, I just have a long semi-sorted list of things to fix/work on so PRs are a good way to get things in. :)

[jhaber]

Seems possible, either that or the fixes for #700. I can send a PR, does it matter which branch I send the PR to?

[cowtowncoder]

Earliest branch where test passes; if things work in 2.13, ideally 2.13, if 2.14 then that branch. .../failing has special status of not breaking the build so would not want to move if test actually fails.

Either way, I can then merge forward the changes so no need to worry about later branches.