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

Add selectOne #265

Merged
merged 9 commits into from
Jun 17, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/Database/Esqueleto.hs
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ module Database.Esqueleto {-# WARNING "This module will switch over to the Exper
, SqlExpr
, SqlEntity
, select
, selectFirst
, selectSource
, delete
, deleteCount
Expand Down Expand Up @@ -129,7 +130,7 @@ module Database.Esqueleto {-# WARNING "This module will switch over to the Exper
) where

import Database.Esqueleto.Legacy
import Database.Esqueleto.Internal.PersistentImport
import Database.Esqueleto.Internal.PersistentImport hiding (selectFirst)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of hiding the import, we should just not re-export it from PersistentImpot



-- $setup
Expand Down
3 changes: 2 additions & 1 deletion src/Database/Esqueleto/Experimental.hs
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ module Database.Esqueleto.Experimental
, SqlExpr
, SqlEntity
, select
, selectFirst
, selectSource
, delete
, deleteCount
Expand Down Expand Up @@ -219,7 +220,7 @@ module Database.Esqueleto.Experimental
) where

import Database.Esqueleto.Internal.Internal hiding (From, from, on)
import Database.Esqueleto.Internal.PersistentImport
import Database.Esqueleto.Internal.PersistentImport hiding (selectFirst)

import Database.Esqueleto.Experimental.From
import Database.Esqueleto.Experimental.From.CommonTableExpression
Expand Down
28 changes: 28 additions & 0 deletions src/Database/Esqueleto/Internal/Internal.hs
Original file line number Diff line number Diff line change
Expand Up @@ -2539,6 +2539,34 @@ select query = do
conn <- R.ask
liftIO $ with res $ flip R.runReaderT conn . runSource

-- | Execute an @esqueleto@ @SELECT@ query inside @persistent@'s
-- 'SqlPersistT' monad and return the first entry wrapped in a @Maybe@.
--
-- === __Example usage__
--
-- @
-- firstPerson :: MonadIO m => SqlPersistT m (Maybe Person)
-- firstPerson =
-- 'selectFirst' $
-- 'from' $ \person ->
-- return person
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as below - let's use the Experimental syntax instead, since all code will be switching to it.

-- @
--
-- The above query is equivalent to a 'select' combined with 'limit' but you
-- would still have to transform the results from a list:
--
-- @
-- firstPerson :: MonadIO m => SqlPersistT m [Person]
-- firstPerson =
-- 'select' $
-- 'from' $ \person -> do
-- 'limit' 1
-- return person
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's use experimental syntax for the new examples.

Suggested change
-- 'select' $
-- 'from' $ \person -> do
-- 'limit' 1
-- return person
-- 'select' $ do
-- person <- 'from' $ table @Person
-- 'limit' 1
-- return person

-- @

selectFirst :: (SqlSelect a r, MonadIO m) => SqlQuery a -> SqlReadT m (Maybe r)
selectFirst query = fmap Maybe.listToMaybe $ select $ limit 1 >> query
Copy link
Collaborator

Choose a reason for hiding this comment

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

I also like the idea of naming this function selectFirst, but the API break with persistent does unfortunately require a major version bump (to 3.6.0.0).

It's such a useful function, I'd like to expose it now. We can do this with a minor version bump (3.5.1.0), as long as it does not conflict. selectOne is another fine name for it, and I'd honestly be fine with having that name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds great to me 🎉 will rename it accordingly and remove the unnecessary hiding statements, thanks!


-- | (Internal) Run a 'C.Source' of rows.
runSource
:: Monad m
Expand Down
3 changes: 2 additions & 1 deletion src/Database/Esqueleto/Legacy.hs
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ module Database.Esqueleto.Legacy
, SqlExpr
, SqlEntity
, select
, selectFirst
, selectSource
, delete
, deleteCount
Expand Down Expand Up @@ -130,7 +131,7 @@ module Database.Esqueleto.Legacy
) where

import Database.Esqueleto.Internal.Internal
import Database.Esqueleto.Internal.PersistentImport
import Database.Esqueleto.Internal.PersistentImport hiding (selectFirst)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same issue here (and elsewhere) - let's keep this an open import and hide the export at the source.

Suggested change
import Database.Esqueleto.Internal.PersistentImport hiding (selectFirst)
import Database.Esqueleto.Internal.PersistentImport



-- $setup
Expand Down
25 changes: 23 additions & 2 deletions test/Common/Test.hs
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,27 @@ testSubSelect = do
Right xs ->
xs `shouldBe` []

testSelectFirst :: SpecDb
testSelectFirst =
describe "selectFirst" $ do
let personQuery =
selectFirst $
from $ \person -> do
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you change this test query to use the Experimental syntax please. Were in the process of deprecating the old syntax so it will eventually need to change anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure @belevy, I still need to get used to the upcoming syntax 😄. You can take a look at those changes in this commit

where_ $ person ^. PersonFavNum >=. val 1
orderBy [asc (person ^. PersonId)]
return $ person ^. PersonId
itDb "returns Just" $ do
person <- insert' p1
_ <- insert' p2
res <- personQuery
asserting $
res `shouldBe` Just (Value $ entityKey person)

itDb "returns Nothing" $ do
res <- personQuery
asserting $
res `shouldBe` (Nothing :: Maybe (Value PersonId))
Copy link
Collaborator

Choose a reason for hiding this comment

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

awesome


testSelectSource :: SpecDb
testSelectSource = do
describe "selectSource" $ do
Expand Down Expand Up @@ -2271,10 +2292,11 @@ listsEqualOn :: (HasCallStack, Show a1, Eq a1) => [a2] -> [a2] -> (a2 -> a1) ->
listsEqualOn a b f = map f a `shouldBe` map f b

tests :: SpecDb
tests = do
tests =
describe "Esqueleto" $ do
testSelect
testSubSelect
testSelectFirst
testSelectSource
testSelectFrom
testSelectJoin
Expand Down Expand Up @@ -2389,4 +2411,3 @@ shouldBeOnClauseWithoutMatchingJoinException ea =
pure ()
_ ->
expectationFailure $ "Expected OnClauseWithMatchingJoinException, got: " <> show ea