-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 DisableSpecialHeaders option #1573
add DisableSpecialHeaders option #1573
Conversation
header.go
Outdated
// If *RequestHeader.Read() is called, special headers will be parsed but not sent. | ||
// e.g. *RequestHeader.Host() will return something, but the header will not be set when the request is sent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// If *RequestHeader.Read() is called, special headers will be parsed but not sent. | |
// e.g. *RequestHeader.Host() will return something, but the header will not be set when the request is sent. | |
// If RequestHeader.Read() is called, special headers will be parsed but not sent. | |
// e.g. RequestHeader.Host() will return something, but the header will not be set when the request is sent. |
No need to specify the pointer receiver here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And wouldn't it be better to change methods like Host()
to be
if h.disableSpecialHeader {
return h.Peek(strHost)
}
return h.host
so not to confuse people? The overhead of one if statement in these is worth it I think.
header_test.go
Outdated
t.Fatalf("unexpected: %q. Expecting %q", h.Host(), "notfoobar") | ||
} | ||
// host is at the end of h.String() | ||
if !strings.HasSuffix(h.String(), "host: notfoobar\r\n\r\n") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change this to a full h.String() == ".."
instead of just a suffix check?
made your suggested changes, you can take another look |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are failing can you have a look?
Ahhh made some silly errors, should be good now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering it DisableSpecialHeader
should affect parsing? It makes things a lot more confusing. Wouldn't it be simpler if it only affects how headers are written? Since that's the main use case for this?
header_test.go
Outdated
t.Fatalf("unexpected error: %v", err) | ||
} | ||
// Host and User-Agent are not parsed | ||
if h.Host() != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why Host()
should return nil
here? Host:
is present in the headers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah this is because for Host and User-Agent they are saved as separate bytes, not present in the headers h.h
. When we call Host()
with special headers disabled it tries to read it from h.h
. I guess technically parsing is currently unaffected, but I agree this behavior is confusing. I'll push another commit that makes sure to parse all special headers as regular headers
header_test.go
Outdated
if h.Host() != nil { | ||
t.Fatalf("unexpected: %q. Expecting nil", h.Host()) | ||
} | ||
if h.Host() != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're checking this twice.
Thanks! |
This is a PR is response to #1572
Thanks for your quick response, and for your work in maintaining this package overall. Here is a PR to entirely disable fasthttp setting special headers for you. This way the user has complete control over headers and the burden is on them to make sure things work correctly.
Feedback is welcome