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

Implement rawSql as a QuasiQuoter #717

Merged
merged 16 commits into from Nov 10, 2017

Conversation

Projects
None yet
3 participants
@felixSchl
Contributor

felixSchl commented Oct 12, 2017

I'm having a go at #716, now you can do:

let lft = 10 :: Int
     rgt = 20 :: Int
     width = 50 :: Int
[executeQQ|
    DELETE FROM category WHERE lft BETWEEN #{lft} AND #{rgt};
    UPDATE category SET rgt = rgt - #{width} WHERE rgt > #{rgt};
    UPDATE category SET lft = lft - #{width} WHERE lft > #{rgt};
|]

Would be great if you could help me figure out how to pass a function into makeExpr so that I don't have to duplicate the whole block of code:

https://github.com/felixSchl/persistent/blob/7856bd3878aadb9a703360d76934a7b7e78c66a4/persistent/Database/Persist/Sql/Raw/QQ.hs#L37-L49

I've also updated the test bed to include a few basic tests for this:

https://github.com/felixSchl/persistent/blob/7856bd3878aadb9a703360d76934a7b7e78c66a4/persistent-test/src/PersistentTest.hs#L891-L913

@cdepillabout

This comment has been minimized.

Show comment
Hide comment
@cdepillabout

cdepillabout Oct 17, 2017

Contributor

@felixSchl Something like this should work:

makeExpr :: TH.ExpQ -> [StringPart] -> TH.ExpQ
makeExpr f s = TH.appE [| uncurry $(f) . first pack |] (go s)
    where
    go [] = [| (mempty, []) |]
    go (Literal a:xs)   = TH.appE [| first (a ++) |] (go xs)
    go (AntiQuote a:xs) = TH.appE [| first ("?" ++) . second (toPersistValue $(reify a) :) |] (go xs)

sqlQQ :: QuasiQuoter
sqlQQ = QuasiQuoter
    (makeExpr [| Raw.rawSql |] . parseStr [] . filter (/= '\r'))
    (error "Cannot use qc as a pattern")
    (error "Cannot use qc as a type")
    (error "Cannot use qc as a dec")

executeQQ :: QuasiQuoter
executeQQ = QuasiQuoter
    (makeExpr [| Raw.rawExecute |] . parseStr [] . filter (/= '\r'))
    (error "Cannot use qc as a pattern")
    (error "Cannot use qc as a type")
    (error "Cannot use qc as a dec")

The problem is that you effectively want to pass a template haskell Name to makeExpr, instead of a regular function. The code is arguably the cleanest if you package this Name up in an Q Exp within executeQQ and sqlQQ (instead of just passing a raw Name). makeExpr can easily make use of this Q Exp representing the function you want to call.

Also, it would be really nice to have some documentation for the sqlQQ and executeQQ functions. As an end-user, it's hard to use quasiquoters (and template-haskell stuff in general) if you don't know the types of the code being generated. That is to say, the haddocks should explicitly state that rawSql produces a value of type (RawSql a, MonadIO m) => ReaderT SqlBackend m [a] (I think?).

Also a small example in the haddocks would probably help.

Contributor

cdepillabout commented Oct 17, 2017

@felixSchl Something like this should work:

makeExpr :: TH.ExpQ -> [StringPart] -> TH.ExpQ
makeExpr f s = TH.appE [| uncurry $(f) . first pack |] (go s)
    where
    go [] = [| (mempty, []) |]
    go (Literal a:xs)   = TH.appE [| first (a ++) |] (go xs)
    go (AntiQuote a:xs) = TH.appE [| first ("?" ++) . second (toPersistValue $(reify a) :) |] (go xs)

sqlQQ :: QuasiQuoter
sqlQQ = QuasiQuoter
    (makeExpr [| Raw.rawSql |] . parseStr [] . filter (/= '\r'))
    (error "Cannot use qc as a pattern")
    (error "Cannot use qc as a type")
    (error "Cannot use qc as a dec")

executeQQ :: QuasiQuoter
executeQQ = QuasiQuoter
    (makeExpr [| Raw.rawExecute |] . parseStr [] . filter (/= '\r'))
    (error "Cannot use qc as a pattern")
    (error "Cannot use qc as a type")
    (error "Cannot use qc as a dec")

The problem is that you effectively want to pass a template haskell Name to makeExpr, instead of a regular function. The code is arguably the cleanest if you package this Name up in an Q Exp within executeQQ and sqlQQ (instead of just passing a raw Name). makeExpr can easily make use of this Q Exp representing the function you want to call.

