bitemyapp / esqueleto

New home of Esqueleto, please file issues so we can get things caught up!
BSD 3-Clause "New" or "Revised" License
370 stars 107 forks source link

Database.Esqueleto.PostgreSQL.values does not perform necessary type coercion at runtime #343

Open rjmholt opened 1 year ago

rjmholt commented 1 year ago

If I define a simple enum type in PostgreSQL like this:

CREATE TYPE behavior AS ENUM ('Good', 'Bad');

And then give it the appropriate matching Haskell implementation (plus all the other stuff which is less material):

data Behavior = Good | Bad
  deriving stock (Show, Eq)

instance PersistField Behavior where
  toPersistValue = PersistText . pack . show
  fromPersistValue = undefined -- Not important here

instance PersistFieldSql Behavior where
  sqlType _ = SqlOther "behavior"

I can do something like join on values of that enum (like in this silly example):

from $
  table @Kids
    `innerJoin` behaviorDinner
      `on` (\(kid :& (bvr, _)) -> kid ^. KidBehavior ==. bvr)
where
  behaviorDinner :: From (Behavior, Dinner)
  behaviorDinner = values $ (Good, Pizza) :| [(Bad, Poison)]

When I run this though, I get an error message something like this (paraphrased from the real error):

uncaught exception: SqlError
       SqlError {sqlState = "42883", sqlExecStatus = FatalError, sqlErrorMsg = "operator does not exist: behavior = text", sqlErrorDetail = "", sqlErrorHint = "No operator matches the given name and argument types. You might need to add explicit type casts."}

Looking at the SQL Esqueleto generates for this, it looks something like this:

INNER JOIN (VALUES (?,?),(?,?)) AS "vq"("v","v2") ON "kids"."behavior" = "vq"."v"

So it seems like we're missing a type coercion (which sqlType makes available) to correctly distill the values. From the PostgreSQL documentation:

When VALUES is used in INSERT, the values are all automatically coerced to the data type of the corresponding destination column. When it's used in other contexts, it might be necessary to specify the correct data type. If the entries are all quoted literal constants, coercing the first is sufficient to determine the assumed type for all

In the implementation itself, a quick read indicates that we might want to add that here:

https://github.com/bitemyapp/esqueleto/blob/754b0b5cccad3756e4f46e3c916a94e5eceb61a6/src/Database/Esqueleto/PostgreSQL.hs#L420

Not knowing much about Esqueleto, I'd guess we'd need to add some type constraints to extract the type underlying a.

Also I'd guess it would be more efficient to only add the coercion to the first row, but that would require breaking up the current mapping (where we currently have NE.toList, we'd want to actually process the head and tail differently).

parsonsmatt commented 1 year ago

We'd need to add a PersistFieldSql constraint into the SomeValue constructor but that should be fine