add an optional pgColumnFilter for the columns plugin #73

mathroc · 2017-09-14T20:59:48Z

it works very nicely for the use case I described in #65 :)

closes #65

benjie · 2017-09-15T07:46:20Z

Looks like you need to run "eslint --fix" (I do this automatically on save in my editor)

benjie

Awesome! Fix linting, check the type, and add a test or two (probably in the form of an acceptance test on postgraphile-core) and we should be good to go.

Though we should probably apply this to the column filters and the order by too, come to think of it...

benjie · 2017-09-15T07:49:34Z

packages/graphile-build-pg/src/plugins/PgColumnsPlugin.js

@@ -151,6 +150,7 @@ export default (function PgColumnsPlugin(
        fields,
        introspectionResultsByKind.attribute
          .filter(attr => attr.classId === table.id)
+          .filter(attr => pgColumnFilter(attr, build, Object.assign({}, context, {type: "GraphQLObjectType"})))


I can't expand on mobile but is this one meant to be GraphQLInputObjectType?

correct, it’s fixed

mathroc · 2017-09-15T14:52:29Z

I’ve fixed most of the linting issue but I don’t know how to fix the flow lint errors :/ do you have hints on this ?

Awesome! Fix linting, check the type, and add a test or two (probably in the form of an acceptance test on postgraphile-core) and we should be good to go.

ah yes I didn’t find tests in this package and didn’t look on the other packages. do you have somewhere in mind for the new tests ? as in, inside an existing test file (I didn’t find one related to building schemas) or just another file next to packages/postgraphile-core/tests/integration/queries.test.js ?

benjie · 2017-09-15T15:37:21Z

do you have somewhere in mind for the new tests ?

You're probably going to want a schema snapshot test to ensure that the fields are indeed missing:

https://github.com/graphile/graphile-build/blob/master/packages/postgraphile-core/__tests__/integration/schema.test.js

It'd also be worth adding some query tests to ensure that it doesn't break anything; for that you could add an alternative createPostGraphQLSchema instance that omits some fields in here, assuming you can omit fields that won't break the queries:

https://github.com/graphile/graphile-build/blob/23ecc372c7c81b2d322d7a31fac5d1090c302bab/packages/postgraphile-core/__tests__/integration/queries.test.js#L31-L41

Failing that make a separate query test file like I do for queries-jwt.test.js

I don’t know how to fix the flow lint errors

I can't expand on this right now, sorry!

mathroc · 2017-09-15T22:00:56Z

ok, I’ve fixed the travis build, can you take a look at the diff again ? also, I had expose the option to createPostGraphQLSchema, is that what you intented ? I don’t use postgraphql-core so I don’t if it’s relevant

benjie · 2017-09-15T23:24:33Z

From a glance over this all looks great 👍 I'm probably not going to be able to merge it until Tuesday. If it's not merged by Friday please give me a nudge ☺️

mathroc · 2017-09-22T12:50:55Z

ping ☺

benjie

This PR is great! I want to make a few changes before I merge it though:

You shouldn't need to do the Object.assign - I'm going to move this to the core engine
I want to call out to this in other contexts too: the column filters, the orderBy props, etc

I want to do these changes before merging because I don't want to discover that the API doesn't quite suit these other use cases and have to break the API (or introduce a second API).

I'm happy to make these changes myself but I'm not sure when I'll get to them.

benjie · 2017-09-22T13:04:17Z

packages/graphile-build-pg/src/plugins/PgColumnsPlugin.js

+          pgColumnFilter(
+            attr,
+            build,
+            Object.assign({}, context, { type: "GraphQLObjectType" })


I want to make this Object.assign unnecessary by moving it to core.

mathroc · 2017-09-22T13:29:14Z

I’ll let you do the first point ( I probably won’t have time to work much on this until next month )
but if you didn’t take care of he second point then, I can work on it. (this means other type value will be available on context for columns, orderBys, etc, right ?)

I also have use cases to filter orderBys (eg: I want to remove order by on uuid v4)

and as we’re talking about filtering, I also would like to remove some relations added by the PgBackwardRelationPlugin

benjie · 2017-09-22T18:29:40Z

Hopefully that first part is handled by #79 👍 (Still need to remove Object.assign from this PR though - it shouldn't be necessary any more)

(this means other type value will be available on context for columns, orderBys, etc, right ?)

Type will be, but it'll just be the object type (GraphQLInputObjectType for orderBy, conditions, etc). But because we pass the whole of context through hopefully there will be other useful things to filter it by.

and as we’re talking about filtering, I also would like to remove some relations added by the PgBackwardRelationPlugin

I think this would probably be a different function (separate PR).

closes graphile#65

mathroc · 2017-09-28T16:48:44Z

Hi,

I’ve made some progress here, tell me what you think. I’m not sure about the callback function name anymore

benjie

This looks good to me; I'm going to need to spend a few extra minutes at some point checking that all the places we use pgIntrospection.attributes use pgColumnFilter if necessary; however I don't expect to find any issues.

Thanks! 🙏

benjie · 2017-09-28T18:52:06Z

packages/graphile-build-pg/src/plugins/PgConnectionArgCondition.js

@@ -1,67 +1,66 @@
 // @flow
 import type { Plugin } from "graphile-build";
+
+const defaultPgColumnFilter = (_table, _build, _context) => true;


I think that's meant to be _attr rather than _table?

I wasn’t sure about this one, the argument, at call time, is table in this plugin (and in PgConnectionArgOrderBy.js too) should I call them _attr in defaultPgColumnFilter regardless ?

Ah! I clearly wasn't reading carefully enough - that explains your concerns calling it pgColumnFilter!

We only want to filter the columns in this PR; so every time we call pgColumnFilter we should be passing an attr. The orderBy columns come from https://github.com/graphile/graphile-build/blob/master/packages/graphile-build-pg/src/plugins/PgOrderAllColumnsPlugin.js

ah yes, I didn’t take enough time figuring out how this plugin worked, maybe you can double check the 2 calls I made to pgColumnFilter in PgConnectionArgOrderBy as I’m not confident I did the right thing™ there either

benjie · 2017-09-28T18:56:38Z

Please don't rebase / squash / --amend again now - just add commits. We'll be doing a squash and merge when it goes out but I'd rather not have to read through the entire commit again when reviewing!

benjie · 2017-09-28T20:44:38Z

packages/graphile-build-pg/src/plugins/PgConnectionArgCondition.js

+    introspectionResultsByKind.class
+      .filter(table => table.isSelectable)
+      .filter(table => !!table.namespace)
+      .filter(table => pgColumnFilter(table, build, context))


This is filtering in the wrong place, we should be filtering the condition fields in the section below 👇

benjie · 2017-09-29T08:54:53Z

packages/graphile-build-pg/src/plugins/PgConnectionArgOrderBy.js

      if (
        !isPgConnectionField ||
        !table ||
        table.kind !== "class" ||
        !table.namespace ||
-        !table.isSelectable
+        !table.isSelectable ||
+        !pgColumnFilter(table, build, context)


benjie · 2017-09-29T08:55:18Z

Sterling work! Keep it up 👍

mathroc · 2017-09-29T20:29:24Z

I’ve fixed the tests, removed the filter call from PgConnectionArgOrderBy.js and add a few // todo where I think the column filter could be called, please tell me if they’re appropriate places and if I missed some. maybe in PgMutationUpdateDeletePlugin.js & PgMutationCreatePlugin.js too ? if you know where it’ll save me some time

benjie · 2017-10-05T08:38:01Z

I've started looking through this, I'm making a few small adjustments. I ran out of time now, but I'll take a further look tomorrow.

benjie · 2017-10-13T09:11:14Z

@mathroc Sorry for the delay on this, life has been really hectic!

I've just pushed a commit that uses pgColumnFilter in more contexts and fixes a few of the contexts it was being passed before. It's quite a big change but so long as we treat this as not a supported feature just yet I'm happy to release it - we might change it slightly in future to make sure it works consistently. Before it gets official support it's going to need a lot more tests doing things in a lot more interesting ways (e.g. preventing ordering by a column but nothing else; preventing a column appearing in the input type but nothing else; etc)

benjie · 2017-10-13T09:12:57Z

GitHub seems to be having some issues right now - my commit is not showing up everywhere yet.

benjie · 2017-10-13T10:42:08Z

Great; it's working now - tests passed 👍

I'd love some more comments on this PR before it gets merged - especially from people willing to give it a try (or even write more tests for it).

benjie · 2017-10-13T10:42:55Z

(Also @mathroc if you can confirm the latest code still works for your use case that'd be great - since you raised the PR from your master branch I had no choice but the push the commits there - I hope that hasn't caused you any issues?)

mathroc · 2017-10-13T11:55:06Z

don’t worry, I made the branch editable just for that. I’ll give this new version a try next week

mathroc · 2017-10-16T17:52:48Z

I just updated with this PR and it’s working nicely ☺

benjie · 2017-10-16T18:59:36Z

Awesome 👍

I'd love to hear from anyone else interested in using this functionality if it works for them or not.

closes graphile#65

benjie

The TODOs might need resolving; will have to come back and review again later.

Other than that, I'm planning to merge this as an EXPERIMENTAL and UNSTABLE feature. I reserve the right to break it at a later time until I add it to the docs.

I wonder if we should use unstable_ prefix like React does? 🤷‍♂️

benjie · 2017-10-28T11:10:14Z

packages/graphile-build-pg/src/plugins/PgJWTPlugin.js

@@ -61,6 +61,7 @@ export default (function PgJWTPlugin(
        throw new Error("JWT type has already been overridden?");
      }
      const attributes = introspectionResultsByKind.attribute
+        // TODO: consider adding to pgColumnFilter?


benjie · 2017-10-28T11:16:12Z

packages/graphile-build-pg/src/plugins/PgOrderByPrimaryKeyPlugin.js

@@ -15,6 +15,7 @@ export default (function PgOrderByPrimaryKeyPlugin(builder) {
      const attributes = introspectionResultsByKind.attribute.filter(
        attr => attr.classId === table.id
      );
+      // todo


mathroc · 2017-10-28T11:53:23Z

I don't mind an _unstable prefix and completely agree that you should break the compatibility as needed. I won't have time to work on this or remaining todos right away though

benjie · 2017-10-28T14:10:32Z

I think we'll just say that it's undocumented and therefore experimental for now. Might rename it if I don't get a chance to test it fully before the v4 release.

All good, as soon as the tests pass I'm going to merge it 👍

benjie requested changes Sep 15, 2017

View reviewed changes

mathroc force-pushed the master branch 4 times, most recently from 95d998d to f3efc27 Compare September 15, 2017 12:09

mathroc force-pushed the master branch 4 times, most recently from 2b9fdf0 to 86a295b Compare September 15, 2017 21:46

benjie mentioned this pull request Sep 17, 2017

Is there a way to remove id from creation mutations? graphile/crystal#401

Closed

benjie requested changes Sep 22, 2017

View reviewed changes

benjie mentioned this pull request Sep 22, 2017

needs foreign table info in inflector.manyRelationByKeys #77

Closed

mathroc force-pushed the master branch 3 times, most recently from cbac06e to 7c1386c Compare September 28, 2017 16:04

add an optional pgColumnFilter for the columns plugin

1d141dd

closes graphile#65

mathroc force-pushed the master branch from 7c1386c to 1d141dd Compare September 28, 2017 16:35

benjie reviewed Sep 28, 2017

View reviewed changes

fixup! add an optional pgColumnFilter for the columns plugin

4b8f640

benjie reviewed Sep 29, 2017

View reviewed changes

mathroc added 3 commits September 29, 2017 22:05

fixup! add an optional pgColumnFilter for the columns plugin

49e8038

fixup! add an optional pgColumnFilter for the columns plugin

5061dd9

fixup! add an optional pgColumnFilter for the columns plugin

2505d7d

mathroc mentioned this pull request Oct 11, 2017

Order by column on related table [plugin available] #88

Closed

benjie added 2 commits October 13, 2017 10:05

Use pgColumnFilter in more contexts

ba64b92

Fix lint

8b09acf

benjie mentioned this pull request Oct 17, 2017

Make nodes array items in Pg Connection non null. #89

Closed

mathroc and others added 2 commits October 28, 2017 11:53

add an optional pgColumnFilter for the columns plugin

6ba8b69

closes graphile#65

Merge branch 'master' into pgColumnFilter

cd2320d

benjie requested changes Oct 28, 2017

View reviewed changes

benjie added 2 commits October 28, 2017 12:28

Merge remote-tracking branch 'mathroc/master' into pgColumnFilter

1964f18

Prettier

b5480bd

benjie added 2 commits October 28, 2017 15:08

Remove unnecessary todo

e412d8c

Update test snapshots

096796b

benjie approved these changes Oct 28, 2017

View reviewed changes

benjie merged commit 0fe8cfb into graphile:master Oct 28, 2017

benjie mentioned this pull request Feb 17, 2018

Grant select certain columns caused permission denied graphile/crystal#549

Closed

Uh oh!

add an optional pgColumnFilter for the columns plugin #73

add an optional pgColumnFilter for the columns plugin #73

Uh oh!

Conversation

mathroc commented Sep 14, 2017

Uh oh!

benjie commented Sep 15, 2017

Uh oh!

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

mathroc commented Sep 15, 2017

Uh oh!

benjie commented Sep 15, 2017

Uh oh!

mathroc commented Sep 15, 2017

Uh oh!

benjie commented Sep 15, 2017

Uh oh!

mathroc commented Sep 22, 2017

Uh oh!

benjie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mathroc commented Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjie commented Sep 22, 2017

Uh oh!

mathroc commented Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

benjie commented Sep 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjie commented Sep 29, 2017

Uh oh!

mathroc commented Sep 29, 2017

Uh oh!

benjie commented Oct 5, 2017

Uh oh!

benjie commented Oct 13, 2017

Uh oh!

benjie commented Oct 13, 2017

Uh oh!

benjie commented Oct 13, 2017

Uh oh!

benjie commented Oct 13, 2017

Uh oh!

mathroc commented Oct 13, 2017

Uh oh!

mathroc commented Oct 16, 2017

Uh oh!

benjie commented Oct 16, 2017

Uh oh!

benjie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mathroc commented Sep 22, 2017 •

edited

Loading

mathroc commented Sep 28, 2017 •

edited

Loading