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

CRUD Endpoint POST response is a list #211

Open
ocervell opened this issue Feb 2, 2023 · 9 comments
Open

CRUD Endpoint POST response is a list #211

ocervell opened this issue Feb 2, 2023 · 9 comments

Comments

@ocervell
Copy link

ocervell commented Feb 2, 2023

On making a POST request to a CRUD endpoint, the response is something like:

[{'id': 4}]

IMHO this should be a dict following good REST practices and not a list since we cannot post multiple objects to the CRUD Endpoint. The client should be able to run something like:

>>> response = requests.post(crud_endpoint_url, json=data).json()
>>> response['id']
# created object id

The problem stems from this line: https://github.com/piccolo-orm/piccolo_api/blob/master/piccolo_api/crud/endpoints.py#L819

Actually row.save().run() seems to always return a list with one object ... assuming this is the case, we could strip the object out of the list and simply return the dict:

response = await row.save().run()
item = response[0]
json = dump_json(item)
return CustomJSONResponse(json, status_code=201)
@mastercoms
Copy link

mastercoms commented Apr 25, 2023

Also it returns 201 instead of 200 as in the OpenAPI schema (for FastAPIWrapper), the schema should be fixed or if it can't, change to 200

@sinisaos
Copy link
Member

@mastercoms I think HTTP 201 CREATED is a good response status code because the request was successful and led to the creation of a new resource. Sorry if I'm wrong.

@dantownsend
Copy link
Member

I think what's going on is we're returning a 201 from PiccoloCRUD, but FastAPIWrapper has the default of 200. They should be consistent really - it should be a quick fix.

def modify_signature(

@sinisaos
Copy link
Member

@dantownsend I don't think we should change anything because both PiccoloCRUD and FastAPIWrapper ,in my case, return correct 201 on POST request? Sorry if I don't understand correctly.

@dantownsend
Copy link
Member

@sinisaos Sorry, I wasn't clear - they both return 201, but in the Swagger docs it probably say 200.

It's because we don't tell FastAPI what status code we're returning.

@sinisaos
Copy link
Member

sinisaos commented Apr 25, 2023

@dantownsend Aha, now I understand! Here we can add status_code=status.HTTP_201_CREATED, to add_api_route of POST method and it works.
swagger

Should we do it for all methods?

@dantownsend
Copy link
Member

Should we do it for all methods?

@sinisaos yeah, at least for any which don't return 200

@sinisaos
Copy link
Member

@dantownsend If you OK with this changes

fastapi_app.add_api_route(
    path=root_url,
    endpoint=post,
    response_model=self.ModelOut,
    status_code=status.HTTP_201_CREATED, # new line
    methods=["POST"],
    **fastapi_kwargs.get_kwargs("post"),
)

I can try to do this tomorrow for all the methods to have the same status codes in the Swagger docs as in the PiccoloCRUD.

@dantownsend
Copy link
Member

@sinisaos Yes, that looks right.

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

No branches or pull requests

4 participants