Convert DatabaseErrorPage to exception filter #24588

JunTaoLuo · 2020-08-05T17:46:06Z

Decoupling this from EF and updating WebHost to add this by default in dev scenarios will be done separately.

JunTaoLuo · 2020-08-06T08:01:14Z

WIP, consider adding some tests.

Tratcher · 2020-08-06T15:36:33Z

Can you give a screenshot of what the new error page looks like?

Tratcher · 2020-08-06T15:38:15Z

You'll need an EF reviewer for half of this.

src/Middleware/Diagnostics.EntityFrameworkCore/src/Views/DatabaseErrorPage.cshtml

...ware/Diagnostics.EntityFrameworkCore/test/FunctionalTests/DatabaseErrorPageMiddlewareTest.cs

src/Middleware/Diagnostics.EntityFrameworkCore/src/Strings.resx

src/Middleware/Diagnostics.EntityFrameworkCore/src/DatabaseExceptionFilter.cs

JunTaoLuo · 2020-08-06T19:31:50Z

Here's a screenshot of a case where there are two contexts with pending migrations and one with a missing database:

JunTaoLuo · 2020-08-06T21:31:54Z

src/Middleware/Diagnostics.EntityFrameworkCore/src/DatabaseExceptionFilter.cs

+using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Options;
+
+namespace Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore


Maybe this belongs in the M.A.Diagnostics namespace along with IDeveloperPageExceptionFilter. It would save most users a using statement.

JunTaoLuo · 2020-08-06T21:32:27Z

🆙📅

javiercn · 2020-08-07T14:54:59Z

Decoupling this from EF and updating WebHost to add this by default in dev scenarios will be done separately.

What's the issue tracking this change?

src/Identity/ApiAuthorization.IdentityServer/samples/ApiAuthSample/Startup.cs

src/Middleware/Diagnostics.EntityFrameworkCore/src/DatabaseErrorPageExtensions.cs

src/Middleware/Diagnostics.EntityFrameworkCore/src/DatabaseExceptionFilter.cs

...iddleware/Diagnostics.EntityFrameworkCore/src/HttpContextDatabaseContextDetailsExtensions.cs

pranavkm

Can you update the PR to point to an issue \ add a description that says why this is necessary? I'm not familiar with this area so I'm not sure why this is required.

src/Identity/samples/IdentitySample.Mvc/Startup.cs

JunTaoLuo · 2020-08-11T22:15:35Z

src/Middleware/Diagnostics.EntityFrameworkCore/src/DatabaseExceptionFilter.cs

+        private readonly ILogger _logger;
+        private readonly DatabaseErrorPageOptions _options;
+
+        public DatabaseExceptionFilter(ILogger<DatabaseExceptionFilter> logger, IOptions<DatabaseErrorPageOptions> options)


Hmm I'm reusing the DatabaseErrorPageOptions type originally used by the middleware. Should we introduce a new type that's exactly the same but has a matching name? Something like DatabaseDeveloperPageExceptionFilterOptions?

If you think the old type is good enough, maybe it's fine. It's unlikely you'll have both of these enabled and want different behavior for the two.

pranavkm

Looks good. Can you send an email for the api review?

src/Middleware/Diagnostics.EntityFrameworkCore/src/DatabaseDeveloperPageExceptionFilter.cs

...Diagnostics.EntityFrameworkCore/src/DatabaseDeveloperPageExceptionFilterServiceExtensions.cs

ajcvickers · 2020-08-13T17:59:20Z

