Skip to content

ObjectResultExecutor could output a clearer log w.r.t the concrete type of ObjectResult #21393

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

Closed
andrewjsaid opened this issue May 1, 2020 · 6 comments
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one good first issue Good for newcomers.

Comments

@andrewjsaid
Copy link
Contributor

Is your feature request related to a problem? Please describe.

ObjectResultExecutor gives potentially misleading log. Specifically "ObjectResultExecuting" is define in MvcCoreLoggerExtensions as follows:
Executing ObjectResult, writing value of type '{Type}'.

The misleading part is when a controller returns, for example, a NotFoundObjectResult(string) or an OkObjectResult(string). The logs look like this in either case:
[Information] Microsoft.AspNetCore.Mvc.Infrastructure.ObjectResultExecutor: Executing ObjectResult, writing value of type 'System.String'.

Describe the solution you'd like

My suggestion is to output the declared runtime/real/concrete type of ObjectResult along the lines of
Executing {ConcreteObjectResultType}, writing value of type '{Type}'.

The above message is more useful because

  1. It provides more detail as to what happened.
  2. It does not leak implementation details of NotFoundObjectResult.

Additional context

Of course, there are other ways to discover the same information (such as the Status Code) so this issue is not critical. Another point against this issue is that the ControllerActionInvoker does specify the runtime type of ActionResult.

@Pilchie Pilchie added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label May 1, 2020
@pranavkm
Copy link
Contributor

pranavkm commented May 1, 2020

My suggestion is to output the declared runtime/real/concrete type of ObjectResult along the lines of
Executing {ConcreteObjectResultType}, writing value of type '{Type}'.

This sounds fine. Would you like to send us a PR for this change?

It does not leak implementation details of NotFoundObjectResult.

Can you elaborate on this?

@andrewjsaid
Copy link
Contributor Author

This sounds fine. Would you like to send us a PR for this change?

Sure thing! I'll give it a try.

It does not leak implementation details of NotFoundObjectResult.
Can you elaborate on this?

The more I try to write an explanation the less I'm sure what I meant. However, I think I meant that from the perspective of a somebody reading the logs, whether or not NotFoundObjectResult relies on the base class's ExecuteResultAsync is not relevant to my concerns and is thus an implementation detail.

@rynowak
Copy link
Member

rynowak commented May 1, 2020

This seems like a nice improvement 👍

@pranavkm pranavkm added the good first issue Good for newcomers. label May 4, 2020
@pranavkm pranavkm added this to the Backlog milestone May 4, 2020
@pranavkm pranavkm added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label May 4, 2020
@docmaster2
Copy link

перспектива
second improvement

@rynowak
Copy link
Member

rynowak commented May 7, 2020

Thanks!

@rynowak rynowak closed this as completed May 7, 2020
@rynowak rynowak added the Done This issue has been fixed label May 7, 2020
@rynowak rynowak modified the milestones: Backlog, 5.0.0-preview4 May 7, 2020
@docmaster2
Copy link

интересуют реквизиты картины,может быть ретро,модерн,современный стиль.

@ghost ghost locked as resolved and limited conversation to collaborators Jun 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one good first issue Good for newcomers.
Projects
None yet
Development

No branches or pull requests

5 participants