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

Add JavaTimeFeature.ALWAYS_ALLOW_STRINGIFIED_TIMESTAMPS to allow parsing quoted numbers when using a custom DateTimeFormatter #263

Closed
mpkorstanje opened this issue Jan 9, 2023 · 6 comments · Fixed by #269
Labels
pr-welcome Issue for which progress most likely if someone submits a Pull Request

Comments

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Jan 9, 2023

I have a scenario where I would like to de-serialize epoch milis, and serialize with a @JsonFormat annotation. In essence:

  final ObjectMapper mapper = new ObjectMapper()
      .disable(DeserializationFeature.READ_DATE_TIMESTAMPS_AS_NANOSECONDS)
      .disable(SerializationFeature.WRITE_DATE_TIMESTAMPS_AS_NANOSECONDS)
      .disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS)
      .setTimeZone(TimeZone.getTimeZone("UTC"))
      .findAndRegisterModules();
  final Instant instant = Instant.ofEpochMilli(123456);

  @Test
  public void test() throws JsonProcessingException {
    assertEquals("{\"time\":\"1970-01-01T00:02:03.456+0000\"}", mapper.writeValueAsString(new TimeHolderWithFormat(instant)));
    assertEquals(instant, mapper.readValue("{\"time\":123456}", TimeHolder.class).time);
    assertEquals(instant, mapper.readValue("{\"time\":\"123456\"}", TimeHolder.class).time);
    assertEquals(instant, mapper.readValue("{\"time\":123456}", TimeHolderWithFormat.class).time);
    assertEquals(instant, mapper.readValue("{\"time\":\"123456\"}", TimeHolderWithFormat.class).time);
  }

  
  static class TimeHolder {
    private Instant time;
    public void setTime(Instant time) {
      this.time = time;
    }
    // Getter ommited
  }

  static class TimeHolderWithFormat {
    private Instant time;
    // Constructors ommited
    @JsonFormat(pattern = "yyyy-MM-dd'T'HH:mm:ss.SSSZ")
    public void setTime(Instant time) {
      this.time = time;
    }
    // Getter ommited
  }

This will fail on the last assertion with:

com.fasterxml.jackson.databind.exc.InvalidFormatException: Cannot deserialize value of type `java.time.Instant` from String "123456": Failed to deserialize java.time.Instant: (java.time.format.DateTimeParseException) Text '123456' could not be parsed at index 0
 at [Source: (String)"{"time":"123456"}"; line: 1, column: 9] (through reference chain: com.example.app.AppTest$TimeHolderWithFormat["time"])

There is an apparent inconsistency in the way Jackson de-serializes numbers that are shaped as a string into an instant.

  • When not providing a custom date format, quoted numbers are treated as epoch milis/nanos
  • When not providing a custom date, quoted numbers are assumed to be handled by the pattern.

// only check for other parsing modes if we are using default formatter
if (_formatter == DateTimeFormatter.ISO_INSTANT ||
_formatter == DateTimeFormatter.ISO_OFFSET_DATE_TIME ||
_formatter == DateTimeFormatter.ISO_ZONED_DATE_TIME) {
// 22-Jan-2016, [datatype-jsr310#16]: Allow quoted numbers too
int dots = _countPeriods(string);
if (dots >= 0) { // negative if not simple number
try {
if (dots == 0) {
return _fromLong(ctxt, NumberInput.parseLong(string));
}

There is however no way to construct a pattern that handles both ISO dates and epoch milis/nanos. Would it be possible to add a feature toggle here?

Details:

@mpkorstanje mpkorstanje changed the title Add a feature toggle to parse qouted numbers as numbers when using a custom DateTimeFormatter Feature toggle to parse qouted numbers when using a custom DateTimeFormatter Jan 9, 2023
@cowtowncoder
Copy link
Member

On adding feature: PRs welcome.

@cowtowncoder cowtowncoder added the pr-welcome Issue for which progress most likely if someone submits a Pull Request label Mar 14, 2023
@mpkorstanje
Copy link
Contributor Author

Cheers! I'll try and rustle up something.

mpkorstanje added a commit to mpkorstanje/jackson-modules-java8 that referenced this issue Mar 15, 2023
There is an apparent inconsistency in the way Jackson de-serializes numbers
that are shaped as a string into an instant.

 * When not providing a custom date format, quoted numbers are treated as
   epoch milis/nanos
 * When not providing a custom date, quoted numbers are assumed to be handled
   by the pattern.

There is however no way to construct a pattern that handles both ISO dates
and epoch milis/nanos. This feature toggle allow numeric strings to be read
as numeric timestamps.

Fixes FasterXML#263
@cowtowncoder
Copy link
Member

Hmmh. But what is the need to use "quoted numbers" instead of plain numbers?

@mpkorstanje
Copy link
Contributor Author

mpkorstanje commented Mar 16, 2023

Hmmh. But what is the need to use "quoted numbers" instead of plain numbers?

Unfortunately the data I'm reading is using timestamps formatted as quoted numbers. The systems producing this data are rather old and peculiar. And I would agree that in principle they need not be this way. But this is the reality I'm working with.

Nevertheless, without a formatter (i.e. @JsonFormat annotation) it is possible to read these dates in the current implementation. It is currently possible to de-serialize "3.141592653" into a ZoneDateTime. However when adding the @JsonFormat annotation this is no-longer the case. This is the inconsistency I'm trying to address.

Unfortunately I can not access the issue that prompted the original addition of the "read qouted numbers as timestamps" feature (datatype-jsr310#16) but looking at FasterXML/jackson-datatype-jsr310@f85c708 that resolved it appears to be a similar use-case.

So I am not looking to enable a novel use for quoted numbers, rather I'm looking to make an already supported use-case consistent with other features of the library.

@cowtowncoder
Copy link
Member

Ok. That makes sense.

So, I am not against supporting such use cases; my main concern is with API. Unfortunately whereas there is now extensive configurability for "alternative Shapes" (accept String where Number is expected) is not quite what we have here, because Date/Time DOES accept String values (ISO-8610) but rather "String that is actually a Number. Because of that extra configurability is indeed needed.

But I am trying to limit use of DeserializationConfig for very specific/targeted uses: it should be for "bigger" features -- there are some legacy settings that are different, but for new things they should be general-purpose ones.

Given this, I think I would actually suggest adding configurability in JavaTimeModule; especially since this is not meant to affect JDK or Joda date/time types.

If this can be done, I'd be happy to get it merged.

I'll add similar note to jackson-databind issue.

@mpkorstanje
Copy link
Contributor Author

Cheers. I'll have a look at how to accomplish this. Might take a little while.

mpkorstanje added a commit to mpkorstanje/jackson-modules-java8 that referenced this issue Nov 12, 2023
There is an apparent inconsistency in the way Jackson de-serializes numbers
that are shaped as a string into an instant.

 * When not providing a custom date format, quoted numbers are treated as
   epoch milis/nanos
 * When not providing a custom date, quoted numbers are assumed to be handled
   by the pattern.

There is however no way to construct a pattern that handles both ISO dates
and epoch milis/nanos. This feature toggle allow numeric strings to be read
as numeric timestamps.

Fixes FasterXML#263
@cowtowncoder cowtowncoder changed the title Feature toggle to parse qouted numbers when using a custom DateTimeFormatter Add JavaTimeFeature.ALWAYS_ALLOW_STRINGIFIED_TIMESTAMPS to allow parsing quoted numbers when using a custom DateTimeFormatter Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-welcome Issue for which progress most likely if someone submits a Pull Request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants