New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce `BackendCompatible` class #701

Merged
merged 6 commits into from Oct 22, 2017

Conversation

Projects
None yet
5 participants
@parsonsmatt
Contributor

parsonsmatt commented Sep 15, 2017

This PR introduces some machinery for generalizing the SQL backend requirement, and uses that to generalize the insertOnDuplicateKeyUpdate functions.

This PR enables support for the persistent-typed-db library which wraps the SqlBackend with a phantom type to indicate which database models are associated with. This turns mixing up your connection and models in a multi-db environment into a compile time error.

This PR is based on work done in a PR to esqueleto. I was able to get esqueleto queries working by incorporating the BackendCompatible class to Esqueleto and instantiating it appropriately in the application.

The BackendCompatible class and the HasPersistBackend / IsPersistBackend classes are very similar, and it's likely that the functionality can be merged in some way.

@parsonsmatt parsonsmatt changed the title from Generalize `insertOnDuplicateKeyUpdate` functions to any compatible SQL backend. to Introduce `BackendCompatible` class Sep 21, 2017

@parsonsmatt

This comment has been minimized.

Show comment
Hide comment
@parsonsmatt

parsonsmatt Sep 21, 2017

Contributor

After the merge of #702, this PR is mostly just incorporating the BackendCompatible class into persistent. I've generalized the two MySQL-specific functions to show how this can be done to allow alternate backends.

Generally speaking, where you might have:

foo :: 
  ( PersistEntity record
  , PeristEntityBackend record ~ BaseBackend backend
  , IsSqlBackend backend
  )

this can be replaced with:

foo ::
  ( PersistEntity record,
  , PersistEntityBackend record ~ backend
  , BackendCompatible SqlBackend backend
  )

This works for SqlReadBackend because of the instance BackendCompatible SqlBackend SqlReadBackend, without needing to go through the BaseBackend type family.

Likewise, functions that are currently hardcoded to use SqlBackend can be generalized:

-- before:
asdf :: ReaderT SqlBackend m ()
asdf = pure ()

-- after:
asdf' :: BackendCompatible SqlBackend backend => ReaderT backend m ()
asdf' = withReaderT projectBackend asdf

This is, as far as I can tell, the least invasive way to add this compatibility. I can extend this PR to also generalize the other functions that hardcode SqlBackend if that's desired, or we can keep the changes smaller.

Contributor

parsonsmatt commented Sep 21, 2017

After the merge of #702, this PR is mostly just incorporating the BackendCompatible class into persistent. I've generalized the two MySQL-specific functions to show how this can be done to allow alternate backends.

Generally speaking, where you might have:

foo :: 
  ( PersistEntity record
  , PeristEntityBackend record ~ BaseBackend backend
  , IsSqlBackend backend
  )

this can be replaced with:

foo ::
  ( PersistEntity record,
  , PersistEntityBackend record ~ backend
  , BackendCompatible SqlBackend backend
  )

This works for SqlReadBackend because of the instance BackendCompatible SqlBackend SqlReadBackend, without needing to go through the BaseBackend type family.

Likewise, functions that are currently hardcoded to use SqlBackend can be generalized:

-- before:
asdf :: ReaderT SqlBackend m ()
asdf = pure ()

-- after:
asdf' :: BackendCompatible SqlBackend backend => ReaderT backend m ()
asdf' = withReaderT projectBackend asdf

This is, as far as I can tell, the least invasive way to add this compatibility. I can extend this PR to also generalize the other functions that hardcode SqlBackend if that's desired, or we can keep the changes smaller.

@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Sep 22, 2017

Contributor

I don't know the persistent code well enough to be able to comment on the principles here; would someone else be able to look at it, please - perhaps @gregwebs or @MaxGabriel ? Obviously there are routine matters still to sort out, such as fixing the tests, perhaps adding some of the explanation from the comments above to the haddock documentation, updating the changelog, and bumping the version.

Contributor

paul-rouse commented Sep 22, 2017

I don't know the persistent code well enough to be able to comment on the principles here; would someone else be able to look at it, please - perhaps @gregwebs or @MaxGabriel ? Obviously there are routine matters still to sort out, such as fixing the tests, perhaps adding some of the explanation from the comments above to the haddock documentation, updating the changelog, and bumping the version.

@MaxGabriel

This comment has been minimized.

Show comment
Hide comment
@MaxGabriel
Member

MaxGabriel commented Sep 22, 2017

Me either @paul-rouse

@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Oct 1, 2017

Contributor

@snoyberg would you mind casting an eye over this one, please? It looks OK to me in principle, but is there anything I am missing, and what about the idea of generalising to other functions? If we are proceeding with it, I will happily review the routine things with @parsonsmatt

Contributor

paul-rouse commented Oct 1, 2017

