Skip to content
This repository has been archived by the owner on Mar 4, 2019. It is now read-only.

Connection wrapper - e.g. to allow row level security #295

Closed
wants to merge 5 commits into from

Conversation

benjie
Copy link
Contributor

@benjie benjie commented Jul 30, 2016

Fixes #291

Rebased upon #297; to see just the changes in this (and not the refactoring): benjie/massive-js@refactor-index...connection-wrapper

The aim of this PR is to enable a new newDb = db.withConnectionWrapper(fn) API that allows users to do interesting things by creating a fast clone of the massive instance, but wherein each query is wrapped with custom functionality.

The functionality could be, for example:

  • calling SET LOCAL TIME ZONE 'Europe/London' before some queries and SET LOCAL TIME ZONE 'America/New_York' on others
  • automatically rolling back every query by wrapping it with begin; ... rollback;
  • switching to a different role before executing a particular function
  • implementing something a bit more complex, like my specific target: row level security functionality (as demonstrated by the few tests I added)

This PR is a proof-of-concept; I think it's safe to merge but we probably ought to add more tests before documenting it as a public API!

To achieve the goal of a connection wrapper, this PR does the following things:

@benjie benjie force-pushed the connection-wrapper branch 3 times, most recently from 92238cf to 7f716f4 Compare July 30, 2016 06:14
@dmfay
Copy link
Owner

dmfay commented Jul 30, 2016

Looks like I gave you a bunch of conflicts from revising test setup and teardown -- at least now you can just define a testing schema for your stuff and not have to worry about editing other testcases!

Some of this is a bit primitive, yeah. It'd be cool to build wrappers instead of requiring users to mess around with pgClient themselves, but that's fine for a non-public API. The big sticking point for me is spinning up a whole new connection pool: I'm not sure there's a better way until promises come in (which afaik is still more or less in limbo), and you've lightened up the connection load as much as possible by passing in the existing resources, but I'm just generally a little wary of the approach. @robconery or @xivSolutions probably know more about node-pg's connection mechanics than I do so could say better whether it'll hold up.

@benjie
Copy link
Contributor Author

benjie commented Jul 31, 2016

I read through pg's source with the same fears before implementing; but it turns out that pg.connect implements the pool based on the config, and this doesn't change the config so it shouldn't cause any more pools - everything will continue to use the same one as before so long as the connectionString doesn't change.

@benjie
Copy link
Contributor Author

benjie commented Aug 1, 2016

I've rebased this on #297 and updated the description at the top.

@benjie
Copy link
Contributor Author

benjie commented Jan 17, 2017

Rebased off of #297 which was just rebased off master

@jmealo
Copy link

jmealo commented Apr 3, 2017

I'll need this for my RLS setup as well. Anything I can do to help get this merged?

@dmfay
Copy link
Owner

dmfay commented Apr 3, 2017

Fix the conflicts first up; functionality-wise, the only critique I really have is that I think it should throw in newDb.connectionWrapper instead of falling back to oldHandler. If initializing the wrapper failed I'd want to know about it, instead of it quietly running whatever I wanted it to run with alternative settings. Especially given the intended first use for RLS -- when those settings mean you're running a query with different privileges, that could be a big problem.

I'll merge with those taken care of. Pull requests against the promises branch are welcome too! All my free time for the next little bit has gotten sucked into preparing for a talk, but I've made a ton of progress towards 3.0 there. It's strictly in "tests pass" rather than "dropped into a moving system" shape, but once I figure out how I want to generate API docs and rework the handwritten documentation it should be basically at parity with 2.6, and then the fun really starts.

@benjie
Copy link
Contributor Author

benjie commented Apr 4, 2017

I'm unavailable to work on this for about 5 weeks, but if you wanted to set up a PR into my PR @jmealo you'd be very welcome 👍

@dmfay
Copy link
Owner

dmfay commented May 26, 2017

@benjie @jmealo I've just switched master over to v3 development. I'll still merge this if either of you would like to continue it, but you'll need to change the base branch to v2.

@sammoore
Copy link

In case anyone lands here, I'm working on a solution similar to @benjie's to add this kind of support to v3. I'll be opening a PR once I have something more concrete.

@dmfay
Copy link
Owner

dmfay commented Jul 10, 2017

@sammoore worth noting that pg-promise added support for multiple connection pools which could be helpful doing something like this. I did a little experimenting with proxies to try to invoke the same attached objects using different pools in the pgp-beta branch, but haven't gotten too far with it as yet.

@dmfay
Copy link
Owner

dmfay commented Sep 6, 2017

Closing since this has been inactive for ages, but anyone interested in getting this functionality into v2 should feel free to open a new one.

@dmfay dmfay closed this Sep 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants