[Issue #69] Remove the BASE_RESPONSE_SCHEMA (#70)

## Summary Fixes #69 ### Time to review: __5 mins__ ## Changes proposed Remove the `BASE_RESPONSE_SCHEMA` configuration. 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 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. ## Additional information 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]>
navapbc · Jun 7, 2024 · 2db4d35 · 2db4d35
1 parent f12d0dc
commit 2db4d35
Show file tree

Hide file tree

Showing 13 changed files with 285 additions and 511 deletions.
diff --git a/api/openapi.generated.yml b/api/openapi.generated.yml
diff --git a/api/src/api/healthcheck.py b/api/src/api/healthcheck.py
@@ -1,33 +1,39 @@
 import logging
-from typing import Tuple
 
 from apiflask import APIBlueprint
-from flask import current_app
 from sqlalchemy import text
 from werkzeug.exceptions import ServiceUnavailable
 
+import src.adapters.db as db
 import src.adapters.db.flask_db as flask_db
 from src.api import response
-from src.api.schemas.extension import Schema, fields
+from src.api.route_utils import raise_flask_error
+from src.api.schemas.extension import fields
+from src.api.schemas.response_schema import AbstractResponseSchema
 
 logger = logging.getLogger(__name__)
 
 
-class HealthcheckSchema(Schema):
-    message = fields.String()
+class HealthcheckResponseSchema(AbstractResponseSchema):
+    # We don't have any data to return with the healthcheck endpoint
+    data = fields.MixinField(metadata={"example": None})
 
 
 healthcheck_blueprint = APIBlueprint("healthcheck", __name__, tag="Health")
 
 
 @healthcheck_blueprint.get("/health")
-@healthcheck_blueprint.output(HealthcheckSchema)
+@healthcheck_blueprint.output(HealthcheckResponseSchema)
 @healthcheck_blueprint.doc(responses=[200, ServiceUnavailable.code])
-def health() -> Tuple[response.ApiResponse, int]:
+@flask_db.with_db_session()
+def health(db_session: db.Session) -> response.ApiResponse:
     try:
-        with flask_db.get_db(current_app).get_connection() as conn:
-            assert conn.scalar(text("SELECT 1 AS healthy")) == 1
-        return response.ApiResponse(message="Service healthy"), 200
+        with db_session.begin():
+            if db_session.scalar(text("SELECT 1 AS healthy")) != 1:
+                raise Exception("Connection to Postgres DB failure")
+
     except Exception:
         logger.exception("Connection to DB failure")
-        return response.ApiResponse(message="Service unavailable"), ServiceUnavailable.code
+        raise_flask_error(ServiceUnavailable.code, message="Service Unavailable")
+
+    return response.ApiResponse(message="Service healthy")
diff --git a/api/src/api/opportunities_v0/opportunity_routes.py b/api/src/api/opportunities_v0/opportunity_routes.py
@@ -34,7 +34,7 @@
     arg_name="feature_flag_config",
 )
 # many=True allows us to return a list of opportunity objects
-@opportunity_blueprint.output(opportunity_schemas.OpportunityV0Schema(many=True))
+@opportunity_blueprint.output(opportunity_schemas.OpportunitySearchResponseV0Schema)
 @opportunity_blueprint.auth_required(api_key_auth)
 @opportunity_blueprint.doc(description=SHARED_ALPHA_DESCRIPTION)
 @flask_db.with_db_session()
