Skip to content
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

Returning newly created models (either as is or in resource) should result in 201 response status #132

Open
romalytvynenko opened this issue Apr 20, 2023 · 11 comments
Labels
enhancement New feature or request

Comments

@romalytvynenko
Copy link
Member

Conv from Discord with context:


I have an issue concerning my responses returning a 201 status code, which is documented as returning a 200 code.

This is because I'm returning a JsonResource instance; it checks if $model->wasRecentlyCreated === true to define the status code. See: https://github.com/laravel/framework/blob/9.x/src/Illuminate/Http/Resources/Json/ResourceResponse.php#L117

How could we do with Scramble to figure this out? It seems complicated, but maybe you've already thought about it.

@antoine
Hey! I just tried Scramble. It works well! I have an issue concerning my responses returning a 201 status code, which is documented as returning a 200 code. This is because I'm returning a JsonResource instance; it checks if $model->wasRecentlyCreated === true to define the status code. See: https://github.com/laravel/framework/blob/9.x/src/Illuminate/Http/Resources/Json/ResourceResponse.php#L117 How could we do with Scramble to figure this out? It seems complicated, but maybe you've already thought about it.

romalyt — 01/15/2023 6:45 PM
Hey hey!

Yeah, I thought about this. My idea was to check if a model you pass to JsonResource is created by using new, ::create, or ::make methods and if so, document a response with 201 status.

I'm wondering, how do you create models you pass to JsonResource?

P.S.: As a quick fix, you always can help Scramble to document what is needed 😻 (https://scramble.dedoc.co/usage/response#response-description)
/** @status 201 */
return new TodoItemResource($todoItem);

antoine — 01/15/2023 6:50 PM
Well, it depends, for some clients, I just do Model::create() in the controller. But for other clients having more complex projets (or bigger teams), I call a service class (or repository) in charge of creating the model

Well, then figuring out if a response is 201 one, is getting tricky as I use static analysis to determine types in code. But I think the good starting point will be analyzing for new/create/make in controller. And in the future as type analysis part getting better Scramble may also be able to check the code of service classes/repositories.

Yes, new/create/make in the controller would cover 70% of the projets I work on

@shamil8124
Copy link

shamil8124 commented Jun 22, 2024

the @status 201 does not work if you've defined the return type as a paginated resource collection.

/**
     * @operationId Bulk create tests
     *
     * @return AnonymousResourceCollection<LengthAwarePaginator<TestResource>>
     */
    public function bulkStore(TestRequest $request)
    {
        /**
         * @status 201
         */
        return (TestResource::collection($this->createTests($request)))->toResponse()->setStatusCode(201);
    }

@SRWieZ
Copy link

SRWieZ commented Jul 1, 2024

hello!

I also have trouble returning the correct Response data and Status code together.

  • If I use @response I have the correct return value but the wrong status code
  • If user @status with @body, the status code is correct but the response body is not wrapped in data anymore
/**
 * @response ZonesRecordResource
 */
public function store(Request $request, string $domain)
{
    // ...

    /**
     * @status 201
     * @body ZonesRecordResource
     */
    return (new ZonesRecordResource($record))->response()->setStatusCode(201);
}

Did I miss something on the docs?

@romalytvynenko
Copy link
Member Author

@SRWieZ looks like a bug to me! Will fix

@romalytvynenko
Copy link
Member Author

@SRWieZ fixed the issue of resource being not properly wrapped when specified in @body.

Also improved type inference a bit and now using ->response()->setStatusCode(201) will be properly documented without any additional annotations. CC @shamil8124

Please upgrade to https://github.com/dedoc/scramble/releases/tag/v0.11.1

@romalytvynenko
Copy link
Member Author

@shamil8124 you should use @body when using status code.

   /**
     * @operationId bulkCreateTests
     */
    public function bulkStore(TestRequest $request)
    {
        /**
         * @status 201
         * @body AnonymousResourceCollection<LengthAwarePaginator<TestResource>>
         */
        return (TestResource::collection($this->createTests($request)))->toResponse()->setStatusCode(201);
    }

But in 0.11.1 the correct response should be automatically inferred:

/**
 * @operationId bulkCreateTests
 */
public function bulkStore(TestRequest $request)
{
    return (TestResource::collection($this->createTests($request)))->toResponse()->setStatusCode(201);
}

@SRWieZ
Copy link

SRWieZ commented Jul 2, 2024

Thanks @romalytvynenko !

Tried but one last thing:

  • With status and body: works great!
  • Without any comment: still not wrapper into data

@romalytvynenko
Copy link
Member Author

@SRWieZ

Without any comment: still not wrapper into data

Can you show the controller's code and the result?

@maxechendu
Copy link

maxechendu commented Sep 12, 2024

Hi, is there a fix or workaround for this? The documentation still shows a 200 instead of a 201 for me, despite trying the suggestions above. Here's my controller method setup:

    /**
     * Create an item
     *
     * @response ItemResource
     */
    public function store(ItemStoreRequest $request): JsonResponse
    {
        return response()->json(new ItemResource(
                $this->service->create($request->validated())
        ), 201);
    }

I've tried annotating @status 201 and setting the status via ->setStatusCode(201) but nothing worked.

@romalytvynenko
Copy link
Member Author

romalytvynenko commented Sep 12, 2024

@maxechendu which Scramble version are you using?

Also please remove the @response annotation and try again

@maxechendu
Copy link

Hi, is there a fix or workaround for this? The documentation still shows a 200 instead of a 201 for me, despite trying the suggestions above. Here's my controller method setup:

    /**
     * Create an item
     *
     * @response ItemResource
     */
    public function store(ItemStoreRequest $request): JsonResponse
    {
        return response()->json(new ItemResource(
                $this->service->create($request->validated())
        ), 201);
    }

I've tried annotating @status 201 and setting the status via ->setStatusCode(201) but nothing worked.

Never mind. Removing the @response ItemResource fixed my issue 👍

@maxechendu
Copy link

@maxechendu which Scramble version are you using

Also please remove the @response annotation and try again

Yes, that was my mistake. Thank you 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants