Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression: ClassCastException with contentConverter and Optional #4413

Closed
1 task done
emlun opened this issue Mar 4, 2024 · 6 comments
Closed
1 task done

Regression: ClassCastException with contentConverter and Optional #4413

emlun opened this issue Mar 4, 2024 · 6 comments
Labels
2.17 Issues planned at earliest for 2.17 has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue

Comments

@emlun
Copy link

emlun commented Mar 4, 2024

Search before asking

  • I searched in the issues and found nothing similar.

Describe the bug

Using @JsonSerialize(contentConverter = ...) on a field with an Optional<...> getter fails with Jackson 2.17.0-rc1 but not with 2.16.1, with a runtime error like this:

com.fasterxml.jackson.databind.JsonMappingException: class java.util.Collections$EmptyList cannot be cast to class java.security.cert.X509Certificate (java.util.Collections$EmptyList and java.security.cert.X509Certificate are in module java.base of loader 'bootstrap') (through reference chain: Jackson2_17_Test$WithContentConverter["x5c"])

It appears the serializer attempts to match the input type of the content converter against the type of the collection itself instead of its contents.

Version Information

2.17.0-rc1

Reproduction

Full reproduction test case:

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.JavaType;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.annotation.JsonSerialize;
import com.fasterxml.jackson.databind.json.JsonMapper;
import com.fasterxml.jackson.databind.type.TypeFactory;
import com.fasterxml.jackson.databind.util.Converter;
import com.fasterxml.jackson.datatype.jdk8.Jdk8Module;
import java.security.cert.X509Certificate;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
import org.junit.Test;

public class Jackson2_17_Test {

    private static ObjectMapper json() {
        return JsonMapper.builder()
                .addModule(new Jdk8Module())
                .build();
    }

    @Test
    public void testWithoutContentConverter() throws JsonProcessingException {
        final WithoutContentConverter value = new WithoutContentConverter(Collections.emptyList());
        final String encoded = json().writeValueAsString(value);
        System.out.println("testWithoutContentConverter encoded: " + encoded);
    }

    @Test
    public void testWithoutOptional() throws JsonProcessingException {
        final WithoutOptional value = new WithoutOptional(Collections.emptyList());
        final String encoded = json().writeValueAsString(value);
        System.out.println("testWithoutOptional encoded: " + encoded);
    }

    @Test
    public void testWithContentConverter() throws JsonProcessingException {
        final WithContentConverter value = new WithContentConverter(Collections.emptyList());
        final String encoded = json().writeValueAsString(value);
        System.out.println("testWithContentConverter encoded: " + encoded);
    }

    static class WithoutContentConverter {
        @JsonSerialize
        private List<X509Certificate> x5c;

        @JsonCreator
        public WithoutContentConverter(@JsonProperty("x5c") List<X509Certificate> x5c) {
            this.x5c = x5c;
        }

        public Optional<List<X509Certificate>> getX5c() {
            return Optional.ofNullable(x5c);
        }
    }

    static class WithoutOptional {
        @JsonSerialize(contentConverter = CertToBase64Converter.class)
        private List<X509Certificate> x5c;

        @JsonCreator
        public WithoutOptional(@JsonProperty("x5c") List<X509Certificate> x5c) {
            this.x5c = x5c;
        }

        public List<X509Certificate> getX5c() {
            return x5c;
        }
    }

    static class WithContentConverter {
        @JsonSerialize(contentConverter = CertToBase64Converter.class)
        public List<X509Certificate> x5c;

        @JsonCreator
        public WithContentConverter(@JsonProperty("x5c") List<X509Certificate> x5c) {
            this.x5c = x5c;
        }

        public Optional<List<X509Certificate>> getX5c() {
            return Optional.ofNullable(x5c);
        }
    }

    static class CertToBase64Converter implements Converter<X509Certificate, String> {
        @Override
        public String convert(X509Certificate value) {
            throw new UnsupportedOperationException("This code is never reached");
        }

        @Override
        public JavaType getInputType(TypeFactory typeFactory) {
            return typeFactory.constructType(X509Certificate.class);
        }

        @Override
        public JavaType getOutputType(TypeFactory typeFactory) {
            return typeFactory.constructType(String.class);
        }
    }

}

The tests testWithoutContentConverter and testWithoutOptional are expected to succeed and testWithContentConverter is expected to fail with Jackson 2.17.0-rc1. All three tests are expected to succeed with Jackson 2.16.1.

Expected behavior

@JsonSerialize(contentConverter = ...) should continue to work as it did in version 2.16.1.

Additional context

Originally detected here: Yubico/java-webauthn-server#350

@emlun emlun added the to-evaluate Issue that has been received but not yet evaluated label Mar 4, 2024
emlun added a commit to Yubico/java-webauthn-server that referenced this issue Mar 4, 2024
@cowtowncoder
Copy link
Member

Thank you for reporting this: it sounds like a bug indeed and would be good to fix before 2.17.0-final.

I was trying to see if there was something in 2.17.0-rc1 release notes, as this sounds vaguely familiar report, but could not see anything immediately relevant looking.

@cowtowncoder cowtowncoder added has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue 2.17 Issues planned at earliest for 2.17 and removed to-evaluate Issue that has been received but not yet evaluated labels Mar 5, 2024
@cowtowncoder
Copy link
Member

cowtowncoder commented Mar 5, 2024

Hmmh. Java 8 modules does have something that looks potentially relevant:

FasterXML/jackson-modules-java8#294

although fix itself would have been here in jackson-databind. I'll try to see if I can locate actual PR.

EDIT: local issue is #4337, and fix via PR #4338. Looks related enough...

@cowtowncoder
Copy link
Member

Looking at changes in #4338, I wonder if call to

        // 23-Jan-2024, tatu: May have a content converter:
        ser = findContextualConvertingSerializer(provider, property, ser);

might be the problem... although it appears to work for tested case.

(note: while this serializer for AtomicReference, same base class implementation is used as-is as base for Optional as well)

@cowtowncoder
Copy link
Member

Hmmh. Oh.

Unfortunately, I'll take it back: I'm afraid things might be working as they actually should be -- and whatever 2.16 did was not the way things should work.

Problem being that the definitions you have are incompatible:

    static class WithContentConverter {
        @JsonSerialize(contentConverter = CertToBase64Converter.class)
        public List<X509Certificate> x5c;

        @JsonCreator
        public WithContentConverter(@JsonProperty("x5c") List<X509Certificate> x5c) {
            this.x5c = x5c;
        }

        public Optional<List<X509Certificate>> getX5c() {
            return Optional.ofNullable(x5c);
        }
    }

since given for type Optional<List<X509Certificate>>, content type is actually List<X509Certificate> (it will only "peel off" one level -- there is no way to further define "content of content" converter).
Since all annotations from logical accessors are combined, @JsonSerialize is used as is, and getter has precedence over use of Field.

Depending on how you want things to work, you could add @JsonIgnore on getX5c() and I think things would work as Field x5c has List<> value and converter would then be used for its elements (as opposed to content of Optional).

@emlun
Copy link
Author

emlun commented Mar 5, 2024

Aaah, right. That makes a lot of sense now that you point it out. And yes, the proposed workaround works for me:

diff a/Jackson2_17_Test.java b/Jackson2_17_Test.java
--- a/Jackson2_17_Test.java
+++ b/Jackson2_17_Test.java
@@ -1,4 +1,5 @@
 import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.fasterxml.jackson.core.JsonProcessingException;
 import com.fasterxml.jackson.databind.JavaType;
@@ -80,6 +81,7 @@ public class Jackson2_17_Test {
             this.x5c = x5c;
         }

+        @JsonIgnore
         public Optional<List<X509Certificate>> getX5c() {
             return Optional.ofNullable(x5c);
         }

Though take note of this from the documentation of @JsonIgnore (emphasis added):

  • Only needs to be added to one of accessors/mutators (field, getter/setter, constructor parameter), but will have effect on the "whole" property: that is, adding annotation to a "getter" will also disable "setter"
    • ... unless "setter" has @JsonProperty, in which case this is considered a "split property" with enabled "setter" but no "getter" ("read-only", so that property may be read from input, but is not written output)

This is not a problem in my use case because my actual type has a @lombok.extern.jackson.Jacksonized builder, so the @JsonDeserialize annotation is propagated to the builder setter.

I considered proposing a small update to the @Json{Se,Dese}rializer JavaDoc to point out this incompatibility edge case,
but I guess this is another example of why you perhaps shouldn't be using Optional<List<T>> etc. in the first place. And also any future attempts to use it this way should fail quickly if Jackson is kept up to date, so there's probably no need to change the JavaDoc.

You may consider this resolved, I will fix the API misuse downstream. Thanks!

emlun added a commit to Yubico/java-webauthn-server that referenced this issue Mar 5, 2024
`@JsonSerialize(contentConverter = ...)` is incompatible with property
type `Optional<List<T>>`, which is the detected serialization type
because of the auto-detected getter, because `contentConverter` only
descends one container level.

See: FasterXML/jackson-databind#4413 (comment)
@cowtowncoder
Copy link
Member

@emlun Great! In the perfect world, Jackson could have figured this out but things do get quite complicated with multiple levels of nesting and possibly asymmetric types for accesors. So I am happy that you can make things work with the work-around.

Improvements to (Java)Docs are always very welcome, FWTW -- so would appreciate any!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.17 Issues planned at earliest for 2.17 has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue
Projects
None yet
Development

No branches or pull requests

2 participants