connUpsert should perform a no-op update with no updates specified

[parsonsmatt]

The current implementation of upsert falls back to the racey defaultUpsertBy if no updates are specified.

instance PersistUniqueWrite SqlBackend where
    upsertBy uniqueKey record updates = do
      conn <- ask
      let refCol n = T.concat [connEscapeTableName conn t, ".", n]
      let mkUpdateText = mkUpdateText' (connEscapeFieldName conn) refCol
      case connUpsertSql conn of
        Just upsertSql -> case updates of
                            [] -> defaultUpsertBy uniqueKey record updates
                            _:_ -> do
                                let upds = T.intercalate "," $ map mkUpdateText updates
                                    sql = upsertSql t (NEL.fromList $ persistUniqueToFieldNames uniqueKey) upds
                                    vals = map toPersistValue (toPersistFields record)
                                        ++ map updatePersistValue updates
                                        ++ unqs uniqueKey

                                x <- rawSql sql vals
                                return $ head x
        Nothing -> defaultUpsertBy uniqueKey record updates
        where
          t = entityDef $ Just record
          unqs uniqueKey' = concatMap persistUniqueToValues [uniqueKey']

In part, this is caused by the connUpsertSql function using a Text for the upds, instead of the list of updates itself.

Postgres supports this with ON CONFLICT DO NOTHING.

MySQL supports this horrifyingly with UPDATE id=id which apparently doesn't trigger any triggers (source).

Transparently, we can fix this by checking if the update text is "" in the backend's upsertSql functions. This is a pretty nasty hack, but we could release it as a patch version bump! no, no, absolutely not, this is absolutely a breaking change in the contract of the function, it would need to be a major version bump, even though the type isn't changing

It's probably better to have the backends render the [Update rec] themselves, but that complicates the signature of connUpsertSql a bit:

connUpsertSql :: Maybe (forall e. PersistEntity e => EntityDef -> ... [Update e] -> Text)

The EntityDef becomes redundant, since we can call entityDef (Proxy @e) and summon the canonical one.

[parsonsmatt]

Ah, jeez, this is more complicated than I thought. I tried to work around this in esqueleto, and did try to figure it out with the updateText == "" trick, but it's more complicated than that.

INSERT ... ON CONFLICT DO NOTHING RETURNING ?? does not return anything if no rows are updated.

This StackOverflow question provides Postgres answers to this, but there's quite a bit of complexity around it.

Hmm.

[MaxGabriel]

Should we have a NonEmpty list of updates required for the RETURNING version, and a separate version for no updates?