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

Fail to serialize TemporalAdjuster type with 2.12 #207

Closed
gokhanoner opened this issue Mar 21, 2021 · 7 comments
Closed

Fail to serialize TemporalAdjuster type with 2.12 #207

gokhanoner opened this issue Mar 21, 2021 · 7 comments
Milestone

Comments

@gokhanoner
Copy link

gokhanoner commented Mar 21, 2021

Starting with 2.12, below code fails & throw exception. I wonder if it's related to FasterXML/jackson-databind#2683

Tested with 2.11.4 & 2.10.5, both serialize successfully.

Note: Adding deprecated jsr310 module also doesn't work.

public class JacksonTest {
    private static ObjectMapper objectMapper;

    @BeforeClass
    public static void setup() {
        objectMapper = JsonMapper.builder()
                .addModules(
                        new JavaTimeModule()
                )
                .build();
    }

    @Test
    public void whenUsingTemporalAdjusterExpectTrue() throws JsonProcessingException {
        TestPojo mrd = new TestPojo();
        System.out.println(objectMapper.writeValueAsString(mrd));
    }

    static class TestPojo {
        private TemporalAdjuster ta;

        TestPojo() {
            this.ta = TemporalAdjusters.firstDayOfMonth();
        }

        public TemporalAdjuster getTa() {
            return ta;
        }

        public void setTa(TemporalAdjuster ta) {
            this.ta = ta;
        }
    }
}

com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Java 8 date/time type `java.time.temporal.TemporalAdjusters$$Lambda$80/0x0000000800c07840` not supported by default: add Module "com.fasterxml.jackson.datatype:jackson-datatype-jsr310" to enable handling (through reference chain: com.oner.JacksonTest$TestPojo["ta"])

	at com.fasterxml.jackson.databind.exc.InvalidDefinitionException.from(InvalidDefinitionException.java:77)
	at com.fasterxml.jackson.databind.SerializerProvider.reportBadDefinition(SerializerProvider.java:1276)
	at com.fasterxml.jackson.databind.ser.impl.UnsupportedTypeSerializer.serialize(UnsupportedTypeSerializer.java:35)
	at com.fasterxml.jackson.databind.ser.BeanPropertyWriter.serializeAsField(BeanPropertyWriter.java:728)
	at com.fasterxml.jackson.databind.ser.std.BeanSerializerBase.serializeFields(BeanSerializerBase.java:770)
	at com.fasterxml.jackson.databind.ser.BeanSerializer.serialize(BeanSerializer.java:178)
	at com.fasterxml.jackson.databind.ser.DefaultSerializerProvider._serialize(DefaultSerializerProvider.java:480)
	at com.fasterxml.jackson.databind.ser.DefaultSerializerProvider.serializeValue(DefaultSerializerProvider.java:319)
	at com.fasterxml.jackson.databind.ObjectMapper._writeValueAndClose(ObjectMapper.java:4487)
	at com.fasterxml.jackson.databind.ObjectMapper.writeValueAsString(ObjectMapper.java:3742)
@cowtowncoder
Copy link
Member

Looking at TemporalAdjuster interface, this does not look like a value type Jackson could handle by default, and not sure what the expectation would be; esp. not sure how it could be deserialized?

How did this get handled before? Is your use case serialization-only?
For latter case I guess serializing as empty JSON Object might make sense. Or alternatively treating as "ignored type" (not included as a property).

@gokhanoner
Copy link
Author

@cowtowncoder yes its serialized as object in <= 2.11

{"ta":{}}

I believe it's related to/side effect of the ticket I shared & fix applied, however, exception is wrong since I already enabled jsr310 module. It probably should be something different like no serializer defined for given type ..., or threat is as object & just serialize an empty object. Whatever the chosen fix, it should be documented better in jsr310 module change log for 2.12.

@gokhanoner
Copy link
Author

@cowtowncoder for my use-case, actually I just ignored that field, since it's not supposed to serialized anyway, but got when running tests & wanted to report since I couldn't find anything, other than the ticket I shared, mentioning this change.

@cowtowncoder
Copy link
Member

@gokhanoner Yes, this is a tricky one since the change itself is in jackson-databind, but its effects overlap with those of this module. This because Java 8 date/time module did nothing in the past and it was databind's default POJO serialization that handled the case. I agree that from the user perspective this falls under this module's domain and 2.12 release notes should mention something.

The underlying challenge is also the open-ended set of types under Java 8 date/time module: it is likely there are others that could exhibit similar problem. The problem databind change solves is a very common FAQ style problem for users -- without blocking these types, and in absence of module, all types are serialized POJO-style, and none of them can be read back. Users are then confused because behavior is very different between module registered and not; as such, it seemed better to try to detect this case. Handling is heuristic, of course, since databind has no knowledge of what this module might do, when it is not actually registered...

But now that I said that, I wonder if handling wrt databind/2683 could try to determine if module actually is registered, and avoid throwing exception. Theoretically ObjectMapper does keep track of module registrations and makes ids accessible via getRegisteredModuleIds(). But I am not sure SerializerProvider has access to this information. Maybe I can look into this.

@cowtowncoder
Copy link
Member

I think I have an idea how to work around the issue here. Maybe the failing check should only apply to direct descendants of java.time but NOT to those under java.time.xxx. So verify package name to be "java.time" and nothing else.

@cowtowncoder cowtowncoder changed the title Serializaation error on TemporalAdjuster type Fail to serialize TemporalAdjuster type with 2.12 Mar 26, 2021
@cowtowncoder cowtowncoder added this to the 2.12.3 milestone Mar 26, 2021
@gokhanoner
Copy link
Author

@cowtowncoder is there a PR in databind module already?

@cowtowncoder
Copy link
Member

@gokhanoner yes. there is a commit for 2.12 branch. I also pushed 2.12.3-SNAPSHOT build to Sonatype OSS repo.

HyukjinKwon referenced this issue in apache/spark May 31, 2021
### What changes were proposed in this pull request?
This pr upgrade Jackson version to 2.12.3.
Jackson Release 2.12.3: [https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.12.3](https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.12.3)

### Why are the changes needed?
Upgrade to a new version to bring potential bug fixes like [https://github.com/FasterXML/jackson-modules-java8/issues/207](https://github.com/FasterXML/jackson-modules-java8/issues/207)  and avro's master has been upgraded to Jackson to 2.12.3

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Pass the Jenkins or GitHub Action

Closes #32688 from LuciferYang/SPARK-35550.

Authored-by: yangjie01 <[email protected]>
Signed-off-by: Hyukjin Kwon <[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

2 participants