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

[release/3.1] Check if parameter.Name is not null before comparing it with query pa… #20521

Merged
merged 1 commit into from
Apr 13, 2020

Conversation

smitpatel
Copy link
Contributor

@smitpatel smitpatel commented Apr 3, 2020

…rameter

Resolves #20485

Parameters which are used inside lambda should get replaced with appropriate shaper/selector.
All other parameters should be query parameters otherwise it is an error.

@smitpatel smitpatel changed the title Check if parameter.Name is not null before comparing it with query pa… [release/3.1] Check if parameter.Name is not null before comparing it with query pa… Apr 3, 2020
@smitpatel
Copy link
Contributor Author

smitpatel commented Apr 3, 2020

Description

EF does special processing for certain parameters which are created by query during parameter extraction (for caching). To do this we compare the prefix of parameter name. This threw NRE when a parameter which had no name encountered.
Fix is to do null safe comparison. If parameter does not have name then we don't need to do any processing required for query parameter.

Customer Impact

Error for dynamically generated queries by multiple different libraries as parameter name is not required during expression tree generation.

How found

Reported by multiple customers.

Test coverage

Added a manually build expression with null parameter.Name which fails without this fix in the manner reported by customers.

Regression?

Yes.

Risk

Low. Fix only affects make comparison null safe without affecting existing functionality.

@ajcvickers
Copy link
Member

@smitpatel Can you add a regression test that creates a couple of expression trees without parameter names?

Also, I think this likely is a regression in the sense that I think that we handled this case in the previous pipeline, or am I wrong about that?

@smitpatel
Copy link
Contributor Author

Updated. Added regression test and quirk.

@ajcvickers
Copy link
Member

@smitpatel Approved by tactics for 3.1.4. Please merge ASAP.

…rameter

Resolves #20485

Parameters which are used inside lambda should get replaced with appropriate shaper/selector.
All other parameters should be query parameters otherwise it is an error.
@smitpatel smitpatel merged commit fc5757c into release/3.1 Apr 13, 2020
@smitpatel smitpatel deleted the smit/parameters branch April 13, 2020 23:28
@ajcvickers ajcvickers removed this from the 3.1.4 milestone Dec 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants