Blank strings should not be deserialized as JsonNullable.undefined() #125

[joshuabrandes]

addresses #125

@wing328 I hope that works for you. If you want any changes, please let me know.

---

Summary by cubic

Blank strings now deserialize to JsonNullable.of(null) (present null) instead of JsonNullable.undefined() for non-String types. Fixes #125 and aligns behavior with default deserialization.

• Bug Fixes
  • Removed the empty-string check from JsonNullableJackson2Deserializer and JsonNullableJackson3Deserializer to let blank strings coerce to null.
  • Updated tests to expect JsonNullable.of(null) for empty or whitespace strings; missing fields still yield JsonNullable.undefined().

^{Written for commit 75b6cea. Summary will update on new commits.}

[cubic-dev-ai]

No issues found across 5 files

_{Re-trigger cubic}

[davidpavlovschi]

I read the full diff and ran this branch locally: ./mvnw clean test on Temurin 17.0.20+8 exits 0 with no failures or errors, including the updated JsonNullableSimpleTest (27/27).

What this gets right, and it is the easy thing to get wrong here: since #117 split the deserializer into a Jackson 2 and a Jackson 3 implementation, any behaviour change has to be applied twice. This PR removes the empty-string branch symmetrically from both JsonNullableJackson2Deserializer and JsonNullableJackson3Deserializer, drops the now-dead isStringDeserializer field and the ReferenceType import in both, and updates the whitespace-only expectations ({"age":" "} and " ") alongside the empty-string ones rather than only the obvious case. The trailing-newline fixes are a bonus.

Three things I would want resolved before this merges.

1. getEmptyValue is now out of step with deserialize. Both deserializers still have:

@Override
public Object getEmptyValue(DeserializationContext ctxt) {
    return JsonNullable.undefined();
}

while getNullValue returns JsonNullable.of(null). That asymmetry predates this PR, but removing the deserialize override is what makes it observable from two directions at once. On this branch:

static class Bean {
    @JsonSetter(nulls = Nulls.AS_EMPTY)
    public JsonNullable<Integer> value = JsonNullable.undefined();
}
// {"value":null}  -> JsonNullable.undefined   (getEmptyValue)
// {"value":""}    -> JsonNullable[null]        (new behaviour)

So a user who opted into AS_EMPTY to get "treat this as cleared" now gets the opposite of what an empty string gives them. Either flip getEmptyValue to JsonNullable.of(null) in the same PR, or add a short comment saying why undefined stays the right "empty" for the reference wrapper. Right now the module does not have one answer to "what does empty mean".

2. JsonNullWithEmptyTest has no whitespace case. JsonNullableSimpleTest covers " ", but JsonNullWithEmptyTest is the file named for this behaviour and it only tests "". Adding quote(" ") there (and to testBooleanWithEmpty) would keep the regression guard in the obvious place. Cheap, and it documents that the old .trim() semantics were deliberately folded into Jackson's own blank-string coercion rather than dropped by accident.

3. Release line and collision with #126. This is an unconditional behaviour change with no opt-out, on the 0.2.x line, and there is no README or changelog note. Anyone deserializing JsonNullable<Integer> from a form-encoded-ish payload today gets undefined() (field ignored on PATCH) and after this gets of(null) (field cleared) — that is a data-loss-shaped difference for exactly the PATCH use case this library exists for. @wing328, is 0.2.x the right home for this, or does it belong on the 0.3.x/Jackson-3 line? Worth deciding explicitly, because #126 implements the same fix as an opt-in mapBlankStringToNull flag defaulting to the current behaviour. The two cannot both land as-is; picking one is the blocking decision here, not the code.