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 adaptor doesn't convert header values correctly #1021

Merged
merged 1 commit into from May 11, 2021

Conversation

reedchan7
Copy link
Contributor

Fix(adaptor): Fixed an issue where the adapter did not convert all values of the header successfully when converting the header.

Change Header.Set() to Header.Add() to support header that has multiple values such as Accept

Relative reference: gofiber/fiber#1329

values of the header successfully when converting the header.
@erikdubbelboer erikdubbelboer merged commit b2f111b into valyala:master May 11, 2021
@erikdubbelboer
Copy link
Collaborator

Thanks!

@ReneWerner87
Copy link
Contributor

@erikdubbelboer with this change a bug has crept in

by doing the Add instead of Set, the ContentType is no longer set and the default ContentType is used, although it was already set by the Add.

gofiber/fiber#1351 (comment)
gofiber/fiber#1351 (comment)
gofiber/fiber#1351 (comment)

We just use
https://github.com/gofiber/fiber/blob/master/middleware/pprof/pprof.go#L13

and our test is broken
https://github.com/gofiber/fiber/blob/master/middleware/pprof/pprof_test.go#L43

although the content type "text/html; charset=utf-8" was set inside
https://golang.org/src/net/http/pprof/pprof.go?s=11637:11687#L377

@ReneWerner87
Copy link
Contributor

@juan-chan can you look at this again and make a new correction to the initial problem with an eye on this resulting error?

@erikdubbelboer
Copy link
Collaborator

@ReneWerner87 a fix is in progress here #1036 and will be merged next week of it doesn't break for anyone.

@ReneWerner87
Copy link
Contributor

Thanks for the quick response and the link

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.

None yet

3 participants