-
Notifications
You must be signed in to change notification settings - Fork 246
Add header support for caching #2650
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
0ab2ed9
135ffe9
de95e01
426a24b
793cb22
bec4051
71dde3e
2be3c03
8c8c6c4
9a5801b
b5556b6
16b2113
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,6 +92,20 @@ public class SqlQueryStructure : BaseSqlQueryStructure | |
/// </summary> | ||
public GroupByMetadata GroupByMetadata { get; private set; } | ||
|
||
public string? CacheControlOption { get; set; } | ||
|
||
public const string CACHE_CONTROL = "Cache-Control"; | ||
|
||
public const string CACHE_CONTROL_NO_STORE = "no-store"; | ||
|
||
public const string CACHE_CONTROL_NO_CACHE = "no-cache"; | ||
|
||
public const string CACHE_CONTROL_ONLY_IF_CACHED = "only-if-cached"; | ||
|
||
public HashSet<string> cacheControlHeaderOptions = new( | ||
new[] { CACHE_CONTROL, CACHE_CONTROL_NO_STORE, CACHE_CONTROL_NO_CACHE, CACHE_CONTROL_ONLY_IF_CACHED }, | ||
StringComparer.OrdinalIgnoreCase); | ||
|
||
/// <summary> | ||
/// Generate the structure for a SQL query based on GraphQL query | ||
/// information. | ||
|
@@ -150,6 +164,7 @@ public SqlQueryStructure( | |
: this(sqlMetadataProvider, | ||
authorizationResolver, | ||
gQLFilterParser, | ||
gQLFilterParser.GetHttpContextFromMiddlewareContext(ctx).Request.Headers, | ||
predicates: null, | ||
entityName: entityName, | ||
counter: counter) | ||
|
@@ -217,6 +232,7 @@ public SqlQueryStructure( | |
} | ||
|
||
HttpContext httpContext = GraphQLFilterParser.GetHttpContextFromMiddlewareContext(ctx); | ||
|
||
// Process Authorization Policy of the entity being processed. | ||
AuthorizationPolicyHelpers.ProcessAuthorizationPolicies(EntityActionOperation.Read, queryStructure: this, httpContext, authorizationResolver, sqlMetadataProvider); | ||
|
||
|
@@ -255,6 +271,7 @@ public SqlQueryStructure( | |
: this(sqlMetadataProvider, | ||
authorizationResolver, | ||
gQLFilterParser, | ||
httpRequestHeaders: httpContext?.Request.Headers, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can httpContext be null here? If not, do we need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it can be null here. For GraphQL read operation we intentionally leave this null, and then use that fact to sort out when to process authorization policies. This flow was causing a bunch of tests to fail when I was forcing an httpContext to always get passed through, which is why now I just grab the Headers, and allow the context to remain null when its passed in as such. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it could be public SqlQueryStructure( also the argument to the constructor itself is not nullable
|
||
predicates: null, | ||
entityName: context.EntityName, | ||
counter: new IncrementingInteger(), | ||
|
@@ -380,10 +397,10 @@ private SqlQueryStructure( | |
: this(sqlMetadataProvider, | ||
authorizationResolver, | ||
gQLFilterParser, | ||
gQLFilterParser.GetHttpContextFromMiddlewareContext(ctx).Request.Headers, | ||
predicates: null, | ||
entityName: entityName, | ||
counter: counter | ||
) | ||
counter: counter) | ||
{ | ||
_ctx = ctx; | ||
IOutputType outputType = schemaField.Type; | ||
|
@@ -541,6 +558,7 @@ private SqlQueryStructure( | |
ISqlMetadataProvider metadataProvider, | ||
IAuthorizationResolver authorizationResolver, | ||
GQLFilterParser gQLFilterParser, | ||
IHeaderDictionary? httpRequestHeaders, | ||
List<Predicate>? predicates = null, | ||
string entityName = "", | ||
IncrementingInteger? counter = null, | ||
|
@@ -559,6 +577,25 @@ private SqlQueryStructure( | |
ColumnLabelToParam = new(); | ||
FilterPredicates = string.Empty; | ||
OrderByColumns = new(); | ||
AddCacheControlOptions(httpRequestHeaders); | ||
} | ||
|
||
private void AddCacheControlOptions(IHeaderDictionary? httpRequestHeaders) | ||
{ | ||
// Set the cache control based on the request header if it exists. | ||
if (httpRequestHeaders is not null && httpRequestHeaders.TryGetValue(CACHE_CONTROL, out Microsoft.Extensions.Primitives.StringValues cacheControlOption)) | ||
{ | ||
CacheControlOption = cacheControlOption; | ||
} | ||
|
||
if (!string.IsNullOrEmpty(CacheControlOption) && | ||
!cacheControlHeaderOptions.Contains(CacheControlOption)) | ||
{ | ||
throw new DataApiBuilderException( | ||
message: "Request Header Cache-Control is invalid: " + CacheControlOption, | ||
statusCode: HttpStatusCode.BadRequest, | ||
subStatusCode: DataApiBuilderException.SubStatusCodes.BadRequest); | ||
} | ||
} | ||
|
||
/// <summary> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ | |
using Azure.DataApiBuilder.Core.Services; | ||
using Azure.DataApiBuilder.Core.Services.Cache; | ||
using Azure.DataApiBuilder.Core.Services.MetadataProviders; | ||
using Azure.DataApiBuilder.Service.Exceptions; | ||
using Azure.DataApiBuilder.Service.GraphQLBuilder; | ||
using Azure.DataApiBuilder.Service.GraphQLBuilder.Queries; | ||
using HotChocolate.Resolvers; | ||
|
@@ -329,10 +330,45 @@ public object ResolveList(JsonElement array, IObjectField fieldSchema, ref IMeta | |
if (!dbPolicyConfigured && entityCacheEnabled) | ||
{ | ||
DatabaseQueryMetadata queryMetadata = new(queryText: queryString, dataSource: dataSourceName, queryParameters: structure.Parameters); | ||
JsonElement result = await _cache.GetOrSetAsync<JsonElement>(queryExecutor, queryMetadata, cacheEntryTtl: runtimeConfig.GetEntityCacheEntryTtl(entityName: structure.EntityName)); | ||
byte[] jsonBytes = JsonSerializer.SerializeToUtf8Bytes(result); | ||
JsonDocument cacheServiceResponse = JsonDocument.Parse(jsonBytes); | ||
return cacheServiceResponse; | ||
JsonElement? result; | ||
switch (structure.CacheControlOption) | ||
{ | ||
// Do not get result from cache even if it exists, still cache result. | ||
case SqlQueryStructure.CACHE_CONTROL_NO_CACHE: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add some automated tests for these control options? |
||
result = await queryExecutor.ExecuteQueryAsync( | ||
sqltext: queryMetadata.QueryText, | ||
parameters: queryMetadata.QueryParameters, | ||
dataReaderHandler: queryExecutor.GetJsonResultAsync<JsonElement>, | ||
httpContext: _httpContextAccessor.HttpContext!, | ||
args: null, | ||
dataSourceName: queryMetadata.DataSource); | ||
_cache.Set<JsonElement?>(queryMetadata, cacheEntryTtl: runtimeConfig.GetEntityCacheEntryTtl(entityName: structure.EntityName), result); | ||
return ParseResultIntoJsonDocument(result); | ||
// Do not store result even if valid, stil get from cache if available. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: add a new line before the comment |
||
case SqlQueryStructure.CACHE_CONTROL_NO_STORE: | ||
result = _cache.TryGet<JsonElement?>(queryMetadata); | ||
result = result.HasValue ? result.Value : | ||
await queryExecutor.ExecuteQueryAsync( | ||
sqltext: queryMetadata.QueryText, | ||
parameters: queryMetadata.QueryParameters, | ||
dataReaderHandler: queryExecutor.GetJsonResultAsync<JsonElement>, | ||
httpContext: _httpContextAccessor.HttpContext!, | ||
args: null, | ||
dataSourceName: queryMetadata.DataSource); | ||
return ParseResultIntoJsonDocument(result); | ||
// Only return query response if it exists in cache, return gateway timeout otherwise. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: add new line before comment |
||
case SqlQueryStructure.CACHE_CONTROL_ONLY_IF_CACHED: | ||
result = _cache.TryGet<JsonElement?>(queryMetadata); | ||
result = result.HasValue ? result : | ||
throw new DataApiBuilderException( | ||
message: "Header 'only-if-cached' was used but item was not found in cache.", | ||
statusCode: System.Net.HttpStatusCode.GatewayTimeout, | ||
aaronburtle marked this conversation as resolved.
Show resolved
Hide resolved
|
||
subStatusCode: DataApiBuilderException.SubStatusCodes.ItemNotFound); | ||
return ParseResultIntoJsonDocument(result); | ||
default: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: add new line before |
||
result = await _cache.GetOrSetAsync<JsonElement>(queryExecutor, queryMetadata, cacheEntryTtl: runtimeConfig.GetEntityCacheEntryTtl(entityName: structure.EntityName)); | ||
return ParseResultIntoJsonDocument(result); | ||
Comment on lines
+369
to
+370
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am confused by this, shouldn't there be two different cases, one if |
||
} | ||
} | ||
} | ||
|
||
|
@@ -351,6 +387,12 @@ public object ResolveList(JsonElement array, IObjectField fieldSchema, ref IMeta | |
return response; | ||
} | ||
|
||
private static JsonDocument? ParseResultIntoJsonDocument(JsonElement? result) | ||
{ | ||
byte[] jsonBytes = JsonSerializer.SerializeToUtf8Bytes(result); | ||
return JsonDocument.Parse(jsonBytes); | ||
} | ||
|
||
// <summary> | ||
// Given the SqlExecuteStructure structure, obtains the query text and executes it against the backend. | ||
// Unlike a normal query, result from database may not be JSON. Instead we treat output as SqlMutationEngine does (extract by row). | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -81,6 +81,40 @@ public DabCacheService(IFusionCache cache, ILogger<DabCacheService>? logger, IHt | |
return result; | ||
} | ||
|
||
/// <summary> | ||
/// Try to get cacheValue from the cache with the derived cache key. | ||
/// </summary> | ||
/// <typeparam name="JsonElement">The type of value in the cache</typeparam> | ||
/// <param name="queryMetadata">Metadata used to create a cache key or fetch a response from the database.</param> | ||
/// <returns>JSON Response</returns> | ||
public MaybeValue<JsonElement>? TryGet<JsonElement>(DatabaseQueryMetadata queryMetadata) | ||
{ | ||
string cacheKey = CreateCacheKey(queryMetadata); | ||
return _cache.TryGet<JsonElement>(key: cacheKey); | ||
} | ||
|
||
/// <summary> | ||
/// Store cacheValue into the cache with the derived cache key. | ||
/// </summary> | ||
/// <typeparam name="JsonElement">The type of value in the cache</typeparam> | ||
/// <param name="queryMetadata">Metadata used to create a cache key or fetch a response from the database.</param> | ||
/// <param name="cacheEntryTtl">Number of seconds the cache entry should be valid before eviction.</param> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't there be a section for |
||
public void Set<JsonElement>( | ||
DatabaseQueryMetadata queryMetadata, | ||
int cacheEntryTtl, | ||
JsonElement? cacheValue) | ||
{ | ||
string cacheKey = CreateCacheKey(queryMetadata); | ||
_cache.Set( | ||
key: cacheKey, | ||
value: cacheValue, | ||
(FusionCacheEntryOptions options) => | ||
{ | ||
options.SetSize(EstimateCacheEntrySize(cacheKey: cacheKey, cacheValue: cacheValue?.ToString())); | ||
options.SetDuration(duration: TimeSpan.FromSeconds(cacheEntryTtl)); | ||
}); | ||
} | ||
|
||
/// <summary> | ||
/// Attempts to fetch response from cache. If there is a cache miss, invoke executeQueryAsync Func to get a response | ||
/// </summary> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is
CACHE_CONTROL
i.e.Cache-Control
itself a valid value of this option? IMO this should not be in the HashSet since you would be giving out an exception for this value if its actually an invalid valueIf its valid, what does it mean?