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

Unable to overwrite request headers as of v0.9.30 #1100

Closed
ctedgar opened this issue Oct 24, 2023 · 6 comments
Closed

Unable to overwrite request headers as of v0.9.30 #1100

ctedgar opened this issue Oct 24, 2023 · 6 comments
Assignees
Labels
1 backlog bug Something isn't working

Comments

@ctedgar
Copy link

ctedgar commented Oct 24, 2023

Which API doesn't behave as documented, and how does it misbehave?
As of version 0.9.30 we are no longer able to overwrite request headers (for streaming requests).

Minimal reproduction project
unable-to-overwrite-request-headers-reproducer

Above reproducer has a print statement to show headers that are sent

To Reproduce (i.e. user steps, not code)

  1. Checkout above reproducer branch
  2. Navigate to just_audo_test.dart and run the 'proxy' test. The test has been modified to send an additional header accept-encoding header with a custom value and to validate that the mock server received it. For this validation the mock server was also modified to send request headers in response.
  3. The test will fail because accept-encoding would have been overwritten by the AudioPlayer accept-encoding header value 'gzip'.

Error messages
N/A

Expected behavior
Expectation is for user supplied headers to override the defaults. i.e. if user supplies icy-metadata with value 0; then the final request should use the user supplied value for icy-metadata rather than the default which is 1

Screenshots
Screenshot depicting scenario where ice-metadata header wasn't sent to server as intended by user.
image

Prior to v0.9.30, the icy-metadata header would've been applied on top of the default headers that are supplied for the final request. From v0.9.30 onwards, I have noticed that the default headers such are overwriting the user supplied ones; which is preventing developers from overwriting the headers.

Desktop (please complete the following information):
Android

Smartphone (please complete the following information):
Pixel 7 pro

Flutter SDK version
4.13.8

Additional context
I have narrowed down the commit that changed the above behaviour to 27ab201

@ryanheise, I am happy to raise a PR to alter the logic so that user headers are applied on top of the defaults rather that the other way around. Please let me know if I may proceed with the PR.

Thanks very much in advance.

Cris

@ctedgar ctedgar added 1 backlog bug Something isn't working labels Oct 24, 2023
@ryanheise
Copy link
Owner

ryanheise commented Oct 24, 2023

Minimal reproduction project

N/A <-- Yes it is applicable

To Reproduce (i.e. user steps, not code) <-- Note: it says "not code"

Consider the below line of code. AudioPlayer.setAudioSource(AudioSource.uri( Uri.parse('server.tld/endpoint'), headers: {'icy-metadata': '0'}));

Can you please complete these sections?

@ryanheise, I am happy to raise a PR to alter the logic so that user headers are applied on top of the defaults rather that the other way around. Please let me know if I may proceed with the PR.

That would be welcome although I would then test the PR against the minimal reproduction project in this bug report to verify the correctness.

@ctedgar
Copy link
Author

ctedgar commented Oct 24, 2023

Hi @ryanheise,

Apologies for not completely filling out the form. I'm not entirely sure how I'd go about generating a reproducer for this specific issue as it would require making a request with supplied headers - which is what the snippet of code I mentioned does - but then you'd need to capture the request that is sent to see the effect (or by inspecting in the debugger). For context, I caught this while debugging on the backend (and worked my way back to just_audio)...

Would it help if I create a flutter project that instantiates an audio player and makes the uri call with the supplied headers ? And then for reproducer steps instruct the user to place a breakpoint where I did on the screenshot and run the project?

Thanks,

Cris

@ryanheise
Copy link
Owner

When you fork the repo, can you add a print statement to _proxyHandlerForUri to show what headers are sent?

ctedgar pushed a commit to ctedgar/just_audio that referenced this issue Oct 26, 2023
ctedgar pushed a commit to ctedgar/just_audio that referenced this issue Oct 26, 2023
ctedgar pushed a commit to ctedgar/just_audio that referenced this issue Oct 26, 2023
@ctedgar
Copy link
Author

ctedgar commented Oct 26, 2023

Hi @ryanheise,

Thanks for pointing me in the right direction.

I have created a reproducer project and updated the issue description with the details. I'm also including it here.

Minimal reproduction project
unable-to-overwrite-request-headers-reproducer

Above reproducer has a print statement to show headers that are sent

To Reproduce (i.e. user steps, not code)

  1. Checkout above reproducer branch
  2. Navigate to just_audo_test.dart and run the 'proxy' test. The test has been modified to send an additional header accept-encoding header with a custom value and to validate that the mock server received it. For this validation the mock server was also modified to send request headers in response.
  3. The test will fail because accept-encoding would have been overwritten by the AudioPlayer accept-encoding header value 'gzip'.

I have also created a branch with a proposed fix; here is the PR

Thanks again,

Cris

@ctedgar
Copy link
Author

ctedgar commented Nov 14, 2023

#1104 closes this issue.

@ctedgar ctedgar closed this as completed Nov 14, 2023
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs, or use StackOverflow if you need help with just_audio.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1 backlog bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants