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

Problem with timestamp formatting #621

Closed
kbareja opened this issue Mar 17, 2023 · 1 comment
Closed

Problem with timestamp formatting #621

kbareja opened this issue Mar 17, 2023 · 1 comment
Milestone

Comments

@kbareja
Copy link

kbareja commented Mar 17, 2023

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 added a commit that referenced this issue Mar 19, 2023
andrus added a commit that referenced this issue Mar 19, 2023
attempt to get rid of custom DateTimeFormatters
andrus added a commit that referenced this issue Mar 19, 2023
release, upgrade notes
@andrus
Copy link
Contributor

andrus commented Mar 19, 2023

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

@andrus andrus closed this as completed Mar 19, 2023
@andrus andrus added this to the 5.0.M16 milestone Mar 19, 2023
andrus added a commit that referenced this issue Mar 19, 2023
release, upgrade notes
andrus added a commit that referenced this issue Mar 19, 2023
release, upgrade notes
andrus added a commit that referenced this issue Mar 19, 2023
release, upgrade notes
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