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

FEAT: Use a LoaderResult object to support origin metadata and error handling #554

Merged
merged 8 commits into from
Sep 30, 2015
Merged

FEAT: Use a LoaderResult object to support origin metadata and error handling #554

merged 8 commits into from
Sep 30, 2015

Conversation

masom
Copy link
Contributor

@masom masom commented Sep 11, 2015

Following a discussion in #529 this PR implements a value object that holds loader response metadata.

Using a value object will allow thumbor to properly respond to upstream failures or errors.

@heynemann
Copy link
Member

Do you think this PR should also include the fix to return 404 when image not found and 504 otherwise?

@heynemann
Copy link
Member

Btw I wish I was half as productive as you are @masom... Thanks for the awesome job!

@masom
Copy link
Contributor Author

masom commented Sep 11, 2015

It should probably include the 404/504 fix although this means a little bit more refactoring. The _fetch method would need to return either a longer array or a value object to relay the information from the loader.

@heynemann
Copy link
Member

I agree we should return Value objects, but do you think it would be an acceptable compromise to return a larger array of values for this PR? Maybe we can get this refactor bit in another PR. If you feel you can do it all in the same PR, by all means go for it. I'm just worried that right now thumbor has a security leak (if people keep sending 404 images they can DDOS the server, since the edge caches won't cache it).

@masom
Copy link
Contributor Author

masom commented Sep 11, 2015

It depends on the edge node configuration. For instance AWS CloudFront will cache 404 / 5xx errors for 5 minutes unless configured otherwise.

@masom
Copy link
Contributor Author

masom commented Sep 14, 2015

The error handling behaviour might have to be changed to match #531 and other PRs/Issues.

@guilhermef @heynemann @phoet What do you guys think of this approach?

The metadata property still has to be passed to the FetchResult object and conditional error handling needs to be improved.

@phoet
Copy link
Contributor

phoet commented Sep 15, 2015

Cool THX. I did not understand why some vows were removed?

@masom
Copy link
Contributor Author

masom commented Sep 15, 2015

@phoet The deleted vows are a copy of the http_loader vows and they don't test anything from the https_loader

@phoet
Copy link
Contributor

phoet commented Sep 15, 2015

@masom ah ok

@masom
Copy link
Contributor Author

masom commented Sep 30, 2015

@guilhermef @heynemann OK to merge?

@guilhermef
Copy link
Member

@masom, It looks really good

masom pushed a commit that referenced this pull request Sep 30, 2015
FEAT: Use a LoaderResult object to support origin metadata and error handling
@masom masom merged commit 7992b4a into thumbor:master Sep 30, 2015
@masom masom deleted the x-loader-return-object branch September 30, 2015 14:49
@masom
Copy link
Contributor Author

masom commented Sep 30, 2015

This merge introduce a way for loaders to return metadata and specify error conditions.

This will give thumbor a lot more flexibility for dealing with loader errors.

The change is backward-compatible with a isinstance check, allowing us to deprecated the old response format in thumbor 6.x.

christianjgreen pushed a commit to fanhero/thumbor that referenced this pull request Aug 29, 2017
FEAT: Use a LoaderResult object to support origin metadata and error handling
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.

None yet

4 participants