Skip to content

Fix code quality: cache invalidation, resource disposal, null safety, and test assertions #218

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.

Sign up for GitHub

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

Merged
niemyjski merged 3 commits into from
Mar 2, 2026
Merged

Fix code quality: cache invalidation, resource disposal, null safety, and test assertions #218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
02092c3
Fix code quality issues across repository and test infrastructure
niemyjski Mar 1, 2026
d767ea8
Validate op name in Operation.Build before creating operation
niemyjski Mar 1, 2026
3284d54
Use ArgumentException.ThrowIfNullOrWhiteSpace in Operation.Build
niemyjski Mar 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions src/Foundatio.Repositories.Elasticsearch/Repositories/ElasticRepositoryBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -653,7 +653,7 @@ public virtual async Task<long> PatchAllAsync(IRepositoryQuery query, IPatchOper

var updatedIds = results.Hits.Select(h => h.Id).ToList();
if (IsCacheEnabled)
await Cache.RemoveAllAsync(updatedIds).AnyContext();
await InvalidateCacheAsync(updatedIds).AnyContext();

try
{
Expand Down Expand Up @@ -728,8 +728,7 @@ public virtual async Task<long> PatchAllAsync(IRepositoryQuery query, IPatchOper
var updatedIds = results.Hits.Select(h => h.Id).ToList();
if (IsCacheEnabled)
{
// TODO: Should this call invalidation by cache.
await Cache.RemoveAllAsync(updatedIds).AnyContext();
await InvalidateCacheAsync(updatedIds).AnyContext();
}

try
Expand All @@ -751,8 +750,7 @@ public virtual async Task<long> PatchAllAsync(IRepositoryQuery query, IPatchOper
if (scriptOperation == null && partialOperation == null)
throw new ArgumentException("Unknown operation type", nameof(operation));

// TODO: Check has doc change listeners
if (!IsCacheEnabled && scriptOperation != null)
if (!IsCacheEnabled && scriptOperation != null && (DocumentsChanged == null || !DocumentsChanged.HasHandlers))
{
var request = new UpdateByQueryRequest(Indices.Index(String.Join(",", ElasticIndex.GetIndexesByQuery(query))))
{
Expand Down
12 changes: 10 additions & 2 deletions src/Foundatio.Repositories/JsonPatch/Operation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using Newtonsoft.Json;
using System;
using Newtonsoft.Json;
using Newtonsoft.Json.Linq;

namespace Foundatio.Repositories.Utility;
Expand Down Expand Up @@ -42,7 +43,14 @@ public static Operation Parse(string json)

public static Operation Build(JObject jOperation)
{
var op = PatchDocument.CreateOperation((string)jOperation["op"]);
ArgumentNullException.ThrowIfNull(jOperation);

var opName = (string)jOperation["op"];
ArgumentException.ThrowIfNullOrWhiteSpace(opName, "op");

var op = PatchDocument.CreateOperation(opName)
?? throw new ArgumentException($"Unsupported JSON patch operation type '{opName}'.", nameof(jOperation));

op.Read(jOperation);
return op;
}
Expand Down
11 changes: 7 additions & 4 deletions src/Foundatio.Repositories/JsonPatch/PatchDocument.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
using System.Collections.Generic;
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Text;
using Newtonsoft.Json;
using Newtonsoft.Json.Linq;
Expand Down Expand Up @@ -44,7 +44,7 @@ public void AddOperation(Operation operation)

public static PatchDocument Load(Stream document)
{
var reader = new StreamReader(document);
using var reader = new StreamReader(document, leaveOpen: true);
Copy link

Copilot AI Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new StreamReader(document, leaveOpen: true) is likely not a valid constructor overload for the target frameworks (net8.0/net10.0) unless an encoding parameter is also provided. This may break the build; consider using an overload that explicitly supplies the encoding and leaveOpen (or otherwise ensures the chosen overload exists across TFMs).

Suggested change
using var reader = new StreamReader(document, leaveOpen: true);
using var reader = new StreamReader(document, Encoding.UTF8, detectEncodingFromByteOrderMarks: true, leaveOpen: true);

Copilot uses AI. Check for mistakes.

return Parse(reader.ReadToEnd());
}
Expand All @@ -56,8 +56,11 @@ public static PatchDocument Load(JArray document)
if (document == null)
return root;

foreach (var jOperation in document.Children().Cast<JObject>())
foreach (var child in document.Children())
{
if (child is not JObject jOperation)
throw new ArgumentException($"Invalid patch operation: expected a JSON object but found {child.Type}");

var op = Operation.Build(jOperation);
root.AddOperation(op);
}
Expand Down
4 changes: 2 additions & 2 deletions src/Foundatio.Repositories/JsonPatch/PatchDocumentConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using Newtonsoft.Json;
using Newtonsoft.Json.Linq;

Expand Down Expand Up @@ -26,7 +26,7 @@ public override object ReadJson(JsonReader reader, Type objectType, object exist
}
catch (Exception ex)
{
throw new ArgumentException("Invalid patch document: " + ex.Message);
throw new ArgumentException("Invalid patch document: " + ex.Message, ex);
}
}

Expand Down
12 changes: 8 additions & 4 deletions tests/Foundatio.Repositories.Elasticsearch.Tests/DailyRepositoryTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ public async Task AddAsyncWithCustomDateIndex()
{
var utcNow = new DateTime(2023, 1, 1, 0, 0, 0, DateTimeKind.Utc);
var history = await _fileAccessHistoryRepository.AddAsync(new FileAccessHistory { Path = "path1", AccessedDateUtc = utcNow }, o => o.ImmediateConsistency());
Assert.NotNull(history?.Id);
Assert.NotNull(history);
Assert.NotNull(history.Id);

var result = await _fileAccessHistoryRepository.FindOneAsync(f => f.Id(history.Id));
Assert.Equal("file-access-history-daily-v1-2023.01.01", result.Data.GetString("index"));
Expand All @@ -47,7 +48,8 @@ public async Task AddAsyncWithCurrentDateViaDocumentsAdding()
_fileAccessHistoryRepository.DocumentsAdding.AddHandler(OnDocumentsAdding);

var history = await _fileAccessHistoryRepository.AddAsync(new FileAccessHistory { Path = "path2" }, o => o.ImmediateConsistency());
Assert.NotNull(history?.Id);
Assert.NotNull(history);
Assert.NotNull(history.Id);

var result = await _fileAccessHistoryRepository.FindOneAsync(f => f.Id(history.Id));
Assert.Equal("file-access-history-daily-v1-2023.02.01", result.Data.GetString("index"));
Expand All @@ -73,7 +75,8 @@ private Task OnDocumentsAdding(object sender, DocumentsEventArgs<FileAccessHisto
public async Task CanAddAsync()
{
var history = await _fileAccessHistoryRepository.AddAsync(new FileAccessHistory { AccessedDateUtc = DateTime.UtcNow });
Assert.NotNull(history?.Id);
Assert.NotNull(history);
Assert.NotNull(history.Id);
}

[Fact]
Expand All @@ -87,7 +90,8 @@ public async Task AddAsyncConcurrentUpdates()
await Parallel.ForEachAsync(Enumerable.Range(0, 10), async (_, _) =>
{
var history = await _fileAccessHistoryRepository.AddAsync(new FileAccessHistory { AccessedDateUtc = DateTime.UtcNow.AddDays(index) });
Assert.NotNull(history?.Id);
Assert.NotNull(history);
Assert.NotNull(history.Id);
});
}
}
Expand Down
45 changes: 30 additions & 15 deletions tests/Foundatio.Repositories.Elasticsearch.Tests/IndexTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ public async Task CanCreateDailyAliasesAsync(DateTime utcNow)
for (int i = 0; i < 35; i += 5)
{
var employee = await repository.AddAsync(EmployeeGenerator.Generate(createdUtc: utcNow.SubtractDays(i)));
Assert.NotNull(employee?.Id);
Assert.NotNull(employee);
Assert.NotNull(employee.Id);

Assert.Equal(1, await index.GetCurrentVersionAsync());
var existsResponse = await _client.Indices.ExistsAsync(index.GetIndex(employee.CreatedUtc), ct: TestCancellationToken);
Expand Down Expand Up @@ -81,7 +82,8 @@ public async Task CanCreateMonthlyAliasesAsync(DateTime utcNow)
for (int i = 0; i < 4; i++)
{
var employee = await repository.AddAsync(EmployeeGenerator.Generate(createdUtc: utcNow.SubtractMonths(i)));
Assert.NotNull(employee?.Id);
Assert.NotNull(employee);
Assert.NotNull(employee.Id);

Assert.Equal(1, await index.GetCurrentVersionAsync());
var existsResponse = await _client.Indices.ExistsAsync(index.GetIndex(employee.CreatedUtc), ct: TestCancellationToken);
Expand Down Expand Up @@ -126,10 +128,12 @@ public async Task GetByDateBasedIndexAsync()
var utcNow = DateTime.UtcNow;
ILogEventRepository repository = new DailyLogEventRepository(_configuration);
var logEvent = await repository.AddAsync(LogEventGenerator.Generate(createdUtc: utcNow));
Assert.NotNull(logEvent?.Id);
Assert.NotNull(logEvent);
Assert.NotNull(logEvent.Id);

logEvent = await repository.AddAsync(LogEventGenerator.Generate(createdUtc: utcNow.SubtractDays(1)), o => o.ImmediateConsistency());
Assert.NotNull(logEvent?.Id);
Assert.NotNull(logEvent);
Assert.NotNull(logEvent.Id);

alias = await _client.Indices.GetAliasAsync(_configuration.DailyLogEvents.Name, ct: TestCancellationToken);
_logger.LogRequest(alias);
Expand Down Expand Up @@ -266,7 +270,8 @@ public async Task MaintainDailyIndexesAsync()
IEmployeeRepository repository = new EmployeeRepository(index);

var employee = await repository.AddAsync(EmployeeGenerator.Generate(createdUtc: timeProvider.GetUtcNow().UtcDateTime), o => o.ImmediateConsistency());
Assert.NotNull(employee?.Id);
Assert.NotNull(employee);
Assert.NotNull(employee.Id);

await index.MaintainAsync();
Assert.Equal(1, await index.GetCurrentVersionAsync());
Expand Down Expand Up @@ -331,7 +336,8 @@ public async Task MaintainMonthlyIndexesAsync()
{
var created = utcNow.SubtractMonths(i);
var employee = await repository.AddAsync(EmployeeGenerator.Generate(createdUtc: created.UtcDateTime));
Assert.NotNull(employee?.Id);
Assert.NotNull(employee);
Assert.NotNull(employee.Id);

Assert.Equal(1, await index.GetCurrentVersionAsync());
var existsResponse = await _client.Indices.ExistsAsync(index.GetIndex(employee.CreatedUtc), ct: TestCancellationToken);
Expand All @@ -356,7 +362,8 @@ public async Task MaintainMonthlyIndexesAsync()
{
var created = utcNow.SubtractMonths(i);
var employee = await repository.AddAsync(EmployeeGenerator.Generate(createdUtc: created.UtcDateTime));
Assert.NotNull(employee?.Id);
Assert.NotNull(employee);
Assert.NotNull(employee.Id);

Assert.Equal(1, await index.GetCurrentVersionAsync());
var existsResponse = await _client.Indices.ExistsAsync(index.GetIndex(employee.CreatedUtc), ct: TestCancellationToken);
Expand Down Expand Up @@ -614,7 +621,8 @@ public async Task DailyAliasMaxAgeAsync(DateTime utcNow)
IEmployeeRepository version1Repository = new EmployeeRepository(index);

var employee = await version1Repository.AddAsync(EmployeeGenerator.Generate(createdUtc: utcNow), o => o.ImmediateConsistency());
Assert.NotNull(employee?.Id);
Assert.NotNull(employee);
Assert.NotNull(employee.Id);

var existsResponse = await _client.Indices.ExistsAsync(index.GetIndex(employee.CreatedUtc), ct: TestCancellationToken);
_logger.LogRequest(existsResponse);
Expand All @@ -630,7 +638,8 @@ public async Task DailyAliasMaxAgeAsync(DateTime utcNow)
Assert.Equal(GetExpectedEmployeeDailyAliases(index, utcNow, employee.CreatedUtc), String.Join(", ", aliases));

employee = await version1Repository.AddAsync(EmployeeGenerator.Generate(createdUtc: utcNow.SubtractDays(2)), o => o.ImmediateConsistency());
Assert.NotNull(employee?.Id);
Assert.NotNull(employee);
Assert.NotNull(employee.Id);

existsResponse = await _client.Indices.ExistsAsync(index.GetIndex(employee.CreatedUtc), ct: TestCancellationToken);
_logger.LogRequest(existsResponse);
Expand All @@ -646,7 +655,8 @@ public async Task DailyAliasMaxAgeAsync(DateTime utcNow)
Assert.Equal(GetExpectedEmployeeDailyAliases(index, utcNow, employee.CreatedUtc), String.Join(", ", aliases));

employee = await version1Repository.AddAsync(EmployeeGenerator.Generate(createdUtc: utcNow.SubtractDays(35)), o => o.ImmediateConsistency());
Assert.NotNull(employee?.Id);
Assert.NotNull(employee);
Assert.NotNull(employee.Id);

existsResponse = await _client.Indices.ExistsAsync(index.GetIndex(employee.CreatedUtc), ct: TestCancellationToken);
_logger.LogRequest(existsResponse);
Expand Down Expand Up @@ -679,7 +689,8 @@ public async Task MonthlyAliasMaxAgeAsync(DateTime utcNow)
IEmployeeRepository repository = new EmployeeRepository(index);

var employee = await repository.AddAsync(EmployeeGenerator.Generate(createdUtc: utcNow), o => o.ImmediateConsistency());
Assert.NotNull(employee?.Id);
Assert.NotNull(employee);
Assert.NotNull(employee.Id);

var existsResponse = await _client.Indices.ExistsAsync(index.GetIndex(employee.CreatedUtc), ct: TestCancellationToken);
_logger.LogRequest(existsResponse);
Expand All @@ -695,7 +706,8 @@ public async Task MonthlyAliasMaxAgeAsync(DateTime utcNow)
Assert.Equal(GetExpectedEmployeeMonthlyAliases(index, utcNow, employee.CreatedUtc), String.Join(", ", aliases));

employee = await repository.AddAsync(EmployeeGenerator.Generate(createdUtc: utcNow.SubtractDays(2)), o => o.ImmediateConsistency());
Assert.NotNull(employee?.Id);
Assert.NotNull(employee);
Assert.NotNull(employee.Id);

existsResponse = await _client.Indices.ExistsAsync(index.GetIndex(employee.CreatedUtc), ct: TestCancellationToken);
_logger.LogRequest(existsResponse);
Expand All @@ -711,7 +723,8 @@ public async Task MonthlyAliasMaxAgeAsync(DateTime utcNow)
Assert.Equal(GetExpectedEmployeeMonthlyAliases(index, utcNow, employee.CreatedUtc), String.Join(", ", aliases));

employee = await repository.AddAsync(EmployeeGenerator.Generate(createdUtc: utcNow.SubtractDays(35)), o => o.ImmediateConsistency());
Assert.NotNull(employee?.Id);
Assert.NotNull(employee);
Assert.NotNull(employee.Id);

existsResponse = await _client.Indices.ExistsAsync(index.GetIndex(employee.CreatedUtc), ct: TestCancellationToken);
_logger.LogRequest(existsResponse);
Expand Down Expand Up @@ -854,7 +867,8 @@ public async Task Index_MaintainThenIndexing_ShouldCreateIndexWhenNeeded()
var employee = await repository.AddAsync(EmployeeGenerator.Generate(createdUtc: utcNow.UtcDateTime));

// Assert
Assert.NotNull(employee?.Id);
Assert.NotNull(employee);
Assert.NotNull(employee.Id);

// Verify the correct versioned index was created
string expectedVersionedIndex = index.GetVersionedIndex(utcNow.UtcDateTime);
Expand Down Expand Up @@ -894,7 +908,8 @@ public async Task Index_ParallelOperations_ShouldNotInterfereWithEachOther()
var employee = await task2;

// Assert
Assert.NotNull(employee?.Id);
Assert.NotNull(employee);
Assert.NotNull(employee.Id);

// Verify the index was created correctly despite the race condition
string expectedVersionedIndex = "monthly-employees-v2-2025.06";
Expand Down
6 changes: 4 additions & 2 deletions tests/Foundatio.Repositories.Elasticsearch.Tests/MonthlyRepositoryTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ public async Task AddAsyncWithCustomDateIndex()
{
var utcNow = new DateTime(2023, 1, 1, 0, 0, 0, DateTimeKind.Utc);
var history = await _fileAccessHistoryRepository.AddAsync(new FileAccessHistory { Path = "path1", AccessedDateUtc = utcNow }, o => o.ImmediateConsistency());
Assert.NotNull(history?.Id);
Assert.NotNull(history);
Assert.NotNull(history.Id);

var result = await _fileAccessHistoryRepository.FindOneAsync(f => f.Id(history.Id));
Assert.Equal("file-access-history-monthly-v1-2023.01", result.Data.GetString("index"));
Expand All @@ -46,7 +47,8 @@ public async Task AddAsyncWithCurrentDateViaDocumentsAdding()
_fileAccessHistoryRepository.DocumentsAdding.AddHandler(OnDocumentsAdding);

var history = await _fileAccessHistoryRepository.AddAsync(new FileAccessHistory { Path = "path2" }, o => o.ImmediateConsistency());
Assert.NotNull(history?.Id);
Assert.NotNull(history);
Assert.NotNull(history.Id);

var result = await _fileAccessHistoryRepository.FindOneAsync(f => f.Id(history.Id));
Assert.Equal("file-access-history-monthly-v1-2023.02", result.Data.GetString("index"));
Expand Down
Loading