List query filter improvements about crystal HOT 48 CLOSED

Ok, I kinda like this. Are we assuming multiple filters are all joined by AND? Do we want to support OR filters?

Also, I’d really like to remove the argument-per column style (as it adds a lot of noise to the field argument list) so I really like your filter name.

The implementation I was originally playing around with would look like the following, tell me what you think:

postNodes(filters: [PostFilter], …)

type PostFilter {
  operator: Operator,
  field: String,
  value: Any
}

enum Operator {
  EQUAL,
  LIKE,
  REGEX,
  …
}

And we could also make a special type for every field to restrict operators depending on type…

This may be way over complicating things.

from crystal.

Here's my first shot at a v4 plugin:

module.exports = function PgConnectionArgFilter(
  builder,
  { pgInflection: inflection }
) {
  builder.hook(
    "init",
    (
      _,
      {
        addType,
        getTypeByName,
        newWithHooks,
        pgIntrospectionResultsByKind: introspectionResultsByKind,
        pgGqlInputTypeByTypeId: gqlTypeByTypeId,
        graphql: { GraphQLInputObjectType, GraphQLBoolean, GraphQLInt, GraphQLString, GraphQLNonNull, GraphQLList },
      }
    ) => {
      const makeFilterFields = (gqlType) => {
        return {
          null: {
            description: "If set to true, checks for null values.  If set to false, checks for non-null values.",
            type: GraphQLBoolean,
          },
          equalTo: {
            description: "Checks for values equal to this value.",
            type: gqlType,
          },
          notEqualTo: {
            description: "Checks for values not equal to this value.",
            type: gqlType,
          },
          lessThan: {
            description: "Checks for values less than this value.",
            type: gqlType,
          },
          lessThanOrEqualTo: {
            description: "Checks for values less than or equal to this value.",
            type: gqlType,
          },
          greaterThan: {
            description: "Checks for values greater than this value.",
            type: gqlType,
          },
          greaterThanOrEqualTo: {
            description: "Checks for values greater than or equal to this value.",
            type: gqlType,
          },
          in: {
            description: "Checks for values that are in this list.",
            type: new GraphQLList(new GraphQLNonNull(gqlType)),
          },
          notIn: {
            description: "Checks for values that are not in this list.",
            type: new GraphQLList(new GraphQLNonNull(gqlType)),
          },
        };
      };

      const StringFilter = new GraphQLInputObjectType({
        name: "StringFilter",
        description: `A filter to be used against String fields. All fields are combined with a logical ‘and.’`,
        fields: makeFilterFields(GraphQLString),
      });
      addType(StringFilter);

      const IntFilter = new GraphQLInputObjectType({
        name: "IntFilter",
        description: `A filter to be used against Int fields. All fields are combined with a logical ‘and.’`,
        fields: makeFilterFields(GraphQLInt),
      });
      addType(IntFilter);

      const DatetimeFilter = new GraphQLInputObjectType({
        name: "DatetimeFilter",
        description: `A filter to be used against Datetime fields. All fields are combined with a logical ‘and.’`,
        fields: makeFilterFields(getTypeByName("Datetime")),
      });
      addType(DatetimeFilter);

      const JSONFilter = new GraphQLInputObjectType({
        name: "JSONFilter",
        description: `A filter to be used against JSON fields. All fields are combined with a logical ‘and.’`,
        fields: {
          null: {
            description: "If set to true, checks for null values.  If set to false, checks for non-null values.",
            type: GraphQLBoolean,
          }
        }
      });
      addType(JSONFilter);

      introspectionResultsByKind.class.map(table => {
        const tableTypeName = inflection.tableType(
          table.name,
          table.namespace && table.namespace.name
        );
        newWithHooks(
          GraphQLInputObjectType,
          {
            description: `A filter to be used against \`${tableTypeName}\` object types. All fields are combined with a logical ‘and.’`,
            name: `${inflection.tableType(
              table.name,
              table.namespace && table.namespace.name
            )}Filter`,
            fields: ({ fieldWithHooks }) =>
              introspectionResultsByKind.attribute
                .filter(attr => attr.classId === table.id)
                .reduce((memo, attr) => {
                  const fieldName = inflection.column(
                    attr.name,
                    table.name,
                    table.namespace && table.namespace.name
                  );
                  const fieldFilterTypeByFieldTypeName = {
                    String: StringFilter,
                    Int: IntFilter,
                    Datetime: DatetimeFilter,
                    JSON: JSONFilter
                  }
                  const fieldType = gqlTypeByTypeId[attr.typeId];
                  const fieldFilterType = fieldFilterTypeByFieldTypeName[fieldType.name];
                  if (fieldFilterType != null) {
                    memo[fieldName] = fieldWithHooks(
                      fieldName,
                      {
                        description: `Filter by the object’s \`${fieldName}\` field.`,
                        type: fieldFilterType,
                      },
                      {
                        isPgConnectionFilterInputField: true,
                      }
                    );
                  }
                  return memo;
                }, {}),
          },
          {
            pgIntrospection: table,
            isPgFilter: true,
          }
        );
      });
      return _;
    }
  );
  builder.hook(
    "GraphQLObjectType:fields:field:args",
    (
      args,
      {
        pgSql: sql,
        gql2pg,
        extend,
        getTypeByName,
        pgIntrospectionResultsByKind: introspectionResultsByKind,
      },
      {
        scope: { isPgConnectionField, pgIntrospection: table },
        addArgDataGenerator,
      }
    ) => {
      if (!isPgConnectionField || !table || table.kind !== "class") {
        return args;
      }
      const TableFilterType = getTypeByName(
        `${inflection.tableType(
          table.name,
          table.namespace && table.namespace.name
        )}Filter`
      );

      addArgDataGenerator(function connectionFilter({ filter }) {
        return {
          pgQuery: queryBuilder => {
            if (filter != null) {
              introspectionResultsByKind.attribute
                .filter(attr => attr.classId === table.id)
                .forEach(attr => {
                  const fieldName = inflection.column(
                    attr.name,
                    table.name,
                    table.namespace.name
                  );
                  const fieldFilter = filter[fieldName];
                  if (fieldFilter != null) {
                    Object.keys(fieldFilter).forEach(operator => {
                      const sqlField = sql.fragment`${queryBuilder.getTableAlias()}.${sql.identifier(attr.name)}`;
                      if (operator === "null") {
                        queryBuilder.where(
                          sql.fragment`${sqlField} ${fieldFilter[operator]
                            ? sql.fragment`IS NULL`
                            : sql.fragment`IS NOT NULL`}`
                        );
                      } else if (operator === "equalTo") {
                        queryBuilder.where(
                          sql.fragment`${sqlField} = ${gql2pg(fieldFilter[operator], attr.type)}`
                        );
                      } else if (operator === "notEqualTo") {
                        queryBuilder.where(
                          sql.fragment`${sqlField} <> ${gql2pg(fieldFilter[operator], attr.type)}`
                        );
                      } else if (operator === "lessThan") {
                        queryBuilder.where(
                          sql.fragment`${sqlField} < ${gql2pg(fieldFilter[operator], attr.type)}`
                        );
                      } else if (operator === "lessThanOrEqualTo") {
                        queryBuilder.where(
                          sql.fragment`${sqlField} <= ${gql2pg(fieldFilter[operator], attr.type)}`
                        );
                      } else if (operator === "greaterThan") {
                        queryBuilder.where(
                          sql.fragment`${sqlField} > ${gql2pg(fieldFilter[operator], attr.type)}`
                        );
                      } else if (operator === "greaterThanOrEqualTo") {
                        queryBuilder.where(
                          sql.fragment`${sqlField} >= ${gql2pg(fieldFilter[operator], attr.type)}`
                        );
                      } else if (operator === "in") {
                        const list = sql.join(
                          fieldFilter[operator].map(val => gql2pg(val, attr.type)),
                          ", "
                        );
                        queryBuilder.where(
                          sql.fragment`${sqlField} IN (${list})`
                        );
                      } else if (operator === "notIn") {
                        const list = sql.join(
                          fieldFilter[operator].map(val => gql2pg(val, attr.type)),
                          ", "
                        );
                        queryBuilder.where(
                          sql.fragment`${sqlField} NOT IN (${list})`
                        );
                      }
                    });
                  }
                });
            }
          },
        };
      });

      return extend(args, {
        filter: {
          description:
            "A filter to be used in determining which values should be returned by the collection.",
          type: TableFilterType,
        },
      });
    }
  );
};

This adds a new filter: arg and does not change the existing condition: arg. I believe I've implemented everything in @benjie 's proposal above, with the exception of logical operators (and/or/not).

If anyone wants to try this out, you can save the code above as PgConnectionArgFilter.js and use the --append-plugins option in the CLI (making sure to use postgraphql@next):

postgraphql --append-plugins '/path/to/PgConnectionArgFilter.js'

Further testing and feedback would be greatly appreciated.

from crystal.

@danstn I’m still waiting for a good proposal on advanced filters since I recognized input unions are not a thing and might not ever be 😞. Ultimately, I’m just not sure if GraphQL input is flexible enough to allow for conditions of the complex nature we’d like to see.

However, this is totally possible with procedures and definitely the recommended way for now. Try something like the following:

create function my_filter_function(numbers int[]) returns setof my_table as $$
  select *
  from my_table
  where
    my_column = any (numbers)
$$ language sql stable;

Note here that we used = any vs in here. See the Node.js pg docs on parameterized queries for a basic explanation why (although it is in a different yet tangentially similar context). Also see the Postgres docs on array searching.

from crystal.

So I wrote a plugin for v4 the other day (as a quick hack) that does a small amount of what you started out suggesting:

module.exports = function PgConnectionArgCondition(
  builder,
  { pgInflection: inflection }
) {
  builder.hook(
    "GraphQLInputObjectType:fields",
    (
      fields,
      {
        extend,
        pgIntrospectionResultsByKind: introspectionResultsByKind,
        graphql: { GraphQLBoolean },
      },
      { scope: { isPgCondition, pgIntrospection: table }, fieldWithHooks }
    ) => {
      if (!isPgCondition || !table || table.kind !== "class") {
        return fields;
      }
      return extend(
        fields,
        introspectionResultsByKind.attribute
          .filter(attr => attr.classId === table.id)
          .filter(attr => !attr.isNotNull)
          .reduce((memo, attr) => {
            const fieldName = inflection.column(
              attr.name,
              table.name,
              table.namespace && table.namespace.name
            );
            // eslint-disable-next-line
            memo[`${fieldName}_null`] = fieldWithHooks(
              fieldName,
              {
                description: `Checks object’s \`${fieldName}\` field is null (true) or not null (false)`,
                type: GraphQLBoolean,
              },
              {
                isPgConnectionConditionInputFieldNull: true,
              }
            );
            return memo;
          }, {})
      );
    }
  );
  builder.hook(
    "GraphQLObjectType:fields:field:args",
    (
      args,
      {
        pgSql: sql,
        gql2pg,
        extend,
        getTypeByName,
        pgIntrospectionResultsByKind: introspectionResultsByKind,
      },
      {
        scope: { isPgConnectionField, pgIntrospection: table },
        addArgDataGenerator,
      }
    ) => {
      if (!isPgConnectionField || !table || table.kind !== "class") {
        return args;
      }

      addArgDataGenerator(({ condition }) => ({
        pgQuery: queryBuilder => {
          if (condition != null) {
            introspectionResultsByKind.attribute
              .filter(attr => attr.classId === table.id)
              .filter(attr => !attr.isNotNull)
              .forEach(attr => {
                const fieldName = inflection.column(
                  attr.name,
                  table.name,
                  table.namespace.name
                );
                const val = condition[`${fieldName}_null`];
                if (val != null) {
                  queryBuilder.where(
                    sql.fragment`${queryBuilder.getTableAlias()}.${sql.identifier(
                      attr.name
                    )} ${val
                      ? sql.fragment`IS NULL`
                      : sql.fragment`IS NOT NULL`}`
                  );
                }
              });
          }
        },
      }));

      return args;
    }
  );
};

This plugin extends the existing Condition type by adding foo_null: Boolean for every column foo in the table.

However, I was discussing with @mgallagher last night that a better approach would be to replace the Condition plugin and implement a strongly typed filter in a similar vein to what you suggest - we even agreed that Mongo filters would be a good model to follow. The schema for such a thing might end up looking something like this:

type MyTable {
  id: ID!
  name: String!
  createdAt: DateTime!
  updatedAt: DateTime!
  number: Int
  json: JSON
}

type MyTableCondition {
  id: IDFilter
  name: StringFilter
  createdAt: DateTimeFilter
  updatedAt: DateTimeFilter
  number: IntFilter
  json: JSONFilter
  and: MyTableCondition
  or: MyTableCondition
  not: MyTableCondition
}

type StringFilter {
  null: Boolean
  eq: String
  ne: String
  gt: String
  gte: String
  lt: String
  lte: String
  in: [String!]
  nin: [String!]
}


type IntFilter {
  null: Boolean
  eq: Int
  ne: Int
  gt: Int
  gte: Int
  lt: Int
  lte: Int
  in: [Int!]
  nin: [Int!]
}

type DateTimeFilter {
  null: Boolean
  eq: DateTime
  ne: DateTime
  gt: DateTime
  gte: DateTime
  lt: DateTime
  lte: DateTime
  in: [DateTime!]
  nin: [DateTime!]
}

type JSONFilter {
  null: Boolean  
}

type MyTableConnection {
  ...
}

type Query {
  allMyTables(condition: MyTableCondition): MyTableConnection!
}

schema {
  query: Query
}

from crystal.

The plugin is available in full above, to load it see:

https://www.graphile.org/postgraphile/extending/#loading-additional-plugins

This works with postgraphql@next (v4) which is the same as postgraphile.

from crystal.

I've just opened up the wiki and created a page for community plugins - if you're aware of any others please post them on the wiki!

https://github.com/postgraphql/postgraphql/wiki/v4-plugins

from crystal.

Ok, I think this is a good proposal. I’m just not totally sold on the $ semantics (yeah I know it’s what everyone else uses 😊). I’d also prefer to expand some names instead of using their acronyms, so greaterThan instead of gt, oneOf instead of in, etc. Also, instead of using $or, how do you feel about:

postNodes(filter: FiltersClause | [FiltersClause])

…where like you said, map has AND semantics and a list has OR semantics. We may even consider:

postNodes(filter: FiltersClause | [FiltersClause] | [[FiltersClause]])

Where [[a, b], [c, d]] represents (a and b) or (c and d). I think that might be overkill though…

from crystal.

The great thing about doing this with a v4 plugin is we can have multiple implementations and people can choose to use the one they prefer. There's no need to have the plugin in core at all - it can be a completely independent npm module.

If the community vote enough on a particular plugin then we could even merge it into core (if the author agrees!).

I might do a dissection of the plugin above so people can see how it works. It's not as hard as it looks - a lot of it is boilerplate.

from crystal.

From what I can tell, the current PostGraphQL condition argument performs equality checks on one or more fields:

query SingleField {
  allPosts(condition: {
    authorId: 2
  }) {
    ...
  }
}
# select ... from post where author_id = 2

query MultipleFields {
  allPosts(condition: {
    authorId: 2
    topic: SHOWCASE
  }) {
    ...
  }
}
# select ... from post where author_id = 2 and topic = 'showcase'

The next logical step would be to implement:

• Additional comparison operators (!=, >, <, etc.)
• Logical operators (and, or, and potentially not)

For comparison operators, the simplest solution is to append the operator name to the field name. PostGraphQL uses a similar approach for the orderBy argument, appending _ASC and _DESC to every field name, e.g. orderBy: CREATED_AT_DESC.

So what should we name the operators? Mongo-style names would look something like this:

Operator  Suffix  Example
--------  ------  -------
=                 authorId_eq
!=        _ne     authorId_ne
>         _gt     createdAt_gt
>=        _gte    createdAt_gte
<         _lt     createdAt_lt
<=        _lte    createdAt_lte
IN        _in     topic_in
NOT IN    _nin    topic_nin

A camelCase approach would look something like this:

Operator   Suffix                 Example
--------   ------                 -------
=                                 authorId
!=         _notEqualTo            authorId_notEqualTo
>          _greaterThan           createdAt_greaterThan
>=         _greaterThanOrEqualTo  createdAt_greaterThanOrEqualTo
<          _lessThan              createdAt_lessThan
<=         _lessThanOrEqualTo     createdAt_lessthanOrEqualTo
IN         _anyOf                 topic_anyOf
NOT IN     _notAnyOf              topic_notAnyOf

I think the camelCase approach is more in line with GraphQL naming conventions. For comparison, consider the readability of topic_ne: SHOWCASE versus topic_notEqualTo: SHOWCASE. It's obviously a trade-off between compactness and clarity, but GraphQL implementations tend to favor clarity. In any case, it would be trivial to allow the preferred syntax to be defined using a config parameter in PostGraphQL, so let's not get hung up on this part.

Finally, here's a simple example of conditional operators in action:

query WithComparisonOperators {
  allPosts(condition: {
    authorId: 2
    topic_anyOf: [SHOWCASE, INSPIRATION]
    createdAt_greaterThan: "2017-07-01T00:00:00Z"
  }) {
    ...
  }
}
# select ... from post where author_id in (2, 5, 6) and topic != 'showcase';

If we omit the suffix on the equality operator (as shown in all of the examples to this point), then I think condition argument could be extended to support all of this without any breaking changes. I don't have a strong opinion on appending _equalTo versus omitting it, but it seems unnecessary and it deviates from the current condition syntax.

With comparison operators in place, I think the logical operators would be pretty straightforward. I couldn't come up with a complex query on allPosts, so instead I present you with this query for Star Wars characters that have unusual eyes and are either short-and-fat or tall-and-skinny:

query {
  allPeople (condition: {
    and: [
      {
        eyeColor_notAnyOf: ["blue", "brown", "green"]
      },
      {
        or: [
          {
            height_lessThan: 200,
            mass_greaterThan: 100
          },
          {
            height_greaterThan: 200,
            mass_lessThan: 50
          }
        ]
      }
    ]
  }) {
    ...
  }
}
# select * from person
# where
#   eye_color not in ('blue','brown','green')
#   and (
#     (height < 200 and mass > 100) or
#     (height > 200 and mass < 50)
#   );

This is the usual approach I've seen taken on JSON logic. Would replacing all/or with allOf/anyOf improve readability? Debatable.

It does become easy to descend into "nesting hell" as @pencilcheck put it, but the only alternatives I can think of are:

• try to emulate SQL syntax with AND and OR keywords and grouping characters (feels dirty); OR
• devise a new JSON schema for representing nested logic (essentially trading nesting hell for implementation and documentation hell)

Bottom line, I think nested and and or blocks are a reasonable starting point for PostGraphQL.

These are just my opinions and I would definitely appreciate feedback. When the dust settles, I'll start working on a plugin for v4.

from crystal.

I'm closing this; if you need this functionality then @mattbretl's plugin is the best bet, and I've added it to the filtering docs as @pyramation suggested 👍

from crystal.

See #46

from crystal.

My opinion : it's better to model filters in mongo-like query syntax. It's supported in many other databases(pouchdb,lokijs,etc) and is slowly became a standard way to describe filters.

postNodes(filters: FiltersClause)

type FiltersClause {
  id: IDFilter
  name: StringFilter
  created: DateFilter
  $or: [FiltersClause]
}

type IDFilter = ID | {
 $eq: ID,
 $in: [ID] 
}

type DateFilter = Date | {
 $eq: Date,
 $between: [Date,Date],
 $in: [Date],
 $gt, $le ...
}

• Map have AND semantics
• List have OR semantics
• Top level - AND clause

About strong-typed filter clauses vs generic filter clauses - I prefer strong typing. It gives us:

• autocompletion in graphqli
• validation of operators during schema parsing, not during database query execution(gives much readable errors)
• user will not be confused what operators is allowed for certain datatypes

from crystal.

Would be really great to have custom filtering.

Any idea on how to simulate in clause behavior with views/functions?

from crystal.

I'am looking forward about this mongo like filtering feature.For example MongoDB/Mongoose provides JSON filter object with all possible kind of operators.

from crystal.

I am not well versed in Postgres procedures, so please bear with me as I am probably not understanding. In my case i would likely want to filter by combinations of fields (and in fact potentially combinations of all fields). However in a lot of cases I will only be passing a subset of those fields. for example I might have in my app:

minimum salary: 20000
maximum salary: 40000
job type: administrator
job title: adm

I can see how I would build a procedure to do all of these, but how do I create this procedure to allow me to only pass values for some of those fields and not query them. i.,e. I might set only job type, so rest of values are null, and would want to query recrods where job type='adm' only.

I want a procedure where I could specify all input fields and values and then build the query based on those which have a value not null.

It is clearly a lack of understanding on my part but if I can nail this then the procedures will work for me.

Please feel free to tell me I am being stupid!!

from crystal.

Here's some pseudocode that might inspire you:

create function search(row my_table) returns setof my_table as $$
const args = [];
const values = [];
for (var k in row) {
  if (row[k] != null) {
    args.push(k);
    values.push(row[k]);
  }
}
if (args.length > 0) {
  var query = 'select * from my_table where ' + args.map(arg => plv8.quote_ident(arg) + ' = ?').join(', ');
  return plv8.execute(query, values)
} else {
  return [];
}
$$ stable language plv8;

PSEUDO CODE, UNTESTED, NOT GUARANTEED IN ANY WAY, USE AT YOUR OWN RISK, PROBABLY DOES NOT ANSWER YOUR QUESTION DIRECTLY.

from crystal.

from crystal.

I'd love to see a plugin for v4 that adds additional (or replacement) condition filters - if one gets enough approval we could look at merging it into core 👍

from crystal.

Can't wait to get generated conditional filters

from crystal.

@pencilcheck How do you see it working?

from crystal.

Thanks for asking me this. As I have a lot of ideas on how to make this work and be more flexible and extensible.

My idea is based on how my experience with writing stored procedures for PostgreSQL and noticing some patterns.

Usually most of the stored procedures follows the rules of

1. Write a function that takes input
2. Convert inputs into boolean or strings
3. Create a query
4. Return that query

In step 3, it uses the input for a lot of stuff, but also a lot of implicit stuff that are not derived in the inputs, could be for deciding which query to return, or which condition to be satisfied, or which filtering (WHERE) should be executed, and which ordering (ORDER BY) should be executed.

Ideally we can express all 100% PostgreSQL syntax so we can filter without changing the stored procedure, but that is too ambitious and might not make sense for some concerns such as security.

But we can setup a syntax which I think could be a useful starting point

query FetchEvents($whereArgs: WhereArgs) {
  fetchEvents(where: $whereArgs) {}
}

WhereArgs = Object[]

Let's say you want to do the following filtering

SELECT ... WHERE id = '123' AND (CHAR_LENGTH(description) > 5 OR liked);

What could be translated into

Variables
{
  "whereArgs": [
    {
      "leftOperand": "id",
      "operation": "=",
      "rightOperand": "'123'",
      "parseClass": 0
    },
    {
      "leftOperand": "CHAR_LENGTH(description)",
      "operation": ">",
      "rightOperand": "5",
      "parseClass": 1
    },
    {
      "leftOperand": "liked",
      "parseClass": 1
    }
  ]
}

WhereArgs is a flat list.

Each object in the list has the following attributes

1. leftOperand
2. operation
3. rightOperand
4. parseClass

1, 2, 3 should be obvious, their values will be inserted as part of the expression directly

4. is very interesting too, this solution come since I wish I don't need to always have to nest to many ANDs and ORs if we make into the mongoDB style.

So by default if you leave out parseClass, it is assume it is always ANDing with the rest of the items.

But if you have some items with the parseClass set, and have the same value but different from other items, then they are interpreted as ORing each other.

The higher the parseClass value, the later it will be parsed in order.

I don't think this is the final form, as I haven't looked into more in depth use cases, nor is the naming is final as parseClass might be confusing to new users. However I think this is probably in my opinion that it might be easier because using the MongoDB style filtering (Scaphold user anyone?), I noticed that we also had a lot of issues with it. Namely:

1. Nesting hell
2. No parse ordering we can enforce (can only keep wrapping everything that enables more nesting hell)
3. Limited expressibility, as the mongoDB syntax limits the expressiveness by the operations that the syntax provides. Of course there are security and performance concerns here, the initial thing the implementation can go is to escape all known sensitive keywords such as semicolon, etc to prevent injections for instance.

Order by follows similar proposed syntax, but instead of WHERE it is after ORDER BY

An example

SELECT ... WHERE id = '123' AND (CHAR_LENGTH(description) > 5 OR liked) ORDER BY child_table.attribute ASC, id DESC, CHAR_LENGTH(description) DESC;

What could be translated into

Variables
{
  "orderByArgs": [
    {
      "attribute": "child_table.attribute",
      "direction": "ASC"
    },
    {
      "attribute": "id",
      "direction": "DESC"
    },
    {
      "attribute": "CHAR_LENGTH(description)",
      "direction": "DESC"
    }
  ]
}

I would like to leave this as a open form, and perhaps move to a new discussion thread but feel free to response and let me know your feedback.

Thanks.

from crystal.

@mattbretl Your feedback looks well structured, however I don't see any functionality difference from the original poster proposal enough to really make an impact in my opinion, except you probably saved one level of nesting hell.

My understanding based on previous discussion is that the current way of structuring is very limited in the expressibility, therefore we are limited just by types of operations that a suffix or a object key can represent. And PostgreSQL has so much more that doesn't depends solely on just the operations, there are a lot of aggregate functions, or buildin functions that allows much more richer comparisons.

As it is impossible to keep up to date on those extensions, all possible comparison between json, string, float, integers, etc, it is in my opinion wiser to start with giving more expressibility first, then start making it more universal.

As for AND and OR, what your syntax is exactly the same what I seen on Scaphold, as you assumed each array within the And and Or keyword are all wrapped with paranthesis, but what if I just simply want to do A and B or C? How would you describe that in that syntax?

I'm not saying the idea is bad, but it is limiting and probably not reaching the goals and standards that both @benjie and @calebmer set for this library.

After reading @benjie's reply, I say awesome!

from crystal.

@benjie Just to make sure I follow, your strongly typed filter approach:

type MyTableCondition {
  id: IDFilter
  name: StringFilter
  createdAt: DateTimeFilter
  updatedAt: DateTimeFilter
  number: IntFilter
  json: JSONFilter
  and: MyTableCondition
  or: MyTableCondition
}

would allow queries like:

{
  authorId: { eq: 2 },
  topic: { in: [SHOWCASE, INSPIRATION] },
  createdAt: { gt: "2017-07-01T00:00:00Z" }
}

whereas my approach (with Mongo syntax):

type MyTableCondition {
  ...
  name_null: Boolean
  name_eq: String
  name_ne: String
  name_gt: String
  name_gte: String
  name_lt: String
  name_lte: String
  name_in: [String!]
  name_nin: [String!]
  ...
}

would allow queries like:

{
    authorId: 2
    topic_in: [SHOWCASE, INSPIRATION]
    createdAt_gt: "2017-07-01T00:00:00Z"
  }

If I'm seeing this clearly, both approaches provide the same filtering capabilities. The difference is my schema is denormalized for the sake of slightly simpler queries (replacing three {,:,} characters with one _). If we were writing the GraphQL schema from scratch instead of generating it from Postgres, yours would obviously be preferable because it's properly normalized and therefore much easier to maintain. Yours is also more extensible, e.g. StringFilter could be extended to accomplish the type of thing that @pencilcheck was proposing. Yours also provides a better GraphiQL experience, since you'd be picking from a short field list,and then an short operator list, instead of one giant list. So, based on the mountain of evidence in your favor, you win. 🎉

@pencilcheck, I agree with your comments on extensibility of the filter model. I think @benjie's most recent proposal would address your concerns. Since StringFilter is an object, we could eventually add support for filters like { description: { gt: 5, functions: ['charLength'] } } which translates to char_length(description) > 5. (That's just one idea, you could structure that object several different ways.)

In comparison, your { leftOperand, operator, rightOperand, parseClass } approach is flexible, but it abandons strong typing and it feels like it's leading us toward eval() land. I don't see any benefits to it over @benjie's strongly typed proposal.

I also took another look at your parseClass concept and I think I have a better grasp of the motivations now. My main concern would be the amount of logic required to confirm that the parseClass integer values given by the user actually constitute a valid execution order. I haven't really thought it through, so maybe it's no big deal. My secondary concern is whether it's actually any easier to write/interpret a big parseClass-style filter versus than the nested and/or filters. I'd have to experiment with a longer list of examples.

As an aside, huge thanks to @benjie for leading the charge on v4 so we can explore all of these tangents indepedently of core. 👏

from crystal.

(Just added not: MyTableCondition to the above example, may be able to drop a few fields due to that (e.g. nin: [1,2] could become not: {in: [1,2]}).)

Thanks @mattbretl :)

I think our solutions are broadly similar. I also agree we should probably go with the longer names @mattbretl suggest because clarity > brevity.

To be clear: I am not writing any code for this right now. I might in future, but if someone else wants to step up please do. I'd recommend doing it as an additional arg (e.g. allFooBars(condition2: FooBarCondition2), but probably better named such as 'filter' or whatever) so it can be used side-by-side with the existing implementation to aid migrating.

from crystal.

@benjie sorry for asking in this thread but can you point out the resources/links to get started? I am just a postgraphql user and I have no idea how the v4 plugin structure is going to work. And it doesn't seems like there are organized resources on actionable examples I can follow to get this started.

Cheers

from crystal.

@pencilcheck I'm still working on the docs over at https://graphile.org but it's very time consuming and I'm doing it unpaid. First thing you should do is familiarise yourself with Facebook's graphql-js library for writing a schema manually because if you don't know that you won't understand Graphile Build.

from crystal.

If you want to support me writing more docs (and making the existing ones make more sense!); donations are gratefully received: https://www.paypal.me/benjie

from crystal.

I really like graph.cool's query parameter syntax, which is also a little reminiscent of Mongo's query operators and uses the filter arg:

query actorsAfter2009 {
  allActors(
    filter: {
      movies_some: {
        releaseDate_gte: "2009"
      }
    }
    first: 3
    orderBy: name_ASC
  ) {
    name
    movies {
      title
    }
  }
}

@benjie, is your quick-and-dirty plugin available on a working branch or will I need to cobble that together? My team kind of hit a wall today when we realized the need to support filtering and the current approach to use stored procedures is less viable due do the data models we're dealing with (whole lotta tables).

The Graph.cool guys are "happy to share our best practices designing powerful GraphQL APIs with the GraphQL community", so I wonder if this might be a point of convergence?

from crystal.

This is super cool and I was actually looking for this already 👍
What do you think if you name the operators according to the query selector from mongodb? https://docs.mongodb.com/manual/reference/operator/query/#query-selectors
They seem to be a sort of standard these days...

I don't have time to try this out now, but have a look next week.

from crystal.

This looks really promising! 🎉

Please change lines such as:

const StringFilter = new GraphQLInputObjectType({...});
addType(StringFilter);

to:

const StringFilter = newWithHooks(GraphQLInputObjectType, {...}, {
  isFilterInputType: true,
  filterType: GraphQLString,
});

This way additional plugins can extend these filters e.g. to add full text search or whatever.

Personally I'd split this into multiple plugins so that "equality" is in the base plugin and other features can be added additively so people can pick and choose which filters they want. The null filter could be added to all isFilterInputTypes via a small plugin, for example, rather than duplicating it for each type as is performed currently.

I'd also split the base filter types (StringFilter, JSONFilter, etc) into their own plugin as they should be valid outside of PostgreSQL (i.e. split your first hook in half).

I'd move fieldFilterTypeByFieldTypeName onto the Build object (using a build hook) so that other plugins can register more filter types here and everything can benefit.

It would be nice to have isDistinctFrom and isNotDistinctFrom added too; but this should be done as a separate plugin as it has performance consequences so people might not want to implement it by default.

I don't have time to review the code in depth right now; hopefully these quick notes are helpful!

Thanks so much for implementing this 🙏

from crystal.

(In general GraphQL API's tend to use more verbose/descriptive field names rather than short abbreviated ones, so I support using greaterThanOrEqualTo: over gte:; GraphQL tools can help auto-complete these for you so it shouldn't be much more effort for the programmer and it's easier to read.)

from crystal.

Haven't fully tested the code but added DateFilter
https://gist.github.com/pencilcheck/cb71850937c0341a9b06a7f8fdee14fd

from crystal.

I just started a new repo for this effort:
https://github.com/mattbretl/postgraphile-filter-plugins

Improvements:

• Support and and or logical operators.
• Support additional types: Date, Time
• Support additional operators: distinctFrom, notDistinctFrom
• Allow Mongo-style operator names by toggling a const at the top of the plugin.. I don't plan to use these personally, but I don't want to exclude anyone based on naming conventions. We can have another debate if/when this reaches the PR stage.

In the process of adding logical operators, this basically became a full rewrite. The build object now has a filterOperators object which holds all of the filtering logic, including how they resolve to SQL. If you take a quick look at PgConnectionArgFilterOperatorsPlugin.js, you'll see how easy it is to add/remove operators or restrict an operator's availability to specific field types. For now, that's the basic way to customize this plugin for a specific use case.

Next on my list:

• Allow overriding of filterable field types (Int, String, etc.) via a hook
• Add tests using the kitchen sink schema

@benjie, please let me know if there's a better way to structure this plugin; I'm sure there's plenty of room for improvement. I considered your suggestion of including the equalTo operator by default and adding other operators as separate plugins, but that dovetailed into a set of questions about whether each operator should be a separate plugin, how to group them, what to call them, etc., so for now they're all included by default.

from crystal.

Allow Mongo-style operator names by toggling a const at the top of the plugin..

Rather than using const useShortNames = false; you can/should make it a plugin option, e.g.:

module.exports = function PgConnectionArgFilterOperatorsPlugin(builder, { connectionFilterUsesShortNames = false } = {}) {

See: https://www.graphile.org/graphile-build/plugins/#plugin-arguments

you'll see how easy it is to add/remove operators or restrict an operator's availability to specific field types

This statement makes me happy 😁

let me know if there's a better way to structure this plugin

Looks good from a quick scan 👍 You could expose an index.js which automatically composes these plugins, assuming they can just be appended to the list? That would make it easier to consume. You could also publish it to npm so people can just install and use graphile-build-pg-contrib-connection-filter or whatever.

from crystal.

Rather than using const useShortNames = false; you can/should make it a plugin option

Your improvement exposes the option to buildSchema(plugins, options), but as far as as I can tell, the options passed when calling postgraphql don't pass through to buildSchema. What am I missing?

from crystal.

postgraphile-core acts as a gateway, you can use graphqlBuildOptions to pass through additional options (this will be renamed soon)

https://github.com/graphile/graphile-build/blob/master/packages/postgraphile-core/src/index.js

from crystal.

👍 Updated the repo

from crystal.

Thanks for this update @mattbretl , could you give an example on how you'd do this request from the front end, we keep getting blocked with "argument filter has invalid value" - at the moment we're doing requests as

exampleRequest(filter: { equalTo: { jobs: { rate: 27 } } })

from crystal.

I think it’s the other way around: filter { rate { equalTo: 27 }} ?

from crystal.

@benjie's example should work, assuming it fits your schema. Did you intend to nest 'rate' under 'jobs'? At the moment, the plugin doesn't support querying on nested (related) fields.

Also, please make sure you're using the latest master from https://github.com/mattbretl/postgraphile-filter-plugins.. I committed a few fixes last week.

from crystal.

@benjie @mattbretl thank you so much for your feedback!

It should be filter { rate { equalTo: 27 }} my apologies for the nesting.

Yes we pulled down the latest from master and started with the index file - we're getting a Uuid issue, but i'm assuming so thats a problem pertaining to our end

Thanks nonetheless!

from crystal.

Do feel free to raise that as a separate issue if you can't get to the bottom of it 👍

from crystal.

Absolutely will do @benjie , thanks for the support and feedback! This has saved my team weeks of work!

from crystal.

@benjie Turns out theres been a change from Uuid to UUID which was crashing our app when we upgraded to v4! :)

from crystal.

That is mentioned in the breaking changes in #506; if someone fancied writing them up as an article on graphile.org that'd be welcome - probably easier to reference!

from crystal.

@benjie is this issue the best one for generic search? Sounds like currently the best option is to build a plugin for this still, unless you're ok with making custom sql functions

from crystal.

Could this be considered the solution to search and filter? https://github.com/mattbretl/postgraphile-plugin-connection-filter

seems solid :) If this issue is regarding filter, this plugin definitely solves the issue.

from crystal.

maybe should officially support or mention on this page: https://www.graphile.org/postgraphile/filtering/

from crystal.