Implement config for customising the FK name #1244

danbroooks · 2021-04-22T20:33:48Z

Before submitting your PR, check that you've:

Documented new APIs with Haddock markup
Added @since declarations to the Haddock
Ran stylish-haskell on any changed files.
Adhered to the code style (see the .editorconfig file for details)

After submitting your PR:

Update the Changelog.md file with a link to your PR
Bumped the version number if there isn't an (unreleased) on the Changelog
Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

Initially, I went with an approach that mirrors what is already done with psToDBName (a function Text -> Text rather than an optional Maybe Text value), though I thought this might not be preferred, and that it may make more sense to provide an optional infix as described on the issue.

I then thought it might make sense to create a newtype wrapper that is private to the module, which you can either go with the default (that just appends the two strings without any infix), or an infixing configuration, as illustrated in the test:

https://github.com/yesodweb/persistent/pull/1244/files#diff-bb8c841390595945f4d325e011ab6d4a0dc800bccaae6ea1ed348164dda7e7bfR362

This might be a bit overkill, so I am happy to change to how it is described in the issue if that is preferred.

parsonsmatt · 2021-04-22T21:58:17Z

Can you base new changes off of the persistent-2.13 branch? I'm going to be working pretty heavily on refactoring and reorganizing code for that release and it'd be a lot easier if these changes were on top of that.

…fixable-fkeys

parsonsmatt

Looks great!

We need to expose a setter in the Database.Persist.Quasi module since the type and fields are only Internal details.

Also the @since declarations are for 2.12.1.2 - adding an export item is a minor bump, so backporting would be persistent-2.12.2.0. And this PR is against persistent-2.13, so it would come out with persistent-2.13 as the initial release currently without a backport.

parsonsmatt · 2021-04-26T17:56:07Z

persistent/Database/Persist/Quasi/Internal.hs

+    , psToFKName :: !ToFKName
+    -- ^ Function used to generate the FK name. Default value: @mappend@
+    --
+    -- @since 2.12.1.2


This PR is currently against persistent-2.13 - are you planning on backporting it?

Note that the PersistSettings is currently hidden, so we'll want to export a setter setPsToFKName :: ToFKName -> PersistSettings -> PersistSettings.

This is just a hangover from my original change request, I will update these @since declarations 👍 looks like I need to rethink how this is switched on as well as you say, as in 2.13 PersistSettings is now in an Internal module, so I will add something like setPsToFKName

I've just pushed some more stuff up, so thats ready for a second look 👍 I will add some annotations to what I've done as there are some additional things in there to consider

danbroooks · 2021-04-27T16:48:01Z

persistent/Database/Persist/Quasi.hs

@@ -413,11 +413,14 @@ Unfortunately, we can't use this to create Haddocks for you, because <https://gi
 -}
 module Database.Persist.Quasi
    ( parse
-    , PersistSettings (..)
+    , PersistSettings


I noticed that the fields for PersistSettings were not actually being hidden, though from your comment I assume this is the desired behaviour in this version of persistent going forward, so I have snipped the fields off the export here.

oh, dang, good catch 👍🏻

danbroooks · 2021-04-27T16:52:28Z

persistent/test/main.hs

+       , preparse
+       , splitExtras
+       , takeColsEx
+       )


Likewise with this test, we are importing Quasi.Internal, but I assume, long term, we want to test the main Quasi module really without touching the Internal module (except other than perhaps the internal types, Line/Token and so on). I can revert this if you want though.

Having worked a little now on these modules/tests, I think it might be nice (especially after this Internal split on the quasi module), to have a test file for Quasi and a test file for Quasi.Internal, though I feel like a lot of these tests in here were added initially to cover as much of the module as possible because of the initial lack of coverage, it might be that a test file for Quasi to test the expected parsing behaviour would be sufficient. Not one for this PR though, just a thought I had while making this change 👍

Yeah I'd eventually like to hollow this out into a more 'typical' hspec test suite, so we'd have Database.Persist.QuasiSpec etc that contains all this stuff.

danbroooks · 2021-04-27T17:05:27Z

persistent/Database/Persist/Quasi.hs

    , nullable
    ) where

 import Database.Persist.Quasi.Internal
+
+setPsUseSnakeCaseForiegnKeys :: PersistSettings -> PersistSettings
+setPsUseSnakeCaseForiegnKeys ps = ps { psToFKName = toFKNameInfixed "_" }


Bit of a long winded function name, but I went with this approach because I think people will want to either have this switched on or off. There is room for further flexibility though if this is wanted, but maybe that would be overkill.

I also thought with this change, that persistent should have this behaviour as default, potentially allowing people to configure this if they wanted to (though why they would I'm not sure), though unfortunately if we did that off the bat then that would break existing persistent codebases, right? Not sure what the long term fix is here, or it might have to be something that came along with a major version bump. Even along with a major version bump we are making people deal with renaming their foreign keys when there is actually technically wrong with them (they're just not aesthetically pleasing to the human reader).

Another approach with a major bump would be to swap this functionality, so rather than opt-in it would become opt-out, if for some reason people would rather just change the value in PersistentSettings than changing things in the DB.

Again, just thinking out loud here :)

IMO the default behavior should be consistent with whatever is currently happening - so end users wouldn't notice a change from this new option.

I'd prefer to see a more general setter exposed, even if this is the most likely variant that folks will want. People can just refer to the .Internal module but that's clunky.

parsonsmatt

Let's export a setter and the types for constructing the foreign key conversion functions.

It'd be nice to document the Text parameters to the function - TableName and ColumnName - somehow. But I'm not sure if you can attach Haddock to function arguments in a record field 🤔

parsonsmatt · 2021-04-27T15:25:44Z

persistent/test/main.hs

+    sentToSecond    Text
+
+    Foreign User fk_noti_user sentToFirst sentToSecond References emailFirst emailSecond
+|]


awesome test <3

parsonsmatt · 2021-04-27T17:32:21Z

persistent/Database/Persist/Quasi.hs

@@ -413,11 +413,14 @@ Unfortunately, we can't use this to create Haddocks for you, because <https://gi
 -}
 module Database.Persist.Quasi
    ( parse
-    , PersistSettings (..)
+    , PersistSettings


oh, dang, good catch 👍🏻

parsonsmatt · 2021-04-27T17:35:02Z

persistent/Database/Persist/Quasi.hs

    , nullable
    ) where

 import Database.Persist.Quasi.Internal
+
+setPsUseSnakeCaseForiegnKeys :: PersistSettings -> PersistSettings
+setPsUseSnakeCaseForiegnKeys ps = ps { psToFKName = toFKNameInfixed "_" }


IMO the default behavior should be consistent with whatever is currently happening - so end users wouldn't notice a change from this new option.

I'd prefer to see a more general setter exposed, even if this is the most likely variant that folks will want. People can just refer to the .Internal module but that's clunky.

parsonsmatt · 2021-04-27T17:36:51Z

persistent/Database/Persist/Quasi/Internal.hs

@@ -98,8 +99,16 @@ parseFieldType t0 =
            PSDone -> PSSuccess (front []) t
            -- _ ->

+newtype ToFKName = ToFKName


As an exposed type, let's put a doc comment on it:

Suggested change

newtype ToFKName = ToFKName

-- | A function for converting a table and column name into a constraint name.

--

-- @since 2.13.0.0

newtype ToFKName = ToFKName

parsonsmatt · 2021-04-27T17:37:53Z

persistent/Database/Persist/Quasi/Internal.hs

+  { unToFKName :: Text -> Text -> Text
+  }


Suggested change

{ unToFKName :: Text -> Text -> Text

}

{ unToFKName :: Text -> Text -> Text

}

parsonsmatt · 2021-04-27T17:38:19Z

persistent/Database/Persist/Quasi.hs

    , upperCaseSettings
    , lowerCaseSettings
