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

Serializing Optional not enabled by default since 2.16.0 #4499

Closed
1 task done
cor3000 opened this issue Apr 26, 2024 · 11 comments
Closed
1 task done

Serializing Optional not enabled by default since 2.16.0 #4499

cor3000 opened this issue Apr 26, 2024 · 11 comments

Comments

@cor3000
Copy link

cor3000 commented Apr 26, 2024

Search before asking

  • I searched in the issues and found nothing similar.

Describe the bug

in version 2.15.4 the following code below succeeded

giving the output

{"test":{"empty":false,"present":true}}
{"test":{"empty":false,"present":true}}

(Obviously this value doesn't make sense, but it doesn't fail with a hard exception, breaking existing projects)

when using version a higher version e.g. 2.17.0 the following Exception occurs instead (in both cases the jackson-datatype-jdk8.jar of the same version is on the classpath)

Exception in thread "main" com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Java 8 optional type `java.util.Optional` not supported by default: add Module "com.fasterxml.jackson.datatype:jackson-datatype-jdk8" to enable handling (through reference chain: java.util.ImmutableCollections$Map1["test"])
	at com.fasterxml.jackson.databind.exc.InvalidDefinitionException.from(InvalidDefinitionException.java:77)
	at com.fasterxml.jackson.databind.SerializerProvider.reportBadDefinition(SerializerProvider.java:1330)
	at com.fasterxml.jackson.databind.ser.impl.UnsupportedTypeSerializer.serialize(UnsupportedTypeSerializer.java:35)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeFields(MapSerializer.java:808)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeWithoutTypeInfo(MapSerializer.java:764)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:720)
	at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:35)
	at com.fasterxml.jackson.databind.ser.DefaultSerializerProvider._serialize(DefaultSerializerProvider.java:502)
	at com.fasterxml.jackson.databind.ser.DefaultSerializerProvider.serializeValue(DefaultSerializerProvider.java:341)
	at com.fasterxml.jackson.databind.ObjectMapper._writeValueAndClose(ObjectMapper.java:4799)
	at com.fasterxml.jackson.databind.ObjectMapper.writeValueAsString(ObjectMapper.java:4040)
	at Jackson2_17_Test_ObjectMapper2.main(Jackson2_17_Test_ObjectMapper2.java:24)

I didn't find this default behavior change in the release notes, so I wonder whether this is an intended change.
As a workaround I can register the Jdk8Module manually (commented out in the code above), to produce the same results as in 2.15.4

Version Information

2.17.0

Reproduction

Run the example using the maven dependencies

  • com.fasterxml.jackson.core:jackson-databind:2.15.4
  • com.fasterxml.jackson.datatype:jackson-datatype-jdk8:2.15.4

after switching to 2.16.0, 2.16.1, 2.17.0 the same code below fails

  • com.fasterxml.jackson.core:jackson-databind:2.17.0
  • com.fasterxml.jackson.datatype:jackson-datatype-jdk8:2.17.0
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.json.JsonMapper;
import com.fasterxml.jackson.datatype.jdk8.Jdk8Module;

import java.util.Map;
import java.util.Optional;

public class Jackson2_17_Test_Optional {

    public static void main(String[] args) throws JsonProcessingException {

        Map<String, Optional<String>> object = Map.of(
                "test", Optional.of("optional value"));

        JsonMapper jsonMapper = JsonMapper.builder()
//                .addModule(new Jdk8Module()) // workaround, register jdk8 module manually
                .build();
        String value = jsonMapper.writeValueAsString(object);
        System.out.println(value);

        ObjectMapper objectMapper = new ObjectMapper();
//        objectMapper.registerModule(new Jdk8Module()); // workaround, register jdk8 module manually
        String value2 = objectMapper.writeValueAsString(object);
        System.out.println(value2);
    }
}

Expected behavior

giving the output as in 2.15.4, not throwing an exception

{"test":{"empty":false,"present":true}}
{"test":{"empty":false,"present":true}}

Additional context

even though we have a manual workaround it's difficult to track and adjust all usages by external libraries.

my apologies in case I missed the related documentation and this is the new expected behavior since 2.16.0

@cor3000 cor3000 added the to-evaluate Issue that has been received but not yet evaluated label Apr 26, 2024
@cor3000
Copy link
Author

cor3000 commented Apr 26, 2024

I found this was changed by #4082
so I guess that's the intended behavior now... if that's the case, please confirm and close the issue

@cowtowncoder
Copy link
Member

@cor3000 Correct: this is the new intended behavior, and exception indicates the root cause. It is unfortunate issue referred does not fully explain logic (or have good title) to make it easier to find the change.

But the goal was to prevent accidental serialization of Optional using structure that was never meant to be used by databind (but happens to "work" due to existence of boolean isEmpty() method).

So registering Java 8 types module is the fix, not workaround. Or, alternatively, registering custom serializer.

@cowtowncoder cowtowncoder removed the to-evaluate Issue that has been received but not yet evaluated label Apr 26, 2024
@schiemon
Copy link

schiemon commented Nov 17, 2024

Thank you, @cowtowncoder, for the clarification! Is there a way to avoid having to register the Jdk8Module for every ObjectMapper instance and instead register it globally?

@cowtowncoder
Copy link
Member

@schiemon if you mean "set some static thing to force all ObjectMapper to add JdkModule" then no.

@JooHyukKim
Copy link
Member

Yeah, globally (statically) setting such thing is not desirable and won't be in the future either.
Try doing such thing at application-level like factory method for generating mapper with such?

@schiemon
Copy link

schiemon commented Nov 18, 2024

@cowtowncoder yes, this exactly what I had in mind.
@JooHyukKim This would be possible but imagine a giant codebase prior to this decision which has not wrapped those ObjectMappers. Now, for the patch upgrade, in such codebase one has to touch every occurance of it which I aim to avoid.

@cowtowncoder
Copy link
Member

@schiemon I can see how that can be problematic. Unfortunately Jackson is so widely used by various frameworks and libraries that any global configuration (at Jackson level, not by application) is potential problem -- meaning that if global configuration existed, it would effect things users wouldn't plan to. So the choice of having no (*) globally configurable entries is conscious.
And in case of modules I am pretty sure we'll never want to expose global set of modules to add. But users can definitely add global singletons for producing either ObjectMappers, or their Builders, that do do that. Takes planning of course.

(*) there are couple of things related to security (max name length) for which there are global static settings one can define as baseline -- but all overridable.

@JooHyukKim
Copy link
Member

@JooHyukKim This would be possible but imagine a giant codebase prior to this decision which has not wrapped those ObjectMappers. Now, for the patch upgrade, in such codebase one has to touch every occurance of it which I aim to avoid.

Yeah, sorry to hear that. I don't know how gigantic you're talking about, but I have experience working with such systems as well. Somethings are better off modularized or isolated. But! Let's consider this as techincal debt we are willing to pay off :-) willing because it could be a sign that system getting bigger means ur business grew as well.

Hopefully but there may be programming patterns/approaches to find!
Cheers
@schiemon

@schiemon
Copy link

schiemon commented Nov 27, 2024

Thank you again for your detailed answers, @JooHyukKim and @cowtowncoder. I now have a much better understanding of the problem from your perspective as library maintainers.

I do have one remaining question, though. Since it is closely related to the original question, I will ask it here. However, feel free to let me know if I should open a separate issue for this.

Let’s say I register the Jdk8Module for all ObjectMappers in this ominous giant codebase. Prior to this registration, entire object trees were effectively "hidden" under an Optional property. These trees were not serialized but were instead represented as {"empty":false,"present":true}. After the upgrade and registration, these sub-trees are now being serialized, which has significant implications for performance and correctness.

Is there a way in the newer version to revert to the previous behavior to ensure that mappers are producing the same output? For instance, could something be configured at the mapper level to achieve this?

This is getting long but I think the following is also important to mention in this context. I noticed a difference in behaviour between older versions when handling Optionals with a mapper where mapper.setVisibility(PropertyAccessor.ALL, JsonAutoDetect.Visibility.NONE) is set. With the small setup here I recognized following behaviour:

2.12.7
{"parentGetter":{}}
2.13.0
{"parentGetter":{"empty":false,"present":true}}

When I am removing the visibility setting, both output the same string ({"parentGetter":{"empty":false,"present":true}}). I would be very grateful if you could explain where this behaviour comes from so that I can consider it when trying to adapt the mappers!

@cowtowncoder
Copy link
Member

@schiemon I do not know why/how settings as applied in 2.12.x (which seem correct) would work differently in 2.13.x. These are relatively old versions by now so nothing much could be done to change them.

But one possibility is that something in handling of "is-getter" ("isPresent()" etc) vs "regular getter" ("getValue()" etc) was not properly handled any longer; that is, visibility setting was not applied to "is-setters" and defaults were.
I would recommend trying with later minor versions since if there was such a visibility bug, it would likely have been found and fixed. I do not remember specific issue off-hand, however. Will see if I can spot anything in release notes.

Finally, while there is no centralized way to revert serialization of Optional<T> values back to no-module-registered case, it is still possible to just register custom JsonSerializer that can output old style output (whether it should be { } or Object with "empty" and "present").
That might be the best way, although requires configuration of ObjectMappers too.

@cowtowncoder
Copy link
Member

cowtowncoder commented Nov 27, 2024

Also tests for attempts to serialize Optional, without module registered, will will result in exception is tested via:

src/test/java/com/fasterxml/jackson/databind/interop/OptionalJava8Fallbacks4082Test.java

cesmarvin pushed a commit to scm-manager/scm-manager that referenced this issue Dec 13, 2024
A minor change in a test class was necessary; cf. FasterXML/jackson-databind#4499

Co-authored-by: Till-André Diegeler<[email protected]>
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

No branches or pull requests

4 participants