Fix uncorrelated parameterized collections in compiled queries #37372
Conversation
roji
force-pushed
the
CompiledQueriesUncorrelatedCollections
branch
from
23fc487 to
9fe65de
Compare
roji
force-pushed
the
CompiledQueriesUncorrelatedCollections
branch
from
9fe65de to
604c7dd
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #37370 by properly handling uncorrelated parameterized collections in compiled queries. When a parameter collection is used in a query without any operation that provides type inference (e.g., comparison to a column), the code now falls back to applying the default type mapping instead of failing.
Key changes:
- Modified type mapping postprocessors to handle collections without inferred type mappings by applying default mappings
- Added test coverage for compiled queries with uncorrelated parameter collection expressions across all database providers
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/EFCore.Relational/Query/RelationalTypeMappingPostprocessor.cs |
Added fallback logic to apply default type mapping when no inferred element type mapping exists for VALUES expressions |
src/EFCore.SqlServer/Query/Internal/SqlServerTypeMappingPostprocessor.cs |
Added fallback logic to apply default type mapping when no inferred element type mapping exists for OPENJSON expressions |
test/EFCore.Specification.Tests/Query/PrimitiveCollectionsQueryTestBase.cs |
Added base test for compiled queries with uncorrelated parameter collection expressions |
test/EFCore.Sqlite.FunctionalTests/Query/PrimitiveCollectionsQuerySqliteTest.cs |
Added SQLite-specific test override with SQL assertion |
test/EFCore.SqlServer.FunctionalTests/Query/PrimitiveCollectionsQuerySqlServerTest.cs |
Added SQL Server-specific test override with SQL assertion |
test/EFCore.SqlServer.FunctionalTests/Query/PrimitiveCollectionsQuerySqlServerJsonTypeTest.cs |
Added SQL Server JSON type-specific test override with SQL assertion |
test/EFCore.SqlServer.FunctionalTests/Query/PrimitiveCollectionsQuerySqlServer160Test.cs |
Added SQL Server 2016-specific test override with SQL assertion |
test/EFCore.SqlServer.FunctionalTests/Query/PrimitiveCollectionsQueryOldSqlServerTest.cs |
Added older SQL Server version-specific test override with SQL assertion |
test/EFCore.Cosmos.FunctionalTests/Query/PrimitiveCollectionsQueryCosmosTest.cs |
Added Cosmos DB-specific test override with SQL assertion |
| } | ||
| else | ||
| { | ||
| // We have no inferred type mapping for the element type. This means that there's was nothing in the query done |
There was a problem hiding this comment.
There's a typo in the comment: "there's was" should be "there was".
| collectionParameterTypeMapping = RelationalDependencies.TypeMappingSource.FindMapping(valuesParameter.Type, QueryCompilationContext.Model); | ||
| } | ||
|
|
||
| return valuesExpression.Update(valuesParameter.ApplyTypeMapping(collectionParameterTypeMapping)); |
There was a problem hiding this comment.
The code path at line 201 doesn't validate that the returned collectionParameterTypeMapping has a non-null ElementTypeMapping before using it at line 204. In the else branch starting at line 194, when calling FindMapping without an element type mapping, there should be a null check similar to the one at lines 189-192 to ensure the returned type mapping has a valid ElementTypeMapping before calling Update.
| var parameterTypeMapping = RelationalDependencies.TypeMappingSource.FindMapping(parameterExpression.Type, QueryCompilationContext.Model); | ||
|
|
||
| return openJsonExpression.Update( | ||
| parameterExpression.ApplyTypeMapping(parameterTypeMapping), | ||
| path: null, | ||
| [new SqlServerOpenJsonExpression.ColumnInfo("value", elementTypeMapping, [])]); | ||
| if (parameterTypeMapping is not { ElementTypeMapping: RelationalTypeMapping elementTypeMapping2 }) | ||
| { | ||
| throw new UnreachableException("Default type mapping has no element type mapping"); | ||
| } | ||
|
|
||
| return openJsonExpression.Update( | ||
| parameterExpression.ApplyTypeMapping(parameterTypeMapping), |
There was a problem hiding this comment.
The code path at line 102 doesn't validate that the returned parameterTypeMapping has a non-null ElementTypeMapping before using it at line 110. The null check at lines 104-107 only validates that ElementTypeMapping is not null, but it doesn't throw or handle the case where parameterTypeMapping itself is null. This could result in a NullReferenceException. Consider adding a null check for parameterTypeMapping similar to the pattern used in lines 85-88.
| else | ||
| { | ||
| throw new UnreachableException("A RelationalTypeMapping collection type mapping could not be found"); | ||
| // We have no inferred type mapping for the element type. This means that there's was nothing in the query done |
There was a problem hiding this comment.
There's a typo in the comment: "there's was" should be "there was".
2 participants
Fixes #37370