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.

Sign up for GitHub

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

Fix error handler type hint, use flask defined route response types #39

Closed
wants to merge 1 commit into from
Closed

Fix error handler type hint, use flask defined route response types #39

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from flask import Flask, Response, request
from flask.typing import ResponseReturnValue
from flask_cors import CORS
from flask_cognito import CognitoAuth
from flasgger import Swagger
Expand All @@ -10,7 +11,6 @@
from config.env import COGNITO_REGION, COGNITO_USERPOOL_ID, COGNITO_APP_CLIENT_ID
from managers import MeManager
from http import HTTPStatus
from utilities.types import FlaskResponseType
import traceback

app = Flask(__name__)
Expand Down Expand Up @@ -72,8 +72,8 @@ def after_request(response: Response) -> Response:
return response


# @app.errorhandler(Exception) # type: ignore[type-var]
def exceptions(e: Exception) -> FlaskResponseType:
@app.errorhandler(Exception)
def exceptions(e: Exception) -> ResponseReturnValue:
tb = traceback.format_exc()
timestamp = strftime('[%Y-%b-%d %H:%M]')
logging.error('%s %s %s %s %s 5xx INTERNAL SERVER ERROR\n%s', timestamp, request.remote_addr, request.method, request.scheme, request.full_path, tb)
Expand Down
4 changes: 2 additions & 2 deletions controllers/contract.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
from flask.typing import ResponseReturnValue
from flask_cognito import cognito_auth_required, current_cognito_jwt, current_user
from managers import ContractManager
from flasgger import swag_from
from .base_controller import BaseController
from utilities.types import FlaskResponseType
from utilities import FlaskResponses, NoApproverException
from .swagger.contract.post import contract_post_schema
from utilities.types import JSONDict
Expand All @@ -13,7 +13,7 @@ class ContractController(BaseController):

@cognito_auth_required
@swag_from(contract_post_schema)
def post(self) -> FlaskResponseType:
def post(self) -> ResponseReturnValue:
data = self.get_request_data(contract_post_schema, "ContractData")

user_db: Optional[JSONDict] = current_cognito_jwt['database']
Expand Down
4 changes: 2 additions & 2 deletions controllers/health.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
from flask.typing import ResponseReturnValue
from flasgger import swag_from
from .base_controller import BaseController
from utilities.types import FlaskResponseType
from utilities import FlaskResponses


class HealthController(BaseController):

@swag_from("swagger/health/get.yaml")
def get(self) -> FlaskResponseType:
def get(self) -> ResponseReturnValue:
return FlaskResponses().success("ok")
8 changes: 4 additions & 4 deletions controllers/me.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from flask.typing import ResponseReturnValue
from flask_cognito import cognito_auth_required, current_user, current_cognito_jwt
from .base_controller import BaseController
from utilities.types import FlaskResponseType
from utilities.flask_responses import FlaskResponses
from managers import MeManager
from flasgger import swag_from
Expand All @@ -12,17 +12,17 @@ class MeController(BaseController):

@cognito_auth_required
@swag_from("swagger/me/get.yaml")
def get(self) -> FlaskResponseType:
def get(self) -> ResponseReturnValue:
result = MeManager().get_user(current_user, current_cognito_jwt)
return FlaskResponses().success(result)

@cognito_auth_required
def patch(self) -> FlaskResponseType:
def patch(self) -> ResponseReturnValue:
return FlaskResponses().not_implemented_yet() # TODO

@cognito_auth_required
@swag_from(ME_POST_SCHEMA)
def post(self) -> FlaskResponseType:
def post(self) -> ResponseReturnValue:
data = self.get_request_data(self.ME_POST_SCHEMA, "NewUserData")
ret = MeManager().create_user(current_cognito_jwt['sub'], data['vendorType'])
if not ret:
Expand Down
20 changes: 11 additions & 9 deletions utilities/flask_responses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,27 @@
from http import HTTPStatus
from utilities.types import FlaskResponseType, JSONType
from flask.typing import ResponseReturnValue
from utilities.types import JSONType
import json


class FlaskResponses():

@classmethod
def not_implemented_yet(cls) -> FlaskResponseType:
def not_implemented_yet(cls) -> ResponseReturnValue:
return {'error': "not implemented yet"}, HTTPStatus.NOT_IMPLEMENTED

@classmethod
def success(cls, data: JSONType) -> FlaskResponseType:
return data, HTTPStatus.OK
def success(cls, data: JSONType) -> ResponseReturnValue:
return json.dumps(data), HTTPStatus.OK
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does change output when using curl. I'm not sure if this will cause issues when we implement the frontend.

Steps to reproduce:

  1. Generate id token. See Write a script to generate a sample id_token #31 for details
  2. Set the ID token to a variable token in shell
  3. Run the backend and run this command
curl -X GET -H "Authorization: Bearer $token" -H 'Content-Type: application/json' localhost:3001/me

Output before MR:

{
    "name": "test",
    "email": "[email protected]",
    "database": null
}

Output after MR:

"{\"name\": \"test\", \"email\": \"[email protected]\", \"database\": null}"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nevermind. I did a test and this isn't a problem for browsers.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did this change the headers in the response? i'm guessing if the headers don't say its a json curl might default to displaying as a string

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it doesn't change the headers. My guess is that json.dumps() changes the formatting to include \

After Changes:

Internal server errorgcarvellas@razer-blade-15:~$ curl -v -X GET localhost:3001/health
Note: Unnecessary use of -X or --request, GET is already inferred.
* processing: localhost:3001/health
*   Trying [::1]:3001...
* Connected to localhost (::1) port 3001
> GET /health HTTP/1.1
> Host: localhost:3001
> User-Agent: curl/8.2.1
> Accept: */*
> 
< HTTP/1.1 200 OK
< Server: gunicorn
< Date: Thu, 14 Sep 2023 18:57:58 GMT
< Connection: keep-alive
< Content-Type: application/json
< Content-Length: 9
< Access-Control-Allow-Origin: *
< 
"\"ok\""

Before Changes:

gcarvellas@razer-blade-15:~$ curl -v -X GET localhost:3001/health
Note: Unnecessary use of -X or --request, GET is already inferred.
* processing: localhost:3001/health
*   Trying [::1]:3001...
* Connected to localhost (::1) port 3001
> GET /health HTTP/1.1
> Host: localhost:3001
> User-Agent: curl/8.2.1
> Accept: */*
> 
< HTTP/1.1 200 OK
< Server: gunicorn
< Date: Thu, 14 Sep 2023 18:58:40 GMT
< Connection: keep-alive
< Content-Type: application/json
< Content-Length: 5
< Access-Control-Allow-Origin: *
< 
"ok"


@classmethod
def created_resource(cls, data: JSONType) -> FlaskResponseType:
return data, HTTPStatus.CREATED
def created_resource(cls, data: JSONType) -> ResponseReturnValue:
return json.dumps(data), HTTPStatus.CREATED

@classmethod
def conflict(cls, data: JSONType) -> FlaskResponseType:
return data, HTTPStatus.CONFLICT
def conflict(cls, data: JSONType) -> ResponseReturnValue:
return json.dumps(data), HTTPStatus.CONFLICT

@classmethod
def bad_request(cls, msg: str) -> FlaskResponseType:
def bad_request(cls, msg: str) -> ResponseReturnValue:
return {'error': msg}, HTTPStatus.BAD_REQUEST
4 changes: 1 addition & 3 deletions utilities/types/flask_response_types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
from typing import Dict, Any, Union, List, Tuple
from http import HTTPStatus
from typing import Dict, Any, Union, List


# https://github.com/python/typing/issues/182
JSONType = Union[str, int, float, bool, None, Dict[str, Any], List[Any]]

JSONDict = Dict[str, Any]
FlaskResponseType = Tuple[JSONType, HTTPStatus]