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

[3.1.1] Query: Don't use VisitAndConvert in projectionBindingExpressionVisitor #19259

Merged
merged 1 commit into from
Dec 10, 2019

Conversation

smitpatel
Copy link
Contributor

Since visiting inner could return null

Resolves #18888

@ajcvickers
Copy link
Member

Removing servicing consider this morning since we don't have the template done yet.

@smitpatel
Copy link
Contributor Author

Description
In new query pipeline, we were using ExpressionVisitor.VisitAndConvert function in certain places. The function throws exception if Visit returned null expression. In 2 specific visitor we return null if we fail to translate something to SQL so that we can throw better exception or client eval the projection. But usage of VisitAndConvert ended up throwing early and useless exception message.

Customer Impact
Query failure when query involves DTO construction with parameter in projection.

How found
Customer-reported (#18888)

Test coverage
We did not have test coverage where DTO took parameter in ctor.

Regression?
Yes, from 2.2.

Risk
Small. Instead of using VisitAndConvert, we call Visit only and not throw any exception if null returned. It is same as underlying code of VisitAndConvert except exception throwing mechanism.

@smitpatel
Copy link
Contributor Author

@ajcvickers - filled it.

@ajcvickers
Copy link
Member

@smitpatel This is approved for 3.1.1 servicing. Please merge ASAP.

@ajcvickers ajcvickers changed the title [3.1.2] Query: Don't use VisitAndConvert in projectionBindingExpressionVisitor [3.1.1] Query: Don't use VisitAndConvert in projectionBindingExpressionVisitor Dec 10, 2019
@ajcvickers ajcvickers removed this from the 3.1.x milestone Dec 10, 2019
@jamshedd jamshedd added this to the 3.1.1 milestone Dec 10, 2019
@jamshedd
Copy link
Member

Approved for 3.1.1

@smitpatel smitpatel merged commit 813358a into release/3.1 Dec 10, 2019
@smitpatel smitpatel deleted the smit/patch18888 branch December 10, 2019 19:56
@ajcvickers ajcvickers removed this from the 3.1.1 milestone Dec 10, 2019
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