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

Avoid AxiosError when requesting bad image links #3468

Closed
krysal opened this issue Dec 5, 2023 · 5 comments · Fixed by #3850
Closed

Avoid AxiosError when requesting bad image links #3468

krysal opened this issue Dec 5, 2023 · 5 comments · Fixed by #3850
Assignees
Labels
💻 aspect: code Concerns the software code in the repository 🛠 goal: fix Bug fix 🟨 priority: medium Not blocking but should be addressed soon 🧱 stack: frontend Related to the Nuxt frontend 🐛 tooling: sentry Sentry issue

Comments

@krysal
Copy link
Member

krysal commented Dec 5, 2023

Description

There is a recurrent error of Axios trying to request an image with a wrong uuid. It has 112 occurrences since first seen on Nov 20, 2023.

GET /image/f51977d1-081a-4e9f-afe1-

It raises an error with the message: Request failed with status code 404 .

It is unknown to me what or who stumbles upon this URL, but it consumes part of our Sentry quota, so it should be prevented somehow from triggering a notification.

Reproduction

Reproduction steps are unknown.

Sentry details

https://openverse.sentry.io/share/issue/20761d85362b468190131dcda0463e9f/

@krysal krysal added 🟨 priority: medium Not blocking but should be addressed soon 🛠 goal: fix Bug fix 💻 aspect: code Concerns the software code in the repository 🐛 tooling: sentry Sentry issue 🧱 stack: frontend Related to the Nuxt frontend labels Dec 5, 2023
@openverse-bot openverse-bot moved this to 📋 Backlog in Openverse Backlog Dec 5, 2023
@AetherUnbound AetherUnbound moved this from 📋 Backlog to 📅 To Do in Openverse Backlog Dec 11, 2023
@krysal krysal moved this from 📅 To Do to 📋 Backlog in Openverse Backlog Jan 15, 2024
@AetherUnbound AetherUnbound moved this from 📋 Backlog to 📅 To Do in Openverse Backlog Jan 26, 2024
@sarayourfriend sarayourfriend self-assigned this Jan 29, 2024
@sarayourfriend sarayourfriend moved this from 📅 To Do to 🏗 In Progress in Openverse Backlog Jan 29, 2024
@sarayourfriend
Copy link
Collaborator

Adding some thoughts about how I am approaching implementation. I think it's a clear win to ignore 404s and not send them to Sentry any more. That's easy to do, we just wrap the call to sentry with if (errorData.statusCode !== 404). However, if 404 is also returned for malformed requests, which is what we are dealing with in the request, then I think we actually need an additional change in the API to return 400 in those cases. We really should not be getting a malformed request sent to the API from the frontend under normal operation, so it is good to know when that happens.

@krysal what do you think? I've got the implementation to filter 404s, that was easy enough. What do you think about making the API return 400 for malformed identifiers? That seems like a clear-cut issue to me.

@sarayourfriend
Copy link
Collaborator

Actually, I'm looking at another AxiosError in Sentry (private maintainer access required) and these are all "NETWORK_ERR" reason codes. That also seems like something we can pretty safely ignore, but it is different than the one linked in the issue description.

Regarding making malformed identifiers 400 instead of 404, it isn't easily possible. We use the lookup value regex to determine when retrieve is called in the router, so when a malformed identifier is passed, the DRF router actually doesn't even see the request match a route, rather than it having an incorrect UUID. Hence the 404 rather than 400.

We really just need to treat all 404s coming from the API for single result requests as bad requests, then, without changes to the API, unless we want to really hack on Django's routing and making things more complex.

@krysal
Copy link
Member Author

krysal commented Jan 31, 2024

@sarayourfriend Yeah, I'm not sure if it's worth making the distinction between the 404 and 400 requests in this case. Under the hood I assume the request to the API is still of the form v1/images/<id> so one can also say it's technically valid but there is no image with that incomplete uuid, so it's a 404 Not Found.

@openverse-bot openverse-bot moved this from 🏗 In Progress to 📋 Backlog in Openverse Backlog Feb 20, 2024
@openverse-bot openverse-bot moved this from 📋 Backlog to 🏗 In Progress in Openverse Backlog Feb 29, 2024
@openverse-bot openverse-bot moved this from 🏗 In Progress to ✅ Done in Openverse Backlog Mar 21, 2024
@AetherUnbound
Copy link
Collaborator

This issue is still occurring: https://openverse.sentry.io/share/issue/20761d85362b468190131dcda0463e9f/

@obulat Do you think it would be best to wait until we migrate to Nuxt 3 to try and handle this?

@sarayourfriend
Copy link
Collaborator

Along with #4538 Axios would go away altogether. I don't know if Axios is the specific problem here though, there might be a general issue we need to contend with.

The issue title is a bit misleading in that regard 🤔 Although, I suppose #3850 should be catching these anyway?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💻 aspect: code Concerns the software code in the repository 🛠 goal: fix Bug fix 🟨 priority: medium Not blocking but should be addressed soon 🧱 stack: frontend Related to the Nuxt frontend 🐛 tooling: sentry Sentry issue
Projects
Archived in project
3 participants