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

Catch API exception and serve helpful error response #460

Merged
merged 5 commits into from
May 5, 2023

Conversation

mrchrisadams
Copy link
Member

@mrchrisadams mrchrisadams commented May 5, 2023

This PR is intended to solve issue #459, where we were seeing a lot of invalid API requests causing 500 errors in Sentry.

This was triggered when people use the old directory to look up what information we have about a given provider, and it looks like someone starting hitting that API pretty hard - triggering hundreds of exceptions in the last few days, from the same client.

With this merged in, we instead serve a 406 response, making it clear to consumers how this endpoint is intended to be used.

#459

This also tidies up the code with Ruff, removing unused variables and so on

Also remove public evidence API tweak - better to add that separately
@mrchrisadams mrchrisadams requested a review from tortila May 5, 2023 08:30
Ruff thought it wasn't used, but we use legacy_views
as a module for importing all the old legacy views from
Copy link
Contributor

@tortila tortila left a comment

Choose a reason for hiding this comment

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

Thanks for delivering this fix @mrchrisadams! Logic-wise it looks good. However, I would like to suggest a different response code. 406 Not Acceptable is a good fit for when the response of a requested format cannot be produced. In this specific case, I think either 400 Bad Request or 422 Unprocessable Entity would be more precise.

I understand that this fix intends to keep Sentry usable for us while the "abusing" of the API is taking place. But I'm wondering: is there any additional action we should take, e.g. introducing rate-limiting?

@mrchrisadams
Copy link
Member Author

Thanks Oliwia!

You're right - I wasn't sure about 406 either, so thanks for the pointer. I think I'll go with 400.

On the rate limiting front - I think we have some rate limiting in front of django with nginx. At least I wrote it up when we set it up a while back in 2019

https://rtl.chrisadams.me.uk/2019/12/how-to-rate-limit-punks-with-nginx/

We don't use any special rate limiting in Django or DRF right now, and it might be worth revisiting again.

Although to be fair, if our reverse proxy server is solving the rate limiting issue before it reaches django, it should allow us to keep the codebase smaller.

I've created an issue below for any further discussion

#462

But for now, I'll add the other commit now to update the status code so we can merge in.

It's a small change - you're happy for me to do so without pinging you again?

@mrchrisadams mrchrisadams merged commit e577147 into master May 5, 2023
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.

2 participants