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

Explicitly mention in the docs that @requires fields cant depend on fields with arguments #2276

Open
rickbijkerk opened this issue Nov 23, 2022 · 9 comments
Assignees

Comments

@rickbijkerk
Copy link

While it makes sense the way it works, I had to find out myself that you can only depend on fields which dont have arguments.

A good place to add would be here: https://www.apollographql.com/docs/federation/federated-types/federated-directives/#requires

@pcmanus
Copy link
Contributor

pcmanus commented Nov 23, 2022

Hi @rickbijkerk, thanks for the report. This behaviour was actually changed "recently" with #2120 and from federation 2.1.2 onward, you should be able to @require a field with arguments, but you would have to pass a concrete argument inside the @require. But maybe you mean that you cannot@require the field itself, but rather only a specific "application" of it for a specific argument.

In any case, I agree that we should better document all of this. And in fact, we should document this for @requires but also @provides and @key. @StephenBarlow: would you be able to find a few cycles to look at this at some point?

And just to summarise the status quo, if you have field with argument, say f(arg: Int!): Int, then currently:

  1. you cannot just @require(fields: "f") (you'll get an error) but since 2.1.2, you can @require(fields: "f(arg: 42)"). Do note that this is because the argument is mandatory. If you have a field g(arg: Int! = 0): Int, then @require(fields: "g") is allowed (since 2.1.2 again, before that no field with arguments could be used in @require at all).
  2. fields with arguments are not allowed in @provides or @key at all. We don't 100% exclude to lift the limitation like we did for @require with Allow fields with arguments in @requires #2120 in the future, but we're a bit less sure that it's more useful than it is confusing for those directives, so we're holding off on getting a good use case for it.

@rickbijkerk
Copy link
Author

Hi @pcmanus Thanks for the elaborate response this really helps a lot! 👍

@agarwal-b-ankit
Copy link

agarwal-b-ankit commented Jan 27, 2023

@pcmanus We should also document what would happen if the user explicitly requests for the field inside the @requires directive with a different argument value than what's defined inside @requires directive selection set.
e.g.,
// schema
type Entity{
fieldA(first: Int) { dummyVal: String } @external
fieldB @requires(fields: "fieldA(first: 50)")
}

// selection set when client requests for both fieldA & fieldB with different arguments
{
fieldA(first: 100) { dummyVal }
fieldB
}

Here, behaviour is ambiguous and it's not defined if sub-graph which provides fieldA will receive request for first 100 or first 50 .

@pcmanus
Copy link
Contributor

pcmanus commented Jan 30, 2023

@agarwal-b-ankit That's a very good callout, and the honest answer is that I failed to properly think about this case, and I'll have to double check this more closely.

But the intent in my mind is that the subgraph with the @provides would always get the field queried with what is in the @provides, as it feels too hard to reason about otherwise. Plus, through aliasing, a request may well have multiple calls to the same field with different arguments, so "reusing" the arguments of the queries feels pretty ambiguous.

I'm just not sure it quite works at the moment and need to do some more testing.

@agarwal-b-ankit
Copy link

Yes, I agree that the arguments defined inside the @requires should be respected, even at the cost of multiple calls. In my example, we only had 1 argument, but this can get really messy when we have more number of arguments and possibly have nested selection sets (which could potentially have arguments as well). I don't see a good way to merge the calls into one call.
Let me know if you think @requires can evolve to support this or not. I am sure it would take some effort to figure out more details though.

Also, I tried to test this out by setting up federation on my local and found that only one call is made and it seemed that the explicitly provided arguments are favored. However, when I try to generate query plan in apollo-workbench, I see the opposite behavior.

@sMorm
Copy link
Contributor

sMorm commented Jun 27, 2023

Hi @pcmanus on a similar note, have we thought about supporting dynamic variables in the @requires fields? For example, this is the query:

query ProductWithAvailability($postalCode: String!) {
  getProduct {
    productId
    availability(postalCode: $postalCode)
  }
}

On the schema side, postalCode isn't a field readily available as it should be provided by clients. Here's an example of the schema:

type Query {
   getProduct: Product
}

type ProductInventory {
  quantitiesAvailable: Int!
}

type ProductAvailabilityMessage {
  message: String!
}

type Product @key(fields: "productId") {
  productId: Int!
  inventory(postalCode: String!): ProductInventory
  availabilityMessage: ProductAvailabilityMessage @requires(fields: "inventory($postalCode) { quantitiesAvailable }")
}

It would be really nice here if we could use a dynamic variable when using @requires.

Another thought is, what if postalCode was an actual field that we could pass into @requires?

type Product {
  productId: Int!
  postalCode: String!
  inventory: ProductInventory
  availabilityMessage: ProductAvailabilityMessage @requires(fields: "inventory(postalCode) { quantitiesAvailable }")
}

@smyrick
Copy link
Member

smyrick commented Mar 18, 2024

To clarify for those following along later. The requires directive does support arguments, but there is still an issue as documented in the above comment that if the client adds the field to the operation and overrides the arguments, the query planner will default to the client arguments, not the ones specified in the schema

Schema 1

type Query {
    locations: [Location!]!
}

type Location @key(fields: "id"){
    id: ID!
    name: String!
    photo(smallVersion: Boolean): String!
}

Schema 2

type Location @key(fields: "id") {
    id: ID!
    photo(smallVersion: Boolean): String! @external
    overallRating: Float @requires(fields: "photo(smallVersion: true)") # Force the smallVersion here
}

This does compose but at runtime if the operation is so

query Locations {
  locations {
    id
    photo
    overallRating
  }
}

Then the query plan looks like this

QueryPlan {
  Sequence {
    Fetch(service: "locations") {
      {
        locations {
          __typename
          id
          photo
        }
      }
    },
    Flatten(path: "locations.@") {
      Fetch(service: "reviews") {
        {
          ... on Location {
            __typename
            id
            photo
          }
        } =>
        {
          ... on Location {
            overallRating
          }
        }
      },
    },
  },
}

VS not having the client select the field

query Locations {
  locations {
    id
    overallRating
  }
}

and the query plan looks like this

QueryPlan {
  Sequence {
    Fetch(service: "locations") {
      {
        locations {
          __typename
          id
          photo(smallVersion: true)
        }
      }
    },
    Flatten(path: "locations.@") {
      Fetch(service: "reviews") {
        {
          ... on Location {
            __typename
            id
            photo
          }
        } =>
        {
          ... on Location {
            overallRating
          }
        }
      },
    },
  },
}

@smyrick
Copy link
Member

smyrick commented Mar 18, 2024

We should update the title here @apollographql/engineering

@spurrkins
Copy link

spurrkins commented Mar 19, 2024

I've run into this same issue. Whatever arguments the client specifies squashes whatever is expressed in @ requires, resulting in the incorrect value being passed to the subgraph that has the @ requires statement.

I think that in the case of mismatching arguments, the router should probably use aliases or a similar strategy to request both. This may have some caching implications for some use cases, but I think that's preferable to providing incorrect inputs to subgraphs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants