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

Add option to set Accept header #282

Merged
merged 2 commits into from Apr 26, 2024
Merged

Add option to set Accept header #282

merged 2 commits into from Apr 26, 2024

Conversation

ghost
Copy link

@ghost ghost commented Apr 18, 2024

This PR adds an option to set the Accept header explicitly in the client. This is useful in the case that the server can hand back a better format e.g. WebP instead of JPEG, and the client simply has to let the server know.

@agourlay
Copy link
Owner

agourlay commented Apr 18, 2024

Thanks for the contribution!

How did you test this?

I can see two things to pay attention to with this change.

First, the extension of the file is based on the input URL or an extra request to determine it.
By passing the accept header only to the download request, we would not use the right extension.

The same logic also apply for the content-length which is determine by a HEAD request before hand.
Downloading a different format would result in incorrect progress bars and download resume.

WDYT?

@ghost
Copy link
Author

ghost commented Apr 18, 2024

Good insights!

For testing, I simply hard-coded image/webp like so: let request = client.get(...).header("Accept", "image/webp") and verified that I had downloaded webp instead of jpeg. The extension is incorrect, but most software I use for images will sniff the data rather than trust the extension. This can occasionally break things though.

To answer the questions:

  1. I think the way to fix this and have the correct extension is to verify the Content-Type the server responds with, and to base the extension on the mime type received. A properly conforming server should inform the client when returning the data.

  2. You are correct that will likely affect the Content-Length, and I'm not sure how common it is for servers to correctly report the length when receiving this header during a HEAD request. If you set Accept during a head request, it might work properly? I'm not sure.

Background:

I originally conceived of this modification when I noticed a discrepancy between browser responses and what dlm was downloading. On checking the console, I noted that some sites return WebP images despite requesting a .jpg in the URL, which led to learning about the Accept header. The quality difference was notable between formats, and I was surprised that the server would respond with a different type than I had requested.

For my own use-case, I leave dlm in the background and only check after a few hours to see if it finished, usually it's pulling a large number of files concurrently.

@agourlay
Copy link
Owner

Thanks for the detailed answer 👍

  1. Sounds good to me
  2. Normally the Content-length is just used for sizing the progress bars, it should be fine if the server does not reply properly on the HEAD request without taking into account the Accept header.

I propose you try to send the extra header for all requests. Maybe the code needs some refactoring to have less parameters passed around, some kind of request builder abstraction?

Glad to hear dlm is useful to you!

@ghost
Copy link
Author

ghost commented Apr 26, 2024

Added a small change to send Accept with the HEAD. So far working on my build. Let me know what you think.

After mulling this over, I think you're right that some request builder abstraction would be a good idea. Some set of headers could be configured by the user up front, and that information passed around the codebase. It might be more work than it's worth for your use-case or mine, but that does seem like a useful idea.

@agourlay
Copy link
Owner

Thanks for the contribution 👍

I will take of the refactoring and extra testing 🤖

@agourlay agourlay merged commit 50248e1 into agourlay:master Apr 26, 2024
2 checks passed
@ghost ghost deleted the feature/accept-header branch April 29, 2024 00:11
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.

1 participant