+    , setPsUseSnakeCaseForiegnKeys


We should export the FKName type and constructor here too, for use with the setter.

parsonsmatt · 2021-04-27T17:39:01Z

persistent/test/main.hs

+       , preparse
+       , splitExtras
+       , takeColsEx
+       )


Yeah I'd eventually like to hollow this out into a more 'typical' hspec test suite, so we'd have Database.Persist.QuasiSpec etc that contains all this stuff.

parsonsmatt

heck yeah! Thank you so much 👐🏻

parsonsmatt · 2021-04-28T17:46:37Z

persistent/Database/Persist/Quasi.hs

+--
+-- @since 2.13.0.0
+setPsToFKName :: (EntityNameHS -> ConstraintNameHS -> Text) -> PersistSettings -> PersistSettings
+setPsToFKName setter ps = ps { psToFKName = setter }


Beautiful 👍🏻

parsonsmatt · 2021-04-28T17:47:16Z

persistent/Database/Persist/Quasi/Internal.hs

@@ -102,6 +103,11 @@ parseFieldType t0 =
 data PersistSettings = PersistSettings
    { psToDBName :: !(Text -> Text)
    -- ^ Modify the Haskell-style name into a database-style name.
+    , psToFKName :: !(EntityNameHS -> ConstraintNameHS -> Text)


blessed, thanks! I thought it was a table name 😂 types FTW

parsonsmatt · 2021-04-28T17:50:25Z

persistent/Database/Persist/Quasi/Internal.hs

+    , parsedEntityDefEntityAttributes :: [Attr]
+    , parsedEntityDefFieldAttributes :: [[Token]]
+    , parsedEntityDefExtras :: M.Map Text [ExtraLine]
+    }


Oh this is fantastic

In #1256 I'm probably going to be only forwarding this type along, instead of the EntityDef. Maybe the UnboundEntityDef.

* Internalize SqlBackend (#1225) * abstractification * abstractification * Internalize SqlBackend * suppor tother sql dbs * bounds * asdf * import data.monoid * limitoffset and backendspecificoverrides * use setter * getConnUpsertSql * formatting * sigh * clean warns * cabal formatting * merge resolve * lol * ok * update changelogs * one more [ci skip] * Implicit ID Column Configuration (#1234) * Implicit ID Column COnfiguration * PersistSettings is internal * start teasing out the module structure * move around, factor out the Names module * start enumerating types, make entitydef abstract * entity def abstraction * teasing out the EntityDef stuff * builds * testinggg * it works * it works * tidy up * sigh * i hate you * tidy * wrote test for mysql, need to set maxlen sigh * mysql test, need to be able to set maxlen * support mysql lmfao * whyyy * Deprecate mpsGeneric (#1250) * discoverEntities (#1253) * discover entities * remove fdescribe * changelog * yupo * remove error * Fix migrations (#1252) * Better migrations * why is the test failing * Columns are present in entityFields now, but the generated code is broken. * th specs work * fixed mkColumns * changelog entry * fix mongo * no idea why this is broken now * why on earth did this work * remove debug trace statements * typo * what no put that back in * Check for existence of entities before generating them (#1255) * wtf * hmmm * refactor and tidy * are foreign fields never right ?! * changelog * fix comments * dead code * Implement config for customising the FK name (#1244) * Implement config for customising the FK name * Update changelog * Tweak test description * Tweaks/better use of types * Review tweaks * Some initial post-review changes * Table name turned out to be EntityNameHS * Do the same thing but for the constraint * Expose more stuff * Some refactoring / cleanup * Fix changelog indentation * Tidy code layout * QuasiQuoter Improvements (#1256) * QQ now returns UnboundEntityDef * Relocate fixForeignKeysAll * deprecate some stuff, reorganize some code * ok, now we need to set sql types appropriately. * dodgy instances are banned * fuse away the EntityDefSqlTypeExp stuff * refactor to top level * fuse sqlTypeExp in there * fix Key vs Id stuff * still need to get the foreign key types right * hmmm * move to QuasiSpec * clean up tests * so close * ok but what if i don't fix foreign keys * wip * oh man please * getting closer... * make some tests * fix json and keyFromValueM * slightly more graceful handling * return dummy field for id, from persist values * got some tests passing * well sqlite works * pg tests running * what happened * hmm mongo is trashed maybe * bye mongo * ok for real bye mongo, for now at least * clean warns * asdf * drop GHC 8.2 support * sigh * lots of commments * Merge branch 'master' into persistent-2.13 (#1261) * Reexport PersistValue * fix json for MigrationOnly * Export onlyOneUniqueDef (fixes #1194) * use onlyOneUniqueDef in persistent-postgresql Co-authored-by: Dan Brooks <danbroooks@users.noreply.github.com>

danbroooks added 4 commits April 22, 2021 21:33

Implement config for customising the FK name

884ca08

Update changelog

8488bab

Tweak test description

ca67249

Tweaks/better use of types

8a4c516

danbroooks marked this pull request as draft April 22, 2021 21:11

danbroooks marked this pull request as ready for review April 22, 2021 21:23

danbroooks changed the base branch from master to persistent-2.13 April 23, 2021 13:20

Merge remote-tracking branch 'upstream/persistent-2.13' into 1124-pre…

4a8359f

…fixable-fkeys

parsonsmatt requested changes Apr 26, 2021

View reviewed changes

Review tweaks

030662e

danbroooks commented Apr 27, 2021

View reviewed changes

parsonsmatt requested changes Apr 27, 2021

View reviewed changes

danbroooks added 8 commits April 27, 2021 19:44

Some initial post-review changes

94941d7

Table name turned out to be EntityNameHS

6200b95

Do the same thing but for the constraint

6bced57

Expose more stuff

a2d4b7f

Merge branch 'persistent-2.13' into 1124-prefixable-fkeys

067d86f

Some refactoring / cleanup

1f326f6

Fix changelog indentation

28395ce

Tidy code layout

33767af

parsonsmatt approved these changes Apr 28, 2021

View reviewed changes

parsonsmatt merged commit be9fb52 into yesodweb:persistent-2.13 Apr 28, 2021

danbroooks deleted the 1124-prefixable-fkeys branch May 5, 2021 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement config for customising the FK name #1244

Implement config for customising the FK name #1244

danbroooks commented Apr 22, 2021 •

edited

Loading

parsonsmatt commented Apr 22, 2021

parsonsmatt left a comment

parsonsmatt Apr 26, 2021

danbroooks Apr 26, 2021

danbroooks Apr 27, 2021

danbroooks Apr 27, 2021

parsonsmatt Apr 27, 2021

danbroooks Apr 27, 2021 •

edited

Loading

parsonsmatt Apr 27, 2021

danbroooks Apr 27, 2021 •

edited

Loading

parsonsmatt Apr 27, 2021

parsonsmatt left a comment

parsonsmatt Apr 27, 2021

parsonsmatt Apr 27, 2021

parsonsmatt Apr 27, 2021

parsonsmatt Apr 27, 2021

parsonsmatt Apr 27, 2021

parsonsmatt Apr 27, 2021

parsonsmatt Apr 27, 2021

parsonsmatt left a comment

parsonsmatt Apr 28, 2021

parsonsmatt Apr 28, 2021

parsonsmatt Apr 28, 2021

Implement config for customising the FK name #1244

Implement config for customising the FK name #1244

Conversation

danbroooks commented Apr 22, 2021 • edited Loading

parsonsmatt commented Apr 22, 2021

parsonsmatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danbroooks Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danbroooks Apr 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parsonsmatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parsonsmatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danbroooks commented Apr 22, 2021 •

edited

Loading

danbroooks Apr 27, 2021 •

edited

Loading

danbroooks Apr 27, 2021 •

edited

Loading