federation icon indicating copy to clipboard operation
federation copied to clipboard

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

Open SidKhanna296 opened this issue 2 years ago • 3 comments

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 avatar Jul 15 '22 19:07 SidKhanna296

@pcmanus @benweatherman any input on this ?

SidKhanna296 avatar Jul 20 '22 12:07 SidKhanna296

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.

benweatherman avatar Jul 22 '22 17:07 benweatherman

@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)

SidKhanna296 avatar Aug 05 '22 17:08 SidKhanna296

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.

benweatherman avatar Aug 12 '22 20:08 benweatherman

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 avatar Aug 31 '22 13:08 pcmanus

thank you for getting to this @pcmanus !!

SidKhanna296 avatar Sep 13 '22 12:09 SidKhanna296