beam

beam copied to clipboard

When I try to store and retrieve a UTCTime representing the year 1, I get a decoding error.

time = UTCTime (fromGregorian 1 0 0) (secondsToDiffTime 0)

The error is BeamRowReadError {brreColumn = Just 5, brreError = ColumnTypeMismatch {ctmHaskellType = "UTCTime", ctmSQLType = "TEXT", ctmMessage = "conversion failed: couldn't parse UTCTime field: Failed reading: year must consist of at least 4 digits"}}.

I think this is because the time is getting encoded like this: 1-01-01T00:00:00.

I've added a failing test to beam-sqlite, I'll open a separate PR with it.

beam-sqlite re-uses the parsers from sqlite-simple. My suggestion for this particular use case, since this is ultimately a non-standard format for dates, is to define

newtype MyTime = MyTime UTCTime
  deriving (Generic)
deriving newtype instance HasSqlValueSyntax syntax UTCTime => HasSqlValueSyntax syntax MyTime
instance FromBackendRow be Text => FromBackendRow be MyTime where
   fromBackendRow = mySpecialFunctionToParseTimes =<< fromBackendRow
mySpecialFunctionToParseTimes :: MonadFail m => Text -> m MyTime

As it is, I don't want to special-case parsing in Beam over whatever sqlite-simple does. IMO, this follows the principle of least surprise for users coming to beam from these libraries. Especially with dates, 01 could mean 2001 or 1901.

beam-sqlite re-uses the parsers from sqlite-simple

I just dug into this a little more, I think the issue is that the encoding part is done by beam-sqlite in a weird way:

https://github.com/haskell-beam/beam/blob/master/beam-sqlite/Database/Beam/Sqlite/Syntax.hs#L951-L953

In the HasSqlValueSyntax instance, beam-sqlite is using its own date format string rather that sqlite-simple's ToField instance.

I updated my PR to use the toField function and it fixes the failing test: https://github.com/haskell-beam/beam/pull/554/commits/877a1b51846609b9b7a4509a7a55ba2b2ae069ce. What do you think? I think a couple other types like Day would also benefit from the same change.

@tathougies Was there some reason originally not to use the sqlite-simple serializations? Wondering if there's some implication I'm missing, otherwise I think we should change this.

Hey @tathougies or @kmicklas, could we merge my fix for this? I'd love to stop maintaining my own fork of Beam that I have to use due to this. AFAIK it's a straightforward bug and the PR (#554) fixes it nicely.

@kmicklas I recall at some point I switched from the sqlite-simple serializations, because there were some cases it did not cover. As for PR 554... I can merge it, unless there are any objections.

FWIW I dug through git history and I don't see it ever being switched -- it seems it was added here.

I'd be interested in what cases sqlite-simple doesn't cover -- it looks to me like the code over there is more careful to be compliant with SQLite's requirements. For example, it is careful to pad the year to 4 digits which the current code doesn't do and which causes this issue.

For the same reason, I think beam should really make the same change for the Day type, which sqlite-simple also handles more carefully. I think LocalTime has the same problem too, although sqlite-simple doesn't have an instance for that type yet.

@kmicklas could you chime in? This seems like a clear win and I've love to get it merged :)

I think we can merge this PR. Maybe Ken has thoughts.

Ken already indicated support above in June 2021. Let's merge!

I just rebased on master and the tests pass, so it should be ready to go.

Thanks for the contribution @thomasjm!