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.

[Sekator778]

Reviewed as part of picking up the triage backlog here (context: #71). No merge rights, so this is a recommendation.

Verdict: needs a decision before it can go in — not as an unconditional default change.

The Jackson2/Jackson3 halves are done correctly and symmetrically, and for JsonNullable<String> nothing changes. But dropping the guard sends blank strings into Jackson's own coercion for every target type, and that coercion is not uniform. Measured against a real ObjectMapper, identical on Jackson2 and Jackson3:

target	before	after
JsonNullable<String>	of("")	of("")
JsonNullable<Integer>, <Boolean>	undefined()	of(null)
JsonNullable<SomeEnum>	undefined()	throws InvalidFormatException
JsonNullable<Pojo> (no single-String ctor)	undefined()	throws InvalidFormatException

Row two is what #125 asked for. Rows three and four turn a silent, working path into a thrown exception for anyone whose clients send a stray blank string into a non-scalar field, and neither is covered: the updated assertions exercise Integer only, which is the one case where Jackson coerces cleanly. #46 suggests the enum path is live rather than theoretical.

Second thing: getEmptyValue() still returns undefined(), so with @JsonSetter(nulls = Nulls.AS_EMPTY) this makes {"value":null} and {"value":""} behave oppositely, where master treats them the same:

master:   {value:null} -> undefined   {value:""} -> undefined
this PR:  {value:null} -> undefined   {value:""} -> JsonNullable[null]

@davidpavlovschi flagged that in this thread; I reproduced it rather than take it on trust.

On this PR versus #126: I re-read #125, and the reporter's example is JsonNullable<Integer> with reasoning anchored to how plain Jackson treats a blank string for Integer — enums and nested objects were never in scope. So I'd take #126 (opt-in flag, default false) on the 0.2.x line and keep this PR's unconditional behaviour as the design target for a documented default flip in 0.3.x/1.0. The version number says 0.2.x, but the library is embedded transitively in generated clients across thousands of repositories whose authors never read this dependency's changelog, and what changes here is what a PATCH does to a stored record. A blank string is ambiguous client input, not a spec-defined "clear this field": some stacks emit it deliberately, others as an artifact upstream of the JSON.

If you'd rather keep this PR as the vehicle: fix the getEmptyValue/AS_EMPTY inconsistency, and decide whether the enum/POJO throw is intentional — testing it either way. That would make it a much easier call.