Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Federation 2 - Prevents @requires directive to accept fields with arguments - Allow it for Nullable Arguments #1987

Closed
SidKhanna296 opened this issue Jul 15, 2022 · 6 comments
Assignees
Labels
Milestone

Comments

@SidKhanna296
Copy link

SidKhanna296 commented Jul 15, 2022

Hi I am trying to upgrade my federated services with Gateway v2 - however while upgrading it I am receiving errors stating that I cannot pass fields / objects to the @requires directive when said field has arguments attached to them

Below is an example of 2 federated services - in Service 2 I have defined a field test_field that will require fields defined in Service 1 - And this throws an error saying that the @requires field cannot pass fields/objects that have arguments defined on them

Example:

Service 1:

type A @key(fields: "name") {
  name: String 
  options(test: String): Options
}

type Options {
  code: String
}

Service 2:

type A @key(fields: "name){ 
  name: String
  options(test:String): Options @external
  
  test_field: String @requires(fields: "options { code }" )
}

type Options (...extend Options from service 1 similar to how we do `type A`)

So the above will throw an error for options { code } field pass to the test_field in Service 2

We have a requirement where the options field can be queried with passed arguments to filter out some results

However when resolving the test_field we would like to fetch the nested code field of the options object in order to resolve it (if it exist) - It was mentioned #1975 by @pcmanus that these error handling could be relaxed

Note: This was working for the prior version (v0 ?) of gateway

@SidKhanna296 SidKhanna296 changed the title Federation 2: Prevents @requires to accept fields with Nullable arguments Federation 2: Prevents @requires to accept fields with arguments - Allow it for Nullable Arguments Jul 15, 2022
@SidKhanna296 SidKhanna296 changed the title Federation 2: Prevents @requires to accept fields with arguments - Allow it for Nullable Arguments Federation 2 - Prevents @requires directive to accept fields with arguments - Allow it for Nullable Arguments Jul 15, 2022
@SidKhanna296
Copy link
Author

@pcmanus @benweatherman any input on this ?

@benweatherman
Copy link
Contributor

Howdy @SidKhanna296! Thanks for the thorough description. Yes, this makes sense to allow @requires fields to include params. In addition to your need for nullable fields, it probably makes sense to allow non-null arguments as well for @requires. We will probably limit using params with arguments to the @requires directive, since using that for other directives doesn't seem valuable in practice and could lead to unintended behaviors.

@SidKhanna296
Copy link
Author

@benweatherman thanks for the response!!
I know you guys are super busy, just circling back on if I could get an idea of when this would be implemented ?
or atleast the nullable part where we don't need to pass params for @requires
(as this kind of prevents us from upgrading our services for the time being)

@benweatherman
Copy link
Contributor

howdy @SidKhanna296! I don't have a good idea on when we'll be able to take a look, but I think it'll be sometime soon. Our team is starting to get back from holiday & other commitments, so might hopefully know more next week. Apologies o the delay, and not having a more concrete answer.

@pcmanus pcmanus self-assigned this Aug 31, 2022
@pcmanus
Copy link
Contributor

pcmanus commented Aug 31, 2022

Apologies for the long delay following up on this. I've just pushed #2120 to allow using fields with arguments in @requires in general. This had been disabled initially because I wanted to take the time to think about the consequences of allowing it and have proper testing. And there was a few additional validation holes that needed to be fixed to enable this properly, but this is in the linked PR.

pcmanus pushed a commit to pcmanus/federation that referenced this issue Sep 13, 2022
If such field with arguments is used in a @requires, then that @requires
must provide a value for any required argument and may provide a value
for any non-required one (like in any normal graphQL selection set).

Fixes apollographql#1987
@SidKhanna296
Copy link
Author

thank you for getting to this @pcmanus !!

@jeffjakub jeffjakub added this to the 2.1.2 milestone Nov 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants