Optionally derive `PathMultiPiece` for composite keys by blujupiter32 · Pull Request #1509 · yesodweb/persistent

blujupiter32 · 2023-07-16T14:14:44Z

This PR enables PathMultiPiece instance derivation for entities with composite keys using mpsDeriveInstances.

Unlike PathPiece, an instance is not derived automatically. This should avoid the coupling concerns expressed in #649 and #1458. A similar technique could be used for PathPiece etc. in a future (breaking) change to address these issues.

Before submitting your PR, check that you've:

Documented new APIs with Haddock markup
Added @since declarations to the Haddock
- Changes to the documentation assume that this feature will be made available in persistent version 2.14.6.0.
Ran stylish-haskell on any changed files.
- stylish-haskell suggests fixes to Database.Persist.Class.PersistEntity which are unrelated to this PR: Tidy language extensions and imports in PersistEntity module blujupiter32/persistent#10
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)

This reverts part of commit 500feba.

This reverts part of commit 7b9e76f.

This avoids creating an orphan instance of PathMultiPiece.

As mentioned in the MkPersistSettings Haddock change, specifying PersistPathMultiPiece directly overrides this behaviour.

parsonsmatt

I don't think I'm in favor of this change. There's a bit too much special-case logic here.

Currently, there are two ways of deriving instances for keys: the mps setting, and manually.

mkPersist sqlSettings [persistLowerCase|
    Blah
      name String
  |]

instance PathMultiPiece (Key Blah) where ...

I'm OK with introducing features to make manual instances easier - ie exporting the function defaults that are based on PersistEntity a => ..., and a newtype for DerivingVia.

parsonsmatt · 2023-07-17T14:45:25Z

persistent/Database/Persist/Class/PersistEntity.hs

+-- | An auxiliary class to enable 'PathMultiPiece' instance derivation for
+-- 'PersistEntity' composite keys. Instances of 'PersistPathMultiPiece' should
+-- be derived using the @DeriveAnyClass@ strategy.
+--
+-- @since 2.14.6.0
+class PersistEntity record => PersistPathMultiPiece record where
+    -- | 'fromPathMultiPiece' wrapper. The default implementation uses
+    -- 'keyFromValues' and 'fromPathPiece', which are provided by the context.
+    keyFromPieces :: [Text] -> Maybe (Key record)
+    keyFromPieces pieces = do
+        Right key <- keyFromValues <$> mapM fromPathPiece pieces
+        pure key
+    -- | 'toPathMultiPiece' wrapper. The default implementation uses
+    -- 'keyToValues' and 'toPathPiece', which are provided by the context.
+    keyToPieces :: Key record -> [Text]
+    keyToPieces = map toPathPiece . keyToValues


Why is this class required? I think that we should be able to inline the definition directly into the PathMultiPiece (Key record) instance.

parsonsmatt · 2023-07-17T14:51:23Z

persistent/Database/Persist/Class/PersistEntity.hs

+instance PersistPathMultiPiece record => PathMultiPiece (Key record) where
+    fromPathMultiPiece = keyFromPieces
+    toPathMultiPiece = keyToPieces


So PersistPathMultiPiece is used to provide custom behavior to the PathMultiPiece instance. Instead of writing,

mkPersist sqlSettings [persistLowerCase| ... |] instance PathMultiPiece (Key MyRecord) where fromPathMultiPiece = keyFromPieces toPathMultiPiece = keyToPieces

We write:

mkPersist sqlSettings [persistLowerCase| ... |] instance PersistPathMultiPiece MyRecord

I think I am a little hesitant on this approach. The more idiomatic approach is to use a newtype wrapper for use with DerivingVia, like so:

newtype ViaPersistEntity a = ViaPersistEntity (Key a) instance (PersistEntity a) => PathMultiPiece (Key a) where fromPathMultiPiece = ... toPathMultiPiece = .... -- Use site: mkPersist sqlSettings [persistLowerCase| ... |] deriving via ViaPersistEntity MyRecord instance PathMultiPiece (Key MyRecord)

I've replaced PersistPathMultiPiece with the more idiomatic approach in 950098c.

parsonsmatt · 2023-07-17T14:59:47Z

persistent/Database/Persist/Quasi.hs

+
+    Primary firstPart secondPart
+
+    deriving PathMultiPiece


This usage is definitely something I'd consider a problem - the deriving clause is for the datatype, not the key, and I don't want to start introducing special-case logic to decide if a class is intended for a key or the datatype (or both).

This usage, and the Haddock change documenting it here, have been removed in 950098c.

This reverts commit 5bc5799.

parsonsmatt · 2023-07-17T17:58:07Z

persistent/Database/Persist/TH.hs

+        | typeclass == ''PathMultiPiece =
+            ViaStrategy $ ConT ''ViaPersistEntity `AppT` recordType


Apologies, I should have been more clear - I'm not going to accept a PR that has a special-case for how an instance is derived.

Suggested change

| typeclass == ''PathMultiPiece =

ViaStrategy $ ConT ''ViaPersistEntity `AppT` recordType

OK, so regarding your first comment,

I'm OK with introducing features to make manual instances easier - ie exporting the function defaults that are based on PersistEntity a => ..., and a newtype for DerivingVia.

should I revert my changes to Database.Persist.TH and remove CompositeKeyPathMultiPieceSpec? That would leave ViaPersistEntity and its PathMultiPiece instance as the additions from this PR. If those don't seem useful on their own, I'm fine with closing the PR.

I'll keep the rest on my fork as automatic PathMultiPiece derivation is helpful for my specific use case, although I don't think it would be used often if it was added here (no issues have been raised to request it).

Yes, that works.

I need to document more about how to hook into the derivation stuff without modifying this function here.

The way that I would approach this is with an additional function that operates on the [UnboundEntityDef] from a thing. So, you have,

$(share [mkPersist sqlSettings, derivePathMultiPiece] ...)

Where derivePathMultiPiece :: [UnboundEntityDef] -> Q [Dec], and you can do something like:

derivePathMultiPiece = concat <$> traverse derivePathMultiPieceFor where derivePathMultiPieceFor unboundEntityDef = let keyTypeName = ... [d| deriving via (ViaPersistentEntity $(keyTypeName)) instance PathMultiPiece $(keyTypeName) |]

This gives you the same automatic instance generation, without modifying the upstream code.

parsonsmatt

Thanks! This is great 🎉

blujupiter32 added 9 commits July 14, 2023 19:21

Move derivation strategy choice to where clause

276d05f

Create PersistPathMultiPiece typeclass

500feba

Create spec and derive PersistPathMultiPiece instance in dataTypeDec

3dcf3b6

Revert changes to mkKeyTypeDec

7b9e76f

This reverts part of commit 500feba.

Fix and test instance derivation using mpsDeriveInstances

c372b4c

This reverts part of commit 7b9e76f.

Move PersistPathMultiPiece into PersistEntity module

c8ecc96

This avoids creating an orphan instance of PathMultiPiece.

Document feature in Database.Persist.Quasi

5bc5799

Add top-level describe to CompositeKeyPathMultiPieceSpec

e2cfd3e

Only derive PathMultiPiece for composite keys

46bcee1

As mentioned in the MkPersistSettings Haddock change, specifying PersistPathMultiPiece directly overrides this behaviour.

parsonsmatt requested changes Jul 17, 2023

View reviewed changes

blujupiter32 added 3 commits July 17, 2023 16:01

Fix PersistPathMultiPiece override and add tests for it

647b79a

Use DerivingVia with newtype wrapper

950098c

This reverts commit 5bc5799.

Only use DerivingVia on GHC versions with support for it

133be9b

parsonsmatt requested changes Jul 17, 2023

View reviewed changes

blujupiter32 added 3 commits July 17, 2023 19:01

Add CPP language pragma to spec

d51585f

Merge branch 'master' into derive-pathmultipiece

392021f

Revert changes to Database.Persist.TH and remove associated spec

b36f615

parsonsmatt approved these changes Oct 3, 2023

View reviewed changes

parsonsmatt merged commit 6ec4de1 into yesodweb:master Oct 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optionally derive `PathMultiPiece` for composite keys#1509

Optionally derive `PathMultiPiece` for composite keys#1509
parsonsmatt merged 15 commits intoyesodweb:masterfrom
blujupiter32:derive-pathmultipiece

blujupiter32 commented Jul 16, 2023 •

edited

Loading

Uh oh!

parsonsmatt left a comment

Uh oh!

parsonsmatt Jul 17, 2023

Uh oh!

parsonsmatt Jul 17, 2023

Uh oh!

blujupiter32 Jul 17, 2023

Uh oh!

parsonsmatt Jul 17, 2023

Uh oh!

blujupiter32 Jul 17, 2023

Uh oh!

parsonsmatt Jul 17, 2023

Uh oh!

blujupiter32 Jul 17, 2023

Uh oh!

parsonsmatt Sep 18, 2023

Uh oh!

parsonsmatt left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		\| typeclass == ''PathMultiPiece =
		ViaStrategy $ ConT ''ViaPersistEntity `AppT` recordType

Conversation

blujupiter32 commented Jul 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parsonsmatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parsonsmatt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

blujupiter32 commented Jul 16, 2023 •

edited

Loading