Use backend-specific upsertBy

[limick]

This fixes #855, where upsertBy only ever used the default implementation.

Before submitting your PR, check that you've:

• Bumped the version number
• Added @since declarations to the Haddock

After submitting your PR:

• Update the Changelog.md file with a link to your PR
• Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

[limick]

Tests currently fail with format errors:

  src/PersistentTest.hs:625: 
  1) persistent.upsertBy adds a new row with no updates
       uncaught exception: FormatError (FormatError {fmtMessage = "6 '?' characters, but 5 parameters", fmtQuery = "INSERT INTO \"UpsertBy\"(\"email\",\"city\",\"attr\") VALUES (?,?,?) ON CONFLICT (\"email\",\"city\") DO UPDATE SET \"attr\"=? WHERE \"UpsertBy\".\"email\" =? AND \"UpsertBy\".\"city\" =? RETURNING \"UpsertBy\".\"id\", \"UpsertBy\".\"email\", \"UpsertBy\".\"city\", \"UpsertBy\".\"attr\"", fmtParams = ["a","Boston","new","update","a"]})

It seems to require a condition for every unique key in the WHERE clause, which is probably not what we want for upsertBy.
Not sure what the right behaviour should be.

[parsonsmatt]

@limick can you merge the latest master to rerun CI against the new test suite? I think we can get this in for 2.9.2 once tests are passing.

[parsonsmatt]

I think you may have discovered a bug!

The code in the Postgresql for upsert is:

upsertSql' :: EntityDef -> Text -> Text
upsertSql' ent updateVal = T.concat
                           [ "INSERT INTO "
                           , escape (entityDB ent)
                           , "("
                           , T.intercalate "," $ map (escape . fieldDB) $ entityFields ent
                           , ") VALUES ("
                           , T.intercalate "," $ map (const "?") (entityFields ent)
                           , ") ON CONFLICT ("
                           , T.intercalate "," $ concat $ map (\x -> map escape (map snd $ uniqueFields x)) (entityUniques ent)
                           , ") DO UPDATE SET "
                           , updateVal
                           , " WHERE "
                           , wher
                           , " RETURNING ??"
                           ]
    where
      wher = T.intercalate " AND " $ map singleCondition $ entityUniques ent

      singleCondition :: UniqueDef -> Text
      singleCondition udef = T.intercalate " AND " (map singleClause $ map snd (uniqueFields udef))

      singleClause :: DBName -> Text
      singleClause field = escape (entityDB ent) <> "." <> (escape field) <> " =?"

The wher clause is making a clause for every unique condition for the record. upsert (before unreleased 2.10) would blow up at runtime if you used it with a record with 0 or >=2 unique keys. So it would only work in the case that the SQL has a single unique key, which we were guarding with a runtime exception and now a compile-time check.

So, I think we need to alter the upsertSql to take the NonEmpty list of unique keys that we're trying to match on instead of getting them from the EntityDef.

# persistent/Database/Persist/Sql?Types/Internal.hs
# line 89
-    , connUpsertSql :: Maybe (EntityDef -> Text -> Text)
+    , connUpsertSql :: Maybe (EntityDef -> NonEmpty UniqueDef -> Text -> Text)

Then, in the persistent-postgresql, you'd want to update the upsertSql' function to use the given UniqueDef list instead of the one given by the EntityDef.

[limick]

Yes, the fact that we had a clause per unique condition was part of what I found confusing.
Your suggestion makes sense to me - does that also work for MySQL, sqlite and other backends?

[parsonsmatt]

Neither MySQL nor SQLite have connUpsertSql defined, so they won't have to be changed.

[parsonsmatt]

@limick I believe the change I made in #895 should fix the problem you found.

[Vlix]

Does this PR still have a purpose after the merged PR #895 from @parsonsmatt ?

[limick]

Yes, it still has a purpose because the aim is to use upsert where available, which is worth fixing and not fixed by anything else as far as I know.
I got stuck on writing tests because I was unfamiliar with the tools used and then life got in the way. If someone wants to contribute tests I'm happy to assist.

[limick]

So, I think we need to alter the upsertSql to take the NonEmpty list of unique keys that we're trying to match on instead of getting them from the EntityDef.
Then, in the persistent-postgresql, you'd want to update the upsertSql' function to use the given UniqueDef list instead of the one given by the EntityDef.

I implemented the changes as suggested by @parsonsmatt and the tests pass.
Feedback is welcome.

[parsonsmatt]

Ah, crap, sorry, I started a review a long time ago but never actually finalized it.

[parsonsmatt]

This is a major-bump change since it's changing the type definition of a public datatype. Is this necessary?

[limick]

I think it's necessary, because I don't see a way to go from Unique record (which is the type of uniqueKey in Persist/Sql/Orphan/PersistUnique.hs) to its corresponding UniqueDef.

I'd be glad to be proven wrong.

[parsonsmatt]

Hmmm. That's irritating.

Looks like we have this possible pathway:

1. With a PersistEntity e in scope, we can call let uniqDefs = entityUniques (entityDef (Proxy @e)) :: [UniqueDef].
2. Then we have persistUniqueToFieldNames :: Unique record -> [(HaskellName, DBName)].
3. We can do a lookup on the [UniqueDef] to find the corresponding uniqueFields - eg find (\u -> uniqueFields u == persistUniqueToFieldNames uniq) uniqDefs

Not the most elegant solution, for sure, but it will work I think.

[parsonsmatt]

Going to get this merged in for a 2.11 release soon.