Skip to content

Fix qmarks#915

Merged
parsonsmatt merged 1 commit intoyesodweb:matt/fix-qmarksfrom
kderme:fix-filter
Jul 17, 2019
Merged

Fix qmarks#915
parsonsmatt merged 1 commit intoyesodweb:matt/fix-qmarksfrom
kderme:fix-filter

Conversation

@kderme
Copy link
Contributor

@kderme kderme commented Jun 2, 2019

This pr fixes a small bug, which appears when using the Filter data type. The test added fails with

  src/PersistentTest.hs:87:3: 
  1) persistent Filter In
       uncaught exception: SqliteException
       SQLite3 returned ErrorError while attempting to perform prepare "SELECT \"id\", \"name\", \"age\", \"color\" FROM \"Person\" WHERE (\"name\" IN ?)": near "?": syntax error

The issue is that ? should be wrapped in ( ) or sqlite can't parse it.

There seems to be more issues related to the Filter data type. For example someone could write Filter PersonName (FilterValues ["a", "b"]) Eq which will also fail. An additional fix could be:

  • Hide Filter from users so that it is a Opaque type. Users will have to use the combinators from Database.Persist
  • make constuctors safer, so that ie Eq can only be combined with FilterValue and not FilterValues.

Before submitting your PR, check that you've:

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

@kderme kderme changed the title fix qmarks Fix qmarks Jun 2, 2019
@parsonsmatt
Copy link
Collaborator

Thanks for the PR! I'm going to merge to a new branch to get CI to run, and if that's green, then I'll get this released tonight.

@parsonsmatt parsonsmatt changed the base branch from master to matt/fix-qmarks July 17, 2019 00:21
@parsonsmatt parsonsmatt merged commit 4073f9e into yesodweb:matt/fix-qmarks Jul 17, 2019
@parsonsmatt parsonsmatt mentioned this pull request Jul 17, 2019
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.

2 participants