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.

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

chore(#82): Consistent Error Presentation #586

Conversation

danielburn5
Copy link

@danielburn5 danielburn5 commented Dec 10, 2023

Intro

👋 This is my first PR to this project! It's highly likely I have misunderstood something along the way. Happy to revisit and rework any of this!

What is this?

I started looking at some of the complexity issues raised by Code Climate for #82, specifically that of app.api.validations.requires_body.

I realised that the cognitive complexity can be reduced by removing the explicit error handling in this function and letting flask raise the appropriate error (see Request.get_json) if the JSON is malformed (and there is already an error handler for this). I think this code only needs to handle the error case which is specific to the app (ie. we must have content in the payload).

However, this uncovered inconsistencies in the formatting of API responses when it comes to errors. It caused a lot of test failures and a lot of head scratching on my part.

It looks as though the codebase already attempts to standardise responses, but as the errors payload is sometimes passed in, it has the potential to be inconsistent. Sometimes it is an array of dicts, sometimes just a single dict.

Long term, I think the creation of error responses could be standardised further but for now I updated each of the manual entries so that errors are always presented as arrays of dicts, like this:

{
    "errors": [
        {
            "message": ""
        }
    ]

Testing

Updated existing tests in line with changes.

Question

Might these changes to error responses have any effect on the front end?

@@ -97,15 +97,15 @@ def test_validate_resource(module_client, module_db, fake_auth_from_oc):
)
assert_missing_body(response)

# Data cannot be empty
# Data must be valid json
Copy link
Author

Choose a reason for hiding this comment

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

This test was not quite working as intended previously. Passing in data='' is not the same as not passing in data at all. This ultimately gets treated as a 400 bad request as there is a payload, but it's not valid json.

Comment on lines +13 to +17
# JSON body is {} or []
# This will throw a 400 if the JSON is malformed
# and drop into handlers.bad_request()
if not request.get_json():
return missing_json_error()
Copy link
Author

Choose a reason for hiding this comment

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

@aaron-junot Is there a way to check if this has had the desired effect on the cognitive complexity in Code Climate whilst I am on a fork?

@danielburn5
Copy link
Author

Closing. Didn't realise this repo was decomissioned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant