-
Notifications
You must be signed in to change notification settings - Fork 215
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
Check the file type of the thumbnail before passing it to Photon #2086
Conversation
60d3e29
to
83b5ace
Compare
5e326ad
to
7cdfeea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code and tests look good, great call on adding caching at various levels! I have a few suggestions/questions but nothing blocking 🚀 🖼️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and tested well! I appreciated the pro-active work to reduce Sentry messages 🚀
Oh, one other thing I was curious about! I know we do the dead link filtering with using
I think that's because they're all happening as part of one request, whereas queries for the thumbnail are each their own request, right? That to say, do we need to try and parallelize this or is it okay to make the |
7cdfeea
to
e8ffe3a
Compare
Co-authored-by: Madison Swain-Bowden <bowdenm@spu.edu>
e8ffe3a
to
9d76006
Compare
Co-authored-by: Staci Mullins <63313398+stacimc@users.noreply.github.com>
814c247
to
7994b8b
Compare
I'm not sure I fully understand the question 🤔 These are all individual request so I don't see how a group of thumbnails requests can be parallelized in this point. We could create another endpoint that receives a group of media IDs to return their thumbnails, and there they can be parallelized, but here it might be a simple request of the link returned from Thank you all for the suggestions! |
Got it, just got myself mixed up and had to talk through it to understand - thanks @krysal! |
Fixes
Fixes #2067 by @krysal
Description
To prevent known errors from Photon due to unsupported file types, this PR adds the following check before making the call to Photon:
images/
), so try to guess it first from the URL. I believe it is worth trying this route, since this is where most of the cases will fall.Testing Instructions
If you want to manually check, try replacing the first row (after the header) of the
sample_data/sample_image.csv
file with the following:The run
just api/recreate
and visit http://localhost:50280/v1/images/0e3315c5-3328-4a99-80ab-567ac32f685f/thumb/You should see the new response message 'Unsupported media type "svg" in request.'
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin