Improve EF Core SQL caching, parameter binding, and tests#1
Conversation
riflosnake
commented
Dec 15, 2025
riflosnake
commented
- Add dual-mode EF Core query execution with SQL template caching
- Parameter binding now uses name analysis (no value collision)
- QueryDefinition supports both DbParameter[] and anonymous params
- TypedQueryResult: new result accessors (GetAll, GetSingle, etc.)
- Add full integration tests and performance benchmarks
- Update README with new performance data and usage docs
- Pin Dapper to >=2.0.4, misc bug fixes and API cleanup
- Add dual-mode EF Core query execution with SQL template caching - Parameter binding now uses name analysis (no value collision) - QueryDefinition supports both DbParameter[] and anonymous params - TypedQueryResult: new result accessors (GetAll, GetSingle, etc.) - Add full integration tests and performance benchmarks - Update README with new performance data and usage docs - Pin Dapper to >=2.0.4, misc bug fixes and API cleanup
There was a problem hiding this comment.
Pull request overview
This pull request introduces significant improvements to TypedQuery's EF Core integration by adding SQL template caching and dual-mode query execution. The changes enable TypedQuery to compile LINQ queries once via EF Core and then execute subsequent calls directly through Dapper using cached SQL templates, resulting in 2.5-3× performance improvements over sequential EF Core queries.
Key Changes:
- Dual-mode execution model: First call compiles LINQ to SQL via EF Core and caches the template; subsequent calls bypass EF Core entirely and execute via Dapper with cached SQL
- Parameter binding improvements: Hybrid parameter matching strategy using name analysis (extracting field names from EF Core's
@__fieldName_0pattern) with fallback to value matching - Anonymous object parameter support: QueryDefinition now accepts Dapper-style anonymous objects (
new { id }) as the recommended approach instead of DbParameter arrays
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| src/TypedQuery/TypedQueryResult.cs | Added new result accessor methods (GetAll, GetSingle, GetFirst, etc.) and marked Get() as obsolete |
| src/TypedQuery/TypedQuery.csproj | Bumped minimum Dapper version from 2.0.0 to 2.0.4 |
| src/TypedQuery/SqlBatchBuilder.cs | Added RewriteAnonymousParametersInPlace to support anonymous object parameters |
| src/TypedQuery/DapperExecutionHelper.cs | Removed ToList materialization to return IEnumerable directly from Dapper |
| src/TypedQuery.Tests/TypedQuery.Tests.csproj | Added new console-based integration test project |
| src/TypedQuery.Tests/Program.cs | Comprehensive integration tests covering raw SQL, EF Core, caching, and edge cases |
| src/TypedQuery.EntityFrameworkCore/Interceptor/EfCoreInterceptor.cs | Implemented SQL template compilation and caching with hybrid parameter binding |
| src/TypedQuery.EntityFrameworkCore/Interceptor/CapturedQuery.cs | Added CompiledSqlTemplate, ParameterBinding, and ParameterNameAnalyzer classes |
| src/TypedQuery.EntityFrameworkCore/ITypedQuery.cs | Implemented dual-mode execution logic with template lookup and fallback |
| src/TypedQuery.EntityFrameworkCore/Extensions.cs | Added generic UseTypedQuery overload |
| src/TypedQuery.Abstractions/QueryDefinition.cs | Added constructor accepting anonymous objects as parameters |
| benchmarks/TypedQuery.Benchmarks/TypedQuery.Benchmarks.csproj | Downgraded EF Core packages from 9.0.0 to 8.0.0 |
| benchmarks/TypedQuery.Benchmarks/Benchmarks/EfCoreCachingBenchmarks.cs | Added comprehensive benchmarks comparing cold/warm execution modes |
| TypedQuery.slnx | Added test and benchmark projects to solution |
| README.md | Updated with new performance benchmarks and SQL caching documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| var queryType = queryInstance.GetType(); | ||
| var fields = queryType.GetFields(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic); | ||
| var fieldsByName = fields.ToDictionary(f => f.Name.ToLowerInvariant(), f => f); |
There was a problem hiding this comment.
The ToDictionary call on line 171 could throw an ArgumentException if there are multiple fields with the same name (after converting to lowercase). This could happen with certain inheritance scenarios or compiler-generated fields. Consider using a more robust approach that handles duplicate keys, such as using a Dictionary with Add checks or grouping fields by name and selecting the most appropriate one.
| var fieldsByName = fields.ToDictionary(f => f.Name.ToLowerInvariant(), f => f); | |
| var fieldsByName = fields | |
| .GroupBy(f => f.Name.ToLowerInvariant()) | |
| .ToDictionary(g => g.Key, g => g.First()); |
| /// Useful for queries that should return a single item. | ||
| /// Gets all results as an enumerable sequence for the specified result type. | ||
| /// This is the recommended method for queries that return multiple items. | ||
| /// Results are buffered by Dapper but not materialized to a list. |
There was a problem hiding this comment.
The documentation states "Results are buffered by Dapper but not materialized to a list" but this is misleading. According to the changes in DapperExecutionHelper.cs, the results are already enumerated and buffered when they come back from Dapper's ReadAsync. GetAll returns an IEnumerable but it's already a materialized collection (likely a List from Dapper), not a lazy enumerable. Consider clarifying the documentation or returning the data more explicitly as a collection type.
| /// Results are buffered by Dapper but not materialized to a list. | |
| /// Results are already fully materialized into a collection (typically a List) by Dapper. |
| <Project Sdk="Microsoft.NET.Sdk"> | ||
|
|
||
| <PropertyGroup> | ||
| <OutputType>Exe</OutputType> |
There was a problem hiding this comment.
The test project is configured with OutputType as "Exe" which suggests it's an executable console application rather than a proper test project. Consider using a test framework like xUnit, NUnit, or MSTest instead of a console application with manual assertions. This would provide better test discovery, reporting, and integration with CI/CD pipelines.
| <OutputType>Exe</OutputType> |
| /// Measures the benefit of EF Core SQL caching in TypedQuery. | ||
| /// | ||
| /// Key scenarios: | ||
| /// 1. Cold (first call) - EF Core compiles LINQ ? SQL |
There was a problem hiding this comment.
The comment uses "?" instead of "→" (arrow) in "EF Core compiles LINQ ? SQL". This appears to be a typo or encoding issue. Should be "EF Core compiles LINQ → SQL" to match the consistent usage of arrows elsewhere in the codebase.
| /// 1. Cold (first call) - EF Core compiles LINQ ? SQL | |
| /// 1. Cold (first call) - EF Core compiles LINQ → SQL |
| <PackageReference Include="Microsoft.Data.Sqlite" Version="8.0.0" /> | ||
| <PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite" Version="8.0.0" /> |
There was a problem hiding this comment.
The EF Core packages were downgraded from version 9.0.0 to 8.0.0. While this may be intentional for compatibility reasons, consider documenting the reason in a comment or the PR description. If the library should support both EF Core 8.x and 9.x, ensure that the version range is properly tested, or consider using a version range like [8.0.0,10.0.0) if appropriate.
| <PackageReference Include="Microsoft.Data.Sqlite" Version="8.0.0" /> | |
| <PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite" Version="8.0.0" /> | |
| <!-- Use a version range to support both EF Core 8.x and 9.x for compatibility and future-proofing. --> | |
| <PackageReference Include="Microsoft.Data.Sqlite" Version="[8.0.0,10.0.0)" /> | |
| <PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite" Version="[8.0.0,10.0.0)" /> |
| var cleanName = originalName.TrimStart('@', ':', '?'); | ||
| var newName = $"@tql{queryIndex}_{cleanName}"; | ||
|
|
||
| modifiedSql = modifiedSql.Replace(paramNameInSql, newName); |
There was a problem hiding this comment.
Using string.Replace for parameter name substitution can lead to incorrect replacements when parameter names are substrings of other identifiers. For example, if you have "@id" and "@idtype" as parameters, replacing "@id" could partially match "@idtype". Consider using a regex with word boundaries or a more precise replacement strategy that ensures you're only replacing actual parameter references.
| if (kvp.Key.Contains(extractedName.ToLowerInvariant())) | ||
| { | ||
| matchedField = kvp.Value; | ||
| break; | ||
| } |
There was a problem hiding this comment.
The fallback matching logic using string.Contains is too broad and could match the wrong field. For example, if you have fields "id" and "userId", extracting "id" would match both. This could lead to incorrect parameter bindings. Consider using a more precise matching strategy, such as checking if the extracted name is a complete suffix or using edit distance for fuzzy matching with a threshold.
| foreach (var kvp in fieldsByName) | ||
| { | ||
| if (usedFields.Contains(kvp.Value)) continue; | ||
|
|
||
| // Check if field name contains the extracted name | ||
| if (kvp.Key.Contains(extractedName.ToLowerInvariant())) | ||
| { | ||
| matchedField = kvp.Value; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (var field in fields) | ||
| { | ||
| if (usedFields.Contains(field)) continue; | ||
|
|
||
| var fieldValue = field.GetValue(queryInstance); | ||
| if (fieldValue != null && AreValuesEqual(paramValue, fieldValue)) | ||
| { | ||
| matchedField = field; | ||
| matchCount++; | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| { | ||
| } | ||
| // Trigger EF Core pipeline - interceptor captures SQL and suppresses execution | ||
| try { _ = query.ToList(); } catch { } |
There was a problem hiding this comment.
Poor error handling: empty catch block.
| try { _ = query.ToList(); } catch { } | |
| try { _ = query.ToList(); } catch (Exception ex) { Console.Error.WriteLine($"Exception during EF Core pipeline trigger: {ex}"); } |
1 participant
