-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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/5.0] Warn for walking back up the Include tree #24225
Conversation
@dotnet/efteam - Since this is for patch, we decided to add a warning using our "great" logging infra. The decision was to bump it to error in 6.0. For patch it feels ok to warn rather than break the app but for 6.0, what is specific reason we are not throwing exception directly rather an generating warning as error? There is no scenario, where it works for you, then you can suppress warning. |
@smitpatel It would make upgrading to 6.0 easier if one ignored this in 5.0.x |
@smitpatel Design meeting? |
Do we want this for 5.0.4? If so please add the servicing-consider label so tactics can look at it tomorrow |
@smitpatel @ajcvickers @Pilchie has this been servicing-approved for 5.0.5❔ |
Not yet - I expect it will be discussed tomorrow morning. |
@smitpatel Approved by Tactics. Please merge immediately so it gets into today's build. |
Resolves #23674
Description
When walking up the include tree to add more navigations, includes are walk back paths are dropped.
Customer Impact
Customers who wrote query by walking back include tree will not get results included. Earlier this was incorrect results from relationship fixup. Now it won't give results to user what they would expect. The scenario is negative case and should be blocked but currently silently ignored without informing user of possible error.
How found
Reported by a customer.
Test coverage
Test coverage for this case has been added in this PR.
Regression?
No.
Risk
Low. This code has good test coverage for all possible variations. It also adds only a warning message.