Fixed the validation result cache serving one document's result for another

Packages.props

@@ -22,6 +22,7 @@
     <PackageReference Update="Giraffe" Version="7.*" />
     <PackageReference Update="IcedTasks" Version="0.11.*" />
     <PackageReference Update="Microsoft.Bcl.AsyncInterfaces" Version="$(SystemVersion)" />
+    <PackageReference Update="Microsoft.Extensions.Caching.Memory" Version="$(MicrosoftExtensionsVersion)" />
     <PackageReference Update="Microsoft.Extensions.Http" Version="$(MicrosoftExtensionsVersion)" />
     <PackageReference Update="Microsoft.Extensions.Logging.Abstractions" Version="$(MicrosoftExtensionsVersion)" />
     <PackageReference Update="Microsoft.NETCore.Platforms" Version="$(SystemVersion)" />

RELEASE_NOTES.md

@@ -307,6 +307,13 @@
 * Added `Microsoft.Bcl.AsyncInterfaces` dependency of `FSharp.Data.GraphQL.Shared` for `netstandard2.0`
 * Added `Human.friendsStream` field to the Star Wars sample to demonstrate `@stream`
 * Added the validation rules of incremental delivery: `@stream` only on list fields, no `@defer` or `@stream` in a subscription operation or on a mutation root field unless disabled with `if: false`, and labels must be string literals unique within each operation, counting the fragments it spreads
+* **Breaking Change** Fixed the validation result cache serving the result of one document for another: it identified results by the 32-bit structural hash codes of the document and of the introspected schema, which different documents share, such as `{ f(x: 0) }` and `{ f(x: 4294967297) }`, so an invalid document could pass validation with the cached result of a valid one. `ValidationResultKey` now holds the `Document`, compared structurally, and the `IntrospectionSchema` instance, compared by reference, instead of the `DocumentId` and `SchemaId` hash codes; its hash code only buckets keys and is mixed with a secret seed chosen per process, so that clients cannot craft many documents sharing one. `ExecutionPlan.DocumentId` is unchanged
+* Fixed `MemoryValidationResultCache` validating a document once per concurrent request for it while it was not cached yet; concurrent requests now share a single validation, whether its result ends up cached or not
+* Fixed the sliding expiration of `MemoryValidationResultCache` and of the client provider's design-time caches, which expired an entry 30 seconds after it was added however often it was used, because a cache hit did not refresh its last use. A provider used at least every 30 seconds therefore keeps its schema, and picks up a changed introspection file or server only once it has gone unused for 30 seconds
+* Fixed every `MemoryValidationResultCache`, including the one an `Executor` creates when given none, starting a timer that was never stopped and kept the cache and all its entries alive until the process exited; expired entries are now removed while the cache is used
+* Changed `MemoryValidationResultCache` and the client provider's design-time cache of provided types to hold their entries in a `MemoryCache` of `Microsoft.Extensions.Caching.Memory`, which `FSharp.Data.GraphQL.Shared` now depends on, instead of in a cache of the library's own. The new constructors `MemoryValidationResultCache (cache)` and `MemoryValidationResultCache (cache, slidingExpiration)` take the `IMemoryCache` to use, so that an application can pass one it configures itself, such as a keyed service; it should hold validation results only and have a `SizeLimit` in bytes
+* Added a size limit to `MemoryValidationResultCache`, which now holds the documents of its keys: every entry declares the estimated memory of its document in bytes, as `ValidationResultKey.DocumentSize` counts it (a fixed amount per node, and two bytes per character of names and strings), as its size. A cache created without an `IMemoryCache` limits the total to `MemoryValidationResultCache.DefaultSizeLimit` (16 MiB) or to the `sizeLimit` of the new constructor `MemoryValidationResultCache (slidingExpiration, sizeLimit)`: an entry that does not fit is not cached, the least recently used entries are evicted to make room for the next ones, and a document larger than the whole limit is validated on every request without being cached
+* Changed `Executor` to read `ISchema.Introspected` once, when it is created, instead of computing the structural hash code of the whole introspected schema on every request
 * Added `GraphQLTransportWS.SubProtocol`, the `graphql-transport-ws` sub-protocol name
 * Fixed `graphql-transport-ws` delivery of `@defer` and `@stream` results, which are now sent as soon as they are produced instead of after a fixed 5 second delay, followed by a final payload with `hasNext: false`
 * Changed `graphql-transport-ws` incremental delivery of `@defer` and `@stream` results to the `pending`/`incremental`/`completed`/`hasNext` wire format used by graphql-js 17 and Apollo Client's `GraphQL17Alpha9Handler`, superseding the previous `data`/`path`/`hasNext` shape. Every deferred or streamed field is announced once, in a `pending` entry, and identified afterwards by a short id instead of its path. A deferred field is announced in the same payload as its own value, while a streamed field is announced as soon as the payload exposing its containing data is sent. A `@stream` field's items are always delivered to the client in list order, buffering an item that arrives out of turn until the item before it fills the gap, and a batch of items (grouped by `preferredBatchSize` or `StreamBatching`) is delivered as the `items` of a single `incremental` entry addressed by that id, rather than one payload per item

docs/execution-pipeline.md

@@ -29,7 +29,7 @@ As the name suggests, `ExecutionPlan` and its components (a tree of objects know
 - Combining information from the query AST (resolved fields / aliases) with server-side information about them (field and type definitions);
 - Preparation of the hooks in the execution chain that will be supplied with potential variables upon execution.
 
-Splitting planning and execution phases is a good idea when you have the same GraphQL query requested many times (with potentially different variables). This way you can compute the execution plan once and cache it. You can use `executionPlan.DocumentId` as a cache identifier. `DocumentId` is also returned as one of the top level fields in the response, so it can be used from the client side. Other GraphQL implementations describe that technique as **persistent queries**.
+Splitting planning and execution phases is a good idea when you have the same GraphQL query requested many times (with potentially different variables). This way you can compute the execution plan once and cache it. Key the cache by the query text or the parsed document itself, not by `executionPlan.DocumentId` alone: that is a hash code, which different documents can share, so a cache keyed by it could execute a document that was never validated with the plan of another. `DocumentId` is also returned as one of the top level fields in the response, but for the same reason a client must not treat it as the identity of a document. Other GraphQL implementations build **persisted queries** on this technique, and identify a document by a cryptographic hash of its text.
 
 ## Execution phase 
 
@@ -52,7 +52,7 @@ The execution phase can be performed using one of the two strategies:
 
 The result of a GraphQL query execution is a `GQLResponse` object with the following fields:
 
-- `documentId`: which is the hash code of the query's AST document - it can be used to implement execution plan caching (persistent queries).
+- `documentId`: which is the hash code of the query's AST document. Different documents can share it, so on its own it identifies neither a document nor its execution plan.
 - `data`: optional, a formatted GraphQL response matching the requested query (`KeyValuePair seq`). Absent in case of an error that does not allow continuing processing and returning any GraphQL results.
 - `errors`: optional, contains a list of errors (`GQLProblemDetails`) that occurred during query execution.
 

src/FSharp.Data.GraphQL.Client.DesignTime/DesignTimeCache.fs

@@ -6,6 +6,7 @@ namespace FSharp.Data.GraphQL
 #if IS_DESIGNTIME
 
 open System
+open Microsoft.Extensions.Caching.Memory
 open FSharp.Data.GraphQL.Client
 open ProviderImplementation.ProvidedTypes
 open FSharp.Data.GraphQL.Validation
@@ -19,10 +20,16 @@ type internal ProviderKey =
       ExplicitOptionalParameters: bool }
 
 module internal ProviderDesignTimeCache =
-    let private expiration = CacheExpirationPolicy.SlidingExpiration(TimeSpan.FromSeconds 30.0)
-    let private cache = MemoryCache<ProviderKey, ProvidedTypeDefinition>(expiration)
+    let private slidingExpiration = TimeSpan.FromSeconds 30.0
+    // The keys are the static arguments of the providers of a project, which the developer writes, so the cache needs no size limit
+    let private cache = new MemoryCache (MemoryCacheOptions ())
     let getOrAdd (key : ProviderKey) (defMaker : unit -> ProvidedTypeDefinition) =
-        cache.GetOrAddResult key defMaker
+        cache.GetOrCreate (
+            key,
+            fun entry ->
+                entry.SlidingExpiration <- Nullable slidingExpiration
+                defMaker ()
+        )
 
 module internal QueryValidationDesignTimeCache =
     let cache : IValidationResultCache = upcast MemoryValidationResultCache()

src/FSharp.Data.GraphQL.Client.DesignTime/ProvidedTypesHelper.fs

@@ -799,9 +799,9 @@ module internal Provider =
                                     match validationResult with
                                     | ValidationError msgs -> failwith (formatValidationExceptionMessage msgs)
                                     | Success -> ()
-                                let key = { DocumentId = queryAst.GetHashCode(); SchemaId = schema.GetHashCode() }
                                 let refMaker = lazy Validation.Ast.validateDocument schema queryAst
                                 if clientQueryValidation then
+                                    let key = ValidationResultKey (schema, queryAst)
                                     refMaker.Force
                                     |> QueryValidationDesignTimeCache.getOrAdd key
                                     |> throwExceptionIfValidationFailed

src/FSharp.Data.GraphQL.Server/Executor.fs

@@ -99,6 +99,10 @@ type Executor<'Root>(schema: ISchema<'Root>, middlewares : IExecutorMiddleware s
         | Success -> ()
         | ValidationError errors -> raise (GQLMessageException (System.String.Join("\n", errors)))
 
+    // Read once, after the compile middlewares above have run: documents are validated against this instance, and the
+    // validation cache identifies the schema by it instead of hashing the whole introspected schema on every request
+    let introspectedSchema = schema.Introspected
+
     let eval (executionPlan: ExecutionPlan, data: 'Root voption, variables: ImmutableDictionary<string, JsonElement>, getInputContext : InputExecutionContextProvider): Async<GQLExecutionResult> =
         let documentId = executionPlan.DocumentId
         let prepareOutput res =
@@ -159,9 +163,9 @@ type Executor<'Root>(schema: ISchema<'Root>, middlewares : IExecutorMiddleware s
                             ErrorKind.Validation
                         )]
                 do!
-                    let schemaId = schema.Introspected.GetHashCode()
-                    let key = { DocumentId = documentId; SchemaId = schemaId }
-                    let producer = fun () -> Validation.Ast.validateDocument schema.Introspected ast
+                    // The document itself is the key, not its documentId: that is a hash code, which another document can share
+                    let key = ValidationResultKey (introspectedSchema, ast)
+                    let producer = fun () -> Validation.Ast.validateDocument introspectedSchema ast
                     validationCache.GetOrAdd producer key
                 let planningCtx =
                     { Schema = schema
@@ -183,9 +187,9 @@ type Executor<'Root>(schema: ISchema<'Root>, middlewares : IExecutorMiddleware s
 
     /// <summary>
     /// Asynchronously executes a provided execution plan. In case of repetitive queries, execution plan may be preprocessed
-    /// and cached using `documentId` as an identifier.
+    /// and cached, keyed by the document itself rather than by its `documentId`.
     /// Returned value is a readonly dictionary consisting of following top level entries:
-    /// 'documentId' (unique identifier of current document's AST, it can be used as a key/identifier of ExecutionPlan as well),
+    /// 'documentId' (hash code of current document's AST, which different documents can share, so it identifies neither a document nor its ExecutionPlan),
     /// 'data' (GraphQL response matching the structure provided in GraphQL query string), and
     /// 'errors' (optional, contains a list of errors that occurred while executing a GraphQL operation).
     /// </summary>
@@ -198,7 +202,7 @@ type Executor<'Root>(schema: ISchema<'Root>, middlewares : IExecutorMiddleware s
 
     /// <summary>
     /// Asynchronously executes parsed GraphQL query AST. Returned value is a readonly dictionary consisting of following top level entries:
-    /// 'documentId' (unique identifier of current document's AST, it can be used as a key/identifier of ExecutionPlan as well),
+    /// 'documentId' (hash code of current document's AST, which different documents can share, so it identifies neither a document nor its ExecutionPlan),
     /// 'data' (GraphQL response matching the structure provided in GraphQL query string), and
     /// 'errors' (optional, contains a list of errors that occurred while executing a GraphQL operation).
     /// </summary>
@@ -216,7 +220,7 @@ type Executor<'Root>(schema: ISchema<'Root>, middlewares : IExecutorMiddleware s
 
     /// <summary>
     /// Asynchronously executes unparsed GraphQL query AST. Returned value is a readonly dictionary consisting of following top level entries:
-    /// 'documentId' (unique identifier of current document's AST, it can be used as a key/identifier of ExecutionPlan as well),
+    /// 'documentId' (hash code of current document's AST, which different documents can share, so it identifies neither a document nor its ExecutionPlan),
     /// 'data' (GraphQL response matching the structure provided in GraphQL query string), and
     /// 'errors' (optional, contains a list of errors that occurred while executing a GraphQL operation).
     /// </summary>
@@ -233,21 +237,35 @@ type Executor<'Root>(schema: ISchema<'Root>, middlewares : IExecutorMiddleware s
         | Ok executionPlan -> execute (executionPlan, data, variables, getInputContext)
         | Error (documentId, errors) -> async.Return <| GQLExecutionResult.Invalid(documentId, errors, meta)
 
+    /// <summary>
     /// Creates an execution plan for provided GraphQL document AST without
     /// executing it. This is useful in cases when you have the same query executed
     /// multiple times with different parameters. In that case, query can be used
-    /// to construct execution plan, which then is cached (using DocumentId as a key) and reused when needed.
+    /// to construct execution plan, which then is cached and reused when needed.
+    /// </summary>
+    /// <remarks>
+    /// Key a cache of execution plans by the document itself, not by <see cref="ExecutionPlan.DocumentId"/> alone: that is
+    /// a hash code, which different documents can share, so a cache keyed by it could execute a document that was never
+    /// validated with the plan of another.
+    /// </remarks>
     /// <param name="ast">The parsed GraphQL query string.</param>
     /// <param name="operationName">The name of the operation that should be executed on the parsed document.</param>
     /// <param name="meta">A plain dictionary of metadata that can be used through execution plan customizations.</param>
     member _.CreateExecutionPlan(ast: Document, [<Struct>] ?operationName: string, [<Struct>] ?meta : Metadata) =
         let meta = defaultValueArg meta Metadata.Empty
         createExecutionPlan (ast, operationName, meta)
 
+    /// <summary>
     /// Creates an execution plan for provided GraphQL query string without
     /// executing it. This is useful in cases when you have the same query executed
     /// multiple times with different parameters. In that case, query can be used
-    /// to construct execution plan, which then is cached (using DocumentId as a key) and reused when needed.
+    /// to construct execution plan, which then is cached and reused when needed.
+    /// </summary>
+    /// <remarks>
+    /// Key a cache of execution plans by the query string itself, not by <see cref="ExecutionPlan.DocumentId"/> alone: that
+    /// is a hash code, which different documents can share, so a cache keyed by it could execute a document that was never
+    /// validated with the plan of another.
+    /// </remarks>
     /// <param name="queryOrMutation">The GraphQL query string.</param>
     /// <param name="operationName">The name of the operation that should be executed on the parsed document.</param>
     /// <param name="meta">A plain dictionary of metadata that can be used through execution plan customizations.</param>

src/FSharp.Data.GraphQL.Shared/FSharp.Data.GraphQL.Shared.fsproj

@@ -26,6 +26,7 @@
     <PackageReference Include="FParsec" />
     <PackageReference Include="FSharp.SystemTextJson" />
     <PackageReference Include="FsToolkit.ErrorHandling" />
+    <PackageReference Include="Microsoft.Extensions.Caching.Memory" />
     <PackageReference Include="System.Collections.Immutable" />
     <PackageReference Include="System.Text.Json" />
     <PackageReference Condition="'$(TargetFramework)' == 'netstandard2.0'" Include="Portable.System.DateTimeOnly" VersionOverride="9.*" />
@@ -41,7 +42,6 @@
     <Compile Include="Helpers\CollectionExtensions.fs" />
     <Compile Include="Helpers\Extensions.fs" />
     <Compile Include="Helpers\Reflection.fs" />
-    <Compile Include="Helpers\MemoryCache.fs" />
     <Compile Include="Errors.fs" />
     <Compile Include="Exception.fs" />
     <Compile Include="ValidationTypes.fs" />