Add endpoint metadata to developer exception page #52668
Conversation
ghost
added
area-middleware
|
Thanks for your PR, @Kahbazi. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
Kahbazi
added
feature-diagnostics
| { | ||
| <tr> | ||
| <td>@metadata.GetType().Name</td> | ||
| <td>@(metadata.ToString())</td> |
There was a problem hiding this comment.
Does Razor escape HTML by default? I want to make sure that a ToString result that returns HTML doesn't accidently break the page.
There was a problem hiding this comment.
Also, what happens if metadata has new lines? They should be kept when displayed on the web page.
There was a problem hiding this comment.
I think HtmlEncodeAndReplaceLineBreaks (which the title above uses) should be called here.
| { | ||
| <p>@Resources.ErrorPageHtml_NoRouteValues</p> | ||
| } | ||
| <h2>@Resources.ErrorPageHtml_Metadata</h2> |
There was a problem hiding this comment.
The endpoint metadata section is very related to the endpoint section. Either it should be:
- nested in the endpoint section, e.g.
Metadatahas an h3 or h4 title. - or the metadata section should be below the endpoint section and the title should be
Endpoint Metadata. If there is no endpoint then there is no need to also say there is no endpoint metadata.
| <p>@Resources.ErrorPageHtml_NoRouteValues</p> | ||
| } | ||
| <h2>@Resources.ErrorPageHtml_Metadata</h2> | ||
| @if (Model.Endpoint?.Metadata != null) |
There was a problem hiding this comment.
Also check that the collection isn't empty, e.g. @if (Model.Endpoint?.Metadata.Count > 0) { ... }
|
Overall, looks good. Just a few changes needed. |
| @foreach (var metadata in Model.Endpoint.Metadata) | ||
| { | ||
| <tr> | ||
| <td>@metadata.GetType().Name</td> |
There was a problem hiding this comment.
Could you add the full type name as a tooltip.
Something like:
| <td>@metadata.GetType().Name</td> | |
| <td><span title="@metadata.GetType().FullName">@metadata.GetType().Name<span></td> |
(untested)
| <th>@Resources.ErrorPageHtml_VariableColumn</th> | ||
| <th>@Resources.ErrorPageHtml_ValueColumn</th> | ||
| <td><span title="@metadata.GetType().FullName">@metadata.GetType().Name</span></td> | ||
| <td>@HtmlEncodeAndReplaceLineBreaks(metadata.ToString())</td> |
There was a problem hiding this comment.
I include both with HtmlEncodeAndReplaceLineBreaks and without in the description. I suggest to keep the one without. The razor would encode the html and omit the new lines.
ghost
added
the
pending-ci-rerun
wtgodbe
removed
the
pending-ci-rerun
dotnet-policy-service
bot
added
the
area-infrastructure
wtgodbe
removed
the
area-infrastructure
dotnet-policy-service
bot
added
the
pending-ci-rerun
|
Is next action here on us -- @JamesNK are you the right person to do the re-review? |
|
Apologies for the delay in reviewing again. Looks good. |
|
/azp run |
dotnet-policy-service
bot
removed
the
pending-ci-rerun
|
Azure Pipelines successfully started running 3 pipeline(s). |
JamesNK
added
the
blog-candidate
|
@Kahbazi, this change will be considered for inclusion in the blog post for the release it'll ship in. Nice work! Please ensure that the original comment in this thread contains a clear explanation of what the change does, why it's important (what problem does it solve?), and, if relevant, include things like code samples and/or performance numbers. This content may not be exactly what goes into the blog post, but it will help the team putting together the announcement. Thanks! |
Fixes #48208