@@ -65,7 +65,7 @@ def opportunity_search(
 
 
 @opportunity_blueprint.get("/opportunities/<int:opportunity_id>")
-@opportunity_blueprint.output(opportunity_schemas.OpportunityV0Schema)
+@opportunity_blueprint.output(opportunity_schemas.OpportunityGetResponseV0Schema)
 @opportunity_blueprint.auth_required(api_key_auth)
 @opportunity_blueprint.doc(description=SHARED_ALPHA_DESCRIPTION)
 @flask_db.with_db_session()

diff --git a/api/src/api/opportunities_v0/opportunity_schemas.py b/api/src/api/opportunities_v0/opportunity_schemas.py
@@ -5,6 +5,7 @@
 from src.api.feature_flags.feature_flag import FeatureFlag
 from src.api.feature_flags.feature_flag_config import FeatureFlagConfig, get_feature_flag_config
 from src.api.schemas.extension import Schema, fields
+from src.api.schemas.response_schema import AbstractResponseSchema, PaginationMixinSchema
 from src.constants.lookup_constants import OpportunityCategoryLegacy
 from src.pagination.pagination_schema import PaginationSchema, generate_sorting_schema
 
@@ -113,3 +114,11 @@ def post_load(self, data: dict, **kwargs: Any) -> FeatureFlagConfig:
             feature_flag_config.enable_opportunity_log_msg = enable_opportunity_log_msg
 
         return feature_flag_config
+
+
+class OpportunityGetResponseV0Schema(AbstractResponseSchema):
+    data = fields.Nested(OpportunityV0Schema())
+
+
+class OpportunitySearchResponseV0Schema(AbstractResponseSchema, PaginationMixinSchema):
+    data = fields.Nested(OpportunityV0Schema(many=True))
diff --git a/api/src/api/opportunities_v0_1/opportunity_routes.py b/api/src/api/opportunities_v0_1/opportunity_routes.py
@@ -67,7 +67,7 @@
     examples=examples,
 )
 # many=True allows us to return a list of opportunity objects
-@opportunity_blueprint.output(opportunity_schemas.OpportunityV01Schema(many=True))
+@opportunity_blueprint.output(opportunity_schemas.OpportunitySearchResponseV01Schema)
 @opportunity_blueprint.auth_required(api_key_auth)
 @opportunity_blueprint.doc(description=SHARED_ALPHA_DESCRIPTION)
 @flask_db.with_db_session()
@@ -92,7 +92,7 @@ def opportunity_search(db_session: db.Session, search_params: dict) -> response.
 
 
 @opportunity_blueprint.get("/opportunities/<int:opportunity_id>")
-@opportunity_blueprint.output(opportunity_schemas.OpportunityV01Schema)
+@opportunity_blueprint.output(opportunity_schemas.OpportunityGetResponseV01Schema)
 @opportunity_blueprint.auth_required(api_key_auth)
 @opportunity_blueprint.doc(description=SHARED_ALPHA_DESCRIPTION)
 @flask_db.with_db_session()

diff --git a/api/src/api/opportunities_v0_1/opportunity_schemas.py b/api/src/api/opportunities_v0_1/opportunity_schemas.py
@@ -1,4 +1,5 @@
 from src.api.schemas.extension import Schema, fields, validators
+from src.api.schemas.response_schema import AbstractResponseSchema, PaginationMixinSchema
 from src.api.schemas.search_schema import StrSearchSchemaBuilder
 from src.constants.lookup_constants import (
     ApplicantType,
@@ -296,3 +297,11 @@ class OpportunitySearchRequestV01Schema(Schema):
         ),
         required=True,
     )
+
+
+class OpportunityGetResponseV01Schema(AbstractResponseSchema):
+    data = fields.Nested(OpportunityV01Schema())
+
+
+class OpportunitySearchResponseV01Schema(AbstractResponseSchema, PaginationMixinSchema):
+    data = fields.Nested(OpportunityV01Schema(many=True))
diff --git a/api/src/api/opportunities_v1/opportunity_routes.py b/api/src/api/opportunities_v1/opportunity_routes.py
@@ -30,7 +30,7 @@
     opportunity_schemas.OpportunitySearchRequestV1Schema, arg_name="search_params"
 )
 # many=True allows us to return a list of opportunity objects
-@opportunity_blueprint.output(opportunity_schemas.OpportunityV1Schema(many=True))
+@opportunity_blueprint.output(opportunity_schemas.OpportunitySearchResponseV1Schema)
 @opportunity_blueprint.auth_required(api_key_auth)
 @opportunity_blueprint.doc(description=SHARED_ALPHA_DESCRIPTION)
 def opportunity_search(search_params: dict) -> response.ApiResponse:
@@ -53,7 +53,7 @@ def opportunity_search(search_params: dict) -> response.ApiResponse:
 
 
 @opportunity_blueprint.get("/opportunities/<int:opportunity_id>")
-@opportunity_blueprint.output(opportunity_schemas.OpportunityV1Schema)
+@opportunity_blueprint.output(opportunity_schemas.OpportunityGetResponseV1Schema)
 @opportunity_blueprint.auth_required(api_key_auth)
 @opportunity_blueprint.doc(description=SHARED_ALPHA_DESCRIPTION)
 @flask_db.with_db_session()

diff --git a/api/src/api/opportunities_v1/opportunity_schemas.py b/api/src/api/opportunities_v1/opportunity_schemas.py
@@ -1,4 +1,5 @@
 from src.api.schemas.extension import Schema, fields, validators
+from src.api.schemas.response_schema import AbstractResponseSchema, PaginationMixinSchema
 from src.api.schemas.search_schema import StrSearchSchemaBuilder
 from src.constants.lookup_constants import (
     ApplicantType,
@@ -296,3 +297,11 @@ class OpportunitySearchRequestV1Schema(Schema):
         ),
         required=True,
     )
+
+
+class OpportunityGetResponseV1Schema(AbstractResponseSchema):
+    data = fields.Nested(OpportunityV1Schema())
+
+
+class OpportunitySearchResponseV1Schema(AbstractResponseSchema, PaginationMixinSchema):
+    data = fields.Nested(OpportunityV1Schema(many=True))
diff --git a/api/src/api/schemas/response_schema.py b/api/src/api/schemas/response_schema.py
@@ -3,25 +3,52 @@
 
 
 class ValidationIssueSchema(Schema):
-    type = fields.String(metadata={"description": "The type of error"})
-    message = fields.String(metadata={"description": "The message to return"})
-    field = fields.String(metadata={"description": "The field that failed"})
+    type = fields.String(metadata={"description": "The type of error", "example": "invalid"})
+    message = fields.String(
+        metadata={"description": "The message to return", "example": "Not a valid string."}
+    )
+    field = fields.String(
+        metadata={"description": "The field that failed", "example": "summary.summary_description"}
+    )
 
 
-class BaseResponseSchema(Schema):
-    message = fields.String(metadata={"description": "The message to return"})
+class AbstractResponseSchema(Schema):
+    message = fields.String(metadata={"description": "The message to return", "example": "Success"})
     data = fields.MixinField(metadata={"description": "The REST resource object"}, dump_default={})
-    status_code = fields.Integer(metadata={"description": "The HTTP status code"}, dump_default=200)
+    status_code = fields.Integer(
+        metadata={"description": "The HTTP status code", "example": 200}, dump_default=200
+    )
 
 
-class ErrorResponseSchema(BaseResponseSchema):
-    errors = fields.List(fields.Nested(ValidationIssueSchema()), dump_default=[])
+class WarningMixinSchema(Schema):
+    warnings = fields.List(
+        fields.Nested(ValidationIssueSchema()),
+        metadata={
+            "description": "A list of warnings - indicating something you may want to be aware of, but did not prevent handling of the request"
+        },
+        dump_default=[],
+    )
 
 
-class ResponseSchema(BaseResponseSchema):
+class PaginationMixinSchema(Schema):
     pagination_info = fields.Nested(
         PaginationInfoSchema(),
         metadata={"description": "The pagination information for paginated endpoints"},
     )
 
-    warnings = fields.List(fields.Nested(ValidationIssueSchema()), dump_default=[])
+
+class ErrorResponseSchema(Schema):
+    data = fields.MixinField(
+        metadata={
+            "description": "Additional data that might be useful in resolving an error (see specific endpoints for details, this is used infrequently)",
+            "example": {},
+        },
+        dump_default={},
+    )
+    message = fields.String(
+        metadata={"description": "General description of the error", "example": "Error"}
+    )
+    status_code = fields.Integer(metadata={"description": "The HTTP status code of the error"})
+    errors = fields.List(
+        fields.Nested(ValidationIssueSchema()), metadata={"example": []}, dump_default=[]
+    )
diff --git a/api/src/app.py b/api/src/app.py
@@ -67,7 +67,7 @@ def configure_app(app: APIFlask) -> None:
     # Modify the response schema to instead use the format of our ApiResponse class
     # which adds additional details to the object.
     # https://apiflask.com/schema/#base-response-schema-customization
-    app.config["BASE_RESPONSE_SCHEMA"] = response_schema.ResponseSchema
+    # app.config["BASE_RESPONSE_SCHEMA"] = response_schema.ResponseSchema
     app.config["HTTP_ERROR_SCHEMA"] = response_schema.ErrorResponseSchema
     app.config["VALIDATION_ERROR_SCHEMA"] = response_schema.ErrorResponseSchema
     app.config["SWAGGER_UI_CSS"] = "/static/swagger-ui.min.css"

diff --git a/api/tests/src/api/test_healthcheck.py b/api/tests/src/api/test_healthcheck.py
@@ -4,15 +4,17 @@
 def test_get_healthcheck_200(client):
     response = client.get("/health")
     assert response.status_code == 200
+    assert response.get_json()["message"] == "Service healthy"
 
 
 def test_get_healthcheck_503_db_bad_state(client, monkeypatch):
     # Make fetching the DB session fail
     def err_method(*args):
         raise Exception("Fake Error")
 
-    # Mock db.DB.get_session method to fail
-    monkeypatch.setattr(db.DBClient, "get_connection", err_method)
+    # Mock db_session.Scalar to fail
+    monkeypatch.setattr(db.Session, "scalar", err_method)
 
     response = client.get("/health")
     assert response.status_code == 503
+    assert response.get_json()["message"] == "Service Unavailable"
diff --git a/api/tests/src/api/test_route_error_format.py b/api/tests/src/api/test_route_error_format.py
@@ -17,6 +17,7 @@
 from src.api.response import ApiResponse, ValidationErrorDetail
 from src.api.route_utils import raise_flask_error
 from src.api.schemas.extension import Schema, fields
+from src.api.schemas.response_schema import AbstractResponseSchema, WarningMixinSchema
 from src.auth.api_key_auth import api_key_auth
 from src.util.dict_util import flatten_dict
 from tests.src.schemas.schema_validation_utils import (
@@ -35,10 +36,14 @@ def header(api_auth_token):
     return {"X-Auth": api_auth_token}
 
 
-class OutputSchema(Schema):
+class OutputData(Schema):
     output_val = fields.String()
 
 
+class OutputSchema(AbstractResponseSchema, WarningMixinSchema):
+    data = fields.Nested(OutputData())
+
+
 test_blueprint = APIBlueprint("test", __name__, tag="test")
 
 
@@ -111,7 +116,6 @@ def override(self):
 
     assert resp.status_code == 500
     resp_json = resp.get_json()
-    assert resp_json["data"] == {}
     assert resp_json["errors"] == []
     assert resp_json["message"] == "Internal Server Error"
 

diff --git a/documentation/api/api-details.md b/documentation/api/api-details.md
@@ -42,6 +42,7 @@ We would define the Marshmallow schema in-python like so:
 ```py
 from enum import StrEnum
 from src.api.schemas.extension import Schema, fields, validators
+from src.api.schemas.response_schema import AbstractResponseSchema
 
 class Suffix(StrEnum):
     SENIOR = "SR"
@@ -65,6 +66,10 @@ class NameSchema(Schema):
 class ExampleSchema(Schema):
     name = fields.Nested(NameSchema())
     birth_date = fields.Date(metadata={"description": "Their birth date"})
+
+class ExampleResponseSchema(AbstractResponseSchema):
+    # Note that AbstractResponseSchema defines a message and status_code field as well
+    data = fields.Nested(ExampleSchema())
 ```
 
 Anything specified in the metadata field is passed to the OpenAPI file that backs the swagger endpoint. The values
@@ -77,9 +82,8 @@ but it's recommended you try to populate the following:
 You can specify validators that will be run when the request is being serialized by APIFlask
 
 Defining a response works the exact same way however field validation does not occur on response, only formatting.
-The response schema only dictates the data portion of the response, the rest of the response is defined in
-[ResponseSchema](../../api/src/api/schemas/response_schema.py) which is connected to APIFlask via the `BASE_RESPONSE_SCHEMA` config.
-
+To keep our response schema following a consistent pattern, we have a few base schema classes like [AbstractResponseSchema](../../api/src/api/schemas/response_schema.py)
+that you can derive from for shared values like the message.
 
 ### Schema tips