Enhance GuavaBeanSerializerModifier to allow unwrapping of Optional

[mukham12]

Should get the current OptionalUnwrappedTest.java under the failing tests directory to pass.

[JooHyukKim]

You can move the test class out of failing, into target test directory

[mukham12]

Will do. I wanted Tatu to take a look and approve first. Thanks 😊

[cowtowncoder]

Looked reasonable, but will need more time to get familiar again wrt how exactly unwrapped handling works.

@mukham12 Did you have a look at Java 8 datatypes module's handling of java.lang.Optional? I don't remember if it already supported unwrapping, or if it also needs improvements.

[mukham12]

I exclusively focused on this project, Tatu. I was curious about the single failing test.

Upon investigation, I noticed that the name transformer in the bean writer disappeared after the serializer modifier was applied. It seemed that the modifier wasn't properly checking for UnwrappingBeanPropertyWriter and consequently ignoring the name transformer.

The change seemed easy enough, so I created a PR. 🚀

[cowtowncoder]

@mukham12 Thank you for working on this!

I remembered that Java 8 Optional already supports this, so thought I'd see how that differs:

https://github.com/FasterXML/jackson-modules-java8/tree/2.17/datatypes/src/main/java/com/fasterxml/jackson/datatype/jdk8

and I think that while what you show seems to work, it might be better to go with example of Jdk8OptionalBeanPropertyWriter, in which non-unwrapping writer overrides unwrappingWriter() to effect the change, instead of requiring BeanPropertyModifier to handle annotation discovery.

Could you try to see if you could make that change? Will be happy to merge the fix.

[cowtowncoder]

@mukham12 Yeah NP for not checking other projects, I just happened to remember this because there are basically 3 datatypes that work similarly:

1. JDK's old AtomicReference
2. Guava Optional
3. Java 8 Optional

(plus Scala has its own Option or something, but that's different story).

so thought I should mention it since (3) is probably the most complete example, and Guava just was never (fully?) retrofitted.

[mukham12]

Sure, will revisit this as soon as I get a chance.

[mukham12]

Tatu,

After further investigation, I believe the modification I made is likely the only solution, unless I’ve misunderstood your request. As shown in the attached screenshot, line 402 of BeanSerializerFactory correctly identifies the beans and includes the name transformer. I traced the method calls and saw that it was internally calling unwrappingWriter method. The issue arises at line 415 of BeanSerializerFactory when the changeProperties() method of BeanSerializerModifier is invoked. Post this method, the name transformer is lost and we’re left with a “standard” bean property writer. This is because that method is not doing any checks for "unwrapping" functionality. I hope my explanation is clear despite its length. I would appreciate it if you can review the code when you have some time.

[cowtowncoder]

@mukham12 If that is the case, are you suggesting that handling of Optional with unwrapping, as implemented by jackson-modules-java8 would not work wrt test you have? (modified to use that type instead of Guava variant)

I guess I'll need to see how test cases differ.

[mukham12]

Correct. As far as I know, the difference between this test and jackson-modules-java8 is that the former has a serializer modifier invoking changeProperties, while the latter isn’t.

[cowtowncoder]

I am not sure I understand. Test that fails for Guava:

    static class Child {
        public String name = "Bob";
    }

    static class Parent {
        private Child child = new Child();

        @JsonUnwrapped
        public Child getChild() { return child; }
    }

    static class OptionalParent {
        @JsonUnwrapped(prefix="XX.")
        public Optional<Child> child = Optional.of(new Child());
    }

    // Test for "old" settings (2.5 and earlier only option; available on later too)
    public void testUntypedWithNullEqOptionals() throws Exception
    {
        final ObjectMapper mapper = mapperWithModule(true);
        String jsonExp = aposToQuotes("{'XX.name':'Bob'}");
        String jsonAct = mapper.writeValueAsString(new OptionalParent());
        assertEquals(jsonExp, jsonAct);
    }

Appears identical to the one from jackson-modules-java8:

    static class Child {
        public String name = "Bob";
    }

    static class Parent {
        private Child child = new Child();

        @JsonUnwrapped
        public Child getChild() {
            return child;
        }
    }

    static class OptionalParent {
        @JsonUnwrapped(prefix = "XX.")
        public Optional<Child> child = Optional.of(new Child());
    }

    public void testUntypedWithOptionalsNotNulls() throws Exception
    {
        final ObjectMapper mapper = mapperWithModule(false);
        String jsonExp = a2q("{'XX.name':'Bob'}");
        String jsonAct = mapper.writeValueAsString(new OptionalParent());
        assertEquals(jsonExp, jsonAct);
    }

with the exception that latter passes with handling implemented by the module.

[cowtowncoder]

Point being that presumably handling of JDK 8 Optional manages to pass NameTransformer as it should; it also uses BeanSerializerModifier (Jdk8BeanSerializerModifier) that modifies BeanPropertyWriters into Jdk8OptionalBeanPropertyWriter first (using changeProperties()); and THEN later on that instances unwrappingWriter() is called. But it also MUST override method

    protected BeanPropertyWriter _new(PropertyName newName);

to keep its identity.

So I am suggesting that to be the cleaner way to obtain the same end result.

[mukham12]

Certainly, Tatu. While the tests may seem identical at first glance, there is one crucial distinction. When constructing the mapper, the absentsAsNulls parameter for jackson-module-java8 is set to false, but for the failing Guava test, it's set to true. Consequently, a serializer modifier is added for Guava Optional, but not for jackson-module-java8 Optional.

Adjusting the Guava test to also pass false for the absentsAsNulls parameter would resolve the issue. That is what I have been trying to convey earlier. The optional tests from JDK8 repo did not seem to have been altered by a modifier, but Guava was.

[cowtowncoder]

Ahhhh. Ok, yes, that makes more sense. I did not realize BeanSerializerModifier was only registered with that particular configuration (or rather did not remember; I wrote it at some point :) ).

[mukham12]

Having established the root cause of the issue, how would you like to handle the resolution?

[cowtowncoder]

@mukham12 Should this be reopened? I am +1 on your original fix since it works.