This repository has been archived by the owner on Sep 18, 2024. It is now read-only.
[Issue #69] Remove the BASE_RESPONSE_SCHEMA #70
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chouinar
requested review from
jamesbursa,
sumiat,
andycochran and
acouch
as code owners
github-actions
bot
added
documentation
rylew1
approved these changes
Jun 5, 2024
acouch
pushed a commit
that referenced
this pull request
Sep 18, 2024
Fixes HHS#2066 Remove the `BASE_RESPONSE_SCHEMA` configuration. Adjust the healthcheck endpoint to be consistent in defining the schema / responses with the other endpoints. APIFlask allows you to set a `BASE_RESPONSE_SCHEMA` - the idea is that its the shared schema that every API endpoint will return, and they will only differ in the `data` object within. This sounds great on paper as it prevents you from needing to define most of the response schema for many endpoints, but in practice, its clunky to use. If we want to modify anything in the response schema outside of the `data` object, it will affect every single endpoint. This means when we add something like the pagination info to our search endpoint, a pagination object appears on the healthcheck response. As we intend to make these docs something the public will use, this is going to be confusing. There is also a "bug" (unsure if it is as it was an intended change a few months ago in APIFlask/apispec) that the error response objects end up nested within themselves in the examples. For example, currently the error response for the healthcheck endpoint (which can literally only return a 5xx error) has an example response of: ```json { "data": { "data": "string", "errors": [ { "field": "string", "message": "string", "type": "string" } ], "message": "string", "status_code": 0 }, "message": "string", "pagination_info": { "order_by": "id", "page_offset": 1, "page_size": 25, "sort_direction": "ascending", "total_pages": 2, "total_records": 42 }, "status_code": 0, "warnings": [ { "field": "string", "message": "string", "type": "string" } ] } ``` When in reality, the error response it actually returns looks like: ```json { "data": {}, "errors": [], "message": "Service Unavailable", "status_code": 503 } ``` This set of changes works around all of these confusing issues and just requires us to define specific response schemas for each endpoint with some small set of details. I've kept some base schema classes to derive from that we can use in most cases. Before & After Examples in OpenAPI <table> <tr><th>Endpoint</th><th>Before</th><th>After</th><th>Actual Response (before change)</th></tr> <tr><td>GET /health (200) </td> <td> ```json { "data": { "message": "string" }, "message": "string", "pagination_info": { "order_by": "id", "page_offset": 1, "page_size": 25, "sort_direction": "ascending", "total_pages": 2, "total_records": 42 }, "status_code": 0, "warnings": [ { "field": "string", "message": "string", "type": "string" } ] } ``` </td> <td> ```json { "data": null, "message": "Success", "status_code": 200 } ``` </td> <td> ```json { "data": {}, "message": "Service healthy", "pagination_info": null, "status_code": 200, "warnings": [] } ``` </td> </tr> <tr><td>GET /health (503)</td> <td> ```json { "data": { "data": "string", "errors": [ { "field": "string", "message": "string", "type": "string" } ], "message": "string", "status_code": 0 }, "message": "string", "pagination_info": { "order_by": "id", "page_offset": 1, "page_size": 25, "sort_direction": "ascending", "total_pages": 2, "total_records": 42 }, "status_code": 0, "warnings": [ { "field": "string", "message": "string", "type": "string" } ] } ``` </td> <td> ```json { "data": {}, "errors": [], "message": "Error", "status_code": 0 } ``` </td> <td> ```json { "data": {}, "message": "Service unavailable", "pagination_info": null, "status_code": 200, "warnings": [] } ``` </td> </tr> <tr><td>POST /v0.1/opportunities/search (200)</td> <td> ```json { "data": [ {.. Excluding for brevity } ], "message": "string", "pagination_info": { "order_by": "id", "page_offset": 1, "page_size": 25, "sort_direction": "ascending", "total_pages": 2, "total_records": 42 }, "status_code": 0, "warnings": [ { "field": "string", "message": "string", "type": "string" } ] } ``` </td> <td> ```json { "data": [ {.. Excluding for brevity } ], "message": "Success", "pagination_info": { "order_by": "id", "page_offset": 1, "page_size": 25, "sort_direction": "ascending", "total_pages": 2, "total_records": 42 }, "status_code": 200 } ``` </td> <td> ```json { "data": [ {}, {}, {} // excluded for brevity ], "message": "Success", "pagination_info": { "order_by": "opportunity_id", "page_offset": 1, "page_size": 3, "sort_direction": "ascending", "total_pages": 1010, "total_records": 3030 }, "status_code": 200, "warnings": [] } ``` </td> </tr> <tr><td>POST /v0.1/opportunities/search (401 or 422)</td> <td> ```json { "data": { "data": "string", "errors": [ { "field": "string", "message": "string", "type": "string" } ], "message": "string", "status_code": 0 }, "message": "string", "pagination_info": { "order_by": "id", "page_offset": 1, "page_size": 25, "sort_direction": "ascending", "total_pages": 2, "total_records": 42 }, "status_code": 0, "warnings": [ { "field": "string", "message": "string", "type": "string" } ] } ``` </td> <td> ```json { "data": {}, "errors": [], "message": "Error", "status_code": 0 } ``` </td> <td> ```json { "data": {}, "errors": [], "message": "The server could not verify that you are authorized to access the URL requested", "status_code": 401 } ``` </td> </tr> </table> --------- Co-authored-by: nava-platform-bot <[email protected]>
acouch
pushed a commit
that referenced
this pull request
Sep 18, 2024
Fixes HHS#2066 Remove the `BASE_RESPONSE_SCHEMA` configuration. Adjust the healthcheck endpoint to be consistent in defining the schema / responses with the other endpoints. APIFlask allows you to set a `BASE_RESPONSE_SCHEMA` - the idea is that its the shared schema that every API endpoint will return, and they will only differ in the `data` object within. This sounds great on paper as it prevents you from needing to define most of the response schema for many endpoints, but in practice, its clunky to use. If we want to modify anything in the response schema outside of the `data` object, it will affect every single endpoint. This means when we add something like the pagination info to our search endpoint, a pagination object appears on the healthcheck response. As we intend to make these docs something the public will use, this is going to be confusing. There is also a "bug" (unsure if it is as it was an intended change a few months ago in APIFlask/apispec) that the error response objects end up nested within themselves in the examples. For example, currently the error response for the healthcheck endpoint (which can literally only return a 5xx error) has an example response of: ```json { "data": { "data": "string", "errors": [ { "field": "string", "message": "string", "type": "string" } ], "message": "string", "status_code": 0 }, "message": "string", "pagination_info": { "order_by": "id", "page_offset": 1, "page_size": 25, "sort_direction": "ascending", "total_pages": 2, "total_records": 42 }, "status_code": 0, "warnings": [ { "field": "string", "message": "string", "type": "string" } ] } ``` When in reality, the error response it actually returns looks like: ```json { "data": {}, "errors": [], "message": "Service Unavailable", "status_code": 503 } ``` This set of changes works around all of these confusing issues and just requires us to define specific response schemas for each endpoint with some small set of details. I've kept some base schema classes to derive from that we can use in most cases. Before & After Examples in OpenAPI <table> <tr><th>Endpoint</th><th>Before</th><th>After</th><th>Actual Response (before change)</th></tr> <tr><td>GET /health (200) </td> <td> ```json { "data": { "message": "string" }, "message": "string", "pagination_info": { "order_by": "id", "page_offset": 1, "page_size": 25, "sort_direction": "ascending", "total_pages": 2, "total_records": 42 }, "status_code": 0, "warnings": [ { "field": "string", "message": "string", "type": "string" } ] } ``` </td> <td> ```json { "data": null, "message": "Success", "status_code": 200 } ``` </td> <td> ```json { "data": {}, "message": "Service healthy", "pagination_info": null, "status_code": 200, "warnings": [] } ``` </td> </tr> <tr><td>GET /health (503)</td> <td> ```json { "data": { "data": "string", "errors": [ { "field": "string", "message": "string", "type": "string" } ], "message": "string", "status_code": 0 }, "message": "string", "pagination_info": { "order_by": "id", "page_offset": 1, "page_size": 25, "sort_direction": "ascending", "total_pages": 2, "total_records": 42 }, "status_code": 0, "warnings": [ { "field": "string", "message": "string", "type": "string" } ] } ``` </td> <td> ```json { "data": {}, "errors": [], "message": "Error", "status_code": 0 } ``` </td> <td> ```json { "data": {}, "message": "Service unavailable", "pagination_info": null, "status_code": 200, "warnings": [] } ``` </td> </tr> <tr><td>POST /v0.1/opportunities/search (200)</td> <td> ```json { "data": [ {.. Excluding for brevity } ], "message": "string", "pagination_info": { "order_by": "id", "page_offset": 1, "page_size": 25, "sort_direction": "ascending", "total_pages": 2, "total_records": 42 }, "status_code": 0, "warnings": [ { "field": "string", "message": "string", "type": "string" } ] } ``` </td> <td> ```json { "data": [ {.. Excluding for brevity } ], "message": "Success", "pagination_info": { "order_by": "id", "page_offset": 1, "page_size": 25, "sort_direction": "ascending", "total_pages": 2, "total_records": 42 }, "status_code": 200 } ``` </td> <td> ```json { "data": [ {}, {}, {} // excluded for brevity ], "message": "Success", "pagination_info": { "order_by": "opportunity_id", "page_offset": 1, "page_size": 3, "sort_direction": "ascending", "total_pages": 1010, "total_records": 3030 }, "status_code": 200, "warnings": [] } ``` </td> </tr> <tr><td>POST /v0.1/opportunities/search (401 or 422)</td> <td> ```json { "data": { "data": "string", "errors": [ { "field": "string", "message": "string", "type": "string" } ], "message": "string", "status_code": 0 }, "message": "string", "pagination_info": { "order_by": "id", "page_offset": 1, "page_size": 25, "sort_direction": "ascending", "total_pages": 2, "total_records": 42 }, "status_code": 0, "warnings": [ { "field": "string", "message": "string", "type": "string" } ] } ``` </td> <td> ```json { "data": {}, "errors": [], "message": "Error", "status_code": 0 } ``` </td> <td> ```json { "data": {}, "errors": [], "message": "The server could not verify that you are authorized to access the URL requested", "status_code": 401 } ``` </td> </tr> </table> --------- Co-authored-by: nava-platform-bot <[email protected]>
acouch
pushed a commit
to HHS/simpler-grants-gov
that referenced
this pull request
Sep 18, 2024
Fixes #2066 Remove the `BASE_RESPONSE_SCHEMA` configuration. Adjust the healthcheck endpoint to be consistent in defining the schema / responses with the other endpoints. APIFlask allows you to set a `BASE_RESPONSE_SCHEMA` - the idea is that its the shared schema that every API endpoint will return, and they will only differ in the `data` object within. This sounds great on paper as it prevents you from needing to define most of the response schema for many endpoints, but in practice, its clunky to use. If we want to modify anything in the response schema outside of the `data` object, it will affect every single endpoint. This means when we add something like the pagination info to our search endpoint, a pagination object appears on the healthcheck response. As we intend to make these docs something the public will use, this is going to be confusing. There is also a "bug" (unsure if it is as it was an intended change a few months ago in APIFlask/apispec) that the error response objects end up nested within themselves in the examples. For example, currently the error response for the healthcheck endpoint (which can literally only return a 5xx error) has an example response of: ```json { "data": { "data": "string", "errors": [ { "field": "string", "message": "string", "type": "string" } ], "message": "string", "status_code": 0 }, "message": "string", "pagination_info": { "order_by": "id", "page_offset": 1, "page_size": 25, "sort_direction": "ascending", "total_pages": 2, "total_records": 42 }, "status_code": 0, "warnings": [ { "field": "string", "message": "string", "type": "string" } ] } ``` When in reality, the error response it actually returns looks like: ```json { "data": {}, "errors": [], "message": "Service Unavailable", "status_code": 503 } ``` This set of changes works around all of these confusing issues and just requires us to define specific response schemas for each endpoint with some small set of details. I've kept some base schema classes to derive from that we can use in most cases. Before & After Examples in OpenAPI <table> <tr><th>Endpoint</th><th>Before</th><th>After</th><th>Actual Response (before change)</th></tr> <tr><td>GET /health (200) </td> <td> ```json { "data": { "message": "string" }, "message": "string", "pagination_info": { "order_by": "id", "page_offset": 1, "page_size": 25, "sort_direction": "ascending", "total_pages": 2, "total_records": 42 }, "status_code": 0, "warnings": [ { "field": "string", "message": "string", "type": "string" } ] } ``` </td> <td> ```json { "data": null, "message": "Success", "status_code": 200 } ``` </td> <td> ```json { "data": {}, "message": "Service healthy", "pagination_info": null, "status_code": 200, "warnings": [] } ``` </td> </tr> <tr><td>GET /health (503)</td> <td> ```json { "data": { "data": "string", "errors": [ { "field": "string", "message": "string", "type": "string" } ], "message": "string", "status_code": 0 }, "message": "string", "pagination_info": { "order_by": "id", "page_offset": 1, "page_size": 25, "sort_direction": "ascending", "total_pages": 2, "total_records": 42 }, "status_code": 0, "warnings": [ { "field": "string", "message": "string", "type": "string" } ] } ``` </td> <td> ```json { "data": {}, "errors": [], "message": "Error", "status_code": 0 } ``` </td> <td> ```json { "data": {}, "message": "Service unavailable", "pagination_info": null, "status_code": 200, "warnings": [] } ``` </td> </tr> <tr><td>POST /v0.1/opportunities/search (200)</td> <td> ```json { "data": [ {.. Excluding for brevity } ], "message": "string", "pagination_info": { "order_by": "id", "page_offset": 1, "page_size": 25, "sort_direction": "ascending", "total_pages": 2, "total_records": 42 }, "status_code": 0, "warnings": [ { "field": "string", "message": "string", "type": "string" } ] } ``` </td> <td> ```json { "data": [ {.. Excluding for brevity } ], "message": "Success", "pagination_info": { "order_by": "id", "page_offset": 1, "page_size": 25, "sort_direction": "ascending", "total_pages": 2, "total_records": 42 }, "status_code": 200 } ``` </td> <td> ```json { "data": [ {}, {}, {} // excluded for brevity ], "message": "Success", "pagination_info": { "order_by": "opportunity_id", "page_offset": 1, "page_size": 3, "sort_direction": "ascending", "total_pages": 1010, "total_records": 3030 }, "status_code": 200, "warnings": [] } ``` </td> </tr> <tr><td>POST /v0.1/opportunities/search (401 or 422)</td> <td> ```json { "data": { "data": "string", "errors": [ { "field": "string", "message": "string", "type": "string" } ], "message": "string", "status_code": 0 }, "message": "string", "pagination_info": { "order_by": "id", "page_offset": 1, "page_size": 25, "sort_direction": "ascending", "total_pages": 2, "total_records": 42 }, "status_code": 0, "warnings": [ { "field": "string", "message": "string", "type": "string" } ] } ``` </td> <td> ```json { "data": {}, "errors": [], "message": "Error", "status_code": 0 } ``` </td> <td> ```json { "data": {}, "errors": [], "message": "The server could not verify that you are authorized to access the URL requested", "status_code": 401 } ``` </td> </tr> </table> --------- Co-authored-by: nava-platform-bot <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #69
Time to review: 5 mins
Changes proposed
Remove the
BASE_RESPONSE_SCHEMAconfiguration.Adjust the healthcheck endpoint to be consistent in defining the schema / responses with the other endpoints.
Context for reviewers
APIFlask allows you to set a
BASE_RESPONSE_SCHEMA- the idea is that its the shared schema that every API endpoint will return, and they will only differ in thedataobject within. This sounds great on paper as it prevents you from needing to define most of the response schema for many endpoints, but in practice, its clunky to use. If we want to modify anything in the response schema outside of thedataobject, it will affect every single endpoint. This means when we add something like the pagination info to our search endpoint, a pagination object appears on the healthcheck response. As we intend to make these docs something the public will use, this is going to be confusing.There is also a "bug" (unsure if it is as it was an intended change a few months ago in APIFlask/apispec) that the error response objects end up nested within themselves in the examples.
For example, currently the error response for the healthcheck endpoint (which can literally only return a 5xx error) has an example response of:
{ "data": { "data": "string", "errors": [ { "field": "string", "message": "string", "type": "string" } ], "message": "string", "status_code": 0 }, "message": "string", "pagination_info": { "order_by": "id", "page_offset": 1, "page_size": 25, "sort_direction": "ascending", "total_pages": 2, "total_records": 42 }, "status_code": 0, "warnings": [ { "field": "string", "message": "string", "type": "string" } ] }When in reality, the error response it actually returns looks like:
{ "data": {}, "errors": [], "message": "Service Unavailable", "status_code": 503 }This set of changes works around all of these confusing issues and just requires us to define specific response schemas for each endpoint with some small set of details. I've kept some base schema classes to derive from that we can use in most cases.
Additional information
Before & After Examples in OpenAPI
{ "data": { "message": "string" }, "message": "string", "pagination_info": { "order_by": "id", "page_offset": 1, "page_size": 25, "sort_direction": "ascending", "total_pages": 2, "total_records": 42 }, "status_code": 0, "warnings": [ { "field": "string", "message": "string", "type": "string" } ] }{ "data": null, "message": "Success", "status_code": 200 }{ "data": {}, "message": "Service healthy", "pagination_info": null, "status_code": 200, "warnings": [] }{ "data": { "data": "string", "errors": [ { "field": "string", "message": "string", "type": "string" } ], "message": "string", "status_code": 0 }, "message": "string", "pagination_info": { "order_by": "id", "page_offset": 1, "page_size": 25, "sort_direction": "ascending", "total_pages": 2, "total_records": 42 }, "status_code": 0, "warnings": [ { "field": "string", "message": "string", "type": "string" } ] }{ "data": {}, "errors": [], "message": "Error", "status_code": 0 }{ "data": {}, "message": "Service unavailable", "pagination_info": null, "status_code": 200, "warnings": [] }{ "data": [ {.. Excluding for brevity } ], "message": "string", "pagination_info": { "order_by": "id", "page_offset": 1, "page_size": 25, "sort_direction": "ascending", "total_pages": 2, "total_records": 42 }, "status_code": 0, "warnings": [ { "field": "string", "message": "string", "type": "string" } ] }{ "data": [ {.. Excluding for brevity } ], "message": "Success", "pagination_info": { "order_by": "id", "page_offset": 1, "page_size": 25, "sort_direction": "ascending", "total_pages": 2, "total_records": 42 }, "status_code": 200 }{ "data": [ {}, {}, {} // excluded for brevity ], "message": "Success", "pagination_info": { "order_by": "opportunity_id", "page_offset": 1, "page_size": 3, "sort_direction": "ascending", "total_pages": 1010, "total_records": 3030 }, "status_code": 200, "warnings": [] }{ "data": { "data": "string", "errors": [ { "field": "string", "message": "string", "type": "string" } ], "message": "string", "status_code": 0 }, "message": "string", "pagination_info": { "order_by": "id", "page_offset": 1, "page_size": 25, "sort_direction": "ascending", "total_pages": 2, "total_records": 42 }, "status_code": 0, "warnings": [ { "field": "string", "message": "string", "type": "string" } ] }{ "data": {}, "errors": [], "message": "Error", "status_code": 0 }{ "data": {}, "errors": [], "message": "The server could not verify that you are authorized to access the URL requested", "status_code": 401 }