SqlReadBackend and SqlWriteBackend are used by clients of the library. I know they're used extensively by Freckle, so as long as those types are still working good well after these changes then we should be good to go.

SqlReadBackend and SqlWriteBackend are used by clients of the library. I know they're used extensively by Freckle, so as long as those types are still working good well after these changes then we should be good to go.

If someone can check with whatever uses it, that'd be great, because I have no idea how they'd be used looking at this library alone.

@akurilin I suggest building this against your org repo. I can't do it for you. I remember y'all using those types too but I have no idea where things are at now.

I'm gonna write some tests for the feature.

Ok, well if the Read/Write types have a function, that still leaves the IsPersistBackend (and consequently IsSqlBackend) to not be used anywhere. If it is decided they are actually redundant, I can remove the definitions and instances of them.

Seeing as this change does shift almost all functionality to BackendCompatible, instead of HasPersistBackend, would that break people's programs?
It'd probably just give type errors when they update their persistent dependency, right?

The BackendCompatible stuff is used so that SqlReadBackend can be used for persistent entities that are defined for SqlBackend but only in read queries. Note that SqlReadBackend doesn't have any instances for the Write classes, so it can't do any updates to the DB. This is used to give certain parts of the program read-only access to the database. I wrote that class because the IsSqlBackend stuff was too restrictive for persistent-typed-db, where I had PersistEntityBackend record ~ SqlFor SomeDb.

I'd love it if the new changes were mostly "drop in." I think changing the functions to return a concrete Sqlbackend and then allowing clients to apply those functions manually will probably be a good thing - should make type inference a lot better and error messages easier to follow.

I think it may be a good idea to deprecate the IsSqlBackend-related stuff so we can give good deprecation warnings for an upgrade/migration path. ideally we only have one mechanism for saying "This backend is compatible with this other backend."

@akurilin I suggest building this against your org repo. I can't do it for you. I remember y'all using those types too but I have no idea where things are at now.

@jdreaver is taking a look 👍

I added some tests in #887 though they're not super complete.

Ok, so here's the dealio:

This is the chain:

wrapConnectionInfo :: SqliteConnectionInfo -> Sqlite.Connection -> LogFunc -> IO SqlBackend
wrapConnectionInfo = {the entire SqlBackend is constructed here}

open' :: SqliteConnectionInfo -> LogFunc -> IO SqlBackend
open' connInfo logFunc = do
    conn <- Sqlite.open $ _sqlConnectionStr connInfo
    wrapConnectionInfo connInfo conn logFunc `E.onException` Sqlite.close conn

withSqliteConnInfo :: (MonadUnliftIO m, MonadLogger m)
                   => SqliteConnectionInfo -> (SqlBackend -> m a) -> m a
withSqliteConnInfo = withSqlConn . open'

withSqlConn
    :: (MonadUnliftIO m, MonadLogger m, BackendCompatible SqlBackend backend)
    => (LogFunc -> IO backend) -> (backend -> m a) -> m a
withSqlConn open f = do
    logFunc <- askLogFunc
    withRunInIO $ \run -> bracket
      (open logFunc)
      close'
      (run . f)

runSqlite :: (MonadUnliftIO m)
          => Text -- ^ connection string
          -> ReaderT SqlBackend (NoLoggingT (ResourceT m)) a -- ^ database action
          -> m a
runSqlite connstr action = runResourceT . runNoLoggingT $
    withSqliteConn connstr $ runSqlConn action

runSqlConn takes the action and will run it with whatever the open argument to withSqlConn is. In this case, that's the SqlBackend from open'. (withSqliteConn is just withSqliteConnInfo, but the Text is first turned into a SqliteConnectionInfo)
I honestly don't see a way to make that SqlBackend into anything else? I don't see a way to get around the fact that the database action ReaderT will receive a SqlBackend. What one does with it is up to them, I guess.

Okay, let's land this PR, then get the SQLite one working, and I'll give these open-etc functions a try to see if I can get them generalized without borking any expected functionality.

I tried to test on my work codebase but I need a new version of persistent-typed-db for that to work out :) May get that done next week if @jdreaver hasn't reported back.

EDIT: This builds fine on the work codebase, however, we don't use SqlReadT or SqlWriteT anywhere, so not a great point of comparison. I'll try it on esqueleto too, which does use it.

Esqueleto works with this PR.

@Vlix There are some merge conflicts from your other PR. Let's get those resolved and merge this in. Freckle and folks can test on master and if it's not working then we can figure out how to work around it there.

I think it may be a good idea to deprecate the IsSqlBackend-related stuff so we can give good deprecation warnings for an upgrade/migration path. ideally we only have one mechanism for saying "This backend is compatible with this other backend."

Agreed and I'm very happy the (more flexible) BackendCompatible ends up the final winner with this PR :)

@Vlix once this PR is merged I'll try and make some time to revisit the SQLite bits you mentioned, since those needed changing in my other PR and since it's grossly out of date by now I need to revisit it anyway.

I'm going to get this PR finished up