Refactor #1
Open
Refactor #1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is the first step towards generalising MonadSqlQuery, which is a useful monad class which is unnecessarily fixed to a particular backend type (and which we would like to generalise by backend type.) This makes the `SqlQueryRep` a data family of the backend type, and replaces the old datatype with a type synonym for the data family corresponding to `SqlBackend`.
The MonadSqlQuery class is a bit overloaded, in that it represents both monads that can execute SQL queries with `SqlQueryRep`, and also monads that can execute transactions. These classes do not have to coincide, and in fact there are many monads which are likely to be one but not the other (speaking from experience.) Without re-arranging any of the existing instances, this simply splits up the `MonadSqlQuery` class into two smaller classes managing each of these responsibilities, and does the same for each instance. The code should be functionally unchanged. In the future, I will want to reconsider which monads fall into each bucket and how to handle `TransactionM`, which is aggressive about lifting when we may in fact want certain effects in the monad stack.
After generalising QueryRep, MonadSqlQuery as a class didn't need to be specific to `SqlBackend`. This generalises it without introducing any functional changes.
This adds in the notion of backend compatibility, based on Persistent's similar `BackendCompatible`. Compatibility is expressed in terms of query representations, and allows a single backend to run queries of multiple different types using its compatibility instances. This is useful to us because it allows for the `Shim` functions to run against backends that are not necessarily *exactly* `SqlBackend`. In the MTL style, we can declare a constraint on our query `Backend` (namely, `QueryRepCompatible SqlBackend`) and have automatic compatibility with a host of backend representations. This avoids having to manually use `withCompatibleQueryRep` everywhere (which, in the MTL analogy, would be like using `lift` all the time.)
I don't think every codebase will want these instances (we certainly don't), so they make sense to put in an orphan module to allow them to be opt-in.
While MonadQuery was generalised, MonadTransaction always required that the underlying backend was a SQL backend, which isn't quite accurate. This generalises that constraint.
This makes it simpler for other libraries to use the shim, since they don't have to manage import lists between Database.Persist.Sql and the shim module.
For the same reasons as MonadTransaction, these instances are made orphans so that they are opt-in. This is especially relevant because the ReaderT instance is incompatible with `SqlPersistT`.
There are two compelling reasons to bin `runMigrationSilent`
* It is the only function that constrains `runSqlQueryRep` to only run
in a `MonadUnliftIO`, since all the other API functions only require
`MonadIO`
* It is, by the admission of the Yesod docs, unsafe - it doesn't work
well with multiple threads, and it's not recommended for use.
`runMigrationQuiet` exists as a safe "replacement" that isn't quite
the same, but should be good enough for most uses
And since this is for our own use, we can be as opinionated as we like
in removing it.
This polymorphic helper is useful for cases where the current backend type is constrained to be compatible with a particular concrete backend, and the programmer wants to call a function which relies on compatibility which that *concrete backend* satisfies. This helps avoid having to constrain the backend type twice when one of the constraints is a strict superset of the other - in particular, when constraining by compatibility with two concrete types which are themselves compatible. This came up for us when switching over to `RawPostgresql`, since the streaming functions constrained the backend to compatibility with that wrapper but also wanted to call `MonadSqlQuery` functions that constrained to compatibility with the inner `SqlBackend`. Note that this newtype can't be replaced with an ambiguous typeclass instance, both because of coherence issues and because the compiler would have no guidance on how to direct its type search. Because `QueryRepCompatible` is so generic, we need to add info for the compiler to use.
Way back in a1bd298, I wanted to generalise MonadSqlQuery so we could use it with different backend types, which I eventually did by making the backend an associated type family and having each backend declare its own QueryRep. This worked, but it wasn't very neat - MonadQuery instances only really declared a Backend so that they would have a particular QueryRep for that backend, and there were no other laws governing how the Backend behaved. Worse still, MonadQuery instances were then *fixed* to that Backend type, no matter what they actually wanted to do with the QueryRep values in runQueryRep. This simplifies the polymorphism by tying MonadQuery instances to their QueryReps directly. The change I made to have QueryRep be an associated data family has been reverted, and the SqlQueryRep type is its own data type again instead of a type synonym. This is because (as it turns out) we want to be able to share QueryReps between MonadQuery instances, so e.g. we can run the same query rep against different backends.
We only depend on a very small and stable part of these packages, so we can relax the bounds to the newest versions.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the branch supercede is running on...
which never got upstreamed so now you've to read code to figure out how this works.