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

files: remove HTTP code from read_content #610

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

egabancho
Copy link
Member

@egabancho egabancho commented Feb 25, 2025

  • Setting the return code directly hides the return code that send_file might be issuing, resulting in odd situations when the storage layer returns a 302, i.e. offload download.

When an instance uses local storage, eventually send_file gets called and the file is streamed through the application server.

If one uses Invenio-S3, instead of serving the file directly, the application issues a redirect to a short-lived signed URL, relaying the work of streaming the file to the storage server, offloading.

The problem is that send_file itself returns a response with its own response code, etc. The 200 here basically overwrites that response code and the redirect doesn't work.

We don't experience that same behavior with EOS because the redirect is happening at nginx level using X-Accel-Redirect.

* Setting the return code directly hides the return code that
  ``send_file`` might be issuing, resulting in odd situations when the
    storage layer returns a 302, i.e. offload download.
@egabancho
Copy link
Member Author

This should probably fix inveniosoftware/invenio-app-rdm#2017 🤔

@tmorrell
Copy link

This broke uploads on my (fairly ancient) test system

flask_resources/responses.py", line 40, in inner
   return resource_requestctx.response_handler.make_response(*res, many=many)
TypeError: make_response() got multiple values for argument 'many'

and didn't seem to fix the content issue. I'll try to test on v13 soon and see if it works better there.

@egabancho
Copy link
Member Author

I tested it locally on v13.0.0b1.dev25, and I didn't get any error.
How did you test it? Did you check-pick the change?

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.

3 participants