Remove filter bindings when SimpleFilterProvider.addFilter receives null by wozhenbang2004 · Pull Request #6369 · FasterXML/jackson-databind

wozhenbang2004 · 2026-11-01T14:16:25Z

Both addFilter overloads now remove the binding when the filter is null, so mapper construction does not encounter a null entry registered through this API. Their Javadocs document the removal behavior.

Six regression tests cover removal of existing bindings and no-op removal of absent IDs through both overloads, call chaining, default-filter fallback after mapper construction, fail-on-unknown and ignore-unknown behavior, and a non-null filter control. The two binding-removal tests fail against the previous implementation.

The final full fork build passes on Java 17 (Ubuntu and Windows), 21 and 25. Java 17 reports 7,618 tests with no failures or errors.

Regression control: https://github.com/wozhenbang2004/jackson-databind/actions/runs/41334074184
Passing final build: https://github.com/wozhenbang2004/jackson-databind/actions/runs/41334351350

cowtowncoder · 2026-11-01T17:50:03Z

I don't think this is right, see my comment on #6365.

Updated addFilter methods to handle null filters by removing existing bindings.

wozhenbang2004 · 2026-11-01T18:04:29Z

Thanks for clarifying the intended behavior in #6365. I reworked this so both addFilter overloads remove the binding when the filter is null, and updated both Javadocs. The original snapshot change is reverted.

The tests now assert that existing and absent IDs have no mapping after null registration through either overload, then check default-filter fallback and unknown-ID handling after mapper construction. The constructor-map test that assumed explicit null bindings should be preserved was removed to match this scope.

The two binding-removal tests fail against the previous implementation. The revised full build passes on Java 17 (Ubuntu and Windows), 21 and 25: https://github.com/wozhenbang2004/jackson-databind/actions/runs/41334351350

github-actions · 2026-11-01T19:13:00Z

🧪 Code Coverage Report

Metric	Coverage	Change
Instructions	82.82%	📉 -0.030%
Branches	76.83%	📉 -0.030%

Coverage data generated from JaCoCo test results

Co-Authored-By: Claude Opus 5.5 <[email protected]>

gitar-bot · 2026-11-01T19:30:07Z

Code Review ⚠️ Changes requested 3 closed / 4 findings

🟡 Medium risk · Null filters now remove bindings, changing serialization filter resolution; affected configurations could use a default filter or serialize more fields.

Makes both addFilter overloads remove the binding when passed null, so mapper construction no longer hits a null entry registered through that API. The change has an open finding: the regression test covering a null value passed through the SimpleFilterProvider(Map) constructor was dropped, and that path can still throw a NullPointerException during mapper build in snapshot(), so it needs a fix before merge.

Restore a test that puts null directly into the constructor map, builds the mapper, and verifies the default filter applies
Keep null values in the copy constructor snapshot instead of dereferencing them in takeSnapshot

⚠️

Bug: Null entries from constructor-supplied map still NPE on mapper build

📄 src/main/java/tools/jackson/databind/ser/std/SimpleFilterProvider.java:55-58 📄 src/main/java/tools/jackson/databind/ser/std/SimpleFilterProvider.java:68

This commit puts back v.snapshot() in the copy constructor and depends on addFilter(id, null) deleting the binding. That only covers the two addFilter overloads. new SimpleFilterProvider(map) uses the caller's map as-is (line 57), so a map like Map.of(...)/HashMap with put("filter", null) still has a null value. JsonMapper.builder().filterProvider(provider).build() then calls snapshot() → line 68 → NullPointerException while the mapper is being built. That is the constructor-mapping case from #6365, and the PR description says it is covered, but its test was dropped from NullFilterSnapshot6365Test. findPropertyFilter already handles a null lookup result (default filter, ignore, or a mapping error), so the copy should keep null values instead of dereferencing them.

Use the null-safe snapshot helper, as is already done for `_defaultFilter`, and restore the constructor-mapping regression test.

f.forEach((k,v) -> _filtersById.put(k, Snapshottable.takeSnapshot(v)));

✅ 3 closed

✅ Bug: New tests expect addFilter(null) to remove binding; production stores null

📄 src/test/java/tools/jackson/databind/ser/filter/NullFilterSnapshot6365Test.java:30-44 📄 src/main/java/tools/jackson/databind/ser/std/SimpleFilterProvider.java:119-121 📄 src/main/java/tools/jackson/databind/ser/std/SimpleFilterProvider.java:141-143
nullPropertyFilterRemovesBinding and nullSimpleBeanPropertyFilterRemovesBinding assert assertFalse(filters.containsKey("filter")) after addFilter("filter", null). Both addFilter overloads in SimpleFilterProvider (lines 119-121, 141-143) still call _filtersById.put(id, filter), and this commit only touches the test file. So the map holds filter -> null, containsKey returns true, and both tests fail on the first assertFalse. The javadoc on those methods also says a null binding is kept and only checked during serialization, which is the behavior #6365 asks to preserve. Pick one: change the assertions to assertTrue(filters.containsKey("filter")); assertNull(filters.get("filter"));, or, if removal is the intended new contract, change addFilter to call _filtersById.remove(id) when filter == null and update the javadoc.

✅ Quality: Test class no longer extends DatabindTestUtil

📄 src/test/java/tools/jackson/databind/ser/filter/NullFilterSnapshot6365Test.java:20
This commit removed extends DatabindTestUtil and its import from NullFilterSnapshot6365Test. The project's test guidelines say tests should use DatabindTestUtil. Moving to text blocks doesn't require dropping the base class, so restore extends DatabindTestUtil and keep the text blocks.

Closed: Quality: Regression test for null entry snapshot (#6365) is gone

📄 src/test/java/tools/jackson/databind/ser/filter/NullFilterSnapshot6365Test.java:57-67 📄 src/main/java/tools/jackson/databind/ser/std/SimpleFilterProvider.java:68
This commit dropped nullMappingUsesDefaultAfterMapperBuild (the constructor map with filters.put("filter", null)), and the removed* tests now register a non-null filter before the null one. Those tests still end with a null value in the map, so they cover the snapshot NPE only by accident, and their names suggest the entry was removed. If addFilter(null) ever starts removing entries, nothing would cover a null value passed through the SimpleFilterProvider(Map) constructor, which is the case Snapshottable.takeSnapshot (line 68) now guards against. Add back a test that puts null directly into the constructor map, builds the mapper, and checks that the default filter is applied.

🤖 Prompt for agents

Code Review: Makes both `addFilter` overloads remove the binding when passed `null`, so mapper construction no longer hits a null entry registered through that API. The change has an open finding: the regression test covering a null value passed through the `SimpleFilterProvider(Map)` constructor was dropped, and that path can still throw a NullPointerException during mapper build in `snapshot()`, so it needs a fix before merge.
  
  - Restore a test that puts `null` directly into the constructor map, builds the mapper, and verifies the default filter applies
  - Keep null values in the copy constructor snapshot instead of dereferencing them in `takeSnapshot`

1. ⚠️ Bug: Null entries from constructor-supplied map still NPE on mapper build
   Files: src/main/java/tools/jackson/databind/ser/std/SimpleFilterProvider.java:55-58, src/main/java/tools/jackson/databind/ser/std/SimpleFilterProvider.java:68

   This commit puts back `v.snapshot()` in the copy constructor and depends on `addFilter(id, null)` deleting the binding. That only covers the two `addFilter` overloads. `new SimpleFilterProvider(map)` uses the caller's map as-is (line 57), so a map like `Map.of(...)`/`HashMap` with `put("filter", null)` still has a null value. `JsonMapper.builder().filterProvider(provider).build()` then calls `snapshot()` → line 68 → NullPointerException while the mapper is being built. That is the constructor-mapping case from #6365, and the PR description says it is covered, but its test was dropped from `NullFilterSnapshot6365Test`. `findPropertyFilter` already handles a null lookup result (default filter, ignore, or a mapping error), so the copy should keep null values instead of dereferencing them.

   Fix (Use the null-safe snapshot helper, as is already done for `_defaultFilter`, and restore the constructor-mapping regression test.):
   f.forEach((k,v) -> _filtersById.put(k, Snapshottable.takeSnapshot(v)));

Verification

Rules No rules evaluated

Functional validation Not enabled

Set up

_{For more verification details, comment gitar display:verbose.}

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing compact verification results.

Comment with these commands to change the behavior for this request:

`Auto-apply`	`Compact`
`gitar auto-apply:on`	`gitar display:verbose`

_{Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source}

github-actions · 2026-11-01T19:33:22Z

🧪 Code Coverage Report

Metric	Coverage	Change
Instructions	82.85%	📈 +0.000%
Branches	76.87%	📈 +0.010%

Coverage data generated from JaCoCo test results

wozhenbang2004 added 3 commits November 1, 2026 22:00

test: cover null filter entries during provider snapshots

f4b5475

test: use JSON quote helper in filter snapshot expectations

4f5466d

fix: preserve null filters when snapshotting providers

09a8b63

test: verify null filters remove provider bindings

7050cc3

gitar-bot Bot reviewed Nov 1, 2026

View reviewed changes

Comment thread src/test/java/tools/jackson/databind/ser/filter/NullFilterSnapshot6365Test.java Resolved

gitar-bot Bot reviewed Nov 1, 2026

View reviewed changes

Comment thread src/test/java/tools/jackson/databind/ser/filter/NullFilterSnapshot6365Test.java Resolved

gitar-bot Bot reviewed Nov 1, 2026

View reviewed changes

Comment thread src/test/java/tools/jackson/databind/ser/filter/NullFilterSnapshot6365Test.java Resolved

fix: remove filter bindings when addFilter receives null

248ff30

Updated addFilter methods to handle null filters by removing existing bindings.

wozhenbang2004 changed the title ~~Preserve null filters when snapshotting SimpleFilterProvider~~ Remove filter bindings when SimpleFilterProvider.addFilter receives null Nov 1, 2026

test: use shared databind test utilities

47ddbbc

Merge branch '3.x' into fix/null-filter-snapshot

e197424

cowtowncoder added cla-needed PR needed, see https://github.com/FasterXML/jackson/blob/main/CONTRIBUTING.md#paperwork 3.3 labels Nov 1, 2026

cowtowncoder added this to the 3.3.0 milestone Nov 1, 2026

Merge branch '3.x' into fix/null-filter-snapshot

8d53133

cowtowncoder added cla-received PR already covered by CLA (optional label) and removed cla-needed PR needed, see https://github.com/FasterXML/jackson/blob/main/CONTRIBUTING.md#paperwork labels Nov 1, 2026

Add release notes for FasterXML#6365

ebaf11f

Co-Authored-By: Claude Opus 5.5 <[email protected]>

cowtowncoder merged commit 023f0a5 into FasterXML:3.x Nov 1, 2026
5 checks passed

Navigation Menu

Repository navigation

Remove filter bindings when SimpleFilterProvider.addFilter receives null - #6369

Remove filter bindings when SimpleFilterProvider.addFilter receives null#6369
cowtowncoder merged 9 commits into
FasterXML:3.xfrom
wozhenbang2004:fix/null-filter-snapshot

wozhenbang2004 commented Nov 1, 2026 •

edited

Loading

Uh oh!

cowtowncoder commented Nov 1, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wozhenbang2004 commented Nov 1, 2026

Uh oh!

github-actions Bot commented Nov 1, 2026

Uh oh!

gitar-bot Bot commented Nov 1, 2026

Uh oh!

Uh oh!

github-actions Bot commented Nov 1, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

wozhenbang2004 commented Nov 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cowtowncoder commented Nov 1, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wozhenbang2004 commented Nov 1, 2026

Uh oh!

github-actions Bot commented Nov 1, 2026

🧪 Code Coverage Report

Uh oh!

gitar-bot Bot commented Nov 1, 2026

Uh oh!

Uh oh!

github-actions Bot commented Nov 1, 2026

🧪 Code Coverage Report

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wozhenbang2004 commented Nov 1, 2026 •

edited

Loading