bugfix: adding the missing operation name

[tapaderster]

This is related to an old issue that cropped up in version 5.
#522

[tapaderster]

Also what is the best way to backport this fix for version 5.0.0?

[yaacovCR]

Thanks for submitting PR!

I think there should also be an option for delegate to schema, with using the original operation name as the default, like in v4.

Are you able to add that?

[tapaderster]

This is what i see being done in version 4.
https://github.com/ardatan/graphql-tools/blob/v4.0.8/src/stitching/delegateToSchema.ts#L62

The request being created with the rawDocument already contains the operation name.

in latest, the request is being created here. And that where I tried to add the op name.

Sorry, I am not extremely familiar with the lib, if you mean something else.

[yaacovCR]

Agreed as to the v4 behavior. SPR returns to the old behavior, whereas I am suggesting that the operation name be configurable.

Upon further reflection, I'm questioning whether a defaulting to the gateway operation name makes sense. The operation on the gateway may delegate to the same subschema with different operations entirely. How is it safe to use the same operation name for all of them?

Maybe a prefix would be best or some way to specify dynamically or otherwise the operation name while stitching?

I assume you are using the operation name for query caching?

I might be off base here as I do not use query caching or really operation names extensively.

Could you maybe elaborate on your use case and why this default makes sense?

[smyrick]

@yaacovCR We were under the assumption that the upgrade from v4 to v5 would not remove the operation name. As we currently forward the operation name to the downstream GraphQL services, some of them have used that for their logging, tracking, and even logic in some cases.

We didn't want to make them change to use some header or custom value as this was working for us in v4. If you want to add some additional optional logic that is fine, but as long as we can get the same behaviour as v4 that would resolve our issues

[yaacovCR]

How do your downstream services handle the fact that they may receive different queries with the same operation name if they originate from the same name operation on gateway? Do you only use stitching for combining subschemas rather than adding connecting resolvers?

I think the new default should not preserve existing behavior that breaks stitching

[smyrick]

@yaacovCR That is up to the downstream service to handle, but we do control the operations that come in to some degree by using a client registry so that we can't just get random queries. However, even if we didn't have this, since the operation name is just debugging information I don't think it really breaks stitching to include the original operation name to a stitched resolver. This can be helpful for matching up logs in the gateway and downstream services to the same requests vs checking the GraphQL fields requested.

[yaacovCR]

Isn't the operation name sometimes used for persistent queries and storing the query for later use?

[smyrick]

@yaacovCR That is another use case and right now there is no operation name being passed so we couldn't utilize the query cache in that case

[yaacovCR]

The reason why I bring up that use case in particular is that I think just passing the existing gateway operation name has the default will break stitching for the sub schema

We need a default option that doesn't do that

If you want to submit a PR that allows you to easily specify the v4 behavior that exposes this potential bug anyway because you are not concerned for your particular use case, that is fine, but I'm concerned about introducing a default that doesn't work by default

[yaacovCR]

I guess we need someone using persisted queries for schema stitching to weigh in on whether there is an existing bug or are any workarounds available

[yaacovCR]

It might make sense to revert to v4 behavior even if broken, but I do think we should explore first if there is an easy fix that we can move to

[smyrick]

Does it break stitching? The operation name is just for logging purposes, and we probably should have any of the logic in this library based on the operation name, for the same reasons you mentioned above.

However why is forwarding the operation name going to affect the stitched service? If in v6 is wasn't getting any operation name and we add it back in, then this is just more logging information for the service to output.

[yaacovCR]

If someone is using a version of persisted or stored queries that caches by operation name, using the same operation name for each operation sent to a subschema, when there may be different operations sent to the same subschema, may be a problem.

No?

Is no one actually doing this in the wild?

[smyrick]

using the same operation name for each operation sent to a subschema

Currently there is no operation name so it is not cacheable at all by name. What we are fixing is forwarding the operation name that came into the gateway as the same operation name to the downstream service.

ie

If I have the following services and stitch them together

# Service A
type Query {
  foo: String
}

# Service B
type Query {
  bar: String
}

I could make the following query as a client

query GetFooAndBar {
  foo
  bar
}

I would expect that Service A receives the following query

query GetFooAndBar {
  foo
}

when instead it just gets

{
  foo
}

[yaacovCR]

I suppose this is not a major issue. My quick research into this topic does not see anyone actually implementing stored queries with the key based solely on the operation name.

[yaacovCR]

Alternatively, you have the following set of schemas, modified from the basic example in docs:

# Service A

    type Chirp {
      id: ID!
      text: String
      authorId: ID!
      reviewerId: ID!
    }

    type Query {
      chirpById(id: ID!): Chirp
      chirpsByAuthorId(authorId: ID!): [Chirp]
      chirpsByReviewerId(reviewerId: ID!): [Chirp]
    }

# Service B
    type User {
      id: ID!
      email: String
    }

    type Query {
      userById(id: ID!): User
    }

# Gateway

  extend type User {
    chirps: [Chirp]
  }

  extend type Chirp {
    author: User
    reviewer: User
  }

I could make the following query as a client

query GetChirpById($id: ID) {
  chirpById(id: $id) {
    id
    author {
      id
    }
    reviewer {
      email
    }
  }
}

Under the v4 behavior, the gateway will receive stitched subquerys that look like this:

query GetChirpById($id: ID) {
 userById(id: $id) {
    id
  }
}

and this:

query GetChirpById($id: ID) {
 userById(id: $id) {
    email
  }
}

But what if the subschema is caching the query based on the operation name, and the executor sends just the operation name and not the actual query? Things would go downhill.

From my quick review, it seems like queries are generally cached based on a hash of the entire query, rather than solely on the basis of the operation name, even though that use case for the operation name is discussed here and there.

[yaacovCR]

By the way, I understand that it currently doesn't work at all... I am just suggesting that instead of reverting to the broken behavior, it makes sense to use the opportunity of a major version bump to think of the right behavior where it would work.

[yaacovCR]

Maybe a solution might be to add an argument to delegateToSchema with an operationName that can be hardcoded as a string or consist of a user-defined function that can transform the gateway operation name as necessary?

[smyrick]

consist of a user-defined function that can transform the gateway operation name as necessary

This would work for us, and if you wanted to keep the default to return undefined as it currently is, then we can customize the name to be from v4 as we do not have the conflicting/duplicate query problems.

[mgilbey]

what if the subschema is caching the query based on the operation name

This looks to be incorrect caching behaviour, the query and variables would be the cache key to use. If the schema is being delegated to a remote service, then the persistent query would have already been fetched upstream in the gateway or before.

the executor sends just the operation name and not the actual query

I don't believe there's any specification for this. Are you suggesting that there are nested persisted queries? There's no way the gateway can map from a query to the operation-name/persisted-query of another service.

[yaacovCR]

Closing for now in favor of #1700.