Possible bug with TokenFilter

[jhaber]

Hello, I was trying to update a library I own, jackson-jaxrs-propertyfiltering, to use a TokenFilter and ran into an issue that struck me as surprising and/or a bug. For some background, this library extends the jackson-jaxrs-json-provider and let's callers filter JSON responses using query params (so if you just need the id and name of each object you could pass ?property=id&property=name).

Previously this library would serialize the response to a JsonNode, apply the filtering to the JsonNode, and then serialize the filtered JsonNode to the HTTP response. This worked fine, but is very inefficient. I tried updating this to use a TokenFilter to apply the filtering in a streaming fashion and things worked great except we had some test failures around empty objects. For example, given a JSON document like:

[
   {
      "id":123,
      "name":"abc",
      "other":"def"
   },
   {
      "id":123,
      "name":"abc",
      "other":"def"
   },
   {
      "id":123,
      "name":"abc",
      "other":"def"
   }
]

If the request looks like ?property=id&property=name then things work great and the response looks like:

[
   {
      "id":123,
      "name":"abc"
   },
   {
      "id":123,
      "name":"abc"
   },
   {
      "id":123,
      "name":"abc"
   }
]

However, if the response filters out all the properties, such as ?property=fake, then we end up with just the empty string:

So we return an HTTP 200 with no JSON at all and the caller blows up. Before the switch to a TokenFilter, this would have filtered out all of the object keys but retained the JSON structure so you end up with something like:

[{},{},{}]

I pushed a complete example to my fork:
jhaber@3e637a8

Is there a way to achieve this sort of behavior currently? If not, would you be open to a PR that adds a new option to FilteringGeneratorDelegate that enables this behavior?

Thanks in advance

[cowtowncoder]

If I understand this correctly, I guess it... depends. For default implementation, I think design is such that only path(s) that lead to match should be retained (or optionally, drop the parent path). I don't think that can be changed. But I would not be opposed to adding alternative that would allow keeping all structure and just dropping non-matching properties.

So basically if this can be added without changing existing behavior this sounds reasonable.
It might even be quite straight-forward given that determination of whether to include an Object or Array would get easier for most part (except maybe not for leaf-arrays?).

Above is of course assuming that existing methods of TokenFilter are sufficient; if not, additions may be possible for 2.11.

[jhaber]

Thanks for the reply; TokenFilter interface seems totally sufficient. For my use-case I actually just need the includeProperty method:
https://github.com/HubSpot/jackson-jaxrs-propertyfiltering/blob/2c1d4ebba50e9aa5c8365ec2ff978b2023495a95/src/main/java/com/hubspot/jackson/jaxrs/PropertyFilter.java#L163-L176

I can work on a PR to add a flag for this mode

[jhaber]

And I think you're right, conceptually it should be pretty straight-forward. Basically if a filter doesn't return null, then you immediately write the token rather than waiting to see what happens later.

[cowtowncoder]

Oh. And as to JAX-RS, I could see this possibly being something to add to "stock" Jackson JAX-RS provider too, if you'd like extra challenge?

[jhaber]

Hmm I'll definitely investigate that as well. I was planning to PR the 2.10 branch with these changes, does that sound ok?

[cowtowncoder]

Depends on what has to change: ideally all API changes would go in next minor version (2.11), and patches only for fixes. But I sometimes allow "dark launches" for safer additions so that while not documented (nor publicized), new features are available on some versions -- mostly to allow sort of more testing. So ... I guess it depends. :)
Bigger question probably is the risk of changes regressing something.

FWTW I do plan on getting 2.11 pre-releases out before end of the year (in contrast to 2.10 that took forever). So that would add some delay, but not quite as bad as with some minor versions.

[cowtowncoder]

I think referenced new issue (#573) replaces this issue so closing.