search

paul-rouse / mysql-simple

A mid-level client library for the MySQL database, intended to be fast and easy to use.
Other
91 stars 35 forks source link

UTCTime decoding for `0000-00-00` datetimes fails #35

Closed parsonsmatt closed 7 years ago

parsonsmatt commented 7 years ago

On LTS-6.30, there were no issues. LTS-8.2 is now giving me this:

ConversionFailed {errSQLType = "DateTime", errHaskellType = "UTCTime", errFieldName = "deliveryLastChecked", errMessage = "could not parse"}

I traced it to this instance

The database value for that is 0000-00-00 00:00:00 (thanks MySQL). A bit of digging shows that LTS-6.30 has time-1.5.0.1 and that 8.2 has time-1.6.0.1. The changelog includes:

Parsing functions now reject invalid dates

which is probably the problem.

How would you like to approach this? MySQL technically always allows for these invalid date values, so in a sense any time-valued sort of thing in MySQL is more appropriately represented as Maybe UTCTime (or similar). Previously, I believe that it would parse it into something like fromGregorian 0 0 0, which would be 0000-01-01.

An easy/non-breaking change is, on Nothing cases, check if it starts with 0000-00-00, and if so, return that. I can put together a PR for that if that's the direction you want to go.

paul-rouse commented 7 years ago

Thank you. I think the non-breaking approach is best, so I've just merged your PR #36

parsonsmatt commented 7 years ago

I think the version bounds should be updated so that mysql-simple-0.4.0.0 depends on time < 1.6 also. A 0.4.0.1 patch release with this change could omit the restriction. Would that be an acceptable solution?

paul-rouse commented 7 years ago

What you suggest would break the guarantee that stackage gives us that the packages in any existing snapshot build against each other. This is "only" a runtime bug, so I think I should just stick to the patch release.

I will do that soon - other things got in the way these last few days!

parsonsmatt commented 7 years ago

I'm interested to learn more about your reasoning there -- my understanding of Stackage snapshots is more that the packages are all compatible with each other, which is a stronger requirement than compilable together. Do you know if Stackage has previous discussions on discovering package incompatibilities after the fact?

I'm in no rush thanks to stack's easy Github dependencies :smile: Thanks for reviewing and accepting the change so quickly!

paul-rouse commented 7 years ago

Well, yes, Stackage does also impose a requirement that the test suites pass as well. I have not seen any stronger claim anywhere that the packages should work together flawlessly in any sense - indeed Michael Snoyman's original goals (http://www.yesodweb.com/blog/2012/11/stable-vetted-hackage) clearly positioned Stackage below the Haskell Platform in that regard. Of course quite a lot has changed since those original goals (http://www.snoyman.com/blog/2017/01/stackage-design-choices), but I don't think anything has altered the fundamental assertion that the packages in a snapshot build together, and continue to do so. In fact I had forgotten that Stackage now works around Hackage revisions by tracking the cabal files at the time of the snapshot.

parsonsmatt commented 7 years ago

That's good to know, thanks. I appreciate the rundown :)