prowdsponsor / esqueleto

Bare bones, type-safe EDSL for SQL queries on persistent backends.
http://hackage.haskell.org/package/esqueleto
BSD 3-Clause "New" or "Revised" License
178 stars 51 forks source link

Equality on nullable (Maybe, Checkmark) types #104

Closed Philonous closed 9 years ago

Philonous commented 9 years ago

Consider the following example program:

{-# LANGUAGE FlexibleContexts #-}
{-# LANGUAGE TypeFamilies #-}
{-# LANGUAGE TemplateHaskell #-}
{-# LANGUAGE GADTs #-}
{-# LANGUAGE GeneralizedNewtypeDeriving #-}
{-# LANGUAGE MultiParamTypeClasses #-}

module Test where

import           Control.Monad.Logger
import           Control.Monad.Trans
import           Data.Monoid
import           Database.Esqueleto
import qualified Database.Persist.Sql as P
import qualified Database.Persist.Sqlite as Sqlite
import           Database.Persist.TH

share [mkPersist sqlSettings, mkMigrate "migrateAll"] [persistLowerCase|
Foo
    foo Int
    bar Int Maybe
    quux Checkmark nullable
|]

test = runStderrLoggingT . Sqlite.withSqliteConn ":memory:" $
       \backend -> do
    runSqlConn (runMigration migrateAll) backend
    flip runSqlConn backend $ do
        insert $ Foo 3 Nothing Inactive

        -- Querying rows where bar is Nothing
        --
        -- Persistent

        mp <- P.selectList [FooBar P.==. Nothing] []
        liftIO . putStrLn $ "Maybe/Parsistent: " <> show (length mp) -- 1

        -- Esqueleto
        me <- select . from $ \f -> do
            where_ (f ^. FooBar ==. val Nothing)
            return f
        liftIO . putStrLn $ "Maybe/Esqueleto: " <> show (length me) -- 0

        -- Querying rows where quux is inactive (NULL)
        --
        -- Persistent
        cp <- P.selectList [FooQuux P.==. Inactive] []
        liftIO . putStrLn $ "Checkmark/Parsistent: " <> show (length cp) -- 1

        -- Esqueleto
        ce <- select . from $ \f -> do
            where_ (f ^. FooQuux ==. val Inactive)
            return f
        liftIO . putStrLn $ "Checkmark/Esqueleto: " <> show (length ce) -- 0

And the ghci output when running "test" :

Migrating: CREATE TABLE "foo"("id" INTEGER PRIMARY KEY,"foo" INTEGER NOT NULL,"bar" INTEGER NULL,"quux" BOOLEAN NULL)
[Debug#SQL] CREATE TABLE "foo"("id" INTEGER PRIMARY KEY,"foo" INTEGER NOT NULL,"bar" INTEGER NULL,"quux" BOOLEAN NULL); [] @(<unknown>:<unknown> <unknown>:0:0)
[Debug#SQL] INSERT INTO "foo"("foo","bar","quux") VALUES(?,?,?); [PersistInt64 3,PersistNull,PersistNull] @(<unknown>:<unknown> <unknown>:0:0)
[Debug#SQL] SELECT "id" FROM "foo" WHERE _ROWID_=last_insert_rowid(); [] @(<unknown>:<unknown> <unknown>:0:0)
[Debug#SQL] SELECT "id", "foo", "bar", "quux" FROM "foo" WHERE ("bar" IS NULL); [] @(<unknown>:<unknown> <unknown>:0:0)
Maybe/Parsistent: 1
[Debug#SQL] SELECT "foo"."id", "foo"."foo", "foo"."bar", "foo"."quux"
FROM "foo"
WHERE "foo"."bar" = ?
; [PersistNull] @(<unknown>:<unknown> <unknown>:0:0)
Maybe/Esqueleto: 0
[Debug#SQL] SELECT "id", "foo", "bar", "quux" FROM "foo" WHERE ("quux" IS NULL); [] @(<unknown>:<unknown> <unknown>:0:0)
Checkmark/Parsistent: 1
[Debug#SQL] SELECT "foo"."id", "foo"."foo", "foo"."bar", "foo"."quux"
FROM "foo"
WHERE "foo"."quux" = ?
; [PersistNull] @(<unknown>:<unknown> <unknown>:0:0)
Checkmark/Esqueleto: 0

It appears that esqueleto naively translates foo ^. FooBar ==. Nothing to the SQL expressions foo.bar = NULL, similarly it translates foo ^. FooQuux ==. Inactive to foo.quux = NULL. Obviously this does not lead to the intended result, because comparisons whith NULL themselves evaluate to NULL. Even though I know this pit I keep falling into it. It is probably even more surprising for newcommers.

I was pleased to see that persistent actually uses the correct "IS NOT NULL" construct, so I am wondering if this could be fixed in esqueleto.

meteficha commented 9 years ago

Why things are the way they are

Persistent uses IS NULL for ==. Nothing because that's the only way of checking NULL using Persistent. So it special cases these comparisons (and some others). Note also that Persistent tries to support a common denominator between many different kinds of DBMS, not only RDBMS.

Esqueleto, on the other hand, tries to closely follow SQL. It has no special cases for ==., so when you say ==. with Haskell you mean = with SQL. Instead, it has isNothing, which still has no special cases and always means IS NULL.

Should this behavior be changed?

One could argue that there is no purpose in writing == NULL, as this is always FALSE. That, in my opinion, is the strongest argument in favor of converting ==. val Nothing to IS NULL.

However, one must also remember that Esqueleto is a lot more flexible than Persistent. In particular, Persistent comparisons are always against a constant value. Esqueleto, though, supports JOINs and arbitrary comparisons. This means that ==. e ^. f will never be translated to something involving IS NULL. Our special case is too special.

Also, as I said above, Esqueleto aims to remain very close to SQL. And you need to remember to use IS NULL on SQL queries.

Veredict: this isn't a bug, it's a feature (namely, staying true to SQL).

Thanks for your report, @Philonous!

Philonous commented 9 years ago

Thanks for the reply. I understand and to some extend agree to why this behaviour is intentional. Purely for the sake of discussion I would like to add some points why I thought it might have been a good idea to change the behaviour:

meteficha commented 9 years ago

Unfortunately a few implementation leaks are inevitable for a SQL EDSL. A relational algebra library wouldn't need these compromises (with other tradeoffs).

The symmetry argument is also double edged: Persistent or SQL, pick one to have a symmetry with.