Skip to content

Conversation

@JacksonJang
Copy link
Contributor

Issue: #1654
Reference: #5477

In BasicDeserializerFactory, when creating a deserializer via createCollectionDeserializer(...),
I added logic so that if contentTypeDeser is an instance of NoOpTypeDeserializer, it is treated as null.
This ensures that @JsonDeserialize(contentUsing = ...) is applied correctly.

I’d appreciate it if you could review this when you have a moment.

@cowtowncoder
Copy link
Member

Thank you @JacksonJang -- sounds like possible path forward. I think I can merge #5477 first, then update this PR, re-review to make sure.

I really appreciate your help here!

@JacksonJang
Copy link
Contributor Author

Sounds great, thank you!

I’ll wait for #5477 to be merged and then update this PR accordingly.

Happy to continue refining the fix together :)

@cowtowncoder cowtowncoder changed the title Fix #1654: @JsonDeserialize(contentUsing) ignored when @JsonTypeInfo(use = Id.NONE) Further fixes to #1654: @JsonDeserialize(contentUsing) ignored when @JsonTypeInfo(use = Id.NONE) Dec 7, 2025
@cowtowncoder
Copy link
Member

@JacksonJang Ok, I added the missing test case, probably need fix in BasicSerializerFactory (similar to one you added in BasicDeserializerFactory)?

@JacksonJang
Copy link
Contributor Author

Thanks for adding the missing test case.

I agree that BasicSerializerFactory needs a similar fix.

I’ll update the serializer side to mirror the changes in BasicDeserializerFactory and push an update to this PR.

@JacksonJang
Copy link
Contributor Author

JacksonJang commented Dec 8, 2025

@cowtowncoder
Could I ask you to review whether the following test case is correct?

  • Value1654UntypedContainer -> expected to fail
  • Value1654UsingCustomSerDeserUntypedContainer -> expected to pass
@Test
void withNoTypeInfoOverrideSer() throws Exception {
    Value1654UsingCustomSerDeserUntypedContainer cont =
            new Value1654UsingCustomSerDeserUntypedContainer(
                    new Value1654(1),
                    new Value1654(2)
            );
    assertEquals(a2q("{'values':[{'v':1},{'v':2}]}"),
            MAPPER.writeValueAsString(cont));
}

@cowtowncoder
Copy link
Member

cowtowncoder commented Dec 8, 2025

@JacksonJang I think both are expected to pass -- Value1654UntypedContainer should use default BeanSerializer and produce

{"x":1}

but Value1654UsingCustomSerDeserUntypedContainer has custom serializer override and should produce

{"v":1}

Unless I misunderstood question here?

EDIT: let me double-check above so I did not make a mistake... :)

@JacksonJang
Copy link
Contributor Author

Sorry for the confusion, I didn’t explain my point clearly earlier.
My point was that the test usage seems inconsistent:

withNoTypeInfoDefaultSer() is correctly testing Value1654UntypedContainer,
but withNoTypeInfoOverrideSer() is also using Value1654UntypedContainer.

Since withNoTypeInfoOverrideSer() is meant to verify the behavior with custom
content (de)serializers, it looks like it should instead be testing
Value1654UsingCustomSerDeserUntypedContainer.

Please let me know if I’m misunderstanding the intention of the test setup.

[AS-IS]

@Test
    void withNoTypeInfoOverrideSer() throws Exception {
        Value1654UntypedContainer cont = new Value1654UntypedContainer(
                new Value1654(1),
                new Value1654(2)
        );
        assertEquals(a2q("{'values':[{'v':1},{'v':2}]}"),
                MAPPER.writeValueAsString(cont));
    }

[TO-BE]

@Test
    void withNoTypeInfoOverrideSer() throws Exception {
        Value1654UsingCustomSerDeserUntypedContainer cont = new Value1654UsingCustomSerDeserUntypedContainer(
                new Value1654(1),
                new Value1654(2)
        );
        assertEquals(a2q("{'values':[{'v':1},{'v':2}]}"),
                MAPPER.writeValueAsString(cont));
    }

@cowtowncoder
Copy link
Member

cowtowncoder commented Dec 8, 2025

@JacksonJang Yes, I think you are correct: I broke that test. Good catch. Let me fix that.

@cowtowncoder
Copy link
Member

Done. Now fails in a bit more interesting way. :)

@JacksonJang
Copy link
Contributor Author

Thanks for taking a look and confirming :)
I’ll check it on my side.

@cowtowncoder
Copy link
Member

cowtowncoder commented Dec 8, 2025

@JacksonJang Ok: I have possible fix for this issue:

diff --git a/src/main/java/tools/jackson/databind/ValueSerializer.java b/src/main/java/tools/jackson/databind/ValueSerializer.java
index 95aaf99a5..90ddfd6e0 100644
--- a/src/main/java/tools/jackson/databind/ValueSerializer.java
+++ b/src/main/java/tools/jackson/databind/ValueSerializer.java
@@ -252,6 +252,14 @@ public abstract class ValueSerializer<T>
             TypeSerializer typeSer)
         throws JacksonException
     {
+        // 07-Dec-2025, tatu: [databind#1654] Check for "no-op" type serializer
+        //   indirectly, by checking if resolver exists -- if not, omit type id
+        //   handling altogether
+        if (typeSer.getTypeIdResolver() == null) {
+            serialize(value, gen, ctxt);
+            return;
+        }
+
         Class<?> clz = handledType();
         if (clz == null) {
             clz = value.getClass();
diff --git a/src/main/java/tools/jackson/databind/jsontype/impl/NoOpTypeSerializer.java b/src/main/java/tools/jackson/databind/jsontype/impl/NoOpTypeSerializer.java
index 0820ea70e..c6ed69414 100644
--- a/src/main/java/tools/jackson/databind/jsontype/impl/NoOpTypeSerializer.java
+++ b/src/main/java/tools/jackson/databind/jsontype/impl/NoOpTypeSerializer.java
@@ -49,6 +49,8 @@ public class NoOpTypeSerializer extends TypeSerializer
 
     @Override
     public TypeIdResolver getTypeIdResolver() {
+        // 07-Dec-2025, tatu: [databind#1654] Important! Indicates
+        //   that no actual Type Id handled.
         return null;
     }

which does work, but I am wondering if there is a way to do it better in BasicSerializerFactory, if you can find something.
Let me know either way.

@cowtowncoder
Copy link
Member

@JacksonJang With As.NOTHING, even smaller change:

diff --git a/src/main/java/tools/jackson/databind/ValueSerializer.java b/src/main/java/tools/jackson/databind/ValueSerializer.java
index 95aaf99a5..dbce05b90 100644
--- a/src/main/java/tools/jackson/databind/ValueSerializer.java
+++ b/src/main/java/tools/jackson/databind/ValueSerializer.java
@@ -4,6 +4,7 @@ import java.util.Iterator;
 import java.util.Set;
 
 import com.fasterxml.jackson.annotation.JsonFormat;
+import com.fasterxml.jackson.annotation.JsonTypeInfo.As;
 
 import tools.jackson.core.*;
 import tools.jackson.databind.jsonFormatVisitors.JsonFormatVisitable;
@@ -252,6 +253,13 @@ public abstract class ValueSerializer<T>
             TypeSerializer typeSer)
         throws JacksonException
     {
+        // 07-Dec-2025, tatu: [databind#1654] Check for "no-op" type serializer
+        //   indirectly
+        if (typeSer.getTypeInclusion() == As.NOTHING) {
+            serialize(value, gen, ctxt);
+            return;
+        }
+
         Class<?> clz = handledType();
         if (clz == null) {
             clz = value.getClass();

@JacksonJang
Copy link
Contributor Author

JacksonJang commented Dec 8, 2025

I'm also looking for a way to resolve this within BasicSerializerFactory, but I haven’t found a clean solution yet.
If we need to move forward with the merge sooner, I’m totally fine proceeding with the approach you wrote earlier.

In the meantime, I’ll keep exploring possible solutions in BasicSerializerFactory.

As.NOTHING is nice :)

@cowtowncoder
Copy link
Member

@JacksonJang There's no hurry so I'll let you investigate. Just let me know if you feel you are not making progress and we can go with my work-around.

@JacksonJang
Copy link
Contributor Author

Thank you :)
I'll do my best.

@JacksonJang
Copy link
Contributor Author

JacksonJang commented Dec 8, 2025

I added the following branch to the code:

else if (typeSer instanceof NoOpTypeSerializer) {
    ser.serialize(elem, g, provider);
}

With this change, all tests are passing.
Would it be okay to open a PR with this approach?

In CollectionSerializer,apply the code:

public void serializeContentsUsing(Collection<?> value, JsonGenerator g, SerializationContext provider,
            ValueSerializer<Object> ser)
        throws JacksonException
    {
        Iterator<?> it = value.iterator();
        if (it.hasNext()) {
            // [databind#4849]/[databind#4214]: need to check for EnumSet
            final TypeSerializer typeSer = (_maybeEnumSet && value instanceof EnumSet<?>)
                    ? null : _valueTypeSerializer;
            int i = 0;
            do {
                Object elem = it.next();
                try {
                    if (elem == null) {
                        provider.defaultSerializeNullValue(g);
                    } else {
                        if (typeSer == null) {
                            ser.serialize(elem, g, provider);
                        } else if (typeSer instanceof NoOpTypeSerializer) {
                            ser.serialize(elem, g, provider);
                        }  else {
                            ser.serializeWithType(elem, g, provider, typeSer);
                        }
                    }
                    ++i;
                } catch (Exception e) {
                    wrapAndThrow(provider, e, value, i);
                }
            } while (it.hasNext());
        }
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants