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

fix(downloads): set content-type and content-length headers #706

Merged
merged 6 commits into from
Oct 13, 2021

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Oct 7, 2021

Fixes #705

This improves the user experience when requesting files for download, particularly through the web-client. Content-Type and Content-Length headers are set where they were previously missing on some GET handlers, so that requesting clients are always provided with this information about the requested asset.

@andrewazores andrewazores added the feat New feature or request label Oct 7, 2021
@andrewazores andrewazores requested review from ebaron and jan-law October 7, 2021 19:38
Copy link
Contributor

@jan-law jan-law left a comment

Choose a reason for hiding this comment

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

For GET recording download, is the [16297 bytes data] the octet-stream equivalent of a content-length header?

> GET /api/v1/targets/service:jmx:rmi:%2F%2F%2Fjndi%2Frmi:%2F%2Flocalhost:0%2Fjmxrmi/recordings/test HTTP/1.1
> Host: 0.0.0.0:8181
> User-Agent: curl/7.71.1
> Accept: */*
> 
{ [5 bytes data]
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< transfer-encoding: chunked
< content-type: application/octet-stream
< 
{ [16297 bytes data]
100 1229k    0 1229k    0     0  6274k      0 --:--:-- --:--:-- --:--:-- 6306k

GET report for comparison:

> GET /api/v1/targets/service:jmx:rmi:%2F%2F%2Fjndi%2Frmi:%2F%2Flocalhost:0%2Fjmxrmi/reports/test HTTP/1.1
> Host: 0.0.0.0:8181
> User-Agent: curl/7.71.1
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< content-type: text/html
< content-length: 120901

@andrewazores
Copy link
Member Author

GET on a report can always provide a Content-Length since once the report is generated as HTML, we always know exactly how large that document is and can report it in the header.

GET on an archived recording can also have a Content-Length, because we have that file on disk and can measure its size when we send it.

GET on an active recording has no Content-Length because Cryostat doesn't know how large the recording is - it just reads it from the remote JVM and pipes it through to the requesting client as a stream. At the time that it sends the response headers, the length is unknown. The transfer-encoding is set to chunked to reflect this streaming nature.

@andrewazores andrewazores force-pushed the download-files-headers branch from 0008574 to 2bd29cd Compare October 8, 2021 16:26
@andrewazores andrewazores force-pushed the download-files-headers branch from 2bd29cd to 29c12db Compare October 12, 2021 13:38
Copy link
Contributor

@jan-law jan-law left a comment

Choose a reason for hiding this comment

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

Looks good to me! I'll leave it to @ebaron for final review comments/approval

Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

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

Looks good!

@andrewazores andrewazores merged commit 450c822 into cryostatio:main Oct 13, 2021
@andrewazores andrewazores deleted the download-files-headers branch October 13, 2021 18:21
tthvo pushed a commit to tthvo/cryostat-legacy that referenced this pull request Dec 9, 2024
…6.5 to 4.8.6.6 (cryostatio#706)

build(deps): bump com.github.spotbugs:spotbugs-maven-plugin

Bumps [com.github.spotbugs:spotbugs-maven-plugin](https://github.com/spotbugs/spotbugs-maven-plugin) from 4.8.6.5 to 4.8.6.6.
- [Release notes](https://github.com/spotbugs/spotbugs-maven-plugin/releases)
- [Commits](spotbugs/spotbugs-maven-plugin@spotbugs-maven-plugin-4.8.6.5...spotbugs-maven-plugin-4.8.6.6)

---
updated-dependencies:
- dependency-name: com.github.spotbugs:spotbugs-maven-plugin
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some file download handlers do not include Content-Length and/or Content-Type headers
3 participants