@include and @skip directives lead to irrelevant errors

[DrumsnChocolate]

I am unsure whether to put this under bugs or feature requests, because I would consider this to be both.

The directives @include and @skip are awesome concepts, but they miss the mark when a certain field is not queryable. That is, if the boolean variable on which the @include directive is conditioned evaluates to false, then it would be reasonable to say that it is not relevant whether the field is queryable or not, because the field will simply be omitted in the result anyway.

Why is this important? Well, consider we have two APIs that are very similar and share a big part of their graphql schema definition. I work with django-graphene combined with relay, so I'll present my example use case with that context.
Suppose both APIs contain design logic for a fruit store. Both stores have a model they call Banana. One store also has a model called BananaBox, because the store has some kind of shipping system, whereas the other store does not define BananaBox because it does not ship bananas in boxes anywhere. The store that has BananaBox simply added a ForeignKey from Banana to BananaBox in order to keep track of which box a banana is in.
Both APIs define appropriate django-graphene Nodes for each model, and both APIs define a query endpoint for bananas.

Now, if we want to perform a query somewhat like the following:

query bananas {
	banana {
		id
		banana_box {
			id
			size
		}
	}
}
		

That query will fail when executed on the store without the BananaBox model. However, one elegant way of solving this problem would be to use the following query:

query bananas($fetch_box: Boolean!) {
	banana {
		id
		banana_box @include(if: $fetch_box) {
			id
			size
		}
	}
}

And then specifying fetch_box: true or fetch_box: false in the store with and without box respectively.
However, this will result in the following error for the store without the BananaBox: Cannot query field "banana_box" on type "BananaNode". I believe this error is irrelevant to answering the query, because having fetch_box: false clearly indicates to the server that we are not interested in the banana_box field.
The graphql documentation about directives mentions that the @include and @skip directives determine what fields are included in the result, and I would argue that excluding a field means that composing the result can be completed without knowledge of whether or not the field is queryable at all.

If this BananaNode has a lot of queryable attributes, and we are interested in all of them, defining the bananas query twice just because the two APIs differ by one field is quite silly and repetitive, especially when working with more than two similar APIs. If this use case seems far-fetched, please understand that it is really relevant to the application I am developing.

[erikwrede]

The @include and @skip behavior in Graphene closely follows what is defined in the GraphQL Specification. Firstly, after parsing the query string, the entire parsed AST is validated during the validation phase. Evaluation of the directives happens after the validation, during execution. Therefore we cannot change the behavior to your request, as this would break the rules of the spec. The purpose of validation is to ensure the type integrity of each query. Therefore, it does not make any sense to query fields that may or may not exist on your query, or to design your schema that way.

GraphQL is a strongly-typed language, and the schema serves as a contract between the client and server. In order for the server to fulfill that contract, it needs to know exactly what fields are queryable and what types they return. If a field is not queryable, it should not be included in the schema to begin with, and the server should not return it as an available field. Therefore, from a design perspective, it makes sense for the server to return an error if a non-queryable field is included in a query, regardless of whether it is excluded using the @include directive.

There's several ways to elegantly solve your issue using the tools GraphQL provides, under the premise that you can modify both APIs:

1. Mark your banana box field as nullable
   This way, you could have it on each query without the resolver failing in case it is not present. Since you already know when it is null on the client-side, you wouldn't even need to add any additional checks.

2. Use an Interface or Union
   You could model your Store as an interface with two Implementations:

interface CommonBanana {
  id: ID!
  common fields
}

type Banana {
  id: ID!
  common fields
}

type BoxableBanana {
  id: ID!
  common fields
  bananaBox: BananaBox
}
Type Query {
  banana: [CommonBanana]
}

And then query it like this:

query bananas {
	banana {
		id
        commonFields
        ... on BoxableBanana {
			banana_box {
				id
				size
			}
        }
	}
}

In any case, I'd strongly recommend exploring solutions such as Apollo Federation (implemented in graphene-federation) to stitch together the two APIs and prevent issues like this from arising. It's important to note that having two almost identical but separate GraphQL APIs is not an issue that can be solved in Graphene, but rather it's a matter of design decisions in your architecture that may need to be reconsidered. If that's not possible, the only way to avoid defining the query for each case is to modify the query AST on each request in your specific GraphQL client. Other than that, using fragments makes your life easier as well in this case 🙂

I hope my suggestions could provide some help, please feel free let me know if you have further questions.

Cheers

[DrumsnChocolate]

Thank you for the quick reply! And thanks a bunch for the clear recommendations. I think this helps me on my way sufficiently. I will definitely look into Apollo Federation, I hadn't heard about that yet.
Regarding our design decisions: I agree that the situation I described is quite specific and it may not be exactly what Graphene is designed to solve, but there is no other straightforward way for us to tackle our use case without introducing a lot of redundancy in our codebase.
Thanks a lot once more!