Skip to content
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

Enable value-level codecs #61

Closed
wants to merge 25 commits into from
Closed

Enable value-level codecs #61

wants to merge 25 commits into from

Conversation

JordanMartinez
Copy link
Contributor

Fixes #60

Currently a WIP. I've opted to make PostgreSQL work first before working on SQLite3.

@@ -46,7 +46,7 @@ in upstream
, "validation"
]
, repo = "https://github.com/jordanmartinez/purescript-postgresql-client.git"
, version = "updateTov0.14.1"
, version = "exposeUnsafeQuery"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is using my fork temporarily.

@@ -47,5 +47,5 @@ You can edit this file as you like.
, "variant"
]
, packages = ./packages.dhall
, sources = [ "src/**/*.purs", "test/**/*.purs", "guide/src/**/*.purs" ]
, sources = [ "src/**/*.purs" ]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be reverted before a final PR is merged.

→ FullQuery B { | i }
→ (Array Foreign -> Either String { | o })
→ Aff (Either PGError (Array { | o }))
query' conn q decodeRow = runSelda conn $ Selda.PG.query' q decodeRow
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eta-reducing these produces compiler errors.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you mean? there's $ so the term cannot be eta-reduced

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I was thinking query' conn = runSelda conn <<< Selda.PG.query'. It's been so long since I worked on this, I'm not sure anymore.

, query1
, query1'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The appended ' indicates value-level codecs are used. I think a different suffix should be used that's clearer (e.g. V for value).

import Type.Proxy (Proxy)

class GenericQuery ∷ ∀ k. k → (Type → Type) → Row Type → Row Type → Constraint
class Monad m <= GenericQuery b m i o | i → o, b → m where
genericQuery
∷ Proxy b
→ FullQuery b { | i }
→ (Array Foreign -> Either String { | o })
→ m (Array { | o })
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the addition of this new argument, I'm not sure what the order of the arguments should be.

genericQuery
∷ Proxy b
→ FullQuery b { | i }
→ (decodeInput -> decodeOutput { | o })
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is necessary because different backends have different ways of decoding things.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these differences are not that serious - the common input could be just Foreign which is convertible to Array Foreign when needed. Similarly the error monad decodeOutput which could be just m

@JordanMartinez
Copy link
Contributor Author

Ok. Pretty sure this code works, but testing it is going to be it's own major pain. I essentially need to change testWith_ to include a decodeRows argument, so that I can pass that into genericQuery when testQueryWith_ is called. However, generating the ReadForeign / FromSQLRow codecs is difficult.

@JordanMartinez
Copy link
Contributor Author

I couldn't figure out how to update the tests, so that a test is defined in only one location and can tested against multiple backends. While I get why you're doing this, I think it's a better tradeoff to define tests separately for each backend.

I'd like to proceed in that direction (i.e. duplicating Common.purs for each backend and implementing each on separately). I'm going to stop working on this PR until I get some feedback from you. Otherwise, if I make such a change, you might not merge the PR.

@Kamirus
Copy link
Owner

Kamirus commented Jun 15, 2021

I'd really want to keep Common.purs in one place, otherwise these duplicates will surely go out of sync.
I'll try to figure out a solution and give you feedback about your comments as soon as I can.

@JordanMartinez
Copy link
Contributor Author

Ok. I've removed the last commit that started to duplicate Common across each backend.

@Kamirus
Copy link
Owner

Kamirus commented Jul 14, 2021

Nice work @JordanMartinez , I'm not yet ready to push this any further, but I have some insights.

  • I see this as an opportunity to simplify how queries are executed (or any SQL statements) - making the whole process more abstract is one thing and simplifying some type classes is the other.
  • type classes like GenericQuery are there for two reasons:
    1. to hide complicated constraints from the user, e.q. so that the type of the query consists of one constraint without any intermediate type parameters
    2. overloading convenience
  • but the user should not be forced to use them
  • we should provide common query variations that could be used by current type classes and users directly. I've included examples that should illustrate my point
primGenericQuery 
   s a i.
  GetCols i 
  String   -- query parameter placeholder, e.g. '$' for pg, and '?' for sqlite
  Int      -- first query parameter index, usually `1`
            -- both of these parameters determine how we generate query
            -- parameter names, e.g. '$1', '$2', ... for pg
  (String  Array Foreign  a) 
  FullQuery s { | i } 
  a
primGenericQuery ph i exec q = do
  -- transform Query AST `q` into a query string and query parameters
  let { strQuery, params } = showM ph i $ showQuery q

  -- execute the query with the parameters
  exec strQuery params

pgPrimGenericQuery 
   s a i m.
  GetCols i              -- needed by query string generation
  MonadSeldaPG m         -- PG specific monad constraints
  (Array Foreign  m a)  -- generic decoder, errors are handled by monad `m`
  FullQuery s { | i }    -- query AST
  m (Array a)             -- result rows of output records, record type `{ | o }` is determined by `i` via the type-function
pgPrimGenericQuery decodeRow = primGenericQuery "$" 1 exec
  where
  exec strQuery params = do
    conn ← ask
    errOrResult ← liftAff $ PostgreSQL.unsafeQuery conn strQuery params
    result ← either throwError pure errOrResult
    traverse decodeRow result.rows

pgGenericQuery 
   s o i m.
  GetCols i 
  MapR UnCol_ i o              -- type-level function that maps over `{ | i }`
                                -- and removes `Col s` from each record member
                                -- thus `o` is now determined by the input `i`
  MonadSeldaPG m 
  (Array Foreign  m { | o }) 
  FullQuery s { | i } 
  m (Array { | o })             -- the additional constraint forces the
                                -- appropriate result type
pgGenericQuery = pgPrimGenericQuery

@JordanMartinez
Copy link
Contributor Author

@Kamirus I'm not likely to get to this anytime soon. But, perhaps those changes your mentioning should be done in a separate PR first before this one would be continued?

@Kamirus
Copy link
Owner

Kamirus commented Jul 22, 2021

Yes, no problem, I just wanted to write these ideas here so we don't lose them.
You're right - ideally in this separate PR we would only add these alternative query operations without changing the old api for now

@JordanMartinez
Copy link
Contributor Author

I've opened #64 to track that idea.

@JordanMartinez
Copy link
Contributor Author

I don't foresee getting back to this anytime soon, so I'm going to close this PR. I won't delete my branch though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable value-level codecs to be used instead of type-level codecs
2 participants