Add header support for caching #2650

aaronburtle · 2025-04-14T15:30:22Z

Why make this change?

What is this change?

Inside of the SqlQueryStructure, there is a private constructor, that all of the public constructors will call. This private constructor offers a single point where all of the queries will bottleneck, and therefore we add the cache control information to the query structure at this point. Then when we are in the SqlQueryEngine, we can check for this cache control information and handle the query appropriately.

The cache control options can be found

here: #2253

and here: https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Cache-Control#request_directives

But for reference, they are no-cache, no-store, only-if-cached, which will mean we do not get from the cache, we do not store in the cache, and if there is a cache miss we return gateway timeout, respectively.

How was this tested?

Run against test suite.

Sample Request(s)

To test you need to include the relevant cache headers in the request, "no-cache", "no-store", or "only-if-cached"

aaronburtle · 2025-04-15T22:03:09Z

/azp run

azure-pipelines · 2025-04-15T22:03:32Z

Azure Pipelines successfully started running 6 pipeline(s).

aaronburtle · 2025-04-16T15:50:14Z

/azp run

azure-pipelines · 2025-04-16T15:50:38Z

Azure Pipelines successfully started running 6 pipeline(s).

aaronburtle · 2025-04-16T16:12:09Z

/azp run

azure-pipelines · 2025-04-16T16:12:31Z

Azure Pipelines successfully started running 6 pipeline(s).

src/Core/Models/RestRequestContexts/RestRequestContext.cs

aaronburtle · 2025-04-16T18:02:17Z

/azp run

azure-pipelines · 2025-04-16T18:02:39Z

Azure Pipelines successfully started running 6 pipeline(s).

Copilot

Pull Request Overview

This PR adds support for handling caching directives via the HTTP Cache-Control header by incorporating new properties and logic into the SQL query processing and cache service layers. It introduces new cache getter and setter methods in the cache service, adds cache control handling in the SQL query engine, and updates the SQL query structure and REST request context to capture and validate the Cache-Control header.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/Core/Services/Cache/DabCacheService.cs	Added TryGet and Set methods to retrieve/store JSON responses in the cache based on a derived cache key.
src/Core/Resolvers/SqlQueryEngine.cs	Updated the query execution flow to handle cache control options and to throw exceptions when only-if-cached is used with a cache miss.
src/Core/Resolvers/Sql Query Structures/SqlQueryStructure.cs	Introduced CacheControlOption property and constants, with logic to extract and validate Cache-Control header values.
src/Core/Models/RestRequestContexts/RestRequestContext.cs	Added a new property to capture the Cache-Control header from REST requests.

src/Core/Resolvers/Sql Query Structures/SqlQueryStructure.cs

aaronburtle · 2025-04-16T18:12:19Z

/azp run

azure-pipelines · 2025-04-16T18:12:45Z

Azure Pipelines successfully started running 6 pipeline(s).

Co-authored-by: Copilot <[email protected]>

Co-authored-by: Aniruddh Munde <[email protected]>

aaronburtle · 2025-04-16T19:37:33Z

/azp run

azure-pipelines · 2025-04-16T19:37:55Z

Azure Pipelines successfully started running 6 pipeline(s).

src/Core/Models/RestRequestContexts/RestRequestContext.cs

src/Core/Resolvers/Sql Query Structures/SqlQueryStructure.cs

Aniruddh25 · 2025-04-18T01:07:47Z

src/Core/Resolvers/Sql Query Structures/SqlQueryStructure.cs

@@ -255,6 +271,7 @@ public SqlQueryStructure(
            : this(sqlMetadataProvider,
                authorizationResolver,
                gQLFilterParser,
+                httpRequestHeaders: httpContext?.Request.Headers,


can httpContext be null here? If not, do we need httpContext?

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.

If it could be null only for GraphQL read, we need not worried here, right? Since the constructor is actually for Rest only:

public SqlQueryStructure(
RestRequestContext context,
ISqlMetadataProvider sqlMetadataProvider,

also the argument to the constructor itself is not nullable

**HttpContext httpContext)**

src/Core/Resolvers/Sql Query Structures/SqlQueryStructure.cs

src/Core/Resolvers/SqlQueryEngine.cs

src/Core/Services/Cache/DabCacheService.cs

Aniruddh25 · 2025-04-18T01:27:15Z

src/Core/Resolvers/SqlQueryEngine.cs

+                    switch (structure.CacheControlOption)
+                    {
+                        // Do not get result from cache even if it exists, still cache result.
+                        case SqlQueryStructure.CACHE_CONTROL_NO_CACHE:


Can we add some automated tests for these control options?

Aniruddh25

Waiting on consistently using nullable or not nullable httpContext .

And a test case.

aaronburtle · 2025-04-18T06:46:47Z

/azp run

azure-pipelines · 2025-04-18T06:47:08Z

Azure Pipelines successfully started running 6 pipeline(s).

Aniruddh25 · 2025-04-18T21:07:39Z

src/Core/Resolvers/Sql Query Structures/SqlQueryStructure.cs

@@ -102,6 +102,10 @@ public class SqlQueryStructure : BaseSqlQueryStructure

        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 },


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 value

If its valid, what does it mean?

RubenCerna2079 · 2025-04-28T21:28:28Z

src/Core/Resolvers/SqlQueryEngine.cs

+                                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.


nit: add a new line before the comment

RubenCerna2079 · 2025-04-28T21:43:42Z

src/Core/Resolvers/SqlQueryEngine.cs

+                                    args: null,
+                                    dataSourceName: queryMetadata.DataSource);
+                            return ParseResultIntoJsonDocument(result);
+                        // Only return query response if it exists in cache, return gateway timeout otherwise.


nit: add new line before comment

RubenCerna2079 · 2025-04-28T21:44:06Z

src/Core/Resolvers/SqlQueryEngine.cs

+                                    statusCode: System.Net.HttpStatusCode.GatewayTimeout,
+                                    subStatusCode: DataApiBuilderException.SubStatusCodes.ItemNotFound);
+                            return ParseResultIntoJsonDocument(result);
+                        default:


nit: add new line before default

RubenCerna2079 · 2025-04-28T22:03:58Z

src/Core/Resolvers/SqlQueryEngine.cs

+                            result = await _cache.GetOrSetAsync<JsonElement>(queryExecutor, queryMetadata, cacheEntryTtl: runtimeConfig.GetEntityCacheEntryTtl(entityName: structure.EntityName));
+                            return ParseResultIntoJsonDocument(result);


I am confused by this, shouldn't there be two different cases, one if CacheControlOption is null and one for CACHE_CONTROL?

RubenCerna2079 · 2025-04-28T22:18:22Z

src/Core/Services/Cache/DabCacheService.cs

+    /// </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>


Shouldn't there be a section for cacheValue?

aaronburtle added 2 commits March 31, 2025 07:26

finished adding cache options to query structure

0ab2ed9

cleanup

135ffe9

aaronburtle requested review from Aniruddh25, RubenCerna2079, ravishetye, neeraj-sharma2592, sourabh1007 and sezal98 as code owners April 14, 2025 15:30

aaronburtle self-assigned this Apr 15, 2025

aaronburtle added the enhancement New feature or request label Apr 15, 2025

aaronburtle added this to the 1.5 milestone Apr 15, 2025

Merge branch 'main' into dev/aaronburtle/HeaderCacheSupport

de95e01

null check

426a24b

format

793cb22

fix bug related to non null http context for GQL read

bec4051

Aniruddh25 reviewed Apr 16, 2025

View reviewed changes

src/Core/Models/RestRequestContexts/RestRequestContext.cs Outdated Show resolved Hide resolved

Aniruddh25 requested a review from Copilot April 16, 2025 18:11

handler null context

71dde3e

Copilot AI reviewed Apr 16, 2025

View reviewed changes

src/Core/Resolvers/Sql Query Structures/SqlQueryStructure.cs Outdated Show resolved Hide resolved

Update src/Core/Resolvers/Sql Query Structures/SqlQueryStructure.cs

2be3c03

Co-authored-by: Copilot <[email protected]>

Update src/Core/Models/RestRequestContexts/RestRequestContext.cs

8c8c6c4

Co-authored-by: Aniruddh Munde <[email protected]>