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

persistent-mysql: treat tinyint as SqlOther "tinyint", not SqlBool #1526

Merged
merged 3 commits into from
Dec 7, 2023

Conversation

fumieval
Copy link
Contributor

@fumieval fumieval commented Nov 28, 2023

I noticed that our application reports schema mismatch after upgrading to MySQL 8.0, even if I change the column type to SqlOther "tinyint" (removing the width). I realised that the recent versions of persistent-mysql parses tinyint as SqlBool, but it doesn't work in our usecase, where some tinyint columns represent non-Boolean values.

After this change, I confirmed that our application works both on MySQL 5.7 and MySQL 8.0, but I'm not quite confident that this change is in the right direction. What are your thoughts?


Before submitting your PR, check that you've:

  • Documented new APIs with Haddock markup
  • Added @since declarations to the Haddock
  • Ran stylish-haskell on any changed files.
  • Adhered to the code style (see the .editorconfig file for details)

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Bumped the version number if there isn't an (unreleased) on the Changelog
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

@paul-rouse
Copy link
Contributor

This looks reasonable to me. It relies on the exception documented in the MySQL 8.0.19 release notes concerning the deprecation of the width specification for integer data types - it does appear that there is an intention to retain tinyint(1) as a special case.

There are a couple of things you could do to tidy up the code:

  • the comment on parseColumnType needs to be updated to refer to this exception to the general deprecation of integer width specification, and the comment in showSqlType specifically about the Bool case can be removed.
  • I can't see any reason for the second condition (the one with stripPrefix), since removing the tinyint alternative from the condition will make anything other than tinyint(1) fall through to the catch-all case of parseColumnType at the bottom. Doesn't that do what you want?

@fumieval
Copy link
Contributor Author

Thank you for a comment. Given that they intend to keep tinyint(1) as an exception, this seems much safer than I originally anticipated. I reflected your suggestions in the code

@fumieval fumieval changed the title RFC: persistent-mysql: treat tinyint as SqlOther "tinyint", not SqlBool persistent-mysql: treat tinyint as SqlOther "tinyint", not SqlBool Nov 28, 2023
@paul-rouse
Copy link
Contributor

@parsonsmatt can you see any issues with this? Just to make sure we don't break anything!

@parsonsmatt
Copy link
Collaborator

Code looks fine to me. I'm curious if you could write a test to demonstrate the behavior you're looking for?

@fumieval
Copy link
Contributor Author

I added a unit test that tests this behaviour. Would that suffice?

@@ -74,6 +74,7 @@ import qualified Data.Text.IO as T
import GHC.Stack
import System.Environment (getEnvironment)

import Database.Persist.MySQL.Internal
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this file wasn't checked in

@parsonsmatt
Copy link
Collaborator

I'd prefer to see that persistent has the right behavior when interacting with a sql=tinyint column. Testing the parsing logic seems less valuable than that the actual interaction with the database layer works appropriately.

Copy link
Collaborator

@parsonsmatt parsonsmatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic, thanks! I'll go ahead and release this.

@parsonsmatt parsonsmatt merged commit 3744ffb into yesodweb:master Dec 7, 2023
8 checks passed
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

Successfully merging this pull request may close these issues.

3 participants