mu-semtech / mu-python-template

Template for running Python/Flask microservices
MIT License
4 stars 8 forks source link

`validate_json_api_content_type` helper has confusing name vs expected use #18

Open sergiofenoll opened 10 months ago

sergiofenoll commented 10 months ago

Context

The validate_json_api_content_type helper checks whether the Content-Type header of a passed in request is JSON:API and otherwise returns an error response object.

Because it just returns an error object that the user needs to handle manually (as opposed to the automatic halting of the request handling as implemented in the ruby-template) its use is a bit confusing.

from helpers import validate_json_api_content_type
from flask import request

@app.route("/")
def index():
    validate_json_api_content_type(request)
    return "Hello from the mu-python-template!"

Given the above code and based on the function name and its docstring one might assume that the route handler would automatically return an error if the request didn't contain JSON:API, but in fact it does nothing. The proper code would look something like this:

from helpers import validate_json_api_content_type
from flask import request

@app.route("/")
def index():
    maybe_error = validate_json_api_content_type(request)
    if maybe_error:
        return maybe_error
    return "Hello from the mu-python-template!"

Possible solutions

Make validate_json_api_content_type raise an exception instead of returning an error object. Users of the template can then either manually catch and handle the raised exception as they deem fit, or add an error handler to their service that automatically handles all such errors:

# In template: helpers.py
class InvalidJSONAPIContentTypeException(Exception):
    """Request does not contain valid JSON:API"""

def validate_json_api_content_type(request):
    """Validate whether the request contains the JSONAPI content-type header (application/vnd.api+json). Raises an exception of type InvalidJSONAPIContentTypeException otherwise"""
    if "application/vnd.api+json" not in request.content_type:
        raise InvalidJSONAPIContentTypeException("Content-Type must be application/vnd.api+json instead of " + request.content_type)

# In service: web.py
from helpers import validate_json_api_content_type, InvalidJSONAPIContentTypeException
from flask import request

@app.route("/")
def index():
    try:
        validate_json_api_content_type(request)
    except InvalidJSONAPIContentTypeException as e:
        return error(str(e), 400)
    return "Hello from the mu-python-template!"

# Or a service-level error handler, which would make the above try-except unnecessary

@app.errorhandler(InvalidJSONAPIContentTypeException)
def handle_invalid_json_api_content_type(e):
    return error(str(e), 400)

Alternatively, there might be a way to make the validate_json_api_content_type abort the request using some Flask function, emulating the behaviour of the ruby-template.

The helper can also be renamed and its documentation can be clarified to make its usage more obvious.

EDIT: the reasoning for opening an issue that I've found one place in Kaleidos code where someone wrote validate_json_api_content_type(request) and it effectively did nothing, making me assume they got confused about what the function actually did.