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

Safely coercing newtype'd Values #302

Closed EggBaconAndSpam closed 2 years ago

EggBaconAndSpam commented 2 years ago

Hi all!

I'm struggling with using Esqueleto in the presence of an abundance of newtype wrappers, where the only way out is to use veryUnsafeCoerceSqlExprValue from Database.Esqueleto.Internal.Internal (which is both ugly and indeed potentially unsafe).

I therefore propose:

cast :: Coercible a b => SqlExpr (Value a) -> SqlExpr (Value b)
cast = veryUnsafeCoerceSqlExprValue

which would complement the existing

castString :: (SqlString s, SqlString r) => SqlExpr (Value s) -> SqlExpr (Value r) 
castNum :: (Num a, Num b) => SqlExpr (Value a) -> SqlExpr (Value b) 

Does anyone see a reason why lifting the coercibility of newtypes to sql expressions should be a bad idea?

EggBaconAndSpam commented 2 years ago

Now that I've written this I realise that just because two types may have the same runtime representation in Haskell doesn't mean that they share the same SQL representation. So this really wouldn't be much safer than just coercing arbitrary types 😞

dnikolovv commented 2 years ago

@EggBaconAndSpam You do have access to the sql representation through PersistFieldSql though.

Although not sure how that can be used to actually make this "safe".

parsonsmatt commented 2 years ago

The PersistFieldSql for a given type is not accessible at the type level, so we can't use it to statically verify a cast.

You can write a class:

class SafeSqlCast a b

safeCast :: forall a b. SafeSqlCast a b => SqlExpr (Value a) -> SqlExpr (Value b)
safeCast = veryUnsafeCoerceSqlExprValue

and then have a soft requirement that all instances of this class are newtype-derived when PersistFieldSql is also newtype-derived.

newtype Quantity = Quantity Int
  deriving newtype (PersistField, PersistFieldSql, SafeSqlCast Int)

Can you post some more specific problems or issues you're having? It's possible there's a nicer way than casting the values.

dnikolovv commented 2 years ago

@parsonsmatt That seems like a decent solution. Thanks for the quick reply!

The problem originated when we were trying to write an on clause similar to this:

newtype SomeId = SomeId UUID
newtype SomeOtherId = SomeOtherId UUID

select $ from $ \something `LeftOuterJoin` anotherThing -> do
  on $ anotherThing ?. SomeOtherIdField ==. something ^. SomeIdField
parsonsmatt commented 2 years ago

Oh, yeah, a AsSqlUUID class is what I'd write for that, probably

If those are persistent Key types, then you can use the ToBaseId class

dnikolovv commented 2 years ago

:bow:

EggBaconAndSpam commented 2 years ago

I think for our use case

castUUID :: (Coercible a UUID, Coercible b UUID) =>  SqlExpr (Value a) -> SqlExpr (Value b)

is a decent compromise since we don't ever expect to use a non-standard sql representation for our uuids. Thanks a lot @parsonsmatt ! I'll close this since it looks like we won't ever get a general solution (unless we were to mess with PersistFieldSql).

Qqwy commented 4 months ago

In our project we ended up doing

sqlRawX :: Esq.SqlExpr (Esq.Value TheNewtype) -> Esq.SqlExpr (Esq.Value TheUnderlyingType)
sqlRawX =
  Control.Exception.assert (Esq.sqlType (Proxy :: Proxy TheNewtype) == Esq.sqlType (Proxy :: Proxy TheUnderlyingType))
    Esq.veryUnsafeCoerceSqlExprValue

Such an assert could of course also be added to the definition of safeCast in @parsonsmatt 's example above.

The only way to make this truly type-safe rather than relying on a runtime assert is if there were a way to check the SQL type at compile-time. But that probably requires a change of sqlType from the land of runtime functions to the land of associated type families.