Response status should be 200 instead of 201 when returned model was created from within a test before making the request

[jsphpl]

• Laravel Version: 5.7
• PHP Version: 7.2
• Database Driver & Version: Postgres 9.6

Description:

In a PHPUnit test, when creating a User through a factory and returning Auth::user() from a controller/action, the response status is 201, where it should be 200.

This was addressed in #21107 #25775 and #23270 (and probably others). However, the suggested solution of ->refresh()ing the user before passing it to ->actingAs() doesn't work for me. Further, i think it should not be necessary to refresh the user in order to work around the framework trying to be smart. It's not difficult to manually set the response status to 201 in an endpoint where a fresh resource is created. So the "smartness" of wasRecentlyCreated resulting in a 201 status code is not that valuable. Especially if it's only half smart and doesn't distinguish between resources created within the request/response lifecycle and resources created by a test before making the request.

I consider the following scenario a valid test that should actually pass. If you agree on that, let's look for a solution together. If you don't agree that the test should pass, feel free to close this issue, and sugggest a reliable approach to get the proper status code that is consistent between test and production environment.

Steps to reproduce:

1. Create a test case:

$user = factory(User::class)->create();

$response = $this->actingAs($user, 'api')->json('GET', '/api/v1/users/me');
$response->assertOk();

2. Create the corresponding action:

<?php

namespace App\Http\Actions\User;

use App\Http\Resources\UserResource;
use Illuminate\Support\Facades\Auth;

class ShowMeAction
{
    public function __invoke()
    {
        return new UserResource(Auth::user());
    }
}

3. Run the test

4. Call ->refresh() on the user before passing it to actingAs()

Contrary to what was suggested as a solution in one of the quoted issues, even with the refresh() call, the test fails for me:

Response status code [201] does not match expected 200 status code.
Failed asserting that false is true.

[jsphpl]

Update: Mixed up two of the issues. It actually works when calling ->fresh() on the model (instead of refresh). Yet, i still think the above test is valid and should pass and the code that overrides the default status code with 201 should be responsible for distinguishing between models created within the request and before the request.

[laurencei]

Why are you doing fresh or refresh in the middle of the call before passing it to actingAs? Just setup the user how you want it to be in the factory() method?

[jsphpl]

@laurencei because without the call to fresh(), laravel thinks that the model was created (wasRecentlyCreated == true) in the controller action and generates a response with a 201 status.

Forget refresh() – it doesn't help anything in the context of this issue. I apparently just misread one of the other issues mentioned.

[laurencei]

You are returning a new UserResource() in your action - that would probably trip it up to think its a new object?

[jsphpl]

You are returning a new UserResource() in your action - that would probably trip it up to think its a new object?

No. It's smarter than that. Test also fails if i omit the UserResource and just return Auth::user().

As can be seen in the PR #21625 (which i assume is causing the issue), it checks against $response->wasRecentlyCreated.

[driesvints]

In the internals of a ResourceResponse I see the following piece of code:

   /**
     * Calculate the appropriate status code for the response.
     *
     * @return int
     */
    protected function calculateStatus()
    {
        return $this->resource->resource instanceof Model &&
               $this->resource->resource->wasRecentlyCreated ? 201 : 200;
    }

Because the user was "recently created" with the factory you'll get back a 201 status code. If you want to get back a 200 status code and mimic a user which already existed you'll have to set the $wasRecentlyCreated property on the user model to false.

It's only possible to do this beforehand because it's just the way resources work for new models so the developer doesn't needs to worry about which status code he's going to have to return.

[laurencei]

@driesvints - but if the model was created inside a factory before the request - then returning 201 is incorrect - because wasRecentlyCreated should only apply to the lifecycle of the request.

I havent fully tested - but this looks like a bug to me.

[driesvints]

@laurencei It's probably best to open a new issue then because it's unrelated to resource responses or status codes.

[jsphpl]

@driesvints my issue description doesn't mention Resources at all – it's just using typical example code containing a Resource. It is mentioned in the description that the issue is around wasRecentlyCreated.

[jsphpl]

…but i still agree with you, @driesvints – it should be clear in the issue title and description that the issue is wasRecentlyCreated :)

[jsphpl]

@laurencei are you opening a new issue or do you want me to do so?

[laurencei]

You can do it @jsphpl .

I'm testing on a fresh 5.7 install - and I can replicate the issue ... but I'm not sure what the fix is. Open the ticket and i'll talk there.

[driesvints]

@jsphpl yes you did. But to be honest I don't see a bug here anywhere at all. ResourceResponse are trying to give an easy way to automatically set the status code based on the wasRecentlyCreated property and newly created models with factories do get their wasRecentlyCreated property set to true because they just use the underlying mechanics of model creation.

I believe that for testing ResourceResponse you specifically need to indicate in the test that the model wasn't "recently" created by changing the property value.

I'll re-open this issue to let @taylorotwell have a look to see what he has to say, no need to create a new issue. Sorry for the confusion.