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

Provide a content-length for streaming zip downloads #4313

Merged
merged 1 commit into from
Nov 15, 2021
Merged

Provide a content-length for streaming zip downloads #4313

merged 1 commit into from
Nov 15, 2021

Conversation

pR0Ps
Copy link
Contributor

@pR0Ps pR0Ps commented Nov 9, 2021

  • Your changes are not possible to do through a plugin and relevant to a large audience (ideally all users of OctoPrint)
  • If your changes are large or otherwise disruptive: You have made sure your changes don't interfere with current development by talking it through with the maintainers, e.g. through a Brainstorming ticket
  • Your PR targets OctoPrint's devel branch if it's a completely new feature, or maintenance if it's a bug fix or improvement of existing functionality for the current stable version (no PRs against master or anything else please)
  • Your PR was opened from a custom branch on your repository (no PRs from your version of master, maintenance or devel please), e.g. dev/my_new_feature or fix/my_bugfix
  • Your PR only contains relevant changes: no unrelated files, no dead code, ideally only one commit - rebase and squash your PR if necessary!
  • Your changes follow the existing coding style
  • If your changes include style sheets: You have modified the .less source files, not the .css files (those are generated with lessc)
  • You have tested your changes (please state how!) - ideally you have added unit tests
  • You have run the existing unit tests against your changes and nothing broke
  • You have added yourself to the AUTHORS.md file :)

What does this PR do and why is it necessary?

This PR improves the existing zip streaming functionality by setting the size of the generated zip as the Content-Length header in the response (where possible). This allows browsers and other clients to check for free space, calculate download progress, and detect if the download fails (they get less data than expected). This is most useful when downloading multiple timelapses due to their potential size.

This is enabled by using zipstream-ng (a library I maintain) to generate the zip streams since it provides the ability to calculate the final size of a zip file before actually creating and streaming it.

Also, a small oversight where the printer's firmware wasn't being added to systeminfo.txt when downloading system info was fixed.

How was it tested? How can it be tested by the reviewer?

Timelapse downloads were tested by adding setting up timelapse recording, adding a virtual printer, and "printing" a sample file multiple times. This generated multiple timelapses. All the timelapses were then selected and downloaded. The download prompt showed the total size of the zip file (see attached screenshot). After downloading the zipfile was extracted and the contents were verified to be the same as when downloading the files individually.

A similar process (but with less setup) was used to verify log and system information zip downloads. Generating a zip file on the local filesystem was tested using octoprint systeminfo .

Any background context you want to provide?

The reason this PR is based against the devel branch is that the added library does not support Python 2.7 which is currently still supported in the maintenance branch.

What are the relevant tickets if any?

N/A

Screenshots (if appropriate)

timelapse-download

@github-actions github-actions bot added targets devel The PR targets the devel branch approved Issue has been approved by the bot or manually for further processing labels Nov 9, 2021
Copy link
Member

@foosel foosel left a comment

Choose a reason for hiding this comment

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

So, in principle I'm all for this. I see one big issue however: the namespace of the zipstream-ng clashes with that of zipstream-new. That won't be an issue for new installations, but anyone who'd upgrade from pre-2.0 to 2.0 would pull in zipstream-ng but leave zipstream-new installed in the venv as well and then it would become a game of chance which one would be loaded at runtime when importing zipstream and the slight API differences would then wreak havoc. We had an issue like that with a slugification library in the past that I finally was forced to bundle instead of fetching from pip, with a collision-free name, and that was a mess I really don't want to have to deal with again.

So either we need to find a way to uninstall zipstream-new automatically (which I'm not sure is possible to make work during a pip install), or we make this be able to handle both libraries (frankly a veto from me due to the increased maintenance overhead), or if possible the content-length determination gets backported into a utility function in OctoPrint. But the way it is now, while beautiful code I cannot merge it because it would cause some severe hiccups on rollout.

@foosel foosel added the needs some work There are some things to do before this PR can be merged label Nov 10, 2021
@pR0Ps
Copy link
Contributor Author

pR0Ps commented Nov 11, 2021

Understood, I'll see what I can do about fixing the namespacing and update the PR soon.

@pR0Ps
Copy link
Contributor Author

pR0Ps commented Nov 14, 2021

I've updated the zipstream-ng library to allow importing the functionality from zipstream.ng and used that namespace here. This will avoid any namespace conflicts with the previous library.

This allows browsers and other clients to calculate download progress
and detect if the download fails (they get less data than expected).
This is most useful when downloading multiple timelapses due to their
potential size.

Also fixes an oversight where the printer's firmware wasn't being added
to `systeminfo.txt` when downloading system info.
@foosel foosel removed the needs some work There are some things to do before this PR can be merged label Nov 15, 2021
@foosel foosel added this to the 2.0.0 milestone Nov 15, 2021
@foosel
Copy link
Member

foosel commented Nov 15, 2021

Thank you, this namespacing solution should work nicely!

@foosel foosel merged commit c284269 into OctoPrint:devel Nov 15, 2021
foosel added a commit that referenced this pull request Nov 22, 2021
foosel pushed a commit that referenced this pull request Feb 3, 2022
This allows browsers and other clients to calculate download progress
and detect if the download fails (they get less data than expected).
This is most useful when downloading multiple timelapses due to their
potential size.

This is a backport of PR #4313
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Issue has been approved by the bot or manually for further processing targets devel The PR targets the devel branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants