Parsing Failure with Variables in Fragments of Non-Executed Queries in the Same Payload

mostafao-spotify commented 1 month ago

Describe the bug

We're running the router image on a Kubernetes cluster. When we send multiple queries in the payload to the router, and specify only one of those queries to be executed using operationName, we encounter an error. This occurs when there is a fragment in the non-executed query contains variables. The error message indicates that the variable is of a different type or is provided incorrectly, as shown in the example below.

To Reproduce

Steps to reproduce the behavior:

Set up the services '...'

Submit the following request:


fragment section1 on MainSection {
sectionItems(offset: $sectionItemsOffset, limit: $sectionItemsLimit) {
items {
  data {
    ... on ActivationItemData {
      activated
    }
  }
}
}
}

fragment section2 on MainSection { data { ... on ActivationItemData { activated } } }

query query2($var2: String!, $sectionItemsOffset: Int!, $sectionItemsLimit: Int!) { field2( input: {someInputVar2: $var2} ) { __typename ... on ResponsePayload1 { sectionContainer { sections(offset: 0, limit: 20) { items { ...section1 } } } } } }

query query1($var1: String!) { field1( input: {someInputVar1: $var1} ) { __typename ... on ResponsePayload1 { ...section2 } } }


And the variables are:
```json
{
  "var1": "StringValue"
}

We use the operation query1, but the returned error complains about variables in query2:

{
"errors": [
{
  "message": "Invalid value $sectionItemsOffset for argument \"MainSection.sectionItems(offset:)\" of type Int",
  "extensions": {
    "code": "GRAPHQL_VALIDATION_FAILED"
  }
}
]
}

Expected behavior

We expected to get back the query1 result as expected.

Output

See the errors in step 3. I suspect that the error is due to the variable name being interpreted as the actual value, ignoring the variable value in the fragments.

Additional context

We're using the router in a container on a Kubernetes cluster.

SimonSapin commented 1 month ago

Hi, thank you for this bug report. It’s expected that the entire document is validated, not just the operation specified in the request’s operationName. Validation rules typically start with “For each operation in document”.

That said, this still looks like a bug. Can you confirm: in your schema, is the expected type for the offset argument of the Mainsection.SectionItems field nullable Int like the error message suggests? A variable usage of type non-null Int! should be compatible to use there.

Interestingly, the phrasing of the error message shows that it comes from JavaScript code whereas Apollo Router switched to Rust-based validation by default in 1.45.0. What version of the Router are you using? To investigate this further we’ll likely need to reproduce the error. Could you provide a schema (minimized or not) where it happens?

abernix commented 1 month ago

Any updates to provide here, @mostafao-spotify? Could you add any available answers to the above, including the Router version?

mostafao-spotify commented 1 month ago

Hi @SimonSapin, thanks a lot for investigating this and sorry for the delayed reply, we're working with the router version 1.48.1 and after updating to 1.49.0 the issue still persists.

The offset and limit field in the schema is indeed nullable as the error message suggests, and this is probably by design as null means something to that service, but I think even if we make the field non-nullable that won't solve the underlying issue of mis-interpreting the value.

Notably, one of our team member found this piece of code here that indicates that there's still a javascript bridge in the latest version of the code which we're using, or is this just a non-relevant comment maybe?

As for the schema setup, it looks something like below, but please take it with a grain of salt, as I've tried to obfuscate some of domain details :)

# Query
type Query {
  field1(input: Field1Input!): ResponsePayload1!
  field2(input: Field2Input!): ResponsePayload1!
}

# input
input Field1Input {
  someInputVar1: String!
}

input Field2Input {
  someInputVar2: String!
  sectionItemsOffset: Int,
  sectionItemsLimit: Int
}

# Types
type ResponsePayload1 {
  data: [ActivationItemData!]!
  sectionContainer: SectionContainer!
}

type ActivationItemData {
  activated: Boolean!
}

type SectionContainer {
  sections(offset: Int, limit: Int): SectionItemsConnection!
}

type SectionItemsConnection {
  items: [MainSection!]!
}

type MainSection {
  sectionItems(offset: Int, limit: Int): SectionItemsPage!
}

type SectionItemsPage {
  items: [SectionItem!]!
  pageInfo: PageInfo!
}

type PageInfo {
  offset: Int!
  limit: Int!
}

type SectionItem {
  data: ActivationItemData
}

SimonSapin commented 1 month ago

Regarding the bridge: validation of incoming queries has moved to Rust but JavaScript is still used for some other components including query planning. We’re porting them to Rust one by one.

Thank you for the schema, with it I managed to the produced the error on the latest dev branch of the Router. However this still needs more investigation. Here is what I have so far:

I composed the schema above into a supergraph schema with Rover and reduced the operation slightly:

supergraph.graphql

```graphql schema @link(url: "https://specs.apollo.dev/link/v1.0") @link(url: "https://specs.apollo.dev/join/v0.4", for: EXECUTION) { query: Query } directive @join__directive(graphs: [join__Graph!], name: String!, args: join__DirectiveArguments) repeatable on SCHEMA | OBJECT | INTERFACE | FIELD_DEFINITION directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION directive @join__graph(name: String!, url: String!) on ENUM_VALUE directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true, isInterfaceObject: Boolean! = false) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA type ActivationItemData @join__type(graph: A) { activated: Boolean! } input Field1Input @join__type(graph: A) { someInputVar1: String! } input Field2Input @join__type(graph: A) { someInputVar2: String! sectionItemsOffset: Int sectionItemsLimit: Int } scalar join__DirectiveArguments scalar join__FieldSet enum join__Graph { A @join__graph(name: "A", url: "https://example.com") } scalar link__Import enum link__Purpose { """ `SECURITY` features provide metadata necessary to securely resolve fields. """ SECURITY """ `EXECUTION` features provide metadata necessary for operation execution. """ EXECUTION } type MainSection @join__type(graph: A) { sectionItems(offset: Int, limit: Int): SectionItemsPage! } type PageInfo @join__type(graph: A) { offset: Int! limit: Int! } type Query @join__type(graph: A) { field1(input: Field1Input!): ResponsePayload1! field2(input: Field2Input!): ResponsePayload1! } type ResponsePayload1 @join__type(graph: A) { data: [ActivationItemData!]! sectionContainer: SectionContainer! } type SectionContainer @join__type(graph: A) { sections(offset: Int, limit: Int): SectionItemsConnection! } type SectionItem @join__type(graph: A) { data: ActivationItemData } type SectionItemsConnection @join__type(graph: A) { items: [MainSection!]! } type SectionItemsPage @join__type(graph: A) { items: [SectionItem!]! pageInfo: PageInfo! } ```

operation.graphql

```graphql fragment section1 on MainSection { sectionItems(offset: $sectionItemsOffset, limit: $sectionItemsLimit) { items { data { ... on ActivationItemData { activated } } } } } query query2($var2: String!, $sectionItemsOffset: Int!, $sectionItemsLimit: Int!) { field2( input: {someInputVar2: $var2} ) { __typename ... on ResponsePayload1 { sectionContainer { sections(offset: 0, limit: 20) { items { ...section1 } } } } } } query query1 { field1( input: {someInputVar1: ""} ) { __typename } } ```

❯ curl --request POST \
  --header 'content-type: application/json' \
  --url 'http://127.0.0.1:4000/' \
  --data @<(jq -n --rawfile op ~/tmp/operation.graphql '{query: $op, operationName: "query1"}')
{"errors":[{"message":"Invalid value $sectionItemsOffset for argument \"MainSection.sectionItems(offset:)\" of type Int","extensions":{"code":"GRAPHQL_VALIDATION_FAILED"}}]}

I happened to have my local Router configured with experimental_query_planner_mode: both for another task which added a log entry showing that the Invalid value $sectionItemsOffset is coming from the JavaScript query planner, after we’ve already validated the origin query. I see JS planner code in FetchGroup.finalizeSelection call validate() on the generated subgraph query as a sanity check. I suspect a bug in the planner makes it generate something invalid. I tried the standalone query planner outside of Router to more easily debug JS code but didn’t manage to reproduce the error there. (I get a query plan that looks correct and no error.)

mostafao-spotify commented 1 month ago

Thank you @SimonSapin for looking into this. It's great news to hear that you're moving away from JS Bridge! 🚀 If I can help with anything else, let me know. 🎈

apollographql / router