destroy / destroyByRef to return number deleted

[charles-cooper]

Would it be a terrible API breakage to have destroy / destroyByRef return the number of records deleted? This would be helpful so client code can check the return value and react appropriately.

[alevy]

It wouldn't be terrible API breakage -- they both return unit now, so the worst it would do is generate warnings for people who don't set -fno-warn-unused-do-bind, which I don't think is a big deal.

Is returning the number of records the right thing? destroy and destroyByRef are meant to only delete one thing, or else the schema is pretty wrong. What would you do with a number > 1? Perhaps a boolean would be more useful?

(Those are genuine questions, I don't feel strongly about booleans here)

[charles-cooper]

@alevy You're right that a number > 1 indicates some serious problem with the schema. We could return the number and let client code handle it, or throw an exception; either way I think they should be notified instead of 'failing silently'. Of course when the number is 0 that's a case that client code would want to reasonably handle.

[charles-cooper]

So perhaps the return value should be Either ValidationError Bool

[alevy]

I see, so the validation error would basically mean: "something in the schema is bad, because you had more than one row with the same primary key". If that doesn't seem too cumbersome to handle, I'm for it.

[charles-cooper]

I don't feel it's too cumbersome to handle, given that it is the same signature as the save series of functions.

The implementation could look something like this:

diff --git a/Database/PostgreSQL/ORM/Model.hs b/Database/PostgreSQL/ORM/Model.hs
index 9732038..26bf902 100644
--- a/Database/PostgreSQL/ORM/Model.hs
+++ b/Database/PostgreSQL/ORM/Model.hs
@@ -1132,17 +1132,33 @@ trySave c r | not . H.null $ validationErrors errors = return $ Left errors
 -- the database.  This function only looks at the primary key in the
 -- data structure.  It is an error to call this function if the
 -- primary key is not set.
-destroy :: forall a. (Model a) => Connection -> a -> IO ()
+destroy :: forall a. (Model a)
+  => Connection -> a -> IO (Either ValidationError Bool)
 destroy c a =
   case primaryKey a of
     NullKey -> fail "destroy: NullKey"
-    DBKey k -> void $ execute c
-               (modelDeleteQuery (modelQueries :: ModelQueries a)) (Only k)
+    DBKey k -> destroyByRef_ "destroy" c (DBRef k)

 -- | Remove a row from the database without fetching it first.
-destroyByRef :: forall a rt. (Model a) => Connection -> GDBRef rt a -> IO ()
-destroyByRef c a =
-  void $ execute c (modelDeleteQuery (modelQueries :: ModelQueries a)) (Only a)
+destroyByRef :: forall a rt. (Model a)
+  => Connection -> GDBRef rt a -> IO (Either ValidationError Bool)
+destroyByRef = destroyByRef_ "destroyByRef"
+
+destroyByRef_ :: forall a rt. (Model a)
+  => T.Text -> Connection -> GDBRef rt a -> IO (Either ValidationError Bool)
+destroyByRef_ msg c a = action
+  where mq     = modelQueries     :: ModelQueries a
+        mi     = modelIdentifiers :: ModelIdentifiers a
+        pkCol  = modelQPrimaryColumn mi
+        action = do
+            n <- execute c (modelDeleteQuery mq) (Only a)
+            return $ case n of
+                0 -> Right False
+                1 -> Right True
+                _ -> Left $ validationError (T.encodeUtf8 pkCol) $
+                    msg ++ ": DELETE modified " ++ show n ++
+                    " rows. This may indicate that your primary key" ++
+                    " accessor field is not actually a primary key."

 -- | Print to stdout the query statement.
 printq :: Query -> IO ()

[charles-cooper]

@alevy shall I submit a PR?

[alevy]

@charles-cooper yes please