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

NPE when serializing a LocalDate or LocalDateTime using AsDeductionTypeSerializer #296

Closed
1 task done
mike-reynolds-savient opened this issue Dec 13, 2023 · 9 comments
Closed
1 task done
Milestone

Comments

@mike-reynolds-savient
Copy link

Search before asking

  • I searched in the issues and found nothing similar.

Describe the bug

In v2.14.2 AsDeductionTypeSerializer was introduced.
Prior to this we could define an immutable interface as follows:

    @Nullable
    @JsonTypeInfo(use = JsonTypeInfo.Id.DEDUCTION)
    /* Complex/ nested classes should be added to this list for Jackson to check */
    @JsonSubTypes({
            @JsonSubTypes.Type(StoreField.class),
            @JsonSubTypes.Type(StringArray.class),
            @JsonSubTypes.Type(NumberArray.class),
            @JsonSubTypes.Type(QueryRequest.class) })
    T getValue();

and set a value of LocalDate, LocalDateTime, OffsetDateTime etc as the return of getValue()

This would serialize correctly with JsonMapper.builder().disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS) set.

We now get a "Cannot read field 'valueShape'" exception because AsDeductionTypeSerializer returns null if the WritableTypeId.valueShape is not a structure start token.
This results in an NPE within the LocalDateTimeSerializer class on line 89;

 if (typeIdDef.valueShape == JsonToken.START_ARRAY) {

Version Information

2.14.2

Reproduction

@Value.Immutable
@JsonSerialize(as = ImmutableQueryPredicate.class)
@JsonDeserialize(as = ImmutableQueryPredicate.class)
public interface QueryPredicate<T> extends Serializable {

    @Nullable
    @JsonTypeInfo(use = JsonTypeInfo.Id.DEDUCTION)
    /* Complex/ nested classes should be added to this list for Jackson to check */
    @JsonSubTypes({
            @JsonSubTypes.Type(StoreField.class),
            @JsonSubTypes.Type(StringArray.class),
            @JsonSubTypes.Type(NumberArray.class),
            @JsonSubTypes.Type(QueryRequest.class) })
    T getValue();

}
    @Test
    void testSerialiseLocalDatePredicate() {

        final QueryPredicate<LocalDateTime> dateTimePredicate = ImmutableQueryPredicate.<LocalDateTime>builder()
                .value(LocalDateTime.of(2022, 06, 24, 10, 23, 51))
                .build();

        final String asString = mapper.writeValueAsString(dateTimePredicate);
}

Expected behavior

If deduction cannot find the class it should return default property behaviour.

Additional context

FasterXML/jackson-databind#3711 (comment)

@pjfanning
Copy link
Member

pjfanning commented Dec 13, 2023

@mike-reynolds-savient
Copy link
Author

Yep, that's the one. Note that this issue also effects com.fasterxml.jackson.datatype.jsr310.ser.LocalDateSerializer

@pjfanning
Copy link
Member

@mike-reynolds-savient you repeated back the name of the class that I sent - did you mean to name a different class?

@mike-reynolds-savient
Copy link
Author

Apologies - LocalDateTimeSerializer

@cowtowncoder
Copy link
Member

Version 2.14.2 is not the latest so would be good repro with 2.16.0.

But also if this requires use of Java 8 Date/Time types, need to move to different repo; jackson-databind tests cannot depend on Java 8 date/time module (cyclic dependency). Will move the isse.

@cowtowncoder cowtowncoder transferred this issue from FasterXML/jackson-databind Dec 13, 2023
@cowtowncoder
Copy link
Member

Ok, so, couple of things now that I re-read this again.

First: I can definitely avoid NPE for LocalDate[Time]Serializer, and that probably makes sense.

But second, I think that DEDUCTION based type deserializer can only work JSON Object represenations, not String-valued ones. So it can work to figure out type of POJOs, but not LocalDate or LocalDateTime. So ultimately I am not sure this can be made to work, if I understand use case correctly (I might misunderstand it tho).

cowtowncoder added a commit that referenced this issue Jan 3, 2024
@cowtowncoder
Copy link
Member

Was able to reproduce (must force serialization as String), partial fix via #298, will try to cover all types for which NPE would be produced.

@cowtowncoder cowtowncoder changed the title NPE when serializing a LocalDate or LocalDateTime using AsDeductionTypeSerializer NPE when serializing a LocalDate or LocalDateTime using AsDeductionTypeSerializer Jan 3, 2024
cowtowncoder added a commit that referenced this issue Jan 3, 2024
@cowtowncoder cowtowncoder added this to the 2.16.2 milestone Jan 3, 2024
@cowtowncoder
Copy link
Member

@mike-reynolds-savient I fixed the NPE part for 2.16(.2) -- although it will be a month or two until that release. If you can try 2.16.2-SNAPSHOT to see how things work that'd be good. I am not confident DEDUCTION will work in general, but at least it should not fail with NPE no matter what.

@mike-reynolds-savient
Copy link
Author

Many thanks @cowtowncoder
When 2.16.2 or 2.17 become available for download I'll give it a go again. (I can't get to the -SNAPSHOT)

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

3 participants