@snoyberg would you mind casting an eye over this one, please? It looks OK to me in principle, but is there anything I am missing, and what about the idea of generalising to other functions? If we are proceeding with it, I will happily review the routine things with @parsonsmatt

@snoyberg

I've looked this over, and it looks fine to me. Thanks for the ping @paul-rouse

@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Oct 1, 2017

Contributor

Thanks @snoyberg !

@parsonsmatt - would you be able to sort out the warning which is getting in the way of the travis tests? Also, I thought your explanation in the comment above was very clear, and it would be really good to copy most of it into the haddock. Then let's go for it!

Contributor

paul-rouse commented Oct 1, 2017

Thanks @snoyberg !

@parsonsmatt - would you be able to sort out the warning which is getting in the way of the travis tests? Also, I thought your explanation in the comment above was very clear, and it would be really good to copy most of it into the haddock. Then let's go for it!

@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Oct 19, 2017

Contributor

@parsonsmatt - do you want to pursue this?

Contributor

paul-rouse commented Oct 19, 2017

@parsonsmatt - do you want to pursue this?

@parsonsmatt

This comment has been minimized.

Show comment
Hide comment
@parsonsmatt

parsonsmatt Oct 19, 2017

Contributor

Yes, sorry, I've had a crazy past few weeks :) I'll try to get it done before the weekend's over.

Contributor

parsonsmatt commented Oct 19, 2017

Yes, sorry, I've had a crazy past few weeks :) I'll try to get it done before the weekend's over.

@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Oct 19, 2017

Contributor

@parsonsmatt No hurry - I just wanted to check it is still open!

Contributor

paul-rouse commented Oct 19, 2017

@parsonsmatt No hurry - I just wanted to check it is still open!

@paul-rouse paul-rouse merged commit 5ad1465 into yesodweb:master Oct 22, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Oct 22, 2017

Contributor

Thanks!

@psibi We have several persistent packages queued up for release, and I don't have access on hackage. Would you be able to upload them, please? Unless someone wants to give me access, that is:-) (I am "paulrouse" on hackage.) I believe the ones currently waiting are:

  • persistent-2.7.1
  • persistent-mysql-2.6.2
  • persistent-sqlite-2.6.3
Contributor

paul-rouse commented Oct 22, 2017

Thanks!

@psibi We have several persistent packages queued up for release, and I don't have access on hackage. Would you be able to upload them, please? Unless someone wants to give me access, that is:-) (I am "paulrouse" on hackage.) I believe the ones currently waiting are:

  • persistent-2.7.1
  • persistent-mysql-2.6.2
  • persistent-sqlite-2.6.3
@psibi

This comment has been minimized.

Show comment
Hide comment
@psibi

psibi Oct 23, 2017

Member

@paul-rouse I have released the three packages to Hackage.

CC: @snoyberg It would be nice if Paul also has access to Hackage for persistent related packages.

Member

psibi commented Oct 23, 2017

@paul-rouse I have released the three packages to Hackage.

CC: @snoyberg It would be nice if Paul also has access to Hackage for persistent related packages.

@parsonsmatt

This comment has been minimized.

Show comment
Hide comment
@parsonsmatt

parsonsmatt Oct 23, 2017

Contributor

Thanks so much! 😄

Contributor

parsonsmatt commented Oct 23, 2017

Thanks so much! 😄

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Oct 23, 2017

Member
Member

snoyberg commented Oct 23, 2017

@psibi

This comment has been minimized.

Show comment
Hide comment
@psibi

psibi Oct 23, 2017

Member

@snoyberg Thanks!

@paul-rouse You should have Hackage access to all of the persistent related repos. Let me know if I missed something.

Member

psibi commented Oct 23, 2017

@snoyberg Thanks!

@paul-rouse You should have Hackage access to all of the persistent related repos. Let me know if I missed something.

@paul-rouse

This comment has been minimized.

Show comment
Hide comment
@paul-rouse

paul-rouse Oct 23, 2017

Contributor

@psibi Thank you for sorting out the access! I don't even know the status of persistent-redis and persistent-zookeeper, except that they are in the Github repo - but those are the only two I can't access on hackage, and they are probably unimportant.

Contributor

paul-rouse commented Oct 23, 2017

@psibi Thank you for sorting out the access! I don't even know the status of persistent-redis and persistent-zookeeper, except that they are in the Github repo - but those are the only two I can't access on hackage, and they are probably unimportant.

@psibi

This comment has been minimized.

Show comment
Hide comment
@psibi

psibi Oct 23, 2017

Member

@paul-rouse That's because I don't have the Hackage access to those two packages and I haven't required them so far.

Member

psibi commented Oct 23, 2017

@paul-rouse That's because I don't have the Hackage access to those two packages and I haven't required them so far.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment