PersistMarshalError for UTCTime when upgrading from lts-14.27 to lts-15.15

@parsonsmatt

I upgraded from lts 14.27 to 15.15 in this commit:
NorfairKing/intray@b3f5fc6

Reads now fail with errors like this:

PersistMarshalError "Couldn't parse field `createdTimestamp` from table `user`. Failed to parse Haskell type `UTCTime`, but received PersistText \"2018-02-27 10:49:42.123\""

This is the relevant column: https://github.com/NorfairKing/intray/blob/master/intray-data/src/Intray/Data/DB.hs#L39

This looks like a regression of #339
and #989 looks relevant.
It looks like the test suite was changed instead of fixing the code.

CC @parsonsmatt who was involved in this.

Hmmm! That's no good. We should not be relying on the Read instance changing, which is the source of the test suite failures. I think the issue here is not coming at the persistent-postgresql layer, but instead from this instance:

instance PersistField UTCTime where
    toPersistValue = PersistUTCTime
    fromPersistValue (PersistUTCTime d) = Right d
#ifdef HIGH_PRECISION_DATE
    fromPersistValue (PersistInt64 i)   = Right $ posixSecondsToUTCTime $ (/ (1000 * 1000 * 1000)) $ fromIntegral $ i
#endif
    fromPersistValue x@(PersistText t)  =
        case reads $ T.unpack t of
            (d, _):_ -> Right d
            _ ->
                case parse8601 $ T.unpack t of
                    Nothing -> Left $ fromPersistValueParseError "UTCTime" x
                    Just x' -> Right x'
      where
#if MIN_VERSION_time(1,5,0)
        parse8601 = parseTimeM True defaultTimeLocale "%FT%T%Q"
#else
        parse8601 = parseTime defaultTimeLocale "%FT%T%Q"
#endif
    fromPersistValue x@(PersistByteString s) =
        case reads $ unpack s of
            (d, _):_ -> Right d
            _ -> Left $ fromPersistValueParseError "UTCTime" x

    fromPersistValue x = Left $ fromPersistValueError "UTCTime" "time, integer, string, or bytestring" x

The PersistText is the one that is triggering the error.

Stackage 15.15 has time-1.9.3, which means we're hitting the parseTimeM True defaultTimeLocal "%F%T%Q". Stackage 14.27 also has time-1.8.0.2, so we're hitting the same code path.

This means we'll need to compare persistent versions. persistent-2.9.2 is on 14.27 while persistent-2.10.5.2 is on 15.15.

Alas! For the code is the same. persistent-2.9.2 vs persistent-2.10.5.2.

So what's goin' on!!

well, the Read instance changing is definitely happening at the time-1.9 release:

λ ~/ stack ghci --resolver lts-15.15
λ> import Data.Time
λ> read "2018-02-27 10:49:42.123" :: UTCTime
*** Exception: Prelude.read: no parse
λ> 
Leaving GHCi.

λ ~/ stack ghci --resolver lts-14.27
λ> import Data.Time
λ> read "2018-02-27 10:49:42.123" :: UTCTime
2018-02-27 10:49:42.123 UTC

But that's not the only path. We also try the custom parseTimeM path.

-- lts-14.27
λ> parseTimeM True defaultTimeLocale "%FT%T%Q" "2018-02-27 10:49:42.123" :: Maybe UTCTime
Nothing

-- lts-15.15
λ> parseTimeM True defaultTimeLocale "%FT%T%Q" "2018-02-27 10:49:42.123" :: Maybe UTCTime
Nothing

wat

Oh! I get it. So in 14.27 we're hitting the successful read and then we don't bother trying to parse this format string.

So what changes?

λ> parseTimeM True defaultTimeLocale "%FT%T%Q" "2018-02-27T10:49:42.123" :: Maybe UTCTime
Just 2018-02-27 10:49:42.123 UTC

The format string has a T that is not present in this input text. Which is weird. It seems that you are using persistent-sqlite, so let's look at how SQLite formats a timestamp.

sqlite> select CURRENT_TIMESTAMP;
2020-08-27 17:18:22

Meanwhile, in psql we get:

matt=# select CURRENT_TIMESTAMP;
       current_timestamp       
-------------------------------
 2020-08-27 11:18:48.321483-06
(1 row)

mysql?

mysql> select CURRENT_TIMESTAMP;
+---------------------+
| CURRENT_TIMESTAMP   |
+---------------------+
| 2020-08-27 11:21:18 |
+---------------------+
1 row in set (0.00 sec)

So this should fail here too. So what informs that %FT%T%Q format string?

So this should fail here too. So what informs that %FT%T%Q format st

No clue ...

Related to #989 , are you sure it's not about the Z at the end?

Yeah, the Read instance was changed to require the time zone (and does not support offsets, sigh). But no SQL databases send that along. So we need the custom format timestamp. But that format appears to be wrong, at least for the databases I'm looking at.

Ah, I think I get it.

Postgres and MySQL both use a specific datatype, not the text format, so we don't go through Read at all. But SQLite seems to assume that the format has the T in it. According to the SQLite docs, the T is optional. persistent-sqlite writes with a T in the format (to conform with the ISO8601) standard. Was this changed?

Nope, identical code.

I think an easy fix might be to accept either the T or not, so another branch on the case there.

Oh interesting! I'm happy to see it's a real bug and not something I did wrong (again)

@parsonsmatt

@parsonsmatt Is there anything I need to do to push this forward?

@parsonsmatt

@parsonsmatt Gentle ping

I laid out the general implementation strategy, so it just needs some developer time. I might be able to get to it soon but my plate is pretty full at the moment.

@parsonsmatt

@parsonsmatt Is this small enough for hacktoberfest? Maybe it would be good to add that label then.

@gregwebs

Gentle ping about this, it's a blocker for upgrading.
Pinging an other listed maintainer: @gregwebs

@parsonsmatt If you point me at the right file and/or a failing test, maybe I can have a look because it seems like a quick fix.

The two comments I posted on August 27th outline the issue and suggest a fix. The instance PersistField UTCTime needs to be modified.

A fix for yesodweb#1117, I think

A fix for #1117, I think (#1135)

@parsonsmatt

@parsonsmatt Any chance we can release? yesod-persistent also needs a bump to the upper bound for persistent then, I think.

I'm working on getting 2.11 ready to go. If you want to chery-pick these commits to the 2.10 commit then we can get a new version released there, too.

@parsonsmatt

@parsonsmatt I made d455d26 at https://github.com/NorfairKing/persistent/tree/persistent-2.10. There was nothing to merge, only the changelog to not edit.

Thanks! Released as 2.10.5.3

@parsonsmatt

@parsonsmatt I think there may be more issues:

PersistMarshalError "Couldn't parse field `created` from table `intray_item`. Failed to parse Haskell type `UTCTime`, but received PersistText \"-3386841531770605-07-07T20:31:01\""

I'll see if this has something to do with this line in the time changelog:

parsing: fix parsing of BCE years

EDIT: jup that was it.

Navigation Menu

Repository navigation

PersistMarshalError for UTCTime when upgrading from lts-14.27 to lts-15.15 #1117

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

PersistMarshalError for UTCTime when upgrading from lts-14.27 to lts-15.15 #1117

Description

Activity

parsonsmatt commented on Aug 27, 2020

parsonsmatt commented on Aug 27, 2020

NorfairKing commented on Aug 27, 2020

parsonsmatt commented on Aug 27, 2020

NorfairKing commented on Aug 27, 2020

NorfairKing commented on Sep 6, 2020

NorfairKing commented on Sep 18, 2020

parsonsmatt commented on Sep 18, 2020

NorfairKing commented on Sep 28, 2020

NorfairKing commented on Oct 21, 2020

parsonsmatt commented on Oct 21, 2020

NorfairKing commented on Nov 1, 2020

parsonsmatt commented on Nov 1, 2020

NorfairKing commented on Nov 2, 2020

parsonsmatt commented on Nov 2, 2020

NorfairKing commented on Nov 3, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions