Skip to content

Conversation

@santino
Copy link
Contributor

@santino santino commented Feb 17, 2021

In order to continue extending bare functionalities for Mesh sources, I'm raising this PR for the "filter-schema" transform.

The motivation and context are the same as per #1554; which introduces the same functionalities to the "rename" transform.

Breaking changes

There are two breaking changes in this PR, the first is mandatory and already discussed and approved for the "rename" transform. The second is a proposal and we can get to an agreement here before finalising.

  1. Filter rules must be wrapped within filters property. This is because I had to add an object to the transform config in order to support mode, so the list of rules must also belong to an object property
  2. I am proposing a new syntax for renaming arguments that is more declarative and hopefully less ambiguous.

The new syntax for arguments is as follows.

 - filterSchema:
    - mode: bare
    - filers:
      - "!User" # <-- This will remove `User` type
      - Query.!admins # <-- This will remove field `admins` from `Query` type
      - Mutation.!{addUser, removeUser} # <-- This will remove fields `addUser` and `removeUser` from `Mutation` type
      - User.{id, username, name, age} # <-- This will remove all fields except `id`, `username`, `name` and `age`

      # Stuff above is still supported, whilst stuff below is new
      - Query.user.{id, name} # <-- This will remove all args for field `user`, in Query type, except `id` and `name`
      - Query.user.id # <-- This will remove all args from field `user`, in Query type, except `id` only
      - Query.user.!{id, name} # <-- This will remove args `id` and `name` from field `user`, in Query type
      - Query.user.!name # <-- This will remove argument `id` from field `user`, in Query type
      - Query.user.!name* # <-- This will remove all arguments from field `user`, in Query type, that start with "name"
      - Query.user.!{name*, address*} # <-- This will remove all arguments from field `user`, in Query type, that start with "name" or "address"

Argument filtering is working well with the above declarative syntax.
The downside of the proposed syntax is that you need one string for each field you want to filter arguments on.
For clarity, this is how the proposed solution differ from the current syntax below, introduced in #1498:

  - Query.{user(!pk),book}
  - Query.{user(!{pk, name}),book}

The existing syntax is potentially useful when you're whitelisting fields and you want to filter arguments for those whitelisted fields in a single line.
However, I see a few problems with that:

  1. Given is built onto whitelisting, it only works when you're whitelist fields. You don't have a clear way to keep all fields and filter arguments to one or more specific fields only.
  2. If you try to filter arguments on a selection of fields only, like this Query.user(!pk), you will be implicitly whitelisting only the user field on Query type. This will probably not be expected.
  3. It introduces more "special characters" to the mix of normally supported globs. In fact you have to use round brackets for args, and also without any dot; where you normally use curly brackets preceded by a dot to create a sub-group and ! to exclude. My proposal does not introduce any new syntax, which makes it consistent.
  4. It makes the source code more complex, which increases maintenance costs.
  5. It breaks the existing pattern of declaring relationships: type, fields, args. In fact for types only rules you specify type name only: "!User". For fields only rules you specify type and field: Query.{user, posts}. For args you would expect to follow the relationship declaration pattern and specify type, field, and arg; like my proposal: Query.user.!{id, name}.

Next steps

The PR is fully functional.
I am seeking agreement on the breaking changes in order to apply the suggested syntax for filtering arguments to the default wrap mode (which currently has not changed).

Potentially we can also consider baking into this PR the change I proposed in #1605. I think it can help potential issues with a more declarative syntax for types, but I'm not too strong about it, so is really up to you.

The only thing to add is the documentation. The restrictions of using bare are the same already explained in the "rename" transform PR, and are available here.
As such, I would suggest merging #1554 first; so that I can then adjust the introduced documentation accordingly.

Checklist:

  • I have followed the CONTRIBUTING doc and the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests and linter rules pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@changeset-bot
Copy link

changeset-bot bot commented Feb 17, 2021

🦋 Changeset detected

Latest commit: c5efe6d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@graphql-mesh/transform-filter-schema Minor
@graphql-mesh/container Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ardatan
Copy link
Owner

ardatan commented Feb 22, 2021

Thanks @santino! Could you fix the tests so we can merge this?

@santino
Copy link
Contributor Author

santino commented Feb 22, 2021

Will do,
I wanted to see if you were happy with the breaking changes before finalising.

I also want to address documentation, hence if you're happy to merge #1554 first, I will then build on top of what was added there for bare and wrap modes.

Or we can merge both as-is and I will raise a separate PR specifically for documenting the two different modes cross-transform.

@ardatan
Copy link
Owner

ardatan commented Feb 22, 2021

Thanks @santino ! I avoided the breaking changes for the existing users by adding Any as a workaround that you can in my PR.
#1647

@santino
Copy link
Contributor Author

santino commented Feb 22, 2021

Ok, will follow this approach for this PR as well.

However, do notice that there is still (currently) another breaking change proposed here for filtering ars:

# current
- Query.{user(!pk),book}
- Query.{user(!{pk, name}),book}

# new
- Query.user.!pk
- Query.user.!{pk, address*}

@ardatan
Copy link
Owner

ardatan commented Feb 22, 2021

@santino If you mention this breaking change in the changeset, I think it is fine :)

hence accepting array of strings as config
this introduces a breaking change but simplifies code significantly
@santino
Copy link
Contributor Author

santino commented Feb 22, 2021

@ardatan, this is all complete now.

  • I kept barward compatibility.
  • Have introduced the breaking change for args filtering to "wrap" mode as well, to have a unique syntax and simplify the code (see 004c501)
  • Updated existing tests.
  • Introduced new tests to cover array of rules config; declarative config (with filters property) and "bare" mode.
  • Added the changeset explaining the breaking change

Hope that's all required in order to merge this.
I will raise a separate PR for the documentation because I would like to add a new page to the documentation website.
That might require additional conversation and I don't think that needs to delay this.

@ardatan ardatan merged commit efab640 into ardatan:master Feb 22, 2021
@santino
Copy link
Contributor Author

santino commented Feb 22, 2021

cc @ntziolis
be aware of new syntax for filtering arguments in upcoming release

@santino santino deleted the filterSchemaBare branch February 23, 2021 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants