Fix `Enum` deserialization with `JsonFormat.Shape.OBJECT` using both `DELEGATING` and `PROPERTIES` creator modes #3851

JooHyukKim · 2023-03-29T13:58:44Z

resolves #3566

JooHyukKim · 2023-03-29T14:03:44Z

src/main/java/com/fasterxml/jackson/databind/deser/std/FactoryBasedEnumDeserializer.java

                }
-                p.nextToken();
-                return deserializeEnumUsingPropertyBased(p, ctxt, _propCreator);
            }


Summary here,

Simply flipped the p.isExpectedStartObjectToken() conditional (line 135)

Allow pass-through of case for possible "delegating-creator" (line 144) -- in other words, "fail if f value cannot possibly be delegating-creator"

cowtowncoder · 2023-03-31T03:31:35Z

src/main/java/com/fasterxml/jackson/databind/deser/std/FactoryBasedEnumDeserializer.java

@@ -132,18 +132,21 @@ public Object deserialize(JsonParser p, DeserializationContext ctxt) throws IOEx
            // 30-Mar-2020, tatu: For properties-based one, MUST get JSON Object (before
            //   2.11, was just assuming match)
            if (_creatorProps != null) {
-                if (!p.isExpectedStartObjectToken()) {
+                if (p.isExpectedStartObjectToken()) {
+                    if (_propCreator == null) {


Ok I know this is existing code but this is wrong (not sure how I added such code :) ), wrt multi-threaded access.
Could you extract this into separate, synchronized method? At least it'd be slightly less bad.

sure! no problem

synchronize assignment to _propCreator right, @cowtowncoder ?

Ok, so it's transient but that won't do anything for access. I think what'd make sense is using AtomicReference here.

Should be thread-safe, now?

cowtowncoder · 2023-03-31T04:43:26Z

src/main/java/com/fasterxml/jackson/databind/deser/std/FactoryBasedEnumDeserializer.java

@@ -132,18 +135,21 @@ public Object deserialize(JsonParser p, DeserializationContext ctxt) throws IOEx
            // 30-Mar-2020, tatu: For properties-based one, MUST get JSON Object (before
            //   2.11, was just assuming match)
            if (_creatorProps != null) {
-                if (!p.isExpectedStartObjectToken()) {
+                if (p.isExpectedStartObjectToken()) {
+                    _propCreatorRef.compareAndSet(null,


But wouldn't this create (and try set) a new instance every time? :-D
(instead of trying to get() and only create-and-set if null)

this create (and try set) a new instance every time? :-D
hmm, Not really my intention 😅

Could you check again? Take below as an example. Invoking get() and set separately wouldn't be atomic anymore 🤔.

public void testAtomicRef() { Bean expected = new Bean(0); AtomicReference<Bean> reference = new AtomicReference<>(null); reference.compareAndSet(null, expected); reference.compareAndSet(null, new Bean(1)); reference.compareAndSet(null, new Bean(2)); // Passes assertEquals(0,reference.get().a); }

According to compareAndSet documentation,

Atomically sets the value to newValue if the current value == expectedValue, with memory effects

Or we can do good-ol' double-checked locking like ones done in EnumDeserializer below.

jackson-databind/src/main/java/com/fasterxml/jackson/databind/deser/std/EnumDeserializer.java

Lines 432 to 445 in 68ba588

protected CompactStringObjectMap _getToStringLookup(DeserializationContext ctxt) {

CompactStringObjectMap lookup = _lookupByToString;

if (lookup == null) {

synchronized (this) {

lookup = _lookupByToString;

if (lookup == null) {

lookup = EnumResolver.constructUsingToString(ctxt.getConfig(), _enumClass())

.constructLookup();

_lookupByToString = lookup;

}

}

}

return lookup;

}

No, my problem is that instead of only creating it if needed, it creates new instance every time and sets -- and never uses value. You are correct in that get + set is not atomic, but I am not worried about atomicity here; if instance gets created 2 or 3 times that's fine. They are all identical. Goal is just to avoid always creating instance eagerly.

And no need to do double-locking with synchronized: just do get() from reference; if (and only if) null returned, construct instance, set() (or compareAndSet() against null). Use the props gotten or created.

Or leave as-is and I can add that as well, not a big deal.

No, my problem is that instead of only creating it if needed, it creates new instance every time and sets -- and never uses value.

Oh, I think I understand now! Thank you for the explanation! 🙏🏼🙏🏼.

The code creates newly instance in compareAndSet(null, PropertyBasedCreator.construct()). The new intance will be ignored, but created "everytime" when _propCreator is not null anymore.

cowtowncoder · 2023-04-01T02:22:43Z

src/main/java/com/fasterxml/jackson/databind/deser/std/FactoryBasedEnumDeserializer.java

-                        PropertyBasedCreator.construct(ctxt, _valueInstantiator, _creatorProps,
-                            ctxt.isEnabled(MapperFeature.ACCEPT_CASE_INSENSITIVE_PROPERTIES)));
-
+                    if (_propCreatorRef.get() == null) {


Ok, I'll change this afterwards.... realized that for serializability of deserializer, better to use volatile after all.
Thank you for changes

@cowtowncoder I can change it to volatile and handle it like before if you'd like?

@JooHyukKim yes, basically I did that; no need to go back and forth. Sorry about confusion.

(the problem with JDK (de)serialization of JsonDeserializers is that we do not want to serialize fields like this, but in case of AtomicReference would need to make sure it's not-null ... volatile is easier, and performance won't matter a lot here-- although to be completely honest I think lazy construction here is probably not a good idea. Could just create eagerly.

@JooHyukKim yes,

~~Sure, I will make a PR soon.·~~

Oh you did already.

Right, just the volatile part.

cowtowncoder · 2023-04-01T02:38:16Z

Merged: thank you once again @JooHyukKim !

JooHyukKim · 2023-04-01T02:43:47Z

@cowtowncoder always fun to help~ 🙏🏼🙏🏼

JooHyukKim added 2 commits March 29, 2023 22:52

Fix

15e34c3

add comment

5b01c7c

JooHyukKim commented Mar 29, 2023

View reviewed changes

Reduce cognitive complexity

c5add48

cowtowncoder reviewed Mar 31, 2023

View reviewed changes

JooHyukKim added 2 commits March 31, 2023 13:01

Synchronize JsonParser

fb7c843

Remove transient keyword

8e38787

cowtowncoder reviewed Mar 31, 2023

View reviewed changes

construct _propCreatorRef lazily

3bc0de2

JooHyukKim requested a review from cowtowncoder April 1, 2023 00:55

cowtowncoder reviewed Apr 1, 2023

View reviewed changes

cowtowncoder approved these changes Apr 1, 2023

View reviewed changes

cowtowncoder merged commit 3aa2de1 into FasterXML:2.15 Apr 1, 2023

cowtowncoder added this to the 2.15.0 milestone Apr 1, 2023

JooHyukKim deleted the 3566-fix branch May 22, 2023 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `Enum` deserialization with `JsonFormat.Shape.OBJECT` using both `DELEGATING` and `PROPERTIES` creator modes #3851

Fix `Enum` deserialization with `JsonFormat.Shape.OBJECT` using both `DELEGATING` and `PROPERTIES` creator modes #3851

JooHyukKim commented Mar 29, 2023 •

edited

Loading

JooHyukKim Mar 29, 2023 •

edited

Loading

cowtowncoder Mar 31, 2023

JooHyukKim Mar 31, 2023

JooHyukKim Mar 31, 2023 •

edited

Loading

cowtowncoder Mar 31, 2023

cowtowncoder Mar 31, 2023

JooHyukKim Mar 31, 2023

cowtowncoder Mar 31, 2023

JooHyukKim Mar 31, 2023 •

edited

Loading

JooHyukKim Mar 31, 2023

cowtowncoder Mar 31, 2023

cowtowncoder Mar 31, 2023

JooHyukKim Mar 31, 2023 •

edited

Loading

cowtowncoder Apr 1, 2023 •

edited

Loading

JooHyukKim Apr 1, 2023 •

edited

Loading

cowtowncoder Apr 1, 2023

JooHyukKim Apr 1, 2023 •

edited

Loading

cowtowncoder Apr 1, 2023

cowtowncoder commented Apr 1, 2023

JooHyukKim commented Apr 1, 2023

	protected CompactStringObjectMap _getToStringLookup(DeserializationContext ctxt) {
	CompactStringObjectMap lookup = _lookupByToString;
	if (lookup == null) {
	synchronized (this) {
	lookup = _lookupByToString;
	if (lookup == null) {
	lookup = EnumResolver.constructUsingToString(ctxt.getConfig(), _enumClass())
	.constructLookup();
	_lookupByToString = lookup;
	}
	}
	}
	return lookup;
	}

Fix Enum deserialization with JsonFormat.Shape.OBJECT using both DELEGATING and PROPERTIES creator modes #3851

Fix Enum deserialization with JsonFormat.Shape.OBJECT using both DELEGATING and PROPERTIES creator modes #3851

Conversation

JooHyukKim commented Mar 29, 2023 • edited Loading

JooHyukKim Mar 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JooHyukKim Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JooHyukKim Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JooHyukKim Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

cowtowncoder Apr 1, 2023 • edited Loading

Choose a reason for hiding this comment

JooHyukKim Apr 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JooHyukKim Apr 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cowtowncoder commented Apr 1, 2023

JooHyukKim commented Apr 1, 2023

Fix `Enum` deserialization with `JsonFormat.Shape.OBJECT` using both `DELEGATING` and `PROPERTIES` creator modes #3851

Fix `Enum` deserialization with `JsonFormat.Shape.OBJECT` using both `DELEGATING` and `PROPERTIES` creator modes #3851

JooHyukKim commented Mar 29, 2023 •

edited

Loading

JooHyukKim Mar 29, 2023 •

edited

Loading

JooHyukKim Mar 31, 2023 •

edited

Loading

JooHyukKim Mar 31, 2023 •

edited

Loading

JooHyukKim Mar 31, 2023 •

edited

Loading

cowtowncoder Apr 1, 2023 •

edited

Loading

JooHyukKim Apr 1, 2023 •

edited

Loading

JooHyukKim Apr 1, 2023 •

edited

Loading