graphile-contrib/postgraphile-plugin-connection-filter

Support for CIDR column types

higherorderfunctor opened this issue · 8 comments

higherorderfunctor commented

I am willing to work on this pending graphile/graphile-engine#520 getting accepted.

I wanted to check if common left hand side operations would be permitted in this plugin before starting to invest in a PR as I am not seeing any other examples.

I have use cases for filtering between IPv4/IPv6 addresses and filtering subnets of specific sizes.

Example README excerpt of the desired operators are below..

CidrAddress (cidr)

PostgreSQL GraphQL Description
>> contains: CidrAddress Contains the specified CIDR address.
>>= containsOrEqualTo: CidrAddress Contains or equal to the specified CIDR address.
<< containedBy: CidrAddress Contained by the specified CIDR address.
<<= containedByOrEqualTo: CidrAddress Contained by or equal to the specified CIDR address.
&& containsOrContainedBy: CidrAddress Contains or contained by the specified CIDR address.
family(...) = familyEqualTo: Int Address family equal to specified value.
masklen(...) = maskLengthEqualTo: Int Mask length equal to specified value.
masklen(...) <> maskLengthNotEqualTo: Int Mask length not equal to specified value.
masklen(...) IN (...) maskLengthIn: [Int] Mask length included in the specified list.
masklen(...) NOT IN (...) maskLengthNotIn: [Int] Mask length not included in the specified list.
masklen(...) < maskLengthLessThan: Int Mask length less than the specified value.
masklen(...) <= maskLengthLessThanOrEqualTo: Int Mask length less than or equal to the specified value.
masklen(...) > maskLengthGreaterThan: Int Mask length greater than the specified value.
masklen(...) >= maskLengthGreaterThanOrEqualTo: Int Mask length greater than or equal to the specified value.
mattbretl commented

👍 I support adding the core containment operators once graphile/graphile-engine#520 is merged. If you need to construct complex operands (e.g. family(...)), I think that should be done in a separate plugin to limit scope creep in this package. For example, the PostGIS operators live in a separate package: https://github.com/graphile-contrib/postgraphile-plugin-connection-filter-postgis.

Writing a small plugin for this purpose is straightforward. Here's an example:
#56 (comment)

In your case, you would just wrap the identifier with the appropriate SQL function (family(...) or masklen(...)).

higherorderfunctor commented

PR is merged but as an opt-in. I started working off my local version while waiting for a release with the commit, but had a few questions regarding design considerations.

  • Can I use the presence of pgUseCustomNetworkScalars from core to enable the operators?
  • Do you want me to do any version checking beyond some flag that enables the operators that doesn't require bumping the dependency version?
  • Am I okay to add a pg10 only schema, data, and tests for the macaddr8 type? It looks pretty straight forward for handling this in the test script and query test.
higherorderfunctor commented

Additionally, in regards to complex operands, I'm thinking it would better to expose unary operators as a sort of derived field/computed column. myIpAddress: CidrAddress would then also produce the fields myIpAddressFamily: Int and myIpAddressMaskLength: Int. These could then be used in both the output and this plugin. Are you aware of any similar examples at the plugin level that can wrap all columns of a type with built-in db functions and produce the corresponding fields?

mattbretl commented
  • Can I use the presence of pgUseCustomNetworkScalars from core to enable the operators?

We shouldn't need to add any code to this plugin to handle that option. When it's set to true, the core library will register the Postgres-to-GraphQL type map, and this plugin just references that type map.

  • Do you want me to do any version checking beyond some flag that enables the operators that doesn't require bumping the dependency version?

Not sure which versions you're referring to, but I don't think we'll need to add any version-specific workarounds. (Unless you're referring to the test scripts.. those will need to check for PG10+ when running a SQL script with macaddr8.)

  • Am I okay to add a pg10 only schema, data, and tests for the macaddr8 type? It looks pretty straight forward for handling this in the test script and query test.

Yes, but.. we're going to run into the fact that there's no released version of PostGraphile (core) to test against, so the new custom scalars aren't going to show up unless we test directly against master, which I don't want to do in this package. It probably makes sense to implement these network operators as separate package temporarily, similar to https://github.com/graphile-contrib/postgraphile-plugin-connection-filter-postgis. You could then set the repo's dev dependency to master (until 4.4.6 is released) and set up CI to only run tests against PG10+. No workarounds needed.

  • Are you aware of any similar examples at the plugin level that can wrap all columns of a type with built-in db functions and produce the corresponding fields?

I'm not aware of any, but we can certainly tackle that after the core operators are added.

higherorderfunctor commented

We shouldn't need to add any code to this plugin to handle that option. When it's set to true, the core library will register the Postgres-to-GraphQL type map, and this plugin just references that type map.

I think that will work, but it relies on alphabetical ordering of the keys to not break string operators. I want to be sure you are okay with this design?

// these default to string without option
const _CidrAddress = gqlTypeNameFromPgTypeName("cidr") || "CidrAddress";
const _MacAddress = gqlTypeNameFromPgTypeName("macaddr") || "MacAddress";

const connectionFilterScalarOperators = {
  [_CidrAddress ]: ... // sets string ops to be cidr ops
  [_MacAddress ]: ... // overwrites string ops to be macaddr ops
  [_String]: ...  // fixes string ops, but only works because string is later in
                  // the alphabet if maintaining ordering found in existing code
}

Not sure which versions you're referring to, but I don't think we'll need to add any version-specific workarounds. (Unless you're referring to the test scripts.. those will need to check for PG10+ when running a SQL script with macaddr8.)

I was referring to checking the postgraphile-core version (once a release comes out) if checking for pgUseCustomNetworkScalars, to display an error/warning to upgrade. This may be moot depending on the above as the new ops would never be added with an older version of postgraphile.

Yes, but.. we're going to run into the fact that there's no released version of PostGraphile (core) to test against...

I'm working on it locally against master, but I'm not planning to submit a PR until there is a release of postgraphile-core with the new types.

mattbretl commented

I think that will work, but it relies on alphabetical ordering of the keys to not break string operators. I want to be sure you are okay with this design?

Good point. Let's tweak gqlTypeNameFromPgTypeName (which is defined right above the code you referenced) to return null if the pgGetGqlTypeByTypeIdAndModifier call returns "String". That will prevent the _CidrAddress and _MacAddress variables in your example from being set to "String".

I was referring to checking the postgraphile-core version (once a release comes out) if checking for pgUseCustomNetworkScalars, to display an error/warning to upgrade. This may be moot depending on the above as the new ops would never be added with an older version of postgraphile.

Yep, I think it's moot.

I'm working on it locally against master, but I'm not planning to submit a PR until there is a release of postgraphile-core with the new types.

👍

higherorderfunctor commented

This fell off my radar waiting for a release. I will start cracking away at it since v4.5.x was released.

mattbretl commented

This is now supported in 2.0.0.