Update to Aff 4.0 #19

tonyd256 · 2017-10-06T03:18:56Z

This updates the dependencies and code to Aff 4.0. There were some breaking changes that Aff no longer supports the success/failure callback method. There is still support under Control.Monad.Aff.Compat using EffFnAff and fromEffFnAff. The update was replacing the old Affs with EffFnAff, switching the success and error functions around, and removing the withClient function as it seemed like a duplicate of withConnection. This builds on the changes made in #18 and fixes #17.

natefaubion · 2017-10-06T03:51:44Z

src/Database/Postgres.js

@@ -4,40 +4,21 @@
 var pg = require('pg');

 exports["connect'"] = function (conString) {
-  return function(success, error) {
+  return function(error, success) {
    var client = new pg.Client(conString);
    client.connect(function(err) {
      if (err) {
        error(err);
      } else {
        success(client);
      }


Note, that these functions still need to return something for a canceler, even if it just invokes a callback (please consult the example). If it's possible to cancel the effect, then it should be done so we don't accidentally leak resources. Does pg.Client support some way to close the pending connection?

For reference: https://github.com/slamdata/purescript-aff/blob/master/src/Control/Monad/Aff/Compat.purs#L35-L38

The canceler isn't optional, even through the FFI, and it's an important part of Aff's semantics.

Ok pushed one for connect' that I think is correct, but not sure about the query ones. I don't think those are cancellable .... does that mean to just call onCancelerSuccess?

If there is no way to cancel a pending request, then yes, the only thing that can be done is call onCancelerSuccess.

cool, I think this is all set then

There is, or at least these was a difference between withClient and withConnection. withClient would pull a connection from the connection pool and withConnection would make a new connection.

Now I'm looking at the pg documentation and they have a new pooling API so I'm not sure how client.connect works currently.

When we've taken a connection from the pool, and we cancel the Aff we should try to cancel the query and return the connection to the pool (there's a pending PR to add query canceling to pg brianc/node-postgres#1392). With single connections to the db without a pool we can just close the connection.

I can take a closer look at the pg API during the weekend, as I'm not sure the API we have here is relevant anymore.

@anttih What do you think about passing that off to the user? Like export the Client and Pool constructors and let the user of this lib choose which to use and pass them into the query functions?

@tonyd256 I'd like to keep it simple for now and concentrate on Pool only. That means we would expose the Pool constructor with something like mkPool :: PoolConfig where the PoolConfig would have all the config flags available. I think we can ditch the connectionstring for now.

We could then replace withClient with connect:

connect :: forall eff a . Pool -> (Client -> Aff (db :: DB | eff) a) -> Aff (db :: DB | eff) a

What do you think? And thanks for working on this!

@anttih I think focusing on Pool makes sense. I'd like to not ditch connection string though since that's the main way I'd like to interact with the api.

natefaubion · 2017-10-06T04:09:25Z

src/Database/Postgres.js

+    return function(cancelError, onCancelerError, onCancelerSuccess) {
+      client.end(function(err) {
+        if (err) {
+          onCancelerError();


This should be called with the err, but this is otherwise correct I think.

The types say this, but it should be documented in a better example.

tonyd256 · 2017-10-10T00:27:33Z

@anttih I did a bunch of work here to abstract the query functions using a type class. Since both client and pool have a query function it seems like they can be abstracted as such. This also switches to more complex connection info objects. I think they should probably all be Maybe values and have the ConnectionInfo type class change them to Data.Nullables. That would require a new dependency so wanted to OK it with you first.

anttih · 2017-10-10T05:38:29Z

Sorry, what I meant by concentrating on the Pool is to leave the Pool querying methods for now and only allow querying a Client. This way you must use a Pool and take a client from there, but I think it keeps the API a bit simpler. I don't think abstracting over any queryable is important for this library.

tonyd256 · 2017-10-10T18:07:44Z

Ya I figured that's not what you meant. I got excited and went overboard haha. I'll revert back to just the pool client. What do you think about the nullable settings though? A lot of the defaults for those settings are different types altogether (yay javascript) so we couldn't replicate the default in purescript.

tonyd256 · 2017-10-10T18:38:44Z

Ok updated.

One thing about using the pool is that we need to call end on it before we finish but since we're passing the client around we don't have reference to it. I attached it to the client as a property so we can properly dispose of it. Seems like we're not really using the power of pools here but it works.

anttih · 2017-10-10T19:04:18Z

I think we could separate ClientConfig and PoolConfig and maybe skip statement_timeout for now.

foreign import data ConnectionInfo :: Type

type ClientConfig =
  { host :: String
   , database :: String
   , port :: Int
   , user :: String
   , password :: String
   , ssl :: Boolean
  }

 type PoolConfig =
  {  connectionTimeoutMillis :: Int
   , idleTimeoutMillis :: Int
   , max :: Int
  }
 
defaultPoolConfig :: PoolConfig
defaultPoolConfig = { ... }

connectionInfoFromConfig :: ClientConfig -> PoolConfig -> ConnectionInfo
connectionInfoFromConfig = unsafeCoerce
    { -- all the props from both here...
    }

newtype ConnectionString = ConnectionString String

connectionInfoFromString :: ConnectionString -> ConnectionInfo
connectionInfoFromString cs = unsafeCoerce { connectionString: cs }

We could also have mkDefaultPoolConfig or something which would use the default PoolConfig so that you wouldn't have to provide them all every time.

anttih · 2017-10-10T19:12:02Z

@tonyd256 I think we need to turn the connect FFI function to a version which calls a callback with the connection and the done callback, then we can use makeAff or whatever to turn that into an Aff.

tonyd256 · 2017-10-10T20:28:26Z

Ok updated the config building code. I'm not clear on how to do what you're saying with the callback and done function. We're already using Aff with connect'. I don't think you're suppose to use callbacks with it like that anymore although I'm not really certain.

anttih · 2017-10-12T19:08:21Z

@tonyd256 Actually we don't have to use any callbacks, all we need to do is call release on the client. You are now calling client.release in end, but end disconnects all clients. withClient needs to only release the client so that the same client can be used again later. We should have

foreign import release :: Client -> Eff _ Unit

which we can use in bracket.

tonyd256 · 2017-10-12T20:03:16Z

@anttih I think we still need to end the pool at some point right? Every time we call connect or withClient it creates a new pool and returns a single client from it. We're never closing that pool so we'll keep creating new pools and not closing them. I don't think this is good. Should the pool be a global variable that we pull clients from?

anttih · 2017-10-12T20:08:48Z

Ah, right. I think se should create the pool separately and pass into withClient. Then have separate end which should be called “manually” once you’re done.

tonyd256 · 2017-10-12T20:12:01Z

Gotcha, I'll make some changes then.

tonyd256 · 2017-10-12T21:21:57Z

Ok, what do you think about this?

anttih

Looks good! Just a few comments.

anttih · 2017-10-14T14:47:00Z

src/Database/Postgres.purs

-withClient :: forall eff a
-  . ConnectionInfo -> (Client -> Aff (db :: DB | eff) a) -> Aff (db :: DB | eff) a
-withClient c p =
+withPool :: forall eff a


We can still use the name withClient here. It tries to describe that your callback is called with the Client that we are pulling out from the Pool.

anttih · 2017-10-14T15:12:10Z

src/Database/Postgres.purs

-  <> ci.host <> ":"
-  <> show ci.port <> "/"
-  <> ci.db
+mkDefaultPoolConfig :: ClientConfig -> ConnectionInfo


Let's actually remove this for now. I think the API of calling either connectionInfoFromString ... or connectionInfoFromConfig {...} defaultPoolConfig is good enough.

anttih · 2017-10-14T15:13:39Z

test/Main.purs

@@ -1,14 +1,13 @@
 module Test.Main where

-import Prelude
+import Prelude (class Eq, class Show, Unit, bind, const, discard, pure, show, unit, ($), (<$>), (<*>), (<>), (=<<), (>>=))


Was there a particular reason for this? You can just open import this like it was, PS allows one open import without any warnings.

ha I didn't know that. It must have given me an error at some point when I had another import in there.

tonyd256 · 2017-10-16T17:13:13Z

I'm finding that I don't like this API. I want to do this withClient (mkPool connectionInfo) do .... but then the pool is gone and I never end it. It seems like every time I want to use this I need to create the pool before hand, run my queries, end the pool, return my results. This seems like a lot of back and forth. What about a separate function that closes the pool when finished? so users can choose between taking advantage of the pool or have the convenience of not having to close it.

tonyd256 · 2017-10-16T21:24:53Z

Wrapping in another bracket does work fine. Not sure if you'd like a convenience function for that, otherwise it's usage will be similar to:

bracket
  (liftEff $ mkPool connectionInfo)
  (liftEff <<< end)
  (_ `withClient` (\c-> do
    -- do stuff here
  ))

or with a transaction

bracket
  (liftEff $ mkPool connectionInfo)
  (liftEff <<< end)
  (_ `withClient` withTransaction (\c-> do
    -- do stuff here
  ))

Once we nail down the API I'll make sure to update the docs too.

tonyd256 · 2017-10-18T17:50:21Z

@anttih Any thoughts on what's left here? We're so close to getting this done :)

anttih · 2017-10-18T18:55:58Z

@tonyd256 what is your use case for these ad-hoc queries? It seems to me that the most common use case is in long running processes where you would always have a pool handy.

tonyd256 · 2017-10-18T19:02:47Z

For our API we want a transaction per request. This is useful for rollback if anything in that request fails. One transaction means one client. We don't persist the pool over the API application.

anttih · 2017-10-18T19:10:56Z

Ok, maybe just call it withConnection and have it call mkClient with the client config, connectClient and then endClient. I think we should use the Client API for this and not create the pool at all.

anttih · 2017-10-18T19:11:56Z

So we could have withConnection :: ClientConfig -> (Client -> Aff _ a) -> Aff _ a

tonyd256 · 2017-10-18T19:12:05Z

So after a chat with coworkers we do want to persist that so you're right.

anttih · 2017-10-18T19:12:48Z

Yes, most likely you want to persist it if it's any way possible.

tonyd256 · 2017-10-18T19:12:56Z

Would you like me to bring back withConnection?

anttih · 2017-10-18T19:13:53Z

No, I think we should just have the Pool-based API until someone really wants it.

anttih · 2017-10-19T19:17:45Z

Thanks a lot!

Upgrade to Aff 4.0

274110c

natefaubion reviewed Oct 6, 2017

View reviewed changes

This was referenced Oct 11, 2017

Put together an Aff 4.0 package set purescript/package-sets#111

Merged

Upgrade to Aff 4.0 #18

Closed

anttih requested changes Oct 14, 2017

View reviewed changes

Update code to use new Aff 4.0 APIs

86ae9a5

tonyd256 force-pushed the td-aff-4 branch from 8befb0b to 86ae9a5 Compare October 16, 2017 03:20

anttih merged commit 9862cba into epost:master Oct 19, 2017

Update to Aff 4.0 #19

Update to Aff 4.0 #19

Uh oh!

Conversation

tonyd256 commented Oct 6, 2017

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!

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!

tonyd256 commented Oct 10, 2017

Uh oh!

anttih commented Oct 10, 2017

Uh oh!

tonyd256 commented Oct 10, 2017

Uh oh!

tonyd256 commented Oct 10, 2017

Uh oh!

anttih commented Oct 10, 2017

Uh oh!

anttih commented Oct 10, 2017

Uh oh!

tonyd256 commented Oct 10, 2017

Uh oh!

anttih commented Oct 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tonyd256 commented Oct 12, 2017

Uh oh!

anttih commented Oct 12, 2017

Uh oh!

tonyd256 commented Oct 12, 2017

Uh oh!

tonyd256 commented Oct 12, 2017

Uh oh!

anttih 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!

tonyd256 commented Oct 16, 2017

Uh oh!

tonyd256 commented Oct 16, 2017

Uh oh!

tonyd256 commented Oct 18, 2017

Uh oh!

anttih commented Oct 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tonyd256 commented Oct 18, 2017

Uh oh!

anttih commented Oct 18, 2017

Uh oh!

anttih commented Oct 18, 2017

Uh oh!

tonyd256 commented Oct 18, 2017

anttih commented Oct 12, 2017 •

edited

Loading

anttih commented Oct 18, 2017 •

edited

Loading