schema assumeValid flag has two meanings

[yaacovCR]

Currently, if I understand the flow correctly, to skip schema validation during an execute call, one must create a schema with the assumeValid option (as opposed to an explicit execute option for this different execute behavior. Using the option sets an internal assumeValid flag, which is picked up by execute.

This same internal schema flag is set to true no matter what when the schema is actually validated, so that validation can be skipped when run on the same schema, even when the option is not set.

These two different use cases cause some problems for schema semantics when mapping schemas (ardatan/graphql-tools#4087). There is currently a bug in the graphql-tools mapping code where a schema that has been previously validated will carry over the flag to a new, potentially invalid schema. The fix would simply be to drop the flag, but that would cause issues for users who are using the option/flag explicitly.

Specifically, if users were creating a schema using the option/flag so as to avoid validation later, and then mapping it using mapSchema, the flag would now be dropped, and they would have to readd it using toConfig and creating a new schema. A new option could be added to mapSchema to add this exact behavior, fixing the issue there, and users could be educated that any execution level flags must be set on the final schema rather than the original schema. [This is especially true because multiple schemas can be created from an original schema

However, this flow seems awkward. An execution level flag should probably be settable at execution.

There is a related discussion with respect to defer/stream directives : graphql/defer-stream-wg#12 for a parallel issue/discussion.

However, there is an important difference there in that the schema is actually different when defer/stream is enabled, i.e. it creates the directives. Is there an actual different within the schema at generation time when assumeValid is set.

I expect I am missing some aspects/angles on this issue. The above just reflects my current thoughts, would be happy to hear any and all additional input!

[yaacovCR]

To be more explicit, I am suggesting that the assumeValid internal flag be renamed and refigured to only record when validation has already been run on that schema, and that the assumeValid option should be moved to execute.

[yaacovCR]

For comparison, graphql-executor no longer validates implicitly within execute at all, as this is a different phase of the lifecycle, and we (lol, I) assume that it should be handled by the server/framework as such.

[IvanGoncharov]

@yaacovCR yes, the current state is a mess.
I propose to split validation into two parts:

1. Basic things that should be valid on "partial schema". For example, validating that root type is an Object type should be done in GraphQLSchema constructor and other similar checks should be done in appropriate GraphQL* constructors. Also, I don't see any use case to disable this validation at all.
2. Stuff that should be valid only "executable schema" (e.g. query type is present should be done as the last step of initializing server. And omitted in some cases, e.g. you want to construct partial schema and pass it somewhere. For how to execute this validation, I proposed solution here: Inconsistent handling of directives in buildASTSchema and buildClientSchema #3419

[yaacovCR]

Thinking more about the above, I think we may benefit from an intermediary abstraction.

We may have 1. a schema and 2. several additional request directives with possibly new input types for the arguments. The schema can validate as a schema. The second collection of types/directives is a cohesive unit that should validate separately as a GraphQLSchemaSubset or something.

A schema should be able to be composed of multiple internally validated SchemaSubsets with validation of the overall schema only checking to make sure the schema subsets do not conflict.

We do not want to create a new schema for just a few additional directives. We want to use what we have as much as possible, so cold start time is as close to 0ms as possible.

I think it's very important to prove that any additional abstraction actually improves performance. Benchmarking is believing!

[in theory, a similarly intermediate abstraction could help us merge fields across different "GraphQLObjectTypeSubset". I am skeptical so far of how this would work in a practically performant way, but it's an idea lurking there. The above discussion assumes types are not merged at all, even root query types, for example]

[yaacovCR]

Fixed more simply than my original suggestion by #4244 => without any changes in behavior.