bitemyapp / bitemyapp/esqueleto

(>=.) and friends have unsound types with respect to Maybe

Open
#357 4 comments 0 reactions 0 assignees View on GitHub
Null Handling
Dominant language
Haskell
Stars
399
Forks
107
Avg merge
1d 17h
Merged PRs (30d)
1

Description

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.

```haskell
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
```

```haskell
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):

```haskell
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

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.