Skip to content

Commit a45f9bc

Browse files
Shane32Shane32
authored
Enhance GraphQL MemoryCache to be compatible with persisted documents (graphql-dotnet#4095)
* Enhance GraphQL MemoryCache to be compatible with persisted documents * update * update * updates * update tests * update * update api approvals * Update * update readme * Update * update api approvals
1 parent 834a5a6 commit a45f9bc

File tree

8 files changed

+218
-27
lines changed

8 files changed

+218
-27
lines changed

‎docs2/site/docs/migrations/migration8.md

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1052,6 +1052,20 @@ Sample persisted document request:
10521052
}
10531053
```
10541054

1055+
#### Caching (v8.3.0+)
1056+
1057+
The persisted document handler does not provide caching by default. You may implement your own caching mechanism
1058+
within the `GetQueryAsync` method to cache the query strings based on the document identifier. Alternatively, you
1059+
may add the `.UseMemoryCache()` method from the `GraphQL.MemoryCache` package to enable in-memory caching. Be sure
1060+
to call `UseMemoryCache` before calling `UsePeristedDocuments` to ensure that the cache is used.
1061+
1062+
```csharp
1063+
services.AddGraphQL(b => b
1064+
.UseMemoryCache()
1065+
.UsePeristedDocuments<MyLoader>(GraphQL.DI.ServiceLifetime.Scoped)
1066+
);
1067+
```
1068+
10551069
### 24. Execution timeout support
10561070

10571071
`ExecutionOptions.Timeout` has been added to allow a maximum time for the execution of a query. If the execution

‎src/GraphQL.ApiTests/GraphQL.MemoryCache.approved.txt

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,14 @@ namespace GraphQL.Caching
3535
public MemoryDocumentCache(Microsoft.Extensions.Options.IOptions<GraphQL.Caching.MemoryDocumentCacheOptions> options) { }
3636
protected MemoryDocumentCache(Microsoft.Extensions.Caching.Memory.IMemoryCache memoryCache, bool disposeMemoryCache, Microsoft.Extensions.Options.IOptions<GraphQL.Caching.MemoryDocumentCacheOptions> options) { }
3737
public virtual float SortOrder { get; }
38+
protected virtual GraphQL.ExecutionResult CreateExecutionResult(GraphQL.ExecutionError error) { }
3839
public virtual void Dispose() { }
3940
public virtual System.Threading.Tasks.Task<GraphQL.ExecutionResult> ExecuteAsync(GraphQL.ExecutionOptions options, GraphQL.DI.ExecutionDelegate next) { }
4041
protected virtual System.Threading.Tasks.ValueTask<GraphQLParser.AST.GraphQLDocument?> GetAsync(GraphQL.ExecutionOptions options) { }
42+
[System.Obsolete("This method is obsolete and will be removed in a future version. Use GetMemoryCac" +
43+
"heEntryOptions(ExecutionOptions, GraphQLDocument) instead.")]
4144
protected virtual Microsoft.Extensions.Caching.Memory.MemoryCacheEntryOptions GetMemoryCacheEntryOptions(GraphQL.ExecutionOptions options) { }
45+
protected virtual Microsoft.Extensions.Caching.Memory.MemoryCacheEntryOptions GetMemoryCacheEntryOptions(GraphQL.ExecutionOptions options, GraphQLParser.AST.GraphQLDocument document) { }
4246
protected virtual System.Threading.Tasks.ValueTask SetAsync(GraphQL.ExecutionOptions options, GraphQLParser.AST.GraphQLDocument value) { }
4347
}
4448
public class MemoryDocumentCacheOptions : Microsoft.Extensions.Caching.Memory.MemoryCacheOptions, Microsoft.Extensions.Options.IOptions<GraphQL.Caching.MemoryDocumentCacheOptions>

‎src/GraphQL.MemoryCache/MemoryCacheGraphQLBuilderExtensions.cs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,9 @@ public static class MemoryCacheGraphQLBuilderExtensions
88
{
99
/// <summary>
1010
/// Caches parsed and validated documents in memory. This is useful for reducing the overhead of parsing
11-
/// and validating the same document multiple times.
11+
/// and validating the same document multiple times. Be sure to call this method before
12+
/// <see cref="GraphQLBuilderExtensions.UsePersistedDocuments(IGraphQLBuilder, Action{PersistedDocuments.PersistedDocumentOptions}?)">UsePersistedDocuemnts</see>
13+
/// if you are using both to eliminate the need to lookup persisted documents from the persisted document storage for cached document identifiers.
1214
/// </summary>
1315
public static IGraphQLBuilder UseMemoryCache(this IGraphQLBuilder builder, Action<MemoryDocumentCacheOptions>? action = null)
1416
=> builder.UseMemoryCache(action == null ? null : (options, _) => action(options));

‎src/GraphQL.MemoryCache/MemoryDocumentCache.cs

Lines changed: 55 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using GraphQL.DI;
2+
using GraphQL.PersistedDocuments;
23
using GraphQL.Validation;
34
using GraphQLParser.AST;
45
using Microsoft.Extensions.Caching.Memory;
@@ -57,6 +58,31 @@ protected MemoryDocumentCache(IMemoryCache memoryCache, bool disposeMemoryCache,
5758
/// Defaults to setting the <see cref="MemoryCacheEntryOptions.SlidingExpiration"/> value as specified
5859
/// in options, and the <see cref="MemoryCacheEntryOptions.Size"/> value to the length of the query.
5960
/// </summary>
61+
protected virtual MemoryCacheEntryOptions GetMemoryCacheEntryOptions(ExecutionOptions options, GraphQLDocument document)
62+
{
63+
// use the length of the Query property if it is present
64+
if (options.Query != null)
65+
{
66+
#pragma warning disable CS0618 // Type or member is obsolete
67+
return GetMemoryCacheEntryOptions(options);
68+
#pragma warning restore CS0618 // Type or member is obsolete
69+
}
70+
71+
// document.Source is a struct and will never be null
72+
// if it is not initialized, it will have a length of 0 (however, this should not occur with vanilla GraphQL code)
73+
// so we set it to 100 if it is 0, to avoid a cache entry with a size of 0
74+
var docLength = document.Source.Length;
75+
if (docLength == 0)
76+
docLength = 100;
77+
return new MemoryCacheEntryOptions { SlidingExpiration = _options.SlidingExpiration, Size = docLength };
78+
}
79+
80+
/// <summary>
81+
/// Returns a <see cref="MemoryCacheEntryOptions"/> instance for the specified query.
82+
/// Defaults to setting the <see cref="MemoryCacheEntryOptions.SlidingExpiration"/> value as specified
83+
/// in options, and the <see cref="MemoryCacheEntryOptions.Size"/> value to the length of the query.
84+
/// </summary>
85+
[Obsolete("This method is obsolete and will be removed in a future version. Use GetMemoryCacheEntryOptions(ExecutionOptions, GraphQLDocument) instead.")]
6086
protected virtual MemoryCacheEntryOptions GetMemoryCacheEntryOptions(ExecutionOptions options)
6187
{
6288
return new MemoryCacheEntryOptions { SlidingExpiration = _options.SlidingExpiration, Size = options.Query!.Length };
@@ -75,7 +101,7 @@ public virtual void Dispose()
75101
/// <param name="options"><see cref="ExecutionOptions"/></param>
76102
/// <returns>The cached document object. Returns <see langword="null"/> if no entry is found.</returns>
77103
protected virtual ValueTask<GraphQLDocument?> GetAsync(ExecutionOptions options) =>
78-
new(_memoryCache.TryGetValue<GraphQLDocument>(options.Query, out var value) ? value : null);
104+
new(_memoryCache.TryGetValue<GraphQLDocument>(new CacheItem(options), out var value) ? value : null);
79105

80106
/// <summary>
81107
/// Sets a document in the cache. Must be thread-safe.
@@ -89,16 +115,26 @@ protected virtual ValueTask SetAsync(ExecutionOptions options, GraphQLDocument v
89115
throw new ArgumentNullException(nameof(value));
90116
}
91117

92-
_memoryCache.Set(options.Query, value, GetMemoryCacheEntryOptions(options));
118+
_memoryCache.Set(new CacheItem(options), value, GetMemoryCacheEntryOptions(options, value));
93119

94120
return default;
95121
}
96122

123+
/// <summary>
124+
/// Create <see cref="ExecutionResult"/> with provided error.
125+
/// Override this method to change the provided error responses.
126+
/// </summary>
127+
protected virtual ExecutionResult CreateExecutionResult(ExecutionError error) => new(error);
128+
97129
/// <inheritdoc />
98130
public virtual async Task<ExecutionResult> ExecuteAsync(ExecutionOptions options, ExecutionDelegate next)
99131
{
100-
if (options.Document == null && options.Query != null)
132+
if (options.Document == null && (options.Query != null || options.DocumentId != null))
101133
{
134+
// ensure that both documentId and query are not provided
135+
if (options.DocumentId != null && options.Query != null)
136+
return CreateExecutionResult(new InvalidRequestError());
137+
102138
var document = await GetAsync(options).ConfigureAwait(false);
103139
if (document != null) // already in cache
104140
{
@@ -115,6 +151,9 @@ public virtual async Task<ExecutionResult> ExecuteAsync(ExecutionOptions options
115151
document == null && // cache miss
116152
result.Document != null)
117153
{
154+
// note: at this point, the persisted document handler may have set Query, in which case both Query and
155+
// DocumentId will be set, and we will cache based on the documentId only
156+
// but if only Query was initially provided, it will still be the only property set
118157
await SetAsync(options, result.Document).ConfigureAwait(false);
119158
}
120159

@@ -128,4 +167,17 @@ public virtual async Task<ExecutionResult> ExecuteAsync(ExecutionOptions options
128167

129168
/// <inheritdoc/>
130169
public virtual float SortOrder => 200;
170+
171+
private record class CacheItem
172+
{
173+
public string? Query { get; }
174+
public string? DocumentId { get; }
175+
176+
public CacheItem(ExecutionOptions options)
177+
{
178+
// cache based on the document id if present, or the query if not, but not both
179+
Query = options.DocumentId != null ? null : options.Query;
180+
DocumentId = options.DocumentId;
181+
}
182+
}
131183
}

‎src/GraphQL.Tests/Caching/MemoryCacheTests.cs

Lines changed: 113 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -41,28 +41,109 @@ public async Task Validate_Cache_Cannot_Be_Removed_Or_Set_To_Null()
4141
(await memoryCache.GetAsyncPublic(options)).ShouldBe(doc);
4242
}
4343

44+
/// <summary>
45+
/// Executes multiple scenarios to verify that caching works correctly with Query strings and DocumentIds.
46+
/// </summary>
47+
/// <param name="cacheQuery">The Query string used when setting the cache. Can be null.</param>
48+
/// <param name="cacheDocumentId">The DocumentId used when setting the cache. Can be null.</param>
49+
/// <param name="retrieveQuery">The Query string used when retrieving from the cache. Can be null.</param>
50+
/// <param name="retrieveDocumentId">The DocumentId used when retrieving from the cache. Can be null.</param>
51+
/// <param name="expectCached">Indicates whether the retrieval is expected to find the cached document.</param>
52+
[Theory]
53+
[InlineData("query1", null, "query1", null, true)] // Cache by Query, retrieve by same Query
54+
[InlineData("query1", null, "query2", null, false)] // Cache by Query, retrieve by different Query
55+
[InlineData(null, "doc1", null, "doc1", true)] // Cache by DocumentId, retrieve by same DocumentId
56+
[InlineData(null, "doc1", null, "doc2", false)] // Cache by DocumentId, retrieve by different DocumentId
57+
[InlineData("query1", null, null, null, false)] // Cache by Query, retrieve without Query or DocumentId
58+
[InlineData("query1", "doc1", "query1", null, false)] // Cache by both, retrieve with only Query
59+
[InlineData("query1", "doc1", null, "doc1", true)] // Cache by both, retrieve with only DocumentId (typical scenario)
60+
[InlineData(null, null, "query1", null, false)] // Cache by neither, retrieve with only Query
61+
[InlineData(null, null, null, "doc1", false)] // Cache by neither, retrieve with only DocumentId
62+
[InlineData(null, null, null, null, false)] // Cache by neither, retrieve with neither
63+
// note: the following scenarios, retrieving by both, will throw an error in ExecuteAsync, but are provided here for completeness
64+
[InlineData("query1", "doc1", "query1", "doc1", true)] // Cache by both, retrieve by both
65+
[InlineData("query1", "doc1", "query1", "doc2", false)] // Cache by both, retrieve with different DocumentId
66+
[InlineData("query1", "doc1", "query2", "doc1", true)] // Cache by both, retrieve with different Query
67+
[InlineData("query1", "doc1", "query2", "doc2", false)] // Cache by both, retrieve with different Query and DocumentId
68+
[InlineData(null, "doc1", "query1", "doc1", true)] // Cache by DocumentId, retrieve with Query and same DocumentId
69+
public async Task GetAsync_And_SetAsync_Should_Handle_Query_And_DocumentId_Correctly(
70+
string? cacheQuery,
71+
string? cacheDocumentId,
72+
string? retrieveQuery,
73+
string? retrieveDocumentId,
74+
bool expectCached)
75+
{
76+
// Arrange
77+
var document = new GraphQLDocument(new());
78+
var cacheOptions = new ExecutionOptions
79+
{
80+
Query = cacheQuery,
81+
DocumentId = cacheDocumentId
82+
};
83+
var retrieveOptions = new ExecutionOptions
84+
{
85+
Query = retrieveQuery,
86+
DocumentId = retrieveDocumentId
87+
};
88+
var memoryCache = new MyMemoryDocumentCache();
89+
90+
// Act
91+
var initialDocument = await memoryCache.GetAsyncPublic(retrieveOptions);
92+
93+
if (cacheQuery != null || cacheDocumentId != null)
94+
{
95+
await memoryCache.SetAsyncPublic(cacheOptions, document);
96+
}
97+
98+
var cachedDocument = await memoryCache.GetAsyncPublic(retrieveOptions);
99+
100+
// Assert
101+
initialDocument.ShouldBeNull();
102+
if (expectCached)
103+
{
104+
cachedDocument.ShouldBe(document);
105+
}
106+
else
107+
{
108+
cachedDocument.ShouldBeNull();
109+
}
110+
}
111+
44112
[Theory]
45113
// no query set
46-
[InlineData(false, false, false, false, true, true, false)]
114+
[InlineData(false, false, false, false, false, true, true, false)]
47115
// doc already set
48-
[InlineData(false, true, false, false, true, true, false)]
49-
[InlineData(true, true, false, false, true, true, false)]
116+
[InlineData(false, false, true, false, false, true, true, false)]
117+
[InlineData(true, false, true, false, false, true, true, false)]
118+
[InlineData(false, true, true, false, false, true, true, false)]
119+
[InlineData(true, true, true, false, false, false, false, false)]
50120
// typical path with cache miss
51-
[InlineData(true, false, true, false, true, true, true)] // passed validation
52-
[InlineData(true, false, true, false, false, true, false)] // failed validation
53-
[InlineData(true, false, true, false, true, false, false)] // didn't set document (should not be possible)
54-
[InlineData(true, false, true, false, false, false, false)] // failed parse
121+
[InlineData(true, false, false, true, false, true, true, true)] // passed validation
122+
[InlineData(true, false, false, true, false, false, true, false)] // failed validation
123+
[InlineData(true, false, false, true, false, true, false, false)] // didn't set document (should not be possible)
124+
[InlineData(true, false, false, true, false, false, false, false)] // failed parse
125+
[InlineData(false, true, false, true, false, true, true, true)] // passed validation
126+
[InlineData(false, true, false, true, false, false, true, false)] // failed validation
127+
[InlineData(false, true, false, true, false, true, false, false)] // didn't set document (should not be possible)
128+
[InlineData(false, true, false, true, false, false, false, false)] // failed parse
55129
// typical path with cache hit; should never call SetAsync
56-
[InlineData(true, false, true, true, true, true, false)]
57-
[InlineData(true, false, true, true, false, true, false)]
58-
[InlineData(true, false, true, true, true, false, false)]
59-
[InlineData(true, false, true, true, false, false, false)]
60-
public async Task ExecuteAsync(bool querySet, bool docSet, bool getCalled, bool getReturned, bool executed, bool exectuedSetDocument, bool setCalled)
130+
[InlineData(true, false, false, true, true, true, true, false)]
131+
[InlineData(true, false, false, true, true, false, true, false)]
132+
[InlineData(true, false, false, true, true, true, false, false)]
133+
[InlineData(true, false, false, true, true, false, false, false)]
134+
[InlineData(false, true, false, true, true, true, true, false)]
135+
[InlineData(false, true, false, true, true, false, true, false)]
136+
[InlineData(false, true, false, true, true, true, false, false)]
137+
[InlineData(false, true, false, true, true, false, false, false)]
138+
// query and documentId set; should return error
139+
[InlineData(true, true, false, false, false, false, false, false)]
140+
public async Task ExecuteAsync(bool querySet, bool documentIdSet, bool docSet, bool getCalled, bool getReturned, bool executed, bool exectuedSetDocument, bool setCalled)
61141
{
62142
var mockDocument = new GraphQLDocument(new());
63143
var options = new ExecutionOptions
64144
{
65145
Query = querySet ? "Some Query" : null,
146+
DocumentId = documentIdSet ? "Some Document Id" : null,
66147
Document = docSet ? mockDocument : null,
67148
};
68149

@@ -86,19 +167,29 @@ public async Task ExecuteAsync(bool querySet, bool docSet, bool getCalled, bool
86167
return default;
87168
});
88169

89-
var result = new ExecutionResult()
170+
if (querySet && documentIdSet && !docSet)
90171
{
91-
Executed = executed,
92-
Document = exectuedSetDocument ? mockDocument : null,
93-
};
94-
95-
var ret = await memoryDocumentCacheMock.Object.ExecuteAsync(options, (opts) =>
172+
var errResult = await memoryDocumentCacheMock.Object.ExecuteAsync(options, (opts) => throw new InvalidOperationException());
173+
errResult.Errors.ShouldNotBeNull();
174+
errResult.Errors.Count.ShouldBe(1);
175+
errResult.Errors[0].ShouldBeAssignableTo<PersistedDocuments.InvalidRequestError>();
176+
}
177+
else
96178
{
97-
opts.ShouldBe(options);
98-
return Task.FromResult(result);
99-
});
179+
var result = new ExecutionResult()
180+
{
181+
Executed = executed,
182+
Document = exectuedSetDocument ? mockDocument : null,
183+
};
184+
185+
var ret = await memoryDocumentCacheMock.Object.ExecuteAsync(options, (opts) =>
186+
{
187+
opts.ShouldBe(options);
188+
return Task.FromResult(result);
189+
});
100190

101-
ret.ShouldBe(result);
191+
ret.ShouldBe(result);
192+
}
102193

103194
memoryDocumentCacheMock.Protected().Verify("GetAsync", getCalled ? Times.Once() : Times.Never(), ItExpr.IsAny<ExecutionOptions>());
104195
memoryDocumentCacheMock.Protected().Verify("SetAsync", setCalled ? Times.Once() : Times.Never(), ItExpr.IsAny<ExecutionOptions>(), ItExpr.IsAny<GraphQLDocument>());

‎src/GraphQL.Tests/PersistedQueries/PersistedDocumentTests.cs

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,4 +151,23 @@ private async Task<string> ExecuteRequestAsync(Action<IGraphQLBuilder> builder,
151151
});
152152
return serializer.Serialize(response);
153153
}
154+
155+
[Theory]
156+
[InlineData(null, null)]
157+
[InlineData("query", null)]
158+
[InlineData(null, "doc")]
159+
[InlineData("query", "doc")]
160+
public async Task SkipExecutionsWithDocumentSet(string? query, string? documentId)
161+
{
162+
var handler = new PersistedDocumentHandler();
163+
var options = new ExecutionOptions
164+
{
165+
Query = query,
166+
DocumentId = documentId,
167+
Document = new(new()),
168+
};
169+
var expected = new ExecutionResult();
170+
var actual = await handler.ExecuteAsync(options, _ => Task.FromResult(expected));
171+
actual.ShouldBe(expected);
172+
}
154173
}

‎src/GraphQL.sln

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = ".Solution Items", ".Solutio
88
..\.gitignore = ..\.gitignore
99
Directory.Build.props = Directory.Build.props
1010
Directory.Build.targets = Directory.Build.targets
11+
global.json = global.json
1112
graphql.snk = graphql.snk
1213
..\LICENSE.md = ..\LICENSE.md
1314
..\README.md = ..\README.md

‎src/GraphQL/PersistedDocuments/PersistedDocumentHandler.cs

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,11 @@
33
namespace GraphQL.PersistedDocuments;
44

55
/// <summary>
6-
/// Handles persisted document requests.
6+
/// Handles persisted document requests. Does not cache documents and the corresponding ids returned from
7+
/// <see cref="IPersistedDocumentLoader"/> or <see cref="PersistedDocumentOptions.GetQueryDelegate"/>.
8+
/// Please use MemoryDocumentCache prior to this handler to cache documents, or implement a custom cache
9+
/// within the <see cref="PersistedDocumentOptions.GetQueryDelegate"/> delegate or the
10+
/// <see cref="IPersistedDocumentLoader"/> implementation.
711
/// </summary>
812
public class PersistedDocumentHandler : IConfigureExecution
913
{
@@ -60,6 +64,10 @@ public PersistedDocumentHandler()
6064
/// <inheritdoc/>
6165
public async Task<ExecutionResult> ExecuteAsync(ExecutionOptions options, ExecutionDelegate next)
6266
{
67+
// if the parsed GraphQL document is already provided (such as by a memory cache), continue execution
68+
if (options.Document != null)
69+
return await next(options).ConfigureAwait(false);
70+
6371
// ensure that both documentId and query are not provided
6472
if (options.DocumentId != null && options.Query != null)
6573
return CreateExecutionResult(new InvalidRequestError());

0 commit comments

Comments
 (0)