agrestio / agrest

Server-side Java REST Framework for easy access to data graphs from various backends
https://agrest.io
Apache License 2.0
81 stars 34 forks source link

Problem with timestamp formatting #621

Closed kbareja closed 1 year ago

kbareja commented 1 year ago

I’ve bumped into some issues related to AgRest time formatting. That time is too far away from unix epoch, time formatting becomes strange (e.g. adds random time to results). For usual dates, like full 20th century seems to work fine, Problem starts when it comes deeper into past:

I’ve created tests to visualise it (note that it might return different results in different local timezone. I use CET ):

    @Test
    void isoFormatterTest() throws IOException {
        final var jsonGenerator = Mockito.mock(JsonGenerator.class);
        final var encoder = ISODateTimeEncoder.encoder();

        encoder.encode("test", Timestamp.from(Instant.ofEpochSecond(0)), jsonGenerator);
        Mockito.verify(jsonGenerator).writeObject("1970-01-01T01:00:00"); // works fine. 
        Mockito.reset(jsonGenerator);

        encoder.encode("test", new Timestamp(-85 /* the year minus 1900 */, 2, 2, 0, 0, 1, 0), jsonGenerator);
        Mockito.verify(jsonGenerator).writeObject("1815-02-02T01:00:01"); // Assertion is failing. Real invocation is for "1815-03-02T00:24:01"
    }

Problem most likely is in io.agrest.encoder.ISODateTimeEncoder line:

DateTimeFormatters.isoLocalDateTime().format(Instant.ofEpochMilli(date.getTime()))

I blame this conversion via ofEpochMilli but honestly didn’t test it too deep. (we still have v 4.10)

andrus commented 1 year ago

Yeah, the use of custom DateTimeFormatter was the culprit. I switched the entire framework to the standard ISO formatters from the JDK. The error no longer occurs. Since this is a somewhat invasive change, it is 5.0 only.

My general recommendation would be to avoid java.sql.Date / java.sql.Time / java.sql.Timestamp, and use java.time.Local* flavor