SQLite deleteCascade does not handle all foreign keys

[rvl]

Hi, I'm trying to use persistent-sqlite and deleteCascade with foreign keys referencing non-primary-key columns.

Here is my example program based on the reproduction template.

When I run the example, deleteCascade correctly deletes rows of tables with foreign keys that reference the username PK column. But it does not delete the rows of tables with foreign keys that reference the non-PK name column.

Is this something that should work with persistent-sqlite? My current workaround is to replace deleteCascade with multiple deleteWhere statements to take care of the related entities.

[parsonsmatt]

Thanks for the reproduction! This strikes me as something that should work, certainly.

[parsonsmatt]

OK, so the issue is in mkDeleteCascade:

-- | Generate a 'DeleteCascade' instance for the given @EntityDef@s.
mkDeleteCascade :: MkPersistSettings -> [EntityDef] -> Q [Dec]
mkDeleteCascade mps defs = do
    let deps = concatMap getDeps defs
    mapM (go deps) defs
  where
    getDeps :: EntityDef -> [Dep]
    getDeps def =
        concatMap getDeps' $ entityFields $ fixEntityDef def
      where
        getDeps' :: FieldDef -> [Dep]
        getDeps' field@FieldDef {..} =
            case foreignReference field of
                Just name ->
                     return Dep
                        { depTarget = name
                        , depSourceTable = entityHaskell def
                        , depSourceField = fieldHaskell
                        , depSourceNull  = nullable fieldAttrs
                        }
                Nothing -> []
    go :: [Dep] -> EntityDef -> Q Dec
    go allDeps EntityDef{entityHaskell = name} = do
        let deps = filter (\x -> depTarget x == name) allDeps
        key <- newName "key"
        let del = VarE 'delete
        let dcw = VarE 'deleteCascadeWhere
        just <- [|Just|]
        filt <- [|Filter|]
        eq <- [|Eq|]
        value <- [|FilterValue|]
        let mkStmt :: Dep -> Stmt
            mkStmt dep = NoBindS
                $ dcw `AppE`
                  ListE
                    [ filt `AppE` ConE filtName
                           `AppE` (value `AppE` val (depSourceNull dep))
                           `AppE` eq
                    ]
              where
                filtName = filterConName' mps (depSourceTable dep) (depSourceField dep)
                val (Nullable ByMaybeAttr) = just `AppE` VarE key
                val _                      =             VarE key



        let stmts :: [Stmt]
            stmts = map mkStmt deps `mappend`
                    [NoBindS $ del `AppE` VarE key]

        let entityT = genericDataType mps name backendT

        return $
            instanceD
            [ mkClassP ''PersistQuery [backendT]
            , mkEqualP (ConT ''PersistEntityBackend `AppT` entityT) (ConT ''BaseBackend `AppT` backendT)
            ]
            (ConT ''DeleteCascade `AppT` entityT `AppT` backendT)
            [ FunD 'deleteCascade
                [normalClause [VarP key] (DoE stmts)]
            ]

We look at the FieldDef and if any FieldDef is a foreign key reference, then we add it as a dependency, and we delete it later on. However, we totally ignore the entityForeigns field of the EntityDef:

λ> entityForeigns $ entityDef (Nothing @Alias)
[ForeignDef {foreignRefTableHaskell = HaskellName {unHaskellName = "Person"}, foreignRefTableDBName = DBName {unDBName = "person"}, foreignConstraintNameHaskell = HaskellName {unHaskellName = "fkperson"}, foreignConstraintNameDBName = DBName {unDBName = "aliasfkperson"}, foreignFields = [((HaskellName {unHaskellName = "person"},DBName {unDBName = "person"}),(HaskellName {unHaskellName = "username"},DBName {unDBName = "username"}))], foreignAttrs = [], foreignNullable = False}]

Adding a dep to this should fix it.

[parsonsmatt]

Hmm, so the Foreign constructor doesn't give you a way to say which column on the target table is supposed to be referenced! It seems to assume that it is referencing the primary key:

ForeignDef 
    {foreignRefTableHaskell = HaskellName {unHaskellName = "Person"}
    , foreignRefTableDBName = DBName {unDBName = "person"}
    , foreignConstraintNameHaskell = HaskellName {unHaskellName
= "fkperson"}
    , foreignConstraintNameDBName = DBName {unDBName = "aliasfkperson"}
    , foreignFields = 
        [ ((HaskellName {unHaskellName = "person"},DBName {unDBName = "person"}),(HaskellName {unHaskellName = "username"},DBName {unDBName = "username"}))
        ]
    , foreignAttrs = []
    , foreignNullable = False
    }

This is the ForeignDef that is generated for your code. If I fixed deleteCascade to work, then it would delete Persons with a username matching the aliasPerson field, not with a name matching that field.

[parsonsmatt]

Note that deleteCascade does a "manual" delete cascade - it deletes anything that could match in separate SQL queries. If you want ON DELETE CASCADE functionality, then you'd want to write that as a migration/alteration of the foreign key.

I'm going to open a separate issue to allow you to set OnDeleteCascade on foreign keys to get this behavior automatically, where applicable.

[schnecki]

The title is misleading. This also happens with postgres. I had to add several deletion commands to prevent SQL errors. It would make absolutely sense to hand this task over to the DB with ON DELETE CASCADE.

[parsonsmatt]

I don't think the foreign key constraints are doing what we expect here.

I suspect that the Foreign Person fkperson person line is making a foreign key reference to the primary key column, not the name column. There's no way to specify what the referred to column is. Indeed, that's kind of a big problem with our foreign key syntax!

Related issue #957 - we can't actually even make a foreign key that refers to a non-primary key.

So I think a full solution here is going to involve modifying the syntax to allow for a References clause that specifies the target column names. Then we need to modify the foreign def to include a Maybe (NonEmpty TargetColumn) - if Nothing, assume primary key, if Just, then match against the entity definition for the column names.

[rvl]

Thanks @parsonsmatt 👍