Skip to content
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

Named parameters (parameter placeholders) and Parameter vs PropertyParameter #14644

Closed
MaxG117 opened this issue Feb 8, 2019 · 7 comments · Fixed by #16469
Closed

Named parameters (parameter placeholders) and Parameter vs PropertyParameter #14644

MaxG117 opened this issue Feb 8, 2019 · 7 comments · Fixed by #16469
Assignees
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Milestone

Comments

@MaxG117
Copy link

MaxG117 commented Feb 8, 2019

Our data store does not support named parameters (a.k.a. parameter placeholders) -- we use question marks ("?") instead in DbCommand.CommandText, and we expect the DbCommand.Parameters collection to contain all the parameters in order.

This presents some challenges for implementing an EntityFrameworkCore provider. First we had to allow for duplicate parameters via #12347. Second, it looks like CommandText is generated independently in multiple places (DefaultQuerySqlGenerator, UpdateSqlGenerator), so the best place I found where I can rearrange command parameters and update command text is by overriding the RelationalCommand.CreateCommand method.

Does this make sense, or is there a better way to solve this?

Assuming that the above makes sense, consider the SimpleQueryTestBase.Where_poco_closure() test case:

            var customer = new Customer
            {
                CustomerID = "ALFKI",
            };

            await AssertQuery<Customer>(
                isAsync,
                cs => cs
                    .Where(c => c.Equals(customer)
                    .Select(c => c.CustomerID));

It's the only test case that fails when using my CreateCommand override method. It fails because I assumed that parameterValues.Keys can be easily matched up with parameter placeholders, but this is not true for PropertyParameters. In the example above, the Key is "__customer_0", while the placeholder is "__customer_0_CustomerId".

I'd rather not go through each TypeMappedRelationalParameter to look at its Name, but I can't think of another way to do it. And I suspect there may be other parameter types with the same problem. Is there an easier solution to this?

@MaxG117 MaxG117 changed the title Named parameters (parameter placeholders) and VisitParameter vs VisitPropertyParameter Named parameters (parameter placeholders) and Parameter vs PropertyParameter Feb 8, 2019
@MaxG117
Copy link
Author

MaxG117 commented Feb 8, 2019

#14645 is related in that it also can't easily be fixed without looking at the Name.

@ajcvickers
Copy link
Contributor

@AndriySvyryd @smitpatel @roji to look at this as part of 3.0 query (but also impacts update pipeline)

@MaxG117 Which database provider are you working on?

@smitpatel
Copy link
Contributor

@MaxG117 - Can you tell where is propertyValues.Keys lives?

@MaxG117
Copy link
Author

MaxG117 commented Feb 11, 2019

@ajcvickers We haven't publicly announced yet, so I'm not at liberty to disclose at this time.

@smitpatel apologies, I meant parameterValues.Keys, not propertyValues.Keys. parameterValues is a dictionary passed in to RelationalCommand.Execute methods. I'm still figuring out how it gets generated.

@smitpatel
Copy link
Contributor

Thanks for additional info. I can see why those keys being mismatched. In 3.0 pipeline I will see any other way to improve what you want to achieve but it is likely to be on the lines of doing something with PropertyParameter rather than RelationalCommand.

@AndriySvyryd - Can you comment if modifying RelationalCommand as mentioned in first post is a good way to go or there is a better way to achieve this (w.r.t. Update pipeline).

@AndriySvyryd
Copy link
Member

AndriySvyryd commented Feb 21, 2019

It would also be possible to override ReaderModificationCommandBatch.CreateStoreCommand() if that's the base class this provider uses. But there's no easy way to make EF generate the expected command text and present the parameters in correct order.

@ajcvickers ajcvickers modified the milestones: 3.0.0, Backlog May 10, 2019
@smitpatel smitpatel removed their assignment Jun 24, 2019
roji added a commit to roji/efcore that referenced this issue Jul 9, 2019
Entity equality introduces member access expressions on what may be a
parameter or a constant. Identify these cases and generate a new
parameter (for access of a parameter) or evaluate the constant.

Fixes dotnet#15855
Fixes dotnet#14645
Fixes dotnet#14644
roji added a commit that referenced this issue Jul 9, 2019
Entity equality introduces member access expressions on what may be a
parameter or a constant. Identify these cases and generate a new
parameter (for access of a parameter) or evaluate the constant.

Fixes #15855
Fixes #14645
Fixes #14644
roji added a commit that referenced this issue Jul 9, 2019
Entity equality introduces member access expressions on what may be a
parameter or a constant. Identify these cases and generate a new
parameter (for access of a parameter) or evaluate the constant.

Fixes #15855
Fixes #14645
Fixes #14644
roji added a commit that referenced this issue Jul 9, 2019
Entity equality introduces member access expressions on what may be a
parameter or a constant. Identify these cases and generate a new
parameter (for access of a parameter) or evaluate the constant.

Fixes #15855
Fixes #14645
Fixes #14644
@roji roji closed this as completed in b5cee2d Jul 9, 2019
@roji roji self-assigned this Jul 9, 2019
@roji roji modified the milestones: Backlog, 3.0.0 Jul 9, 2019
@roji
Copy link
Member

roji commented Jul 9, 2019

Note: moved this to the 3.0.0 milestone.

@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jul 25, 2019
@ajcvickers ajcvickers modified the milestones: 3.0.0, 3.0.0-preview8 Jul 29, 2019
@ajcvickers ajcvickers modified the milestones: 3.0.0-preview8, 3.0.0 Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants