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

Fix: Error handlers respond with json if applicable #442

Merged
merged 1 commit into from Aug 20, 2020

Conversation

0xdade
Copy link
Member

@0xdade 0xdade commented Aug 19, 2020

This actually changes the default behavior of the error handlers to json and then if text/html is in the Accept header then we'll serve up html instead.

@0xdade 0xdade added the server affecting natlas-server label Aug 19, 2020
@codeclimate
Copy link

codeclimate bot commented Aug 19, 2020

Code Climate has analyzed commit 7a7cc70 and detected 0 issues on this pull request.

View more on Code Climate.

from app.errors import bp
from app import db
import elasticsearch
import sentry_sdk


def json_response(status: int, msg: str) -> str:
err = json.dumps({"status": status, "message": msg})
Copy link
Member

Choose a reason for hiding this comment

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

Hey sir, not sure if you noticed, but this is technically part of your API contract that you've added here, but what's the forcing function to ensure that you use a consistent format. Can we share an object, method, something to make sure this "API contract schema" is actually enforced and shared?

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically this is enforced and shared because all of these handlers return build_response, and build_response always calls it the same way. So the keys in the dict will always be the same so long as we exit through an error handler.

Copy link
Member

@ajacques ajacques Aug 20, 2020

Choose a reason for hiding this comment

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

They always call build_response?

{"status": 400, "message": errmsg, "retry": False}

Or here?
{"status": 400, "message": errmsg, "retry": False}

Here?
Here?
"status": status_code,

"status": status_code,

"status": status_code,

@0xdade 0xdade force-pushed the jsonify_errors branch 3 times, most recently from a31d5ff to 9498b5d Compare August 20, 2020 04:59
@0xdade 0xdade requested a review from ajacques August 20, 2020 05:16
return {"status": self.status_code, "message": self.message}

def get_json(self):
return json.dumps(self.get_dict(), sort_keys=True, indent=4)
Copy link
Member

Choose a reason for hiding this comment

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

Indentions in my serialized responses?

natlas-server/app/errors/handlers.py Outdated Show resolved Hide resolved
Return json by default unless the client explicitly says text/html is what they want.
@0xdade 0xdade merged commit b363369 into natlas:main Aug 20, 2020
@0xdade 0xdade mentioned this pull request Aug 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
server affecting natlas-server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants