Add mapBlankStringToNull option to JsonNullableModule and JsonNullableJackson3Module - #186
Thorsrud22 wants to merge 1 commit into
Conversation
…eJackson3Module Blank strings sent for a non-String JsonNullable deserialize to JsonNullable.undefined(), which silently drops a client's intent to clear a value in a PATCH request (OpenAPITools#125). Add an opt-in module option that maps them to JsonNullable.of(null) instead. Default stays false, so nothing changes for existing users, and String targets are never affected. This carries OpenAPITools#126 by krangerich forward onto the post-OpenAPITools#117 code base: the option is applied to both the Jackson 2 and Jackson 3 deserializer stacks, threaded through withResolved() so it survives contextualization, and the tests run against both generations via the existing JsonProcessor parameterization. The previous public constructors are kept and delegate with the option off. Co-authored-by: krangerich <7890659+krangerich@users.noreply.github.com> Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
3 issues found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/main/java/org/openapitools/jackson/nullable/JsonNullableJackson3Deserializer.java">
<violation number="1" location="src/main/java/org/openapitools/jackson/nullable/JsonNullableJackson3Deserializer.java:54">
P2: When enabled, a Unicode whitespace-only string such as `"\u2003"` bypasses this return and Jackson attempts to parse it as the target value, causing an exception instead of producing present null. Use a Unicode-aware blank predicate before this branch.</violation>
</file>
<file name="README.md">
<violation number="1" location="README.md:91">
P3: The new "Blank strings" example references `Person.class`, but `Person` is never defined in the README, so a reader cannot run the example. Every other example in the README defines its bean class (`Pet`). Define a `Person` bean with a `JsonNullable<Integer> age` field (as the preceding comment describes) before the assertion, or restructure the example to use an already-defined class.</violation>
</file>
<file name="src/main/java/org/openapitools/jackson/nullable/JsonNullableModule.java">
<violation number="1" location="src/main/java/org/openapitools/jackson/nullable/JsonNullableModule.java:10">
P3: The flag is read only inside setupModule(), which runs once at registration. Calling mapBlankStringToNull() on an already-registered or shared/singleton module silently does nothing, because the deserializers were already constructed with the previous flag. Reserve the setter for pre-registration configuration (e.g. document that it must be called before registerModule) or make the module immutable.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| String str = p.getString().trim(); | ||
| if (str.isEmpty()) { | ||
| return JsonNullable.undefined(); | ||
| return mapBlankStringToNull ? JsonNullable.of(null) : JsonNullable.undefined(); |
There was a problem hiding this comment.
P2: When enabled, a Unicode whitespace-only string such as "\u2003" bypasses this return and Jackson attempts to parse it as the target value, causing an exception instead of producing present null. Use a Unicode-aware blank predicate before this branch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/main/java/org/openapitools/jackson/nullable/JsonNullableJackson3Deserializer.java, line 54:
<comment>When enabled, a Unicode whitespace-only string such as `"\u2003"` bypasses this return and Jackson attempts to parse it as the target value, causing an exception instead of producing present null. Use a Unicode-aware blank predicate before this branch.</comment>
<file context>
@@ -43,7 +51,7 @@ public JsonNullable<Object> deserialize(JsonParser p, DeserializationContext ctx
String str = p.getString().trim();
if (str.isEmpty()) {
- return JsonNullable.undefined();
+ return mapBlankStringToNull ? JsonNullable.of(null) : JsonNullable.undefined();
}
}
</file context>
| mapper.registerModule(new JsonNullableModule().mapBlankStringToNull(true)); | ||
| // Jackson 3: JsonMapper.builder().addModule(new JsonNullableJackson3Module().mapBlankStringToNull(true)) | ||
|
|
||
| // given a bean with a JsonNullable<Integer> age property: |
There was a problem hiding this comment.
P3: The new "Blank strings" example references Person.class, but Person is never defined in the README, so a reader cannot run the example. Every other example in the README defines its bean class (Pet). Define a Person bean with a JsonNullable<Integer> age field (as the preceding comment describes) before the assertion, or restructure the example to use an already-defined class.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At README.md, line 91:
<comment>The new "Blank strings" example references `Person.class`, but `Person` is never defined in the README, so a reader cannot run the example. Every other example in the README defines its bean class (`Pet`). Define a `Person` bean with a `JsonNullable<Integer> age` field (as the preceding comment describes) before the assertion, or restructure the example to use an already-defined class.</comment>
<file context>
@@ -1,95 +1,110 @@
+mapper.registerModule(new JsonNullableModule().mapBlankStringToNull(true));
+// Jackson 3: JsonMapper.builder().addModule(new JsonNullableJackson3Module().mapBlankStringToNull(true))
+
+// given a bean with a JsonNullable<Integer> age property:
+assertEquals(JsonNullable.<Integer>of(null), mapper.readValue("{\"age\":\"\"}", Person.class).age);
+```
</file context>
| // given a bean with a JsonNullable<Integer> age property: | |
| static class Person { | |
| public JsonNullable<Integer> age = JsonNullable.undefined(); | |
| } | |
| public class JsonNullableModule extends Module { | ||
|
|
||
| private final String NAME = "JsonNullableModule"; | ||
| private boolean mapBlankStringToNull = false; |
There was a problem hiding this comment.
P3: The flag is read only inside setupModule(), which runs once at registration. Calling mapBlankStringToNull() on an already-registered or shared/singleton module silently does nothing, because the deserializers were already constructed with the previous flag. Reserve the setter for pre-registration configuration (e.g. document that it must be called before registerModule) or make the module immutable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/main/java/org/openapitools/jackson/nullable/JsonNullableModule.java, line 10:
<comment>The flag is read only inside setupModule(), which runs once at registration. Calling mapBlankStringToNull() on an already-registered or shared/singleton module silently does nothing, because the deserializers were already constructed with the previous flag. Reserve the setter for pre-registration configuration (e.g. document that it must be called before registerModule) or make the module immutable.</comment>
<file context>
@@ -7,11 +7,31 @@
public class JsonNullableModule extends Module {
private final String NAME = "JsonNullableModule";
+ private boolean mapBlankStringToNull = false;
+
+ /**
</file context>
|
Read this against #126 and #152, and it's the one I'd take for 0.2.x. The early return stays, so the content deserializer never sees the blank string — that's why enum and POJO targets don't throw here, where they do in #152. Porting rather than rebasing was right after #117 split the deserializers. I've approved the workflow run; green on JDK 17, 21 and 25. One thing before I merge. That enum/POJO behaviour is the whole argument for this over #152, and nothing tests it — the new cases cover Integer, Boolean and String. It also rests on the guard sitting before super.deserialize(), which a later refactor could move without a test going red. Could you add a JsonNullable and a JsonNullable case with mapBlankStringToNull(true)? Both should come back present-null. Minor: the README diff is a whole-file CRLF-to-LF conversion — worth keeping the original endings so it doesn't collide with other README changes. Once this lands I'll close #126 and #152 pointing here. Thanks for carrying #126 forward with krangerich credited. |
Carries #126 by @krangerich forward onto the current code base, with credit (co-author on the commit). Addresses #125. @Sekator778 asked on #126 whether the author would port it and offered to let someone carry it if not; four days without a reply, so here it is.
What this does
Adds an opt-in
mapBlankStringToNull(boolean)option to bothJsonNullableModule(Jackson 2) andJsonNullableJackson3Module(Jackson 3). When enabled, a blank string (""or whitespace only) sent for a non-StringJsonNullabledeserializes toJsonNullable.of(null)instead ofJsonNullable.undefined(), so a client's intent to clear a value in a PATCH request is preserved. Default isfalse;JsonNullable<String>targets are never affected.Why a port rather than a rebase
#117 split the deserializer into Jackson 2 and Jackson 3 implementations after #126 was opened, so a textual rebase would have left the option Jackson 2 only (the trap @davidpavlovschi and @Sekator778 both pointed out on #126). This applies the same change symmetrically:
JsonNullableJackson2Deserializer/JsonNullableJackson3Deserializer: the flag is a constructor parameter, consulted indeserialize, and threaded throughwithResolved(...)so it survives contextualization.JsonNullableJackson2Deserializers/JsonNullableJackson3Deserializers: carry the flag into the deserializer.JsonNullableModule/JsonNullableJackson3Module: the builder-style setter, with javadoc explaining the PATCH motivation.Tests
Added to
JsonNullWithEmptyTest, which is parameterized over both Jackson generations viaJsonProcessor, so each new test runs twice:JsonNullable<Integer>become a present nullJsonNullable<Boolean>given""becomes a present nullJsonNullable<String>is unaffected and still yieldsof("")./mvnw teston Temurin 17: 328 tests, 0 failures, 0 errors. The README gains a short "Blank strings" subsection.Relation to #152
#152 changes the default and, as measured on that thread, turns the enum and POJO cases into thrown exceptions. This PR keeps the early return unless the option is on, so it's the conservative answer for the 0.2.x line, as @Sekator778 recommended.
🤖 Generated with Claude Code
Summary by cubic
Adds an opt-in
mapBlankStringToNull(boolean)option toJsonNullableModule(Jackson 2) andJsonNullableJackson3Module(Jackson 3). When enabled, a blank string (""or whitespace only) sent for a non-StringJsonNullabledeserializes toJsonNullable.of(null)instead ofJsonNullable.undefined(), preserving a client's intent to clear a value in a PATCH request; the default isfalse, andJsonNullable<String>targets are never affected.withResolved(...)so it survives contextualization.JsonNullWithEmptyTestrun against both Jackson generations via theJsonProcessorparameterization.Written for commit dd3c486. Summary will update on new commits.