bitemyapp
(>=.) and friends have unsound types with respect to Maybe
If you use a comparison operator on a Maybe value, there is nothing stopping a null getting into your conditionals if the values going into >=. are actually Nothing. This may wind up ruining your day or otherwise creating ... funny runtime bugs, since you create a NULL in a Value Bool, which is not supposed to be there.
oops :: SqlQuery (SqlExpr (Value Bool))
oops = do
pure (val Nothing >=. (val . Just) (2::Int))
I think that in the current type signatures, val . Just is probably universally an invitation for bugs with the sole exception of if the result goes into ||..
Real-world example
If you write a query like this and omit the isNothing check you can cause this issue:
-- | Schedule to accept calls
ScheduleEntry sql=schedule_entries
-- | Time at which this schedule starts applying
startDate Day Maybe
endDate Day Maybe
-- | Start time in each day that this schedule applies
startHours Int
startMins Int
endHours Int
endMins Int
applicableRules :: LocalTime -> SqlQuery (SqlExpr (Entity ScheduleEntry))
applicableRules ts = do
sched <- from $ table @ScheduleEntry
let day = localDay ts
let TimeOfDay {todHour, todMin} = localTimeOfDay ts
where_ $ isNothing sched.startDate ||. (val . Just) day >=. sched.startDate
where_ $ isNothing sched.endDate ||. (val . Just) day <=. sched.endDate
where_ $ val todHour >=. sched.startHours &&. val todHour <=. sched.endHours
where_ $ val todMin >=. sched.startMins &&. val todMin <=. sched.endMins
pure sched
This could possibly be fixed in a way that only breaks suspicious uses (although my condolences extend to anyone with a large codebase dealing with migrating this; probably would want to keep around the old buggy signatures for compat under a different name, so you could just replace all usages and migrate piecewise):
type family BoolOpResult (a :: Type) :: Type where
BoolOpResult (Maybe a) = Maybe Bool
BoolOpResult a = Bool
(>=.) :: (PersistField typ) => SqlExpr (Value typ) -> SqlExpr (Value typ) -> SqlExpr (Value (BoolOpResult typ))
(>=.) = unsafeSqlBinOp ">= "
cc @parsonsmatt, I'm unsure if this type level shenanigans is fully sound either
A note about a consequent API change to fixing all the boolean operators to properly model tristate: you would need to fix stuff like
q =
from $ table @Catgirls
`leftJoin` table @Catears
`on` (\(cg :& earb) -> just cg.id ==. earb.owner)
where owner may be null, so you use just in this comparison. Under the suggested fix, this comparison would now return a Maybe, which is totally reasonable as an on clause input; it will do the right thing if it gets a null, but the on function would have to be adjusted to accept this.
I am increasingly thinking that perhaps the sound comparison operators should be introduced into a new module that would become part of the default set when Experimental is actually stabilized, and then you could migrate module by module, because I can already see how much this might suck for industry code.
I think this is duplicate with #130 - the writeup here is much more detailed though. :grin:
