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

DurationSerializer ignores format pattern if nano-second serialization enabled #224

Closed
Sam-Kruglov opened this issue Jul 18, 2021 · 5 comments
Assignees
Labels
json-format Missing handling of `@JsonFormat` property annotation

Comments

@Sam-Kruglov
Copy link

Sam-Kruglov commented Jul 18, 2021

@Override
protected DurationSerializer withFormat(DateTimeFormatter dtf,
Boolean useTimestamp, JsonFormat.Shape shape) {
return new DurationSerializer(this, dtf, useTimestamp);
}

If I do this, I expect an integer number in JSON representing the number minutes:

@JsonFormat(pattern = "MINUTES")
@JsonProperty("durationInMins")
Duration duration;

But it serializes it as nanoseconds. After some debugging, I decided to add , shape = NUMBER_INT in order to force com.fasterxml.jackson.datatype.jsr310.ser.JSR310FormattedSerializerBase#useNanoseconds respond false, but it has no effect because shape is not used in the factory method I linked above.

Found a workaround: adding , without = JsonFormat.Feature.WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS to @JsonFormat. However, it only works if I add it to each property, adding it on class level has no effect.

@Sam-Kruglov
Copy link
Author

Actually same happens to some other classes like Instant

@cowtowncoder
Copy link
Member

Yes, looks like DurationSerializer is only looking at pattern from @JsonFormat and not shape -- it definitely should.

One thing that would help would be to show a test case: description is helpful (and should be quite easy to create a test), but often the original use case is a very good test case on its own.
I hope someone can look into this as it seems like a potentially easy fix (I will have very little time for coding for next 3 weeks unfortunately) and would get in 2.13.0 final (theoretically 2.12 backport is possible too but 2.13.0 likely released before 2.12.5).

@cowtowncoder cowtowncoder added 2.13 json-format Missing handling of `@JsonFormat` property annotation labels Jul 19, 2021
@Sam-Kruglov Sam-Kruglov changed the title DurationSerializer ignores format pattern DurationSerializer ignores format shape Jul 19, 2021
@Sam-Kruglov Sam-Kruglov changed the title DurationSerializer ignores format shape DurationSerializer ignores format pattern Jul 19, 2021
@Sam-Kruglov
Copy link
Author

Sam-Kruglov commented Jul 19, 2021

This is my primary issue:

class MyDto {
  @JsonFormat(pattern = "MINUTES")
  @JsonProperty("durationInMins")
  private final Duration duration;

  public MyDto(Duration d) { duration = d; }

  public Duration getDuration() { return duration; }
}

var mapper = new ObjectMapper().findAndRegisterModules();
Map<String, Object> map = mapper.convertValue(new MyDto(Duration.ofHours(2)), new TypeReference<>(){});
assertEqual(map.get("durationInMins"), 120);

This will fail since the serialized value is the number of nanoseconds, so the format is totally ignored, even though the duration converter under the hood is correct, it's just not used as I explained earlier. Fixing the shape not being used would allow me to pass the test case by adding , shape = NUMBER_INT to the format, which is fine but it'd be better if that wouldn't be required.

P.S. wrote the test case by hand

cowtowncoder added a commit that referenced this issue Sep 26, 2021
@cowtowncoder
Copy link
Member

I added a modified, failing test to reproduce the issue. Unfortunately I don't have time to pursue this further at this point, but I hope someone else can -- I will make time to review PRs and so on, just not actively write a patch myself.

@kupci Not sure what you think -- might be able to add in a 2.13.x patch although I am also worried that a change might be needed in construction/contextualization.

@kupci kupci self-assigned this Sep 27, 2021
@kupci kupci removed the 2.13 label Oct 7, 2021
@cowtowncoder cowtowncoder changed the title DurationSerializer ignores format pattern DurationSerializer ignores format pattern if nano-second serialization enabled Aug 3, 2022
@cowtowncoder
Copy link
Member

@Sam-Kruglov FINALLY found time to get back to this and hope I fixed it correctly -- trivially simple change but I'm slightly worried about regression. Still, as things are tests pass and it'll go in 2.14.0 unless something is found.

But I was wondering wrt Instant you mentioned has similar issues: would it be possible to show similar unit test? If so, could you please file a separate new issue for type(s) and I should be able to see if I could do more fixes before 2.14.0 release (planning to start release candidates soon).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
json-format Missing handling of `@JsonFormat` property annotation
Projects
None yet
Development

No branches or pull requests

3 participants