Skip to content

Allow TokenFilter to preserve empty #716

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Allow TokenFilter to preserve empty #716

wants to merge 1 commit into from

Conversation

nik9000
Copy link
Contributor

@nik9000 nik9000 commented Sep 12, 2021

This creates two new method on TokenFilter which you can override to
decide if empty arrays and objects should be included or excluded. An
override like this, for example, will include all arrays and objects
that were sent empty but strip any arrays or objects that were
filtered to be empty:

        @Override
        public boolean includeEmptyArray(boolean contentsFiltered) {
            return !contentsFiltered;
        }

        @Override
        public boolean includeEmptyObject(boolean contentsFiltered) {
            return !contentsFiltered;
        }

The default to preserve backwards compatibility is to always exclude
empty objects.

Closes #715

@nik9000
Copy link
Contributor Author

nik9000 commented Sep 12, 2021

OK! I've got my tests working for this, but, like I said on the issue, I don't know the code base and very well could be screwing things up in ways I don't understand.

I haven't tested this with ONLY_INCLUDE_ALL or INCLUDE_NON_NULL because I'm not sure of a good way to do so. These things also feel like they operate at a different level of precision.

@nik9000
Allow TokenFilter to preserve empty
5dbfd5e 
This creates two new method on `TokenFilter` which you can override to
decide if empty arrays and objects should be included or excluded. An
override like this, for example, will include all arrays and objects
that were sent empty but strip any arrays or objects that were
*filtered* to be empty:
```
        @OverRide
        public boolean includeEmptyArray(boolean contentsFiltered) {
            return !contentsFiltered;
        }

        @OverRide
        public boolean includeEmptyObject(boolean contentsFiltered) {
            return !contentsFiltered;
        }
```

The default to preserve backwards compatibility is to always *exclude*
empty objects.

Closes FasterXML#715
nik9000
nik9000 commented Sep 12, 2021
View reviewed changes
src/test/java/com/fasterxml/jackson/core/BaseTest.java
@@ -451,11 +451,15 @@ protected String readAndWrite(JsonFactory f, JsonParser p) throws IOException
g.disable(JsonGenerator.Feature.AUTO_CLOSE_JSON_CONTENT);
try {
while (p.nextToken() != null) {
System.err.println(p.currentToken() + " " + p.currentName() + " " + p.currentValue());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left this in to remind myself to ask about currentName on the tokens to close objects. Have a look at what this prints in some of the tests. I'm not sure that currentName has the value you'd expect there. I don't think I use it in that context, but I wanted to bring it up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just make sure this gets removed before merging -- no System.err or System.out output from Jackson code ever unless absolutely required.

src/test/java/com/fasterxml/jackson/core/BaseTest.java
throw new AssertionError(
"Unexpected problem during `readAndWrite`. Output so far: '" +
sw + "'; problem: " + e.getMessage(),
e);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't required at all but it sure made all the failures easier to see. I had a lot of failures.....

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But isn't there already fail() call there? Is this to get stack trace or ... ?

src/main/java/com/fasterxml/jackson/core/filter/FilteringParserDelegate.java
{
boolean returnEnd = _headContext.isStartHandled();
f = _headContext.getFilter();
if ((f != null) && (f != TokenFilter.INCLUDE_ALL)) {
boolean includeEmpty = f.includeEmptyArray(_headContext.hasCurrentIndex());
f.filterFinishArray();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in the old code this was calling the wrong callback here.

@nik9000
Copy link
Contributor Author

nik9000 commented Oct 15, 2021

Hello! It's been a while. I'm still quite interested in this if any maintainers are willing to work with me.

@cowtowncoder
Copy link
Member

Hi @nik9000 ! Thank you for pinging me. I have had less time on Jackson lately, but hoping to pick up things now.
And this is one of the things I'd like to get done.

@cowtowncoder
Copy link
Member

@nik9000 Apologies for dropping this from 2.13, but I am now back working on getting 2.14 branch going; and I would be interested in getting this patch in. Could you rebase this PR against 2.14?

@nik9000
Copy link
Contributor Author

nik9000 commented Dec 6, 2021 via email

@nik9000
Copy link
Contributor Author

nik9000 commented Dec 16, 2021 •
edited
Loading

I opened #729.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cowtowncoder cowtowncoder cowtowncoder left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow TokenFilters to keep empty arrays and objects
2 participants
@nik9000 @cowtowncoder