Mvc: ActionResults returned from controller actions rendered as JSON, instead of executed

Created on 4 Jul 2016 · 12Comments · Source: aspnet/Mvc

Steps to reproduce

git clone https://github.com/nil4/MvcCoreObjectResultRepro.git
cd MvcCoreObjectResultRepro
dotnet restore
dotnet run

Open http://localhost:5000/ in your browser.

Expected results

The page displays {"a":"test"}, i.e. only the ObjectResult value is returned to the client.

Actual results

The page displays {"value":{"a":"test"},"formatters":[],"contentTypes":[],"declaredType":null,"statusCode":null} instead, i.e. all the properties of ObjectResult are returned to the client, not just the value.

3 - Done bug

Source

nil4

👍1

Most helpful comment

The problem is here and here. Both of these cases should have a conditional cast to IActionResult.

var resultAsObject = await _executor.ExecuteAsync(_controller, arguments);
result = new ObjectResult(resultAsObject)
{
    DeclaredType = _executor.TaskGenericType,
};

Should change to:

var resultAsObject = await _executor.ExecuteAsync(_controller, arguments);
result = resultAsObject as IActionResult;
if (result == null)
{
    result = new ObjectResult(resultAsObject)
    {
        DeclaredType = _executor.TaskGenericType,
    };
}

If you want a workaround, I'd suggest the following:

public class Fix4960ActionFilter : IActionFilter
{
    public void OnActionExecuting(ActionExecutingContext context) { }

    public void OnActionExecuted(ActionExecutedContext context)
    {
        var objectResult = context.Result as ObjectResult;
        if (objectResult?.Value is IActionResult)
        {
            context.Result = (IActionResult)objectResult.Value;
        }
    }
}

rynowak on 5 Jul 2016

👍3

All 12 comments

The incorrect behavior is new to 1.0 RTM, and used to work fine in RC2.

nil4 on 4 Jul 2016

Yep, this is definitely not intended. Thanks for the repro

rynowak on 5 Jul 2016

I updated the repro code to test other ActionResult subclasses. Here are the test URLs and results:

/201 (CreatedResult):

expected no content, status code = 201, Location header set;
actual {"location":"/","value":null,"formatters":[],"contentTypes":[],"declaredType":null,"statusCode":201} content, status code = 200, Location header missing.

/204 (NoContentResult):

expected no content, status code = 204
actual {"statusCode":204} content, status code = 200.

/302 (LocalRedirectResult):

expected no content, status code = 302, Location header set
actual {"permanent":false,"url":"/","urlHelper":null} content, status code = 200, Location header missing.

/304 (StatusCodeResult):

expected no content, status code = 304
actual {"statusCode":304} content, status code = 200.

/400 (BadRequestResult):

expected no content, status code = 400
actual {"statusCode":400} content, status code = 200.

/404 (NotFoundObjectResult):

expected no content, status code = 404
actual {"value":null,"formatters":[],"contentTypes":[],"declaredType":null,"statusCode":404} content, status code = 400.

nil4 on 5 Jul 2016

The problem is here and here. Both of these cases should have a conditional cast to IActionResult.

var resultAsObject = await _executor.ExecuteAsync(_controller, arguments);
result = new ObjectResult(resultAsObject)
{
    DeclaredType = _executor.TaskGenericType,
};

Should change to:

var resultAsObject = await _executor.ExecuteAsync(_controller, arguments);
result = resultAsObject as IActionResult;
if (result == null)
{
    result = new ObjectResult(resultAsObject)
    {
        DeclaredType = _executor.TaskGenericType,
    };
}

If you want a workaround, I'd suggest the following:

public class Fix4960ActionFilter : IActionFilter
{
    public void OnActionExecuting(ActionExecutingContext context) { }

    public void OnActionExecuted(ActionExecutedContext context)
    {
        var objectResult = context.Result as ObjectResult;
        if (objectResult?.Value is IActionResult)
        {
            context.Result = (IActionResult)objectResult.Value;
        }
    }
}

rynowak on 5 Jul 2016

👍3

@rynowak thank you for the workaround, will test and report back.

nil4 on 5 Jul 2016

@rynowak looking at the code you linked to, it seems both cases deal with non-async method return values. The code I posted is a minimal repro derived from an actual project where all actions are async, and those are affected as well. Thought I should mention this now to avoid a partial fix.

nil4 on 5 Jul 2016

Lines 641- 648 is the synchronous case. The fix for that case is the same as the sample I provided. This code has had a lot of perf work done on it, so it's a little confusing :laughing:

rynowak on 5 Jul 2016

@rynowak workaround confirmed, thank you!

nil4 on 5 Jul 2016

/cc @Eilon @DamianEdwards FYI

nil4 on 7 Jul 2016

@javiercn @Eilon @danroth27 The issue was reported three weeks ago but the fix is not yet merged. Is there still a chance of this making it into v1.0.1 or v1.1.0?

nil4 on 25 Jul 2016

It's currently slated to be fixed in 1.1.0