-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Implement EF.MultipleParameters method. #36358
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces support for the new EF.MultipleParameters
query method by extending the core query pipeline, expression visitors, and SQL generation, and updates tests to exercise the new parameterization modes.
- Add a
ParameterExpressionMode
enum and thread it throughQueryParameterExpression
, normalization, funcletization, and SQL translation. - Extend relational and SQL Server nullability and in‐memory processors to handle multiple parameter expansions.
- Update relational, SQL Server, SQLite, and Cosmos tests to parameterize collection tests over
ParameterizedCollectionMode
and add newEF.MultipleParameters
tests.
Reviewed Changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/EFCore/Query/QueryParameterExpression.cs | Replace boolean flags with ParameterExpressionMode property. |
src/EFCore/Query/Internal/QueryableMethodNormalizingExpressionVisitor.cs | Normalize EF.MultipleParameters calls into QueryParameterExpression with the new mode. |
src/EFCore/Query/Internal/ExpressionTreeFuncletizer.cs | Throw appropriate generic error for non-evaluatable EF.MultipleParameters<T> . |
src/EFCore.Relational/Query/SqlNullabilityProcessor.cs | Handle multiple parameter expansion based on ParameterExpressionMode . |
src/EFCore.Relational/Query/SqlExpressions/SqlParameterExpression.cs | Replace ShouldBeConstantized with ParameterExpressionMode . |
src/EFCore.Relational/Query/RelationalSqlTranslatingExpressionVisitor.cs | Apply new mode flags when generating IN and JSON queries. |
src/EFCore.Relational/Query/Internal/RelationalParameterProcessor.cs | Thread ParameterExpressionMode through parameter processing. |
src/EFCore/Properties/CoreStrings.resx | Consolidate EF.Constant /EF.Parameter error messages into generic template. |
test/.../NonSharedPrimitiveCollections* | Convert collection tests to theories over ParameterizedCollectionMode . |
test/.../NorthwindWhereQuery* | Add EF_MultipleParameters_with_non_evaluatable_argument_throws tests for relational and provider-specific projects. |
Files not reviewed (1)
- src/EFCore/Properties/CoreStrings.Designer.cs: Language not supported
src/EFCore/Query/Internal/QueryableMethodNormalizingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
...Relational.Specification.Tests/Query/NonSharedPrimitiveCollectionsQueryRelationalTestBase.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, looks good - see code quailty comments below.
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
src/EFCore/Query/Internal/QueryableMethodNormalizingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See last minor suggestions, otherwise LGTM.
src/EFCore.Relational/Infrastructure/RelationalDbContextOptionsBuilder.cs
Outdated
Show resolved
Hide resolved
@@ -16,15 +16,15 @@ public sealed record RelationalParameterBasedSqlProcessorParameters | |||
/// <summary> | |||
/// Which parametrized collection translation mode should be used. | |||
/// </summary> | |||
public ParameterizedCollectionMode ParameterizedCollectionMode { get; init; } | |||
public ParameterTranslationMode ParameterizedCollectionMode { get; init; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the new naming, maybe call this CollectionParameterTranslationMode for consistency etc. (also elsewhere)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UseParameterizedCollectionMode
stays?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess in the interest of avoiding another design discussion on the public API at this late hour, let's just keep things as they are - disregard my above comment...
Closes #36357.