dedoc / scramble

Modern Laravel OpenAPI (Swagger) documentation generator. No PHPDoc annotations required.
https://scramble.dedoc.co/
MIT License
1.24k stars 119 forks source link

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

Open romalytvynenko opened 1 year ago

romalytvynenko commented 1 year ago

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 commented 4 months ago

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 commented 4 months ago

hello!

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

/**
 * @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 commented 4 months ago

@SRWieZ looks like a bug to me! Will fix

romalytvynenko commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

Thanks @romalytvynenko !

Tried but one last thing:

romalytvynenko commented 4 months ago

@SRWieZ

Without any comment: still not wrapper into data

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

maxechendu commented 2 months ago

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 commented 2 months ago

@maxechendu which Scramble version are you using?

Also please remove the @response annotation and try again

maxechendu commented 2 months ago

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 commented 2 months ago

@maxechendu which Scramble version are you using

Also please remove the @response annotation and try again

Yes, that was my mistake. Thank you 😃