@bricelam Can you take a look at this? Specifically, the code that finds the DbContext registered in D.I. and the code that uses it for Migrations? (Note that we're not attempting to handle every possible way a DbContext might be used in the project, just the cases where it is registered in the ASP.NET container using one of the normal AddDbContext type calls. This is why the discovery code is simpler than we have for EF tooling in general.)

bricelam

LGTM

bricelam · 2020-08-14T18:41:33Z

...iddleware/Diagnostics.EntityFrameworkCore/src/HttpContextDatabaseContextDetailsExtensions.cs

+            var snapshotModel = migrationsAssembly.ModelSnapshot?.Model;
+            if (snapshotModel is IConventionModel conventionModel)
+            {
+                var conventionSet = context.GetService<IConventionSetBuilder>().CreateConventionSet();
+
+                var typeMappingConvention = conventionSet.ModelFinalizingConventions.OfType<TypeMappingConvention>().FirstOrDefault();
+                if (typeMappingConvention != null)
+                {
+                    typeMappingConvention.ProcessModelFinalizing(conventionModel.Builder, null);
+                }
+
+                var relationalModelConvention = conventionSet.ModelFinalizedConventions.OfType<RelationalModelConvention>().FirstOrDefault();
+                if (relationalModelConvention != null)
+                {
+                    snapshotModel = relationalModelConvention.ProcessModelFinalized(conventionModel);
+                }
+            }
+
+            if (snapshotModel is IMutableModel mutableModel)
+            {
+                snapshotModel = mutableModel.FinalizeModel();
+            }


@AndriySvyryd Is all this still necessary? Is there an easier way?

Yes. Easier way will be enabled by dotnet/efcore#22031

Technically we should also call SnapshotModelProcessor.Process() to fix old snapshots, but this would bring in all kinds of dependencies

Pilchie added the area-middleware label Aug 5, 2020

Convert DatabaseErrorPage middleware to exception filter

cbe1da3

JunTaoLuo force-pushed the johluo/db-errors branch from 6423058 to cbe1da3 Compare August 6, 2020 07:39

JunTaoLuo changed the title ~~Database error filter~~ Convert DatabaseErrorPage to exception filter Aug 6, 2020

JunTaoLuo marked this pull request as ready for review August 6, 2020 08:00

JunTaoLuo requested a review from Tratcher as a code owner August 6, 2020 08:00

Remove decoupling attempts

37e2433

JunTaoLuo force-pushed the johluo/db-errors branch from 3b9ab63 to 37e2433 Compare August 6, 2020 08:04

Tratcher reviewed Aug 6, 2020

View reviewed changes

src/Middleware/Diagnostics.EntityFrameworkCore/src/Views/DatabaseErrorPage.cshtml Outdated Show resolved Hide resolved

Tratcher approved these changes Aug 6, 2020

View reviewed changes

JunTaoLuo requested a review from ajcvickers August 6, 2020 19:31

Feedback and fixups

b47657f

JunTaoLuo requested review from SteveSandersonMS and a team as code owners August 6, 2020 21:18

Revert webassembly file change

b0007b3

JunTaoLuo removed request for a team and SteveSandersonMS August 6, 2020 21:22

JunTaoLuo commented Aug 6, 2020

View reviewed changes

Fix musicstore

ed20f89

javiercn reviewed Aug 7, 2020

View reviewed changes

src/Identity/ApiAuthorization.IdentityServer/samples/ApiAuthSample/Startup.cs Outdated Show resolved Hide resolved

javiercn reviewed Aug 7, 2020

View reviewed changes

src/Middleware/Diagnostics.EntityFrameworkCore/src/DatabaseErrorPageExtensions.cs Outdated Show resolved Hide resolved

pranavkm reviewed Aug 7, 2020

View reviewed changes

pranavkm added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Aug 7, 2020

pranavkm reviewed Aug 7, 2020

View reviewed changes

src/Identity/samples/IdentitySample.Mvc/Startup.cs Outdated Show resolved Hide resolved

JunTaoLuo commented Aug 11, 2020

View reviewed changes

Feedback

0f6ad6e

pranavkm approved these changes Aug 11, 2020

View reviewed changes

John Luo added 2 commits August 11, 2020 22:07

Fixup

69801ce

Fix test

cd6386d

ajcvickers requested a review from bricelam August 13, 2020 17:56

bricelam approved these changes Aug 14, 2020

View reviewed changes

JunTaoLuo merged commit cfe158c into master Aug 17, 2020

JunTaoLuo deleted the johluo/db-errors branch August 17, 2020 18:13

JunTaoLuo mentioned this pull request Sep 3, 2020

Add MigrationsEndPoint when DatabaseDeveloperPageExceptionFilter is used #25554

Merged

amcasey added the area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares label Jun 2, 2023

Convert DatabaseErrorPage to exception filter #24588

Convert DatabaseErrorPage to exception filter #24588

Uh oh!

Conversation

JunTaoLuo commented Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JunTaoLuo commented Aug 6, 2020

Uh oh!

Tratcher commented Aug 6, 2020

Uh oh!

Tratcher commented Aug 6, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JunTaoLuo commented Aug 6, 2020

Uh oh!

JunTaoLuo Aug 6, 2020

Choose a reason for hiding this comment

Uh oh!

JunTaoLuo commented Aug 6, 2020

Uh oh!

javiercn commented Aug 7, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pranavkm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JunTaoLuo Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

pranavkm Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

pranavkm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ajcvickers commented Aug 13, 2020

Uh oh!

bricelam left a comment

Choose a reason for hiding this comment

Uh oh!

bricelam Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

AndriySvyryd Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

AndriySvyryd Aug 14, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JunTaoLuo commented Aug 5, 2020 •

edited

Loading