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/5.0] Warn for walking back up the Include tree #24225

Merged
merged 2 commits into from Mar 11, 2021

Conversation

@smitpatel
Copy link
Member

@smitpatel smitpatel commented Feb 22, 2021

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.

@smitpatel smitpatel requested a review from Feb 22, 2021
@smitpatel
Copy link
Member Author

@smitpatel smitpatel commented Feb 22, 2021

@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 smitpatel changed the title Warn for walking back up the Include tree [release/5.0] Warn for walking back up the Include tree Feb 22, 2021
@AndriySvyryd
Copy link
Member

@AndriySvyryd AndriySvyryd commented Feb 23, 2021

@smitpatel It would make upgrading to 6.0 easier if one ignored this in 5.0.x

@ajcvickers
Copy link
Member

@ajcvickers ajcvickers commented Feb 23, 2021

@smitpatel Design meeting?

@wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Mar 10, 2021

Do we want this for 5.0.4? If so please add the servicing-consider label so tactics can look at it tomorrow

@dougbu dougbu added this to the 5.0.5 milestone Mar 10, 2021
@dougbu
Copy link
Contributor

@dougbu dougbu commented Mar 10, 2021

@smitpatel @ajcvickers @Pilchie has this been servicing-approved for 5.0.5

@ajcvickers ajcvickers removed this from the 5.0.5 milestone Mar 10, 2021
@ajcvickers ajcvickers added this to the 5.0.x milestone Mar 10, 2021
@Pilchie
Copy link
Member

@Pilchie Pilchie commented Mar 10, 2021

Not yet - I expect it will be discussed tomorrow morning.

@leecow leecow removed this from the 5.0.x milestone Mar 11, 2021
@leecow leecow added this to the 5.0.4 milestone Mar 11, 2021
@ajcvickers
Copy link
Member

@ajcvickers ajcvickers commented Mar 11, 2021

@smitpatel Approved by Tactics. Please merge immediately so it gets into today's build.

@smitpatel smitpatel merged commit 5420351 into release/5.0 Mar 11, 2021
7 checks passed
@smitpatel smitpatel deleted the smit/somewarnings branch Mar 11, 2021
@smitpatel smitpatel restored the smit/somewarnings branch Mar 11, 2021
@smitpatel smitpatel deleted the smit/somewarnings branch Mar 11, 2021
@smitpatel smitpatel restored the smit/somewarnings branch Mar 11, 2021
@smitpatel smitpatel deleted the smit/somewarnings branch Mar 11, 2021
@ajcvickers ajcvickers removed this from the 5.0.4 milestone Mar 11, 2021
@ajcvickers ajcvickers added this to the 5.0.5 milestone Mar 11, 2021
@ajcvickers ajcvickers removed this from the 5.0.5 milestone Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants