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 a team February 22, 2021 22:47
@smitpatel
Copy link
Member Author

@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

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

@ajcvickers
Copy link
Member

@smitpatel Design meeting?

@wtgodbe
Copy link
Member

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
Member

dougbu commented Mar 10, 2021

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

@Pilchie
Copy link
Member

Pilchie commented Mar 10, 2021

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

@ajcvickers
Copy link
Member

@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
@smitpatel smitpatel deleted the smit/somewarnings branch March 11, 2021 18:23
@smitpatel smitpatel restored the smit/somewarnings branch March 11, 2021 18:23
@smitpatel smitpatel deleted the smit/somewarnings branch March 11, 2021 18:23
@smitpatel smitpatel restored the smit/somewarnings branch March 11, 2021 18:23
@smitpatel smitpatel deleted the smit/somewarnings branch March 11, 2021 18:23
@ajcvickers ajcvickers modified the milestones: 5.0.4, 5.0.5 Mar 11, 2021
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.

None yet

7 participants