Fixed a bug where get by id(s) with cache key would also set the id b… (#161)

* Fixed a bug where get by id(s) with cache key would also set the id based cache route when there was an include or exclude defined. Also ensure we look up by the cache key first.

* Update src/Foundatio.Repositories.Elasticsearch/Repositories/ElasticReadOnlyRepositoryBase.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Apply suggestion from @niemyjski

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Author: niemyjski

src/Foundatio.Repositories.Elasticsearch/Repositories/ElasticReadOnlyRepositoryBase.cs

@@ -97,7 +97,7 @@ public virtual async Task<T> GetByIdAsync(Id id, ICommandOptions options = null)
 
         if (IsCacheEnabled && options.ShouldReadCache())
         {
-            var value = await GetCachedFindHit(id).AnyContext();
+            var value = await GetCachedFindHit(id, options.GetCacheKey()).AnyContext();
 
             if (value?.Document != null)
             {
@@ -144,7 +144,7 @@ public virtual async Task<IReadOnlyCollection<T>> GetByIdsAsync(Ids ids, IComman
 
         var hits = new List<FindHit<T>>();
         if (IsCacheEnabled && options.ShouldReadCache())
-            hits.AddRange(await GetCachedFindHit(idList).AnyContext());
+            hits.AddRange(await GetCachedFindHit(idList, options.GetCacheKey()).AnyContext());
 
         var itemsToFind = idList.Except(hits.Select(i => (Id)i.Id)).ToList();
         if (itemsToFind.Count == 0)
@@ -892,7 +892,6 @@ protected async Task<FindHit<T>> GetCachedFindHit(Id id, string cacheKey = null)
                 : null;
 
             _logger.LogTrace("Cache {HitOrMiss}: type={EntityType} key={CacheKey}", (result != null ? "hit" : "miss"), EntityTypeName, cacheKey ?? id);
-
             return result;
         }
         catch (Exception ex)
@@ -967,8 +966,15 @@ protected Task AddDocumentsToCacheAsync(FindHit<T> findHit, ICommandOptions opti
     protected virtual async Task AddDocumentsToCacheAsync(ICollection<FindHit<T>> findHits, ICommandOptions options, bool isDirtyRead)
     {
         if (options.HasCacheKey())
+        {
             await Cache.SetAsync(options.GetCacheKey(), findHits, options.GetExpiresIn()).AnyContext();
 
+            // NOTE: Custom cache keys store the complete filtered result, but ID-based caching is skipped when includes/excludes are present to avoid incomplete data.
+            // This method also doesn't take into account any query includes or excludes but GetById(s) requests don't specify a query.
+            if (options.GetIncludes().Count > 0 || options.GetExcludes().Count > 0)
+                return;
+        }
+
         // don't add dirty read documents by id as they may be out of sync due to eventual consistency
         if (isDirtyRead)
             return;

tests/Foundatio.Repositories.Elasticsearch.Tests/QueryTests.cs

@@ -248,6 +248,45 @@ public async Task CanHandleIncludeAndExcludeOnGetById()
         Assert.Null(companyLog.CompanyId);
     }
 
+
+    [Fact]
+    public async Task CanHandleIncludeAndExcludeOnGetByIdWithCaching()
+    {
+        var log = await _dailyRepository.AddAsync(LogEventGenerator.Generate(companyId: "1234567890", message: "test", stuff: "stuff"), o => o.ImmediateConsistency());
+        Assert.NotNull(log?.Id);
+
+        Assert.Equal(1, _cache.Misses);
+        Assert.Equal(0, _cache.Hits);
+        Assert.Equal(1, _cache.Writes);
+        Assert.Collection(_cache.Items, c => Assert.StartsWith("alias:daily-logevents-", c.Key));
+
+        const string cacheKey = "company:1234567890";
+        var companyLog = await _dailyRepository.GetByIdAsync(log!.Id, o => o.QueryLogLevel(LogLevel.Warning).Exclude(e => e.Date).Include(e => e.Id).Include("createdUtc").Cache(cacheKey));
+        Assert.Equal(log.Id, companyLog.Id);
+        Assert.Equal(log.CreatedUtc, companyLog.CreatedUtc);
+        Assert.Equal(default, companyLog.Date);
+        Assert.Null(companyLog.Message);
+        Assert.Null(companyLog.CompanyId);
+
+        Assert.Equal(2, _cache.Misses);
+        Assert.Equal(0, _cache.Hits);
+        Assert.Equal(2, _cache.Writes);
+        Assert.Collection(_cache.Items, c => Assert.StartsWith("alias:daily-logevents-", c.Key), c => Assert.Equal($"LogEvent:{cacheKey}", c.Key));
+
+        // Ensure cache hit by cache key.
+        companyLog = await _dailyRepository.GetByIdAsync(log!.Id, o => o.QueryLogLevel(LogLevel.Warning).Exclude(e => e.Date).Include(e => e.Id).Include("createdUtc").Cache(cacheKey));
+        Assert.Equal(log.Id, companyLog.Id);
+        Assert.Equal(log.CreatedUtc, companyLog.CreatedUtc);
+        Assert.Equal(default, companyLog.Date);
+        Assert.Null(companyLog.Message);
+        Assert.Null(companyLog.CompanyId);
+
+        Assert.Equal(2, _cache.Misses);
+        Assert.Equal(1, _cache.Hits);
+        Assert.Equal(2, _cache.Writes);
+        Assert.Collection(_cache.Items, c => Assert.StartsWith("alias:daily-logevents-", c.Key), c => Assert.Equal($"LogEvent:{cacheKey}", c.Key));
+    }
+
     [Fact]
     public async Task CanHandleIncludeAndExcludeOnGetByIds()
     {
@@ -264,6 +303,48 @@ public async Task CanHandleIncludeAndExcludeOnGetByIds()
         Assert.Null(companyLog.CompanyId);
     }
 
+    [Fact]
+    public async Task CanHandleIncludeAndExcludeOnGetByIdsWithCaching()
+    {
+        var log = await _dailyRepository.AddAsync(LogEventGenerator.Generate(companyId: "1234567890", message: "test", stuff: "stuff"), o => o.ImmediateConsistency());
+        Assert.NotNull(log?.Id);
+
+        Assert.Equal(1, _cache.Misses);
+        Assert.Equal(0, _cache.Hits);
+        Assert.Equal(1, _cache.Writes);
+        Assert.Collection(_cache.Items, c => Assert.StartsWith("alias:daily-logevents-", c.Key));
+
+        const string cacheKey = "company:1234567890";
+        var results = await _dailyRepository.GetByIdsAsync([log!.Id], o => o.QueryLogLevel(LogLevel.Warning).Exclude(e => e.Date).Include(e => e.Id).Include("createdUtc").Cache(cacheKey));
+        Assert.Single(results);
+        var companyLog = results.First();
+        Assert.Equal(log.Id, companyLog.Id);
+        Assert.Equal(log.CreatedUtc, companyLog.CreatedUtc);
+        Assert.Equal(default, companyLog.Date);
+        Assert.Null(companyLog.Message);
+        Assert.Null(companyLog.CompanyId);
+
+        Assert.Equal(2, _cache.Misses);
+        Assert.Equal(0, _cache.Hits);
+        Assert.Equal(2, _cache.Writes);
+        Assert.Collection(_cache.Items, c => Assert.StartsWith("alias:daily-logevents-", c.Key), c => Assert.Equal($"LogEvent:{cacheKey}", c.Key));
+
+        // Ensure cache hit by cache key.
+        results = await _dailyRepository.GetByIdsAsync([log!.Id], o => o.QueryLogLevel(LogLevel.Warning).Exclude(e => e.Date).Include(e => e.Id).Include("createdUtc").Cache(cacheKey));
+        Assert.Single(results);
+        companyLog = results.First();
+        Assert.Equal(log.Id, companyLog.Id);
+        Assert.Equal(log.CreatedUtc, companyLog.CreatedUtc);
+        Assert.Equal(default, companyLog.Date);
+        Assert.Null(companyLog.Message);
+        Assert.Null(companyLog.CompanyId);
+
+        Assert.Equal(2, _cache.Misses);
+        Assert.Equal(1, _cache.Hits);
+        Assert.Equal(2, _cache.Writes);
+        Assert.Collection(_cache.Items, c => Assert.StartsWith("alias:daily-logevents-", c.Key), c => Assert.Equal($"LogEvent:{cacheKey}", c.Key));
+    }
+
     [Fact]
     public async Task GetByCompanyWithIncludeWillOverrideDefaultExclude()
     {