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.

[jsphpl]

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.

Not sure if i read you correctly, but the issue also exists when i just return Auth::user() from the controller with no Resource involved at all.

I don't see a bug here anywhere at all.

I think the test i posted initially should pass. And as it doesn't pass, i think there is a bug.

My point is: It's only half cool if Laravel is only half smart. Don't get me wrong, i love its smartness, but i'd love it even more if it was fully smart. That would mean for me, the developer, that i don't have to work around Laravel thinking a resource was created within the request/response lifecycle when it was actually created before the request.

So it's not a critical bug, but some potential for full smartness and even more developer joy.

[laurencei]

So fresh install of Laravel 5.7:

public function testBasicTest()
{
    $user = factory(\App\User::class)->create();
    $response = $this->actingAs($user)->json('GET', '/test');
    $response->assertOk();
}

and

Route::get('/test', function () {
    return Auth::user();
});

gives a 201.

This is a bug - because the user existed before the start of the reqeust. i.e. we did not create a new user and return it inside of /test.

The problem seems to be actingAs() is taking the literal output of $user and using that inside of the request.

[driesvints]

I still don't consider it a bug because you did create the user just before you made the request. We could change the default behavior of factories to not indicate newly created models as such but I think that would be a breaking change.

[jsphpl]

@driesvints yeah but just before is not during. So this is basically a discussion of whether wasRecentlyCreated should leak from the test into the controller. I think the (feature-) test is there to simulate a real request as made by a client to the production environment. Therefore, a resource created by the test before making the request should be treated the same way as if it had existed in the database before a real client made the request via real HTTP.

[laurencei]

Remember the point of 201 is the request was successful and you created a resource.

But it /test above - I didnt create a resource inside of that request.

The fact I created a $user prior to the test is irrelevant to the purpose of the request lifecycle.

[laurencei]

Anyway - 2 ways to approach this:

1. Factory sets wasRecentlyCreated to false - but I dont think that is valid

2. Anything you pass to actingAs() overrides wasRecentlyCreated to false - because you cant be pre-authenticated against a new resource. I cant think of a time this would ever be valid? Can anyone think of any?

[driesvints]

Anyway - 2 ways to approach this:

1. Factory sets wasRecentlyCreated to false - but I dont think that is valid

2. Anything you pass to actingAs() overrides wasRecentlyCreated to false - because you cant be pre-authenticated against a new resource.

Number 2 seems a valid point to me 👍

[jsphpl]

because you cant be pre-authenticated against a new resource. I cant think of a time this would ever be valid? Can anyone think of any?

Makes perfect sense to me.

Can't fully wrap my head around it, but what if authentication happens against a third party? In that case, the server doesn't necessarily need to have prior knowledge about the user? But not sure if that matters in the context of actingAs at all, because the method's sole purpose is to set the "user" in the request to whatever you pass it, right?

[laurencei]

Can't fully wrap my head around it, but what if authentication happens against a third party? In that case, the server doesn't necessarily need to have prior knowledge about the user?

But for actingAs() - which is a test function - the server must know about the user before hand.

I've done a PR with a proposed fux

[jsphpl]

I've done a PR with a proposed fux

Great, thanks a lot!

[memochou1993]

use:

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

Passport::actingAs(User::find($user->id));

This will return 200.