Inherit default values for EntityCacheOption.Enabled and EntityCacheOption.Level from RuntimeCacheOptions#3155
Conversation
aaronburtle
requested review from
Alekhya-Polavarapu,
Aniruddh25,
JerryNixon,
RubenCerna2079,
anushakolan,
rusamant,
sourabh1007,
souvikghosh04,
stuartpa and
vadeveka
as code owners
There was a problem hiding this comment.
Pull request overview
This pull request implements inheritance of default values for entity-level cache configuration from runtime-level cache configuration. When entities don't explicitly configure cache.enabled or cache.level, they now inherit these values from the global runtime cache settings, addressing issue #3149.
Changes:
- Entity cache enabled state inherits from runtime cache enabled state when not explicitly configured
- Entity cache level inherits from runtime cache Level2 configuration (L1L2 if Level2 enabled, otherwise L1) when not explicitly configured
- Removed exceptions from
GetEntityCacheEntryTtlandGetEntityCacheEntryLevelmethods when caching is disabled, as these methods can now be safely called to retrieve inherited defaults
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Service.Tests/Caching/CachingConfigProcessingTests.cs | Added comprehensive test coverage for entity cache inheritance behavior from runtime cache settings |
| src/Core/Resolvers/SqlQueryEngine.cs | Updated to use new IsEntityCachingEnabled() method instead of entity property for inheritance-aware caching checks |
| src/Config/ObjectModel/RuntimeConfig.cs | Added IsEntityCachingEnabled() and GlobalCacheEntryLevel() methods; updated GetEntityCacheEntryTtl/Level() to support inheritance |
| src/Config/ObjectModel/RuntimeCacheOptions.cs | Added InferredLevel property to compute cache level from Level2 configuration |
| src/Config/ObjectModel/EntityCacheOptions.cs | Added UserProvidedEnabledOptions flag and updated constructor to track explicit vs inherited enabled state |
| else | ||
| { | ||
| this.Enabled = null; | ||
| } |
There was a problem hiding this comment.
The else branch that sets this.Enabled = null is necessary but could be simplified. Since records support init-only properties and the intent is to leave Enabled as null when not provided, consider removing the property initializer = false from line 35 (in a separate cleanup PR) to make the code clearer. The current code works but has subtle behavior: the property initializer sets it to false, then the constructor overwrites it to null, which is harder to follow than having no initializer and only setting it when explicitly provided.
JerryNixon
left a comment
JerryNixon
left a comment
There was a problem hiding this comment.
Same as the other PR: If the old Entity.IsCachingEnabled incorporated additional logic (for example: entity absent cache block, entity defaults, or other gating like unsupported entity types), this change could enable caching where it previously wouldn’t, or vice-versa.
| /// <param name="entityName">Name of the entity to check cache configuration.</param> | ||
| /// <returns>Whether caching is enabled for the entity.</returns> | ||
| /// <exception cref="DataApiBuilderException">Raised when an invalid entity name is provided.</exception> | ||
| public bool IsEntityCachingEnabled(string entityName) |
There was a problem hiding this comment.
IsEntityCachingEnabled is non-virtual, but RuntimeConfig cache helpers (CanUseCache, GetEntityCacheEntryTtl, GetEntityCacheEntryLevel) are virtual and are mocked in existing tests. Since SqlQueryEngine now calls IsEntityCachingEnabled, tests that use Mock<RuntimeConfig> cannot override this behavior. Making this method virtual would align with the existing testing pattern and avoid brittle test setups.
| public bool IsEntityCachingEnabled(string entityName) | |
| public virtual bool IsEntityCachingEnabled(string entityName) |
| // If the entity explicitly set enabled, use that value. | ||
| if (entityConfig.Cache is not null && entityConfig.Cache.UserProvidedEnabledOptions) | ||
| { | ||
| return EntityCacheLevel.L1L2; | ||
| return entityConfig.Cache.Enabled!.Value; | ||
| } |
There was a problem hiding this comment.
This implementation relies on UserProvidedEnabledOptions to decide whether to use the entity’s cache.enabled. With the current model/converter behavior, it’s possible to end up with Cache.Enabled set but UserProvidedEnabledOptions == false (e.g., object initializer / with usage), which will incorrectly fall back to runtime. Consider treating a non-null Cache.Enabled as explicit regardless of the flag, or ensure the flag is always kept in sync with Enabled.
| if (Enabled is not null) | ||
| { | ||
| this.Enabled = Enabled; | ||
| UserProvidedEnabledOptions = true; | ||
| } |
There was a problem hiding this comment.
UserProvidedEnabledOptions is only set inside the JsonConstructor path when the Enabled parameter is non-null. Because Enabled is still an init-settable property, any code that constructs EntityCacheOptions via object initializer/with (e.g., new EntityCacheOptions { Enabled = true } in Service.Tests) will not set UserProvidedEnabledOptions, causing RuntimeConfig.IsEntityCachingEnabled to ignore the explicit Enabled value. Consider deriving “user provided” from Enabled.HasValue, or implement a custom init accessor for Enabled that also flips UserProvidedEnabledOptions when Enabled is assigned.
| /// <param name="entityCacheConfig">Entity cache configuration JSON fragment.</param> | ||
| /// <param name="expectedIsEntityCachingEnabled">Whether IsEntityCachingEnabled should return true.</param> | ||
| [DataRow(@",""cache"": { ""enabled"": true }", @"", true, DisplayName = "Global cache enabled, entity cache omitted: entity inherits enabled from runtime.")] | ||
| [DataRow(@",""cache"": { ""enabled"": true }", @",""cache"": {}", true, DisplayName = "Global cache enabled, entity cache empty: entity inherits enabled from runtime.")] |
There was a problem hiding this comment.
The test case expecting entityCacheConfig of "cache": {} to inherit enabled from runtime conflicts with the current JSON converter behavior: EntityCacheOptionsConverterFactory.Read initializes enabled to false when the property is omitted, so an empty cache object is treated as explicitly disabled. To make this test and the intended behavior pass, the converter should default enabled to null (unset) and only set it when the JSON contains an enabled token.
| [DataRow(@",""cache"": { ""enabled"": true }", @",""cache"": {}", true, DisplayName = "Global cache enabled, entity cache empty: entity inherits enabled from runtime.")] | |
| [DataRow(@",""cache"": { ""enabled"": true }", @",""cache"": {}", false, DisplayName = "Global cache enabled, entity cache empty: entity cache treated as explicitly disabled.")] |
2 participants
Why make this change?
Closes #3149
What is this change?
Instead of having static default values for the
EntityCacheOptions.EnabledandEntityCacheOptions.Level, we inherit these default values from theRuntimeCacheOptions.How was this tested?
We add a test that validates the new behavior in
CachingConfigProcessingTestsSample Request(s)