feat: Add response models #231
Conversation
…ix endpoints that were returning plain strings instead of JSON objects
|
|
||
| if cur.rowcount == 1: | ||
| return "ok" | ||
| return {"status": "ok"} |
There was a problem hiding this comment.
We're changing the API here. @CharlesNepote is that ok?
There was a problem hiding this comment.
We're changing the API here. @CharlesNepote is that ok?
Regarding the change in the response model from a string ("ok") to a dict ({"status": "ok"}): while this change might affect the API, upgrading all responses to Pydantic models is key in the long run. One of FastAPI's core conventions is to use Pydantic models as much as possible, and changing the response structure now could potentially save many headaches in the future. Additionally, for validation failures (422), the response is already a JSON structure; depending on how the endpoint is used, it might be harder to check types on the frontend than to maintain consistency. Just my two cents.
There was a problem hiding this comment.
@suchithh I understand your concern, but API change might have impact on reusers: we need to talk with them first and evaluate potential impacts.
So you should push a first PR without modifying the API, and a second one to push API changes.
@suchithh you can add a commit. We will squash commits before merging. |
CharlesNepote
left a comment
CharlesNepote
left a comment
There was a problem hiding this comment.
Thanks @suchithh, we're almost done. Please distinguish your changes into two different PRs (see my comment below).
|
|
||
| if cur.rowcount == 1: | ||
| return "ok" | ||
| return {"status": "ok"} |
There was a problem hiding this comment.
@suchithh I understand your concern, but API change might have impact on reusers: we need to talk with them first and evaluate potential impacts.
So you should push a first PR without modifying the API, and a second one to push API changes.
| ) | ||
| if cur.rowcount == 1: | ||
| return "ok" | ||
| return {"status": "ok"} |
| """, | ||
| (product, owner, k.lower()), | ||
| ) | ||
| if cur.rowcount == 1: |
There was a problem hiding this comment.
This and the next line are redundant with line 510-511.
|
Hi @CharlesNepote, thanks for the feedback. That makes sense and I figured I should've done that in the first place. I'll make the changes asap and make new PRs. |
|
@CharlesNepote Hi Charles, please check my new PRs #238 #239 |
4 participants
What
This PR adds proper response models for all endpoints to ensure the FastAPI documentation accurately represents the actual response structure. Fixes #118
Changes
HelloResponsemodel for root endpointTokenResponsemodel for auth endpointsKeyStatisticsmodel for /keys endpoint Add typing for OpenAPI response for/keys#149PingResponsemodel for /ping endpointValueCountmodel for /values endpointresponse_modelparametersThese changes improve the documentation accuracy in the OpenAPI spec and potentially type safety for the API.