Also, it would be really nice to have some documentation for the sqlQQ and executeQQ functions. As an end-user, it's hard to use quasiquoters (and template-haskell stuff in general) if you don't know the types of the code being generated. That is to say, the haddocks should explicitly state that rawSql produces a value of type (RawSql a, MonadIO m) => ReaderT SqlBackend m [a] (I think?).

Also a small example in the haddocks would probably help.

@felixSchl

This comment has been minimized.

Show comment
Hide comment
@felixSchl

felixSchl Oct 17, 2017

Contributor

Thanks @cdepillabout I managed to implement your advice, but I have doubts about the readability of this

https://github.com/felixSchl/persistent/blob/79804b88bc7f19c424cb57a06b6e1b085f0f7c9d/persistent/Database/Persist/Sql/Raw/QQ.hs#L37-L42

Still better than duplicate code I guess.

The transform is really, really simple. It just substitutes all #{foo} with question marks and collects them into a list. For simplicity it also applies toPersistValue on each value, but that could be removed. It then simply applies rawSql or rawExecute to the substituted string and collected values

Contributor

felixSchl commented Oct 17, 2017

Thanks @cdepillabout I managed to implement your advice, but I have doubts about the readability of this

https://github.com/felixSchl/persistent/blob/79804b88bc7f19c424cb57a06b6e1b085f0f7c9d/persistent/Database/Persist/Sql/Raw/QQ.hs#L37-L42

Still better than duplicate code I guess.

The transform is really, really simple. It just substitutes all #{foo} with question marks and collects them into a list. For simplicity it also applies toPersistValue on each value, but that could be removed. It then simply applies rawSql or rawExecute to the substituted string and collected values

@felixSchl

This comment has been minimized.

Show comment
Hide comment
@felixSchl

felixSchl Oct 17, 2017

Contributor

@cdepillabout This is my first feature contribution to a Haskell project. Would you be able to advise me how / where to put that documentation?

Contributor

felixSchl commented Oct 17, 2017

@cdepillabout This is my first feature contribution to a Haskell project. Would you be able to advise me how / where to put that documentation?

Show outdated Hide outdated persistent/Database/Persist/Sql/Raw/QQ.hs Outdated
@cdepillabout

This comment has been minimized.

Show comment
Hide comment
@cdepillabout

cdepillabout Oct 18, 2017

Contributor

@felixSchl

The transform is really, really simple. It just substitutes all #{foo} with question marks and collects them into a list. For simplicity it also applies toPersistValue on each value, but that could be removed. It then simply applies rawSql or rawExecute to the substituted string and collected values

Yeah, from looking at your example in the first comment, it does look like a very simple transformation. I was trying to say that it would be nice to have haddocks showing the type of the value produced by sqlQQ. You'll have to use the haddock syntax to write this.

Here's an example of what I'm thinking about (using your explanation):

-- | This is a quasiquoter for creating raw SQL expressions.
--
-- It substitutes all @#{foo}@ with question marks and collects them into a list. It applies
-- 'toPersistValue' on each value. It then simply applies 'rawSql' or 'rawExecute' to the
-- substituted string and collected values.
--
-- It produces a value of type 
-- @('RawSql' a, 'MonadIO' m) => 'ReaderT' 'SqlBackend' m [a]@.
--
-- Here is a small example:
--
-- @
--    let lft = 10 :: Int
--        rgt = 20 :: Int
--        width = 50 :: Int
--    in [sqlQQ|
--            DELETE FROM category WHERE lft BETWEEN #{lft} AND #{rgt};
--            UPDATE category SET rgt = rgt - #{width} WHERE rgt > #{rgt};
--            UPDATE category SET lft = lft - #{width} WHERE lft > #{rgt};
--            |]
-- @
sqlQQ :: QuasiQuoter
sqlQQ = QuasiQuoter ...

This is my first feature contribution to a Haskell project.

Thanks a lot for your contribution! It is very much appreciated. 👍 (Although keep in mind I am not a maintainer for persistent. I just happened to be clicking around on github and realized I knew how to reduce the code duplication in makeExpr. You'll have to wait for a response from someone like @gregwebs or @snoyberg.)

Contributor

cdepillabout commented Oct 18, 2017

@felixSchl

The transform is really, really simple. It just substitutes all #{foo} with question marks and collects them into a list. For simplicity it also applies toPersistValue on each value, but that could be removed. It then simply applies rawSql or rawExecute to the substituted string and collected values

Yeah, from looking at your example in the first comment, it does look like a very simple transformation. I was trying to say that it would be nice to have haddocks showing the type of the value produced by sqlQQ. You'll have to use the haddock syntax to write this.

Here's an example of what I'm thinking about (using your explanation):

-- | This is a quasiquoter for creating raw SQL expressions.
--
-- It substitutes all @#{foo}@ with question marks and collects them into a list. It applies
-- 'toPersistValue' on each value. It then simply applies 'rawSql' or 'rawExecute' to the
-- substituted string and collected values.
--
-- It produces a value of type 
-- @('RawSql' a, 'MonadIO' m) => 'ReaderT' 'SqlBackend' m [a]@.
--
-- Here is a small example:
--
-- @
--    let lft = 10 :: Int
--        rgt = 20 :: Int
--        width = 50 :: Int
--    in [sqlQQ|
--            DELETE FROM category WHERE lft BETWEEN #{lft} AND #{rgt};
--            UPDATE category SET rgt = rgt - #{width} WHERE rgt > #{rgt};
--            UPDATE category SET lft = lft - #{width} WHERE lft > #{rgt};
--            |]
-- @
sqlQQ :: QuasiQuoter
sqlQQ = QuasiQuoter ...

This is my first feature contribution to a Haskell project.

Thanks a lot for your contribution! It is very much appreciated. 👍 (Although keep in mind I am not a maintainer for persistent. I just happened to be clicking around on github and realized I knew how to reduce the code duplication in makeExpr. You'll have to wait for a response from someone like @gregwebs or @snoyberg.)

@felixSchl

This comment has been minimized.

Show comment
Hide comment
@felixSchl

felixSchl Oct 24, 2017

Contributor

I am still keen to work on this, but I am also somewhat complacent because I was able to simple copy that code into a module for my project, so before proceeding would be great to hear the maintainers' opinions on where this PR should go.

Contributor

felixSchl commented Oct 24, 2017

I am still keen to work on this, but I am also somewhat complacent because I was able to simple copy that code into a module for my project, so before proceeding would be great to hear the maintainers' opinions on where this PR should go.

@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Oct 24, 2017

Contributor

Sorry, I'll help with it - we were probably all waiting to see if one of the other maintainers was more interested than ourselves😄

I don't see any problem with this, especially since it is almost all in a new module. Could you please have a look at these things - I know a list looks daunting, but they should all be easy:

  • There is still an outstanding comment in the code diffs about first pack and uncurry - I think you should make that change, assuming it works.
  • Add haddock documentation. At the module level, it would be good to show the intended usage, including something like your example from above. Please also include @since comments in both the module and function docs (assume the new version will be 2.7.2).
  • Bump the version in the cabal file, and add a ChangeLog entry.

I'll check it again when you've done those things. Thanks for the PR!

Contributor

paul-rouse commented Oct 24, 2017

Sorry, I'll help with it - we were probably all waiting to see if one of the other maintainers was more interested than ourselves😄

I don't see any problem with this, especially since it is almost all in a new module. Could you please have a look at these things - I know a list looks daunting, but they should all be easy:

  • There is still an outstanding comment in the code diffs about first pack and uncurry - I think you should make that change, assuming it works.
  • Add haddock documentation. At the module level, it would be good to show the intended usage, including something like your example from above. Please also include @since comments in both the module and function docs (assume the new version will be 2.7.2).
  • Bump the version in the cabal file, and add a ChangeLog entry.

I'll check it again when you've done those things. Thanks for the PR!

@felixSchl

This comment has been minimized.

Show comment
Hide comment
@felixSchl

felixSchl Oct 30, 2017

Contributor

I've made all the changes and simplified even further. I hope documenting each function becomes somewhat irrelevant now:

-- | Analoguous to 'Database.Persist.Sql.Raw.rawSql'
-- @since 2.7.2
sqlQQ :: QuasiQuoter
sqlQQ = makeQQ [| rawSql |]

I haven't had time to run the tests locally, let's see what CI says. Also, @paul-rouse, I was not sure where to put the @since attribute in the module docs, top, bottom, after description? Seems random

Contributor

felixSchl commented Oct 30, 2017

I've made all the changes and simplified even further. I hope documenting each function becomes somewhat irrelevant now:

-- | Analoguous to 'Database.Persist.Sql.Raw.rawSql'
-- @since 2.7.2
sqlQQ :: QuasiQuoter
sqlQQ = makeQQ [| rawSql |]

I haven't had time to run the tests locally, let's see what CI says. Also, @paul-rouse, I was not sure where to put the @since attribute in the module docs, top, bottom, after description? Seems random

@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Oct 30, 2017

Contributor

Thanks for continuing to work on this! The test failure is due to a weakness in the tests themselves, but I would like to see a clean run - give me a day or two, and I'll try to sort them out. So far my attempts have only managed to produce a bug report against the latest pre-release version of stack ...

Contributor

paul-rouse commented Oct 30, 2017

Thanks for continuing to work on this! The test failure is due to a weakness in the tests themselves, but I would like to see a clean run - give me a day or two, and I'll try to sort them out. So far my attempts have only managed to produce a bug report against the latest pre-release version of stack ...

@felixSchl

This comment has been minimized.

Show comment
Hide comment
@felixSchl

felixSchl Oct 30, 2017

Contributor

Awesome, thanks. I actually hit another uncovered usecase just minutes ago:

updateItemsInCart products cartPk = do
  escape <- connEscapeName <$> ask

  let tbl          = escape $ entityDB $ entityDef (Nothing :: Maybe CartItem)
      cartIdCol    = escape $ fieldDB  $ persistFieldDef CartItemCartId
      productIdCol = escape $ fieldDB  $ persistFieldDef CartItemProductId
      qtyCol       = escape $ fieldDB  $ persistFieldDef CartItemQuantity

  -- update the items
  forM_ products $ \(productPk, qty) ->
    if N.unpack qty <= 0
      then deleteBy $ CartItemUniqueCartIdProductId cartPk productPk
      else
        [executeQQ|
          INSERT INTO #{tbl} (#{cartIdCol}, #{productIdCol}, #{qtyCol})
          VALUES (#{cartPk}, #{productPk}, #{qty})
          ON CONFLICT (#{cartIdCol}, #{productIdCol})
          DO UPDATE SET
            #{qtyCol} = EXCLUDED.#{qtyCol}
        |]

This generates the following query:

[Debug#SQL] 
          INSERT INTO ? (?, ?, ?)
          VALUES (?, ?, ?)
          ON CONFLICT (?, ?)
          DO UPDATE SET
            ? = EXCLUDED.?

But my intention was:

[Debug#SQL] 
          INSERT INTO "cart_item" ("cart_id", "product_id", "quantity")
          VALUES (?, ?, ?)
          ON CONFLICT ("cart_id", "product_id", "quantity")
          DO UPDATE SET
            ? = EXCLUDED.?
Contributor

felixSchl commented Oct 30, 2017

Awesome, thanks. I actually hit another uncovered usecase just minutes ago:

updateItemsInCart products cartPk = do
  escape <- connEscapeName <$> ask

  let tbl          = escape $ entityDB $ entityDef (Nothing :: Maybe CartItem)
      cartIdCol    = escape $ fieldDB  $ persistFieldDef CartItemCartId
      productIdCol = escape $ fieldDB  $ persistFieldDef CartItemProductId
      qtyCol       = escape $ fieldDB  $ persistFieldDef CartItemQuantity

  -- update the items
  forM_ products $ \(productPk, qty) ->
    if N.unpack qty <= 0
      then deleteBy $ CartItemUniqueCartIdProductId cartPk productPk
      else
        [executeQQ|
          INSERT INTO #{tbl} (#{cartIdCol}, #{productIdCol}, #{qtyCol})
          VALUES (#{cartPk}, #{productPk}, #{qty})
          ON CONFLICT (#{cartIdCol}, #{productIdCol})
          DO UPDATE SET
            #{qtyCol} = EXCLUDED.#{qtyCol}
        |]

This generates the following query:

[Debug#SQL] 
          INSERT INTO ? (?, ?, ?)
          VALUES (?, ?, ?)
          ON CONFLICT (?, ?)
          DO UPDATE SET
            ? = EXCLUDED.?

But my intention was:

[Debug#SQL] 
          INSERT INTO "cart_item" ("cart_id", "product_id", "quantity")
          VALUES (?, ?, ?)
          ON CONFLICT ("cart_id", "product_id", "quantity")
          DO UPDATE SET
            ? = EXCLUDED.?
@felixSchl

This comment has been minimized.

Show comment
Hide comment
@felixSchl

felixSchl Oct 30, 2017

Contributor

I think it would be cool if this was possible:

  forM_ products $ \(productPk, qty) ->
    if N.unpack qty <= 0
      then deleteBy $ CartItemUniqueCartIdProductId cartPk productPk
      else
        [executeQQ|
          INSERT INTO ^{CartItem} (@{CartItemCardId}, @{CartItemProductId}, @{CartItemQuantity})
          VALUES (#{cartPk}, #{productPk}, #{qty})
          ON CONFLICT (@{CartItemCardId}, @{CartItemProductId})
          DO UPDATE SET
            @{CartItemQuantity} = EXCLUDED.@{CartItemQuantity}
        |]

So ^{...} does table references, @{...} does column references and #{...} continues to do value substitutions. It's all possible because we are in the ReaderT, so connEscapeName <$> ask should be possible to generate, but I ran into staging issues when trying to pass the escape function to go..

At least doing it this way "somewhat" type checks the program and makes sure things are escaped automatically.

Contributor

felixSchl commented Oct 30, 2017

I think it would be cool if this was possible:

  forM_ products $ \(productPk, qty) ->
    if N.unpack qty <= 0
      then deleteBy $ CartItemUniqueCartIdProductId cartPk productPk
      else
        [executeQQ|
          INSERT INTO ^{CartItem} (@{CartItemCardId}, @{CartItemProductId}, @{CartItemQuantity})
          VALUES (#{cartPk}, #{productPk}, #{qty})
          ON CONFLICT (@{CartItemCardId}, @{CartItemProductId})
          DO UPDATE SET
            @{CartItemQuantity} = EXCLUDED.@{CartItemQuantity}
        |]

So ^{...} does table references, @{...} does column references and #{...} continues to do value substitutions. It's all possible because we are in the ReaderT, so connEscapeName <$> ask should be possible to generate, but I ran into staging issues when trying to pass the escape function to go..

At least doing it this way "somewhat" type checks the program and makes sure things are escaped automatically.

paul-rouse added a commit to paul-rouse/persistent that referenced this pull request Nov 1, 2017

paul-rouse added a commit that referenced this pull request Nov 1, 2017

Merge pull request #728 from paul-rouse/master
Fix tests to allow testing #717
@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Nov 1, 2017

Contributor

I have fixed the travis tests in master to work around the problem meeting the requirement for happy in the lts-2 case, so I think they should work next time you push a commit in this PR. However, when I ran them in my own fork, plus your patch, I got a different error in the lts-2 case, and this is one you need to look at: example log.

Contributor

paul-rouse commented Nov 1, 2017

I have fixed the travis tests in master to work around the problem meeting the requirement for happy in the lts-2 case, so I think they should work next time you push a commit in this PR. However, when I ran them in my own fork, plus your patch, I got a different error in the lts-2 case, and this is one you need to look at: example log.

@felixSchl

This comment has been minimized.

Show comment
Hide comment
@felixSchl

felixSchl Nov 1, 2017

Contributor

Hm, not sure how to fix the mempty error. If I include it explicitly import Data.Monoid (mempty), it fails to compile on lts-6 and suggests "Perhaps you meant this: import Data.Monoid ()" which in turn does not work on lts-2 again.

Contributor

felixSchl commented Nov 1, 2017

Hm, not sure how to fix the mempty error. If I include it explicitly import Data.Monoid (mempty), it fails to compile on lts-6 and suggests "Perhaps you meant this: import Data.Monoid ()" which in turn does not work on lts-2 again.

@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Nov 1, 2017

Contributor

The compiler's suggestion is misleading in this case - you really do need import Data.Monoid (mempty) for lts-2.

In this project, all compiler warnings are switched on and are treated as errors, which is quite strict! Of course the problem in your QQ.hs module is that the Prelude in base-4.7 didn't export the Monoid stuff, but from base-4.8 onwards it does, making the explicit import redundant. Since we know what we are doing (😄) it is simplest to just silence the warning:

{-# OPTIONS_GHC -fno-warn-unused-imports #-}
...
import Data.Monoid (mappend)
Contributor

paul-rouse commented Nov 1, 2017

The compiler's suggestion is misleading in this case - you really do need import Data.Monoid (mempty) for lts-2.

In this project, all compiler warnings are switched on and are treated as errors, which is quite strict! Of course the problem in your QQ.hs module is that the Prelude in base-4.7 didn't export the Monoid stuff, but from base-4.8 onwards it does, making the explicit import redundant. Since we know what we are doing (😄) it is simplest to just silence the warning:

{-# OPTIONS_GHC -fno-warn-unused-imports #-}
...
import Data.Monoid (mappend)
@felixSchl

This comment has been minimized.

Show comment
Hide comment
@felixSchl

felixSchl Nov 1, 2017

Contributor

Does that suggest I could do import Prelude hiding (mempty) and later import Data.Monoid (mempty). A bit awkward though without the context :)

Contributor

felixSchl commented Nov 1, 2017

Does that suggest I could do import Prelude hiding (mempty) and later import Data.Monoid (mempty). A bit awkward though without the context :)

@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Nov 1, 2017

Contributor

I think import Prelude hiding (mempty) will complain in lts-2, when mempty is not exported by Prelude

Contributor

paul-rouse commented Nov 1, 2017

I think import Prelude hiding (mempty) will complain in lts-2, when mempty is not exported by Prelude

@felixSchl

This comment has been minimized.

Show comment
Hide comment
@felixSchl

felixSchl Nov 1, 2017

Contributor

Ah yeah, of course. Hopefully this round of CI will pass. Did you have any response to the previous comments I made (interpolating table and column names)? I think that would be really cool to have, but I still haven't worked out how to do to the fmap connEscapeName ask >>= \escape -> dance inside TH.

Contributor

felixSchl commented Nov 1, 2017

Ah yeah, of course. Hopefully this round of CI will pass. Did you have any response to the previous comments I made (interpolating table and column names)? I think that would be really cool to have, but I still haven't worked out how to do to the fmap connEscapeName ask >>= \escape -> dance inside TH.

@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Nov 1, 2017

Contributor

I can certainly see the benefit of having a syntax to interpolate table and column names. It is up to you - if you want to work that out now, then I'll wait for it; otherwise, if you prefer, I can merge this version and you can submit a separate PR when you are ready to extend it with the other types of substitution. Of course that assumes that the extended version would not break anything you have already done here.

Contributor

paul-rouse commented Nov 1, 2017

I can certainly see the benefit of having a syntax to interpolate table and column names. It is up to you - if you want to work that out now, then I'll wait for it; otherwise, if you prefer, I can merge this version and you can submit a separate PR when you are ready to extend it with the other types of substitution. Of course that assumes that the extended version would not break anything you have already done here.

@felixSchl

This comment has been minimized.

Show comment
Hide comment
@felixSchl

felixSchl Nov 1, 2017

Contributor

I don't see how it would break anything as the types don't have to change and the syntax is only extended. But if you bear with me, I'd like to give it another go before hand off

Contributor

felixSchl commented Nov 1, 2017

I don't see how it would break anything as the types don't have to change and the syntax is only extended. But if you bear with me, I'd like to give it another go before hand off

@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Nov 1, 2017

Contributor

OK, I'll wait for you to say!

Contributor

paul-rouse commented Nov 1, 2017

OK, I'll wait for you to say!

@felixSchl

This comment has been minimized.

Show comment
Hide comment
@felixSchl

felixSchl Nov 2, 2017

Contributor

I've pretty much got it going, but I've met an issue cannot work out, any help would be really appreciated. I am trying to add tests that use the new quasi quoting feature to escape column names, but somehow it fails to deduce "PersistStoreWrite backend".

Here's the error message:

/home/felix/projects/persistent/persistent-test/src/PersistentTest.hs:1013:20: error:
    • Could not deduce (PersistStoreWrite backend0)
        arising from a use of ‘persistFieldDef’
      from the context: (RawSql a,
                         MonadIO m,
                         PersistStoreWrite backend,
                         backend ~ SqlBackend)
        bound by the type signature for:
                   runQuery :: (RawSql a, MonadIO m, PersistStoreWrite backend,
                                backend ~ SqlBackend) =>
                               Int64 -> ReaderT backend m [a]
        at src/PersistentTest.hs:(1006,11)-(1011,36)
      The type variable ‘backend0’ is ambiguous
      These potential instances exist:
        instance PersistStoreWrite SqlBackend
          -- Defined in ‘persistent-2.7.0:Database.Persist.Sql.Orphan.PersistStore’
        instance PersistStoreWrite SqlWriteBackend
          -- Defined in ‘persistent-2.7.0:Database.Persist.Sql.Orphan.PersistStore’
    • In the second argument of ‘($)’, namely
        ‘(persistFieldDef $ PersonAge)’
      In the first argument of ‘escape_aJV7’, namely
        ‘(fieldDB $ (persistFieldDef $ PersonAge))’
      In the first argument of ‘Control.Arrow.first
                                . ((++) . T.unpack)’, namely
        ‘escape_aJV7 (fieldDB $ (persistFieldDef $ PersonAge))’
Contributor

felixSchl commented Nov 2, 2017

I've pretty much got it going, but I've met an issue cannot work out, any help would be really appreciated. I am trying to add tests that use the new quasi quoting feature to escape column names, but somehow it fails to deduce "PersistStoreWrite backend".

Here's the error message:

/home/felix/projects/persistent/persistent-test/src/PersistentTest.hs:1013:20: error:
    • Could not deduce (PersistStoreWrite backend0)
        arising from a use of ‘persistFieldDef’
      from the context: (RawSql a,
                         MonadIO m,
                         PersistStoreWrite backend,
                         backend ~ SqlBackend)
        bound by the type signature for:
                   runQuery :: (RawSql a, MonadIO m, PersistStoreWrite backend,
                                backend ~ SqlBackend) =>
                               Int64 -> ReaderT backend m [a]
        at src/PersistentTest.hs:(1006,11)-(1011,36)
      The type variable ‘backend0’ is ambiguous
      These potential instances exist:
        instance PersistStoreWrite SqlBackend
          -- Defined in ‘persistent-2.7.0:Database.Persist.Sql.Orphan.PersistStore’
        instance PersistStoreWrite SqlWriteBackend
          -- Defined in ‘persistent-2.7.0:Database.Persist.Sql.Orphan.PersistStore’
    • In the second argument of ‘($)’, namely
        ‘(persistFieldDef $ PersonAge)’
      In the first argument of ‘escape_aJV7’, namely
        ‘(fieldDB $ (persistFieldDef $ PersonAge))’
      In the first argument of ‘Control.Arrow.first
                                . ((++) . T.unpack)’, namely
        ‘escape_aJV7 (fieldDB $ (persistFieldDef $ PersonAge))’
@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Nov 2, 2017

Contributor

It looks as if some extra constraint or functional dependency is needed, but I'd need to look harder. I would isolate it in a minimal example, and if you still can't see what's going on, perhaps dump the template haskell expansion (-ddump-splices).

However, I am becoming a little concerned. If it is hard for the user to get the types right, the API is probably wrong! Should we, after all, separate out this extra table/column name interpolation into another PR?

Contributor

paul-rouse commented Nov 2, 2017

It looks as if some extra constraint or functional dependency is needed, but I'd need to look harder. I would isolate it in a minimal example, and if you still can't see what's going on, perhaps dump the template haskell expansion (-ddump-splices).

However, I am becoming a little concerned. If it is hard for the user to get the types right, the API is probably wrong! Should we, after all, separate out this extra table/column name interpolation into another PR?

@felixSchl

This comment has been minimized.

Show comment
Hide comment
@felixSchl

felixSchl Nov 3, 2017

Contributor

I went back on using persistFieldDef in favor of getFieldName and getTableName which means I did not have to grab the escape function, but which also means the query construction itself runs in ReaderT SqlBackend m. Even thought the TH code now evaluates to running in a monad, I think this version is by far the cleanest and most readable.

With respect to fundeps and error messages, after the change above I found it intuitive to work with downstream when fixing up the tests. For example, passing a wrong table type, it would say "Type not in scope 'Name'", if you pass anything but the expected type you would get "Couldn't match type ‘PersistEntityBackend "Type"’ with ‘SqlBackend’". For columns you get a "DataConstructor not in scope" compile error.

Edit, also to make my case for the inclusion of this feature, here's a before and after (from the test suite):

Before:

      escape <- ((. DBName) . connEscapeName) `fmap` ask
      let query = T.concat [ "SELECT ??, ?? "
                           , "FROM ", person
                           , "LEFT OUTER JOIN ", pet
                           , " ON ", person, ".", escape "id"
                           , " = ", pet, ".", escape "ownerId"
                           , " ORDER BY ", person, ".", escape "name"]
          person = escape "Person"
          pet    = escape "Pet"

After:

      ret <- [sqlQQ|
        SELECT ??, ??
        FROM ^{Person}
        LEFT OUTER JOIN ^{Pet}
            ON ^{Person}.@{PersonId} = ^{Pet}.@{PetOwnerId}
        ORDER BY ^{Person}.@{PersonName}
      |]
Contributor

felixSchl commented Nov 3, 2017

I went back on using persistFieldDef in favor of getFieldName and getTableName which means I did not have to grab the escape function, but which also means the query construction itself runs in ReaderT SqlBackend m. Even thought the TH code now evaluates to running in a monad, I think this version is by far the cleanest and most readable.

With respect to fundeps and error messages, after the change above I found it intuitive to work with downstream when fixing up the tests. For example, passing a wrong table type, it would say "Type not in scope 'Name'", if you pass anything but the expected type you would get "Couldn't match type ‘PersistEntityBackend "Type"’ with ‘SqlBackend’". For columns you get a "DataConstructor not in scope" compile error.

Edit, also to make my case for the inclusion of this feature, here's a before and after (from the test suite):

Before:

      escape <- ((. DBName) . connEscapeName) `fmap` ask
      let query = T.concat [ "SELECT ??, ?? "
                           , "FROM ", person
                           , "LEFT OUTER JOIN ", pet
                           , " ON ", person, ".", escape "id"
                           , " = ", pet, ".", escape "ownerId"
                           , " ORDER BY ", person, ".", escape "name"]
          person = escape "Person"
          pet    = escape "Pet"

After:

      ret <- [sqlQQ|
        SELECT ??, ??
        FROM ^{Person}
        LEFT OUTER JOIN ^{Pet}
            ON ^{Person}.@{PersonId} = ^{Pet}.@{PetOwnerId}
        ORDER BY ^{Person}.@{PersonName}
      |]
@felixSchl

This comment has been minimized.

Show comment
Hide comment
@felixSchl

felixSchl Nov 9, 2017

Contributor

@paul-rouse, Have you had any chance to look over this since? I've been using this successfully on a project of mine for a week now without problems.

Contributor

felixSchl commented Nov 9, 2017

@paul-rouse, Have you had any chance to look over this since? I've been using this successfully on a project of mine for a week now without problems.

@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Nov 9, 2017

Contributor

Apologies - I saw the latest commit while the tests were still running, so I waited for them to complete😄 Sorry!

Could you please add an entry in the changelog (including a link to this PR). You can still change the entry which has been started for version 2.7.2, because it is not yet released. It looks as if you will need to rebase your branch to master so that we don't get a merge conflict in ChangeLog.md

Then we should be good to go.

Contributor

paul-rouse commented Nov 9, 2017

Apologies - I saw the latest commit while the tests were still running, so I waited for them to complete😄 Sorry!

Could you please add an entry in the changelog (including a link to this PR). You can still change the entry which has been started for version 2.7.2, because it is not yet released. It looks as if you will need to rebase your branch to master so that we don't get a merge conflict in ChangeLog.md

Then we should be good to go.

@felixSchl

This comment has been minimized.

Show comment
Hide comment
@felixSchl

felixSchl Nov 9, 2017

Contributor

Not sure what's going on there but it seems the tests are failing for reasons other than the actual code changes, looks it's having trouble fetching some dependencies for the old lts2.2 https://travis-ci.org/yesodweb/persistent/jobs/299921263

Contributor

felixSchl commented Nov 9, 2017

Not sure what's going on there but it seems the tests are failing for reasons other than the actual code changes, looks it's having trouble fetching some dependencies for the old lts2.2 https://travis-ci.org/yesodweb/persistent/jobs/299921263

@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Nov 10, 2017

Contributor

I restarted that test, and it is OK now.

Sorry to be a pain - could you add a link to the PR in ChangeLog.md, please, to be consistent with other recent entries?

Contributor

paul-rouse commented Nov 10, 2017

I restarted that test, and it is OK now.

Sorry to be a pain - could you add a link to the PR in ChangeLog.md, please, to be consistent with other recent entries?

@felixSchl

This comment has been minimized.

Show comment
Hide comment
@felixSchl

felixSchl Nov 10, 2017

Contributor
Contributor

felixSchl commented Nov 10, 2017

@felixSchl

This comment has been minimized.

Show comment
Hide comment
@felixSchl

felixSchl Nov 10, 2017

Contributor

Alright, updated and CI still OK :)

Contributor

felixSchl commented Nov 10, 2017

Alright, updated and CI still OK :)

@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Nov 10, 2017

Contributor

Great! Thanks for all of your work on this!

Contributor

paul-rouse commented Nov 10, 2017

Great! Thanks for all of your work on this!

@paul-rouse paul-rouse merged commit 0b8c716 into yesodweb:master Nov 10, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

paul-rouse added a commit to paul-rouse/persistent that referenced this pull request Nov 25, 2017

paul-rouse added a commit that referenced this pull request Nov 25, 2017

Merge pull request #737 from paul-rouse/master
Rearrange documentation added in #717
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment