Skip to content

Commit c7bdbe1

Browse files
authored
Let Character, CharSequence and custom deserializers see empty strings (#187)
* Let Character/CharSequence and custom deserializers see empty strings JsonNullableJackson{2,3}Deserializer shortcut a VALUE_STRING token whose (trimmed) text is empty into JsonNullable.undefined() whenever the referenced type isn't String. That shortcut ran unconditionally, before the delegate deserializer ever saw the token, which caused two issues: - #26: JsonNullable<Character> (and CharSequence) got undefined() for "" instead of whatever Jackson's own deserializer does with it. Broaden the existing String-only exemption (renamed isStringDeserializer to isStringLike) to CharSequence, its implementations (e.g. StringBuilder) and Character. - #46: a deserializer an application registers itself for the wrapped type (directly or via a module) never received the empty string at all, because this shortcut ran first regardless of what the delegate was. Gate the shortcut on the resolved delegate being implemented by Jackson itself, so a user-registered deserializer always sees the token and decides for itself. The "is this Jackson's own deserializer" check is namespace-wide (concrete class under com.fasterxml.jackson. / tools.jackson.), not limited to jackson-databind's own deser package. A narrower, deser-only check would silently have changed every type handled by another FasterXML module instead of just String-like ones: on Jackson 2, jackson-datatype-jsr310's java.time deserializers live under com.fasterxml.jackson.datatype.jsr310.deser; on Jackson 3, java.time support moved inside jackson-databind itself but as a sibling of, not nested under, its deser package (tools.jackson.databind.ext.javatime.deser, alongside its scalars having also moved from .std to .jdk, and BeanDeserializer from the bare deser package to .bean). Measured with the narrower check: JsonNullable<LocalDate>/<OffsetDateTime> given "" or " " silently turned from undefined() into a present null on both stacks instead of throwing, which a generated OpenAPI client wrapping every optional date field in JsonNullable would hit routinely. Added jackson-datatype-jsr310 (version managed by the existing jackson-bom import) as a test-scoped dependency to cover the Jackson 2 side; the Jackson 3 side needs no extra module. Measured on Jackson 2.22.2 and 3.2.2: Jackson's own CharacterDeserializer does not throw for "" (yields a present null) or for a single blank character (yields that character), so this only ever surfaces values Jackson was already willing to produce. Adds coverage for Character/CharSequence/StringBuilder with empty, blank and non-blank strings, a SimpleModule-registered deserializer now receiving the empty string on both stacks (its own test class, so it runs once per stack instead of once per JsonProcessor), a java.time regression class for LocalDate/OffsetDateTime, and regression guards that Integer/Boolean/enum/plain-POJO targets still get undefined() by default. * Classify wrapped deserializers by the innermost delegate isJacksonOwnDeserializer classified a deserializer by its own concrete class name. That misclassified an application-registered DelegatingDeserializer subclass that does nothing but forward - e.g. a logging or validation wrapper a BeanDeserializerModifier installs around Jackson's own BeanDeserializer - as application-owned, so JsonNullable<Pojo> given "" started throwing InvalidFormatException instead of returning undefined() as it does without the wrapper. Follow JsonDeserializer#getDelegatee() / ValueDeserializer#getDelegatee() (bounded at 16 hops so a misbehaving or cyclic wrapper cannot loop forever) to the innermost deserializer of a delegation chain and classify by that one. A pass-through wrapper decides nothing about blank strings itself, so classifying by what it wraps keeps the historical shortcut for it exactly as before delegation was considered. The trade-off is the mirror image: a DelegatingDeserializer subclass that special-cases "" itself, rather than merely forwarding, is now classified by what it wraps rather than by itself. @JsonDeserialize(contentConverter = ...) is unaffected: it already resolves to Jackson's own StdDelegatingDeserializer (Jackson 3: StdConvertingDeserializer, renamed) for the wrapped value, which was already classified as standard without any unwrapping. Documented on both modules' mapBlankStringToNull javadoc and in the README's blank-strings section, alongside the new innermost-delegate rule. Tests on both stacks: a BeanDeserializerModifier-installed pass-through wrapper around Jackson's own BeanDeserializer now yields undefined()/a present null for "" instead of throwing (confirmed failing on the pre-change code); the same wrapper around an application-registered deserializer still lets it decide; and @JsonDeserialize(contentConverter = ...) keeps skipping the converter for "" and invoking it otherwise. Reported by @Thorsrud22 in review of #187. * Classify by nearest concrete or abstract ancestor isJacksonOwnDeserializer treated any class name outside Jackson's own namespace as application-owned. That misclassified two shapes it never considered: - An application subclass of one of Jackson's own CONCRETE deserializers (EnumDeserializer, BeanDeserializer, NumberDeserializers.NumberDeserializer) that changes nothing about deserialize() - the subclass exists to specialise something else, e.g. a BeanDeserializerModifier hook. Its own class name is not under Jackson's namespace, so it was classified as application-owned and the blank-string shortcut stopped running: undefined() became InvalidFormatException (enum, bean) or a present null instead of undefined() (number). - JsonNullable<JsonNullable<String>>: the outer's content deserializer is another instance of this library's own class, whose package is not under Jackson's namespace either, so the outer stopped shortcutting "" and let it fall through to the inner JsonNullable, turning undefined() into of(of("")). Fix: from the innermost deserializer (after the existing getDelegatee() unwrap), walk up the superclass chain to the nearest ancestor whose class is either this library's own JsonNullableJackson{2,3}Deserializer (matched by identity, not by package prefix - this project's own test fixtures live in the library's package for package-private access, and a prefix match misclassified all of them as "the library's own" too, breaking every existing custom-deserializer test; measured directly before settling on identity) or is under Jackson's own namespace. This library's own class is always standard. A Jackson ancestor that is abstract (JsonDeserializer, StdDeserializer, StdScalarDeserializer, ...) means the application implemented deserialization itself (the #46 authoring style) and stays application-owned. A Jackson ancestor that is concrete (EnumDeserializer, BeanDeserializer, NumberDeserializers.NumberDeserializer, ...) means the subclass adds nothing to Jackson's own blank-string handling and is standard. Documented, not fixed: a plain forwarding wrapper - a JsonDeserializer/ ValueDeserializer subclass that holds the delegate in a field without extending DelegatingDeserializer or overriding getDelegatee() - around Jackson's own Bean/Enum/Collection/Map deserializer cannot be told apart from an application deserializer built from scratch; it is classified by its own class. README and both modules' mapBlankStringToNull javadoc now say a wrapper should extend DelegatingDeserializer to be seen through. Also worth stating for the release notes: a subclass of the abstract FromStringDeserializer is application-owned under this rule and receives the token; FromStringDeserializer's own template maps a blank string to null before the subclass's _deserialize hook runs, so such a property yields of(null) where earlier versions returned undefined(). Measured on both stacks. Tests on both stacks: an application subclass of EnumDeserializer registered via addDeserializer and of BeanDeserializer installed by a BeanDeserializerModifier/ValueDeserializerModifier (undefined() and, with the option on, of(null)); a subclass of NumberDeserializers.NumberDeserializer for a JsonNullable<Number> (undefined()); and JsonNullable<JsonNullable<String>> (undefined() for "", of(of("x")) for "x", proving the nesting itself still works). All four failed on the pre-fix code before this commit: the enum and bean cases threw (InvalidFormatException, plus a Jackson 3 NullPointerException from EnumDeserializer's own toString-lookup path once the token reached it), the number case returned a present null instead of undefined(), and the nested case returned of(of("")) instead of undefined(). Found by an independent case sweep after @Thorsrud22's review of #187.
1 parent 649a6c1 commit c7bdbe1

9 files changed

Lines changed: 1280 additions & 12 deletions

File tree

‎README.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,9 @@ mapper.registerModule(new JsonNullableModule().mapBlankStringToNull(true));
9292
assertEquals(JsonNullable.<Integer>of(null), mapper.readValue("{\"age\":\"\"}", Person.class).age);
9393
```
9494
`JsonNullable<String>` properties are never affected: a blank string is a valid string value.
95+
A converter declared with `@JsonDeserialize(contentConverter = ...)` on a `JsonNullable<T>` property resolves to Jackson's own `StdDelegatingDeserializer` (Jackson 3: `StdConvertingDeserializer`) for the wrapped value, so a blank string still takes the shortcut above and the converter is not invoked for it; unchanged from earlier versions.
96+
A `DelegatingDeserializer` an application registers around a type's deserializer (directly, or via a `BeanDeserializerModifier`) is classified by the innermost deserializer it ultimately wraps, not by its own class, so a pass-through wrapper around Jackson's own deserializer keeps the shortcut above and one around an application deserializer keeps deferring to it.
97+
A deserializer that extends one of Jackson's own concrete deserializers (`EnumDeserializer`, `BeanDeserializer`, `NumberDeserializers.NumberDeserializer`, ...) keeps the shortcut above, since it inherits Jackson's own blank-string handling; one that implements deserialization on Jackson's abstract bases (`JsonDeserializer`, `StdDeserializer`, `StdScalarDeserializer`, ...) receives the token instead; and a wrapper that does not extend `DelegatingDeserializer` (or does not override `getDelegatee()`) is classified by its own class rather than by what it wraps, so a wrapper should extend `DelegatingDeserializer` to be seen through.
9598

9699
`JsonNullable` can also be used as a `@JsonCreator` constructor parameter.
97100
An absent property is passed to the constructor as `JsonNullable.undefined()` rather than as `null`, so it stays distinguishable from an explicit `null`.

‎pom.xml‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,15 @@
113113
<version>${hibernate-validator.version}</version>
114114
<scope>test</scope>
115115
</dependency>
116+
<!-- java.time (JsonNullable<LocalDate>/<OffsetDateTime>) regression coverage on the
117+
Jackson 2 stack; version managed by the jackson-bom import above. Jackson 3's
118+
java.time support ships inside jackson-databind itself, so it needs no
119+
equivalent dependency. -->
120+
<dependency>
121+
<groupId>com.fasterxml.jackson.datatype</groupId>
122+
<artifactId>jackson-datatype-jsr310</artifactId>
123+
<scope>test</scope>
124+
</dependency>
116125
<dependency>
117126
<groupId>org.junit.jupiter</groupId>
118127
<artifactId>junit-jupiter-api</artifactId>

‎src/main/java/org/openapitools/jackson/nullable/JsonNullableJackson2Deserializer.java‎

Lines changed: 146 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,56 @@
1313
import com.fasterxml.jackson.databind.type.ReferenceType;
1414

1515
import java.io.IOException;
16+
import java.lang.reflect.Modifier;
1617

1718
public class JsonNullableJackson2Deserializer extends ReferenceTypeDeserializer<JsonNullable<Object>> {
1819

1920
private static final long serialVersionUID = 1L;
2021

21-
private boolean isStringDeserializer = false;
22+
/**
23+
* Namespace prefix of Jackson 2 itself: not just {@code jackson-databind}'s own
24+
* {@code deser} package, but every FasterXML-published module an application might
25+
* add on top of it, e.g. {@code com.fasterxml.jackson.datatype.jsr310.deser}
26+
* (java.time support). Used to tell a deserializer Jackson (in the broad sense,
27+
* including datatype/other FasterXML modules) ships from one the application
28+
* registered itself (directly, via {@code @JsonDeserialize}, or via its own module).
29+
*/
30+
private static final String JACKSON_NAMESPACE_PREFIX = "com.fasterxml.jackson.";
31+
32+
/**
33+
* This library's own deserializer class, matched by identity rather than by package
34+
* prefix: a package-prefix match would claim any class placed in this package,
35+
* including this project's own tests and an application's own. Identity matches
36+
* exactly this class, which is what the content deserializer of a nested {@code
37+
* JsonNullable} is, e.g. {@code JsonNullable<JsonNullable<String>>}. This library's
38+
* own blank-string handling is what the outer {@code JsonNullable} should defer to.
39+
*/
40+
private static final Class<?> OWN_DESERIALIZER_CLASS = JsonNullableJackson2Deserializer.class;
41+
42+
/**
43+
* Bound on how many {@link JsonDeserializer#getDelegatee()} hops {@link
44+
* #isJacksonOwnDeserializer} will follow while unwrapping a chain of delegating
45+
* deserializers, so that a wrapper whose {@code getDelegatee()} misbehaves (e.g.
46+
* returns itself, or another wrapper in a cycle) cannot loop forever.
47+
*/
48+
private static final int MAX_DELEGATEE_UNWRAP_HOPS = 16;
49+
50+
/**
51+
* True when the referenced type is String-like: {@code String}, {@code CharSequence}
52+
* (and its implementations, e.g. {@code StringBuilder}) or {@code Character}. For
53+
* these types an empty/blank JSON string is a legitimate value in its own right, so
54+
* it must never be special-cased into {@link JsonNullable#undefined()}.
55+
*/
56+
private boolean isStringLike = false;
57+
58+
/**
59+
* Deserializers shipped by Jackson itself (databind, datatype and other FasterXML
60+
* modules) keep the historical empty/blank-string-means-absent shortcut below;
61+
* deserializers from any other namespace, i.e. the application's own or third-party
62+
* ones, receive the token (see issue #46).
63+
*/
64+
private boolean isStandardDeserializer = true;
65+
2266
private final boolean mapBlankStringToNull;
2367

2468
/*
@@ -37,8 +81,107 @@ public JsonNullableJackson2Deserializer(JavaType fullType, ValueInstantiator ins
3781
super(fullType, inst, typeDeser, deser);
3882
this.mapBlankStringToNull = mapBlankStringToNull;
3983
if (fullType instanceof ReferenceType && ((ReferenceType) fullType).getReferencedType() != null) {
40-
this.isStringDeserializer = ((ReferenceType) fullType).getReferencedType().isTypeOrSubTypeOf(String.class);
84+
JavaType referencedType = ((ReferenceType) fullType).getReferencedType();
85+
this.isStringLike = referencedType.isTypeOrSubTypeOf(CharSequence.class)
86+
|| referencedType.hasRawClass(Character.class);
87+
}
88+
this.isStandardDeserializer = isJacksonOwnDeserializer(deser);
89+
}
90+
91+
/**
92+
* @param deser the resolved delegate (content) deserializer for the referenced type,
93+
* or {@code null} if it has not been resolved yet.
94+
* @return whether the innermost deserializer {@code deser} ultimately delegates to
95+
* (see below) is classified as Jackson's own: walking up from its concrete class to
96+
* the nearest ancestor whose class name lives under Jackson's own namespace ({@code
97+
* com.fasterxml.jackson.}) or is this library's own class ({@link #OWN_DESERIALIZER_CLASS}),
98+
* that ancestor being this library's own class or a concrete Jackson class counts as
99+
* standard, an abstract Jackson class counts as application-owned (see below for the
100+
* full rule). This is deliberately namespace-wide, not limited to {@code
101+
* jackson-databind}'s own {@code deser} package: FasterXML's own datatype modules
102+
* (e.g. {@code jackson-datatype-jsr310} for java.time) ship deserializers outside that
103+
* package too, and narrowing the check to it would silently change behaviour for every
104+
* type such a module handles, on top of the deliberate change for String-like types.
105+
* A {@code null} deserializer (not yet resolved) is conservatively treated as standard,
106+
* preserving this class's pre-existing always-intercept behaviour until the real
107+
* delegate is known.
108+
* <p>
109+
* Before walking the class hierarchy, this follows {@link JsonDeserializer#getDelegatee()}
110+
* (bounded by {@link #MAX_DELEGATEE_UNWRAP_HOPS}) to the innermost deserializer of a
111+
* delegation chain and classifies by that one instead of by {@code deser} itself. A
112+
* wrapper an application registers around Jackson's own deserializer (a logging or
113+
* validation decorator, or one a {@code BeanDeserializerModifier} installs around
114+
* Jackson's {@code BeanDeserializer}) decides nothing about blank strings itself; it
115+
* only forwards to what it wraps. Classifying by what it wraps instead of by the
116+
* wrapper keeps the historical blank-string shortcut for it exactly as it was before
117+
* delegation was considered at all. The trade-off is the mirror image: a {@code
118+
* DelegatingDeserializer} subclass that special-cases {@code ""} itself, rather than
119+
* merely forwarding, is now classified by what it wraps rather than by itself, so its
120+
* own handling of blank strings is bypassed whenever what it wraps is Jackson's own. A
121+
* wrapper that does not extend {@code DelegatingDeserializer} (or does not override
122+
* {@code getDelegatee()}) is not seen through this way and is classified by its own
123+
* class instead.
124+
* <p>
125+
* From the innermost deserializer's concrete class, this walks up the superclass chain
126+
* (the class itself included) to the nearest ancestor whose name starts with either
127+
* prefix above:
128+
* <ul>
129+
* <li>no such ancestor exists (a class implementing the deserializer interface from
130+
* scratch without extending anything of Jackson's or this library's): application-owned.
131+
* On Jackson 2 this branch is unreachable, since {@code JsonDeserializer} itself is an
132+
* abstract class under Jackson's own namespace and therefore always matches, but it is
133+
* kept for safety.
134+
* <li>the ancestor is this library's own class (nested {@code JsonNullable}, e.g.
135+
* {@code JsonNullable<JsonNullable<String>>}, whose content deserializer is another
136+
* instance of this class): standard, since this library's own blank-string handling is
137+
* exactly what the outer {@code JsonNullable} should defer to.
138+
* <li>the ancestor is one of Jackson's own classes and is <em>abstract</em> (e.g.
139+
* {@code JsonDeserializer}, {@code StdDeserializer}, {@code StdScalarDeserializer}):
140+
* application-owned. Extending an abstract base and implementing the actual
141+
* deserialization is exactly the #46 authoring style, and it must keep receiving the
142+
* token regardless of how many abstract layers of Jackson's own it is built on.
143+
* <li>the ancestor is one of Jackson's own classes and is <em>concrete</em> (e.g.
144+
* {@code EnumDeserializer}, {@code BeanDeserializer}, {@code
145+
* NumberDeserializers.NumberDeserializer}): standard, whether or not the subclass
146+
* overrides {@code deserialize()}. The shortcut runs before the subclass is ever
147+
* reached, exactly as it did for that concrete Jackson deserializer before delegation
148+
* was considered at all; to handle blank strings itself, a deserializer extends one of
149+
* Jackson's abstract bases instead (the bullet above).
150+
* </ul>
151+
* <p>
152+
* This deliberately does not use a bare {@code instanceof StdDeserializer} /
153+
* {@code instanceof StdScalarDeserializer} check: both are public base classes that an
154+
* application's own custom deserializer is free to extend for convenience, and treating
155+
* every instance as "standard" would misclassify such a deserializer, silently
156+
* reintroducing the bug tracked as issue #46 for that authoring style. The
157+
* abstract/concrete distinction above is exactly what keeps that authoring style
158+
* application-owned: {@code StdDeserializer} and {@code StdScalarDeserializer} are
159+
* themselves abstract, so an application class built directly on either one still
160+
* receives the token, while a subclass of one of Jackson's own concrete deserializers
161+
* (which has already implemented {@code deserialize()} itself) does not.
162+
*/
163+
private static boolean isJacksonOwnDeserializer(JsonDeserializer<?> deser) {
164+
if (deser == null) {
165+
return true;
166+
}
167+
JsonDeserializer<?> innermost = deser;
168+
for (int hops = 0; hops < MAX_DELEGATEE_UNWRAP_HOPS; hops++) {
169+
JsonDeserializer<?> delegatee = innermost.getDelegatee();
170+
if (delegatee == null) {
171+
break;
172+
}
173+
innermost = delegatee;
174+
}
175+
for (Class<?> ancestor = innermost.getClass(); ancestor != null; ancestor = ancestor.getSuperclass()) {
176+
String name = ancestor.getName();
177+
if (ancestor == OWN_DESERIALIZER_CLASS) {
178+
return true;
179+
}
180+
if (name.startsWith(JACKSON_NAMESPACE_PREFIX)) {
181+
return !Modifier.isAbstract(ancestor.getModifiers());
182+
}
41183
}
184+
return false;
42185
}
43186

44187
/*
@@ -50,7 +193,7 @@ public JsonNullableJackson2Deserializer(JavaType fullType, ValueInstantiator ins
50193
@Override
51194
public JsonNullable<Object> deserialize(JsonParser p, DeserializationContext ctxt) throws IOException {
52195
JsonToken t = p.getCurrentToken();
53-
if (t == JsonToken.VALUE_STRING && !isStringDeserializer) {
196+
if (t == JsonToken.VALUE_STRING && !isStringLike && isStandardDeserializer) {
54197
String str = p.getText().trim();
55198
if (str.isEmpty()) {
56199
return mapBlankStringToNull ? JsonNullable.of(null) : JsonNullable.undefined();

0 commit comments

Comments
 (0)