Improve checking of InnerJoin clause ordering

[erikd]

It seems that Postgresql is much more sensitive to the ordering of the on clauses for the InnerJoin or a from clause than Sqlite.

If one writes a working query against Sqlite and then runs it against Postgresql it can give a rather ugly run-time failure. Unless one reads (again) the documentation for the on clause one is like to start digging into the Esqueleto code to figure out why.

I propose that we add code to Esqueleto that enforces the on clause restrictions and print better error messages. I'm even willing to do the work :-)

Is that a good idea?

[meteficha]

I assume you're talking about run time checks. Then the only reason I'd be opposed to them was if they had a perceivable performance hit, which I don't expect.

Perhaps it may be possible fixing this at the type level using a type level stack. I may oppose a type level check if it turns out to be too complicated.

Other than these, I can't think of any other caveat, so go ahead! =)

[erikd]

Well personally, I'd much prefer a compile time check but I'd settle for run-time. Might be worth doing a run-time check first and then trying to promote it to compile time.

[meteficha]

I'd too prefer a compile time check, I'm just careful not to turn esqueleto into a bigger monster than it already is =). It depends on how complex the solution needs to be.

[mitchellwrosen]

@erikd Can you give an example of an expression that Sqlite accepts and Postgres rejects?

[erikd]

Ok, simple program (which from a strict reading of the documentation has a bug) here:

{-# LANGUAGE CPP, ConstraintKinds, QuasiQuotes, TypeFamilies, GeneralizedNewtypeDeriving,
            TemplateHaskell, OverloadedStrings, GADTs, FlexibleContexts, ScopedTypeVariables #-}

import Control.Monad (void)
import Control.Monad.IO.Class (liftIO)
import Control.Monad.Logger (NoLoggingT, runNoLoggingT)
import Control.Monad.Trans.Resource (runResourceT)
import Database.Esqueleto
import Database.Persist.TH

#ifdef PSQL
import Database.Persist.Postgresql (withPostgresqlConn)
#else
import Database.Persist.Sqlite (withSqliteConn)
#endif

share [mkPersist sqlSettings, mkMigrate "migrateAll"] [persistLowerCase|
    Ant
        name Int
        deriving Show
    Bat
        antId AntId
        deriving Show
    Cat
        batId BatId
        deriving Show
    Dog
        catId CatId
        deriving Show
    |]

main :: IO ()
main = do
    let withConn =
#ifdef PSQL
            withPostgresqlConn "host=localhost port=5432 user=test dbname=test password=test"
#else
            withSqliteConn "test.sqlite3"
#endif
    runResourceT . runNoLoggingT . withConn . runSqlConn $ do
        void $ runMigrationSilent migrateAll
        rows <- select . from $ \ (a `InnerJoin` b `InnerJoin` c `InnerJoin` d) -> do
                            on (a ^. AntId ==. b ^. BatAntId)
                            on (b ^. BatId ==. c ^. CatBatId)
                            on (c ^. CatId ==. d ^. DogCatId)
                            return (a ^. AntId, b ^. BatId, c ^. CatId, d ^. DogId)

        liftIO $ print rows
    putStrLn "Done"

which can be run with Sqlite using:

runghc test-program.hs

and with Postgresql using:

runghc -DPSQL test-program.hs

Sqlite accepts the generated query, but Postgresql does not.

The bug is that the on clauses are in reverse order. The query should be:

        rows <- select . from $ \ (a `InnerJoin` b `InnerJoin` c `InnerJoin` d) -> do
                            on (c ^. CatId ==. d ^. DogCatId)
                            on (b ^. BatId ==. c ^. CatBatId)
                            on (a ^. AntId ==. b ^. BatAntId)
                            return (a ^. AntId, b ^. BatId, c ^. CatId, d ^. DogId)

which is then accepted by both Sqlite and Postgresql.

It would be nice if this could be caught at compile time or better yet, corrected on-the-fly :-).

[meteficha]

Your first example is wrong, on clauses should be in reverse order. If that's not clear from the documentation, it should be improved.

I would like to hear suggestions with respect to how this bug could have been caught by the type system :).

[erikd]

@meteficha I did say " from a strict reading of the documentation has a bug" :-). The problem with this bug is that it only manifests at runtime.

I am not a type level hacker so have no idea.

[meteficha]

The best practical solution I can think of is doing sanity checks at runtime.

[erikd]

Sanity checks at runtime already exist. Its called Postgres :-)

[meteficha]

Fair enough =).