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

[6.0-rc2] Fix DatabaseDeveloperPageExceptionFilter to handle wrapped DbExceptions #36803

Merged
merged 1 commit into from
Sep 21, 2021

Conversation

ajcvickers
Copy link
Member

@ajcvickers ajcvickers commented Sep 21, 2021

Description

Sometimes EF (or other libraries) wrap database errors. The exception filter should account for this, but was not doing. This was revealed by dotnet/efcore#25050 where we started treating more error numbers as transient and hence wrapping their exceptions.

Note that the original Diagnostics.EFCore.FunctionalTests have a test for this, but it appears that these tests were never updated when the mechanism was changed in .NET 5.

Customer Impact

When first creating a site and the DB doesn't exist, you get an error message rather than the custom database page that allows you to create it.

Regression?

  • Yes
  • No

Regressed since RC1, as EF started wrapping more exceptions in dotnet/efcore#25050

Risk

  • High
  • Medium
  • Low

Just falls back to InnerException if the exception isn't DBException.

Verification

  • Manual (required) - @javiercn did some e2e testing on the scenario
  • Automated - added in this PR.

Packaging changes reviewed?

  • Yes
  • No
  • N/A

Addresses AzDO#1399210

Sometimes EF (or other libraries) wrap database errors. The exception filter should account for this, but was not doing. This was revealed by dotnet/efcore#25050 where we started treating more error numbers as transient and hence wrapping their exceptions.

Note that the original Diagnostics.EFCore.FunctionalTests have a test for this, but it appears that these tests were never updated when the mechanism was changed in .NET 5.
@ghost ghost added the area-runtime label Sep 21, 2021
@ajcvickers ajcvickers changed the title Fix DatabaseDeveloperPageExceptionFilter to handle wrapped DbExceptions [6.0-rc2] Fix DatabaseDeveloperPageExceptionFilter to handle wrapped DbExceptions Sep 21, 2021
@ajcvickers
Copy link
Member Author

@javiercn @Pilchie RC2 fix for https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1399210

The database error page won't show when using EF Core 6.0 RC2 without this fix. It doesn't stop anything else working. That is, if the database is created as normal, then everything else should work. So the reason to get this into RC2 is so that people see the error page and get instructions on what to do.

Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@ajcvickers
Copy link
Member Author

/cc @AndriySvyryd FYI

@Pilchie Pilchie added the Servicing-consider Shiproom approval is required for the issue label Sep 21, 2021
@ghost
Copy link

ghost commented Sep 21, 2021

"Hi ajcvickers. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@jamshedd jamshedd added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Sep 21, 2021
@Pilchie Pilchie merged commit a50363f into release/6.0-rc2 Sep 21, 2021
@Pilchie Pilchie deleted the LookInsideYourselfForAnswers0921 branch September 21, 2021 19:32
@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants