-
-
Notifications
You must be signed in to change notification settings - Fork 844
test(helper/proxy): does not propagate undefined request headers #3921
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
test(helper/proxy): does not propagate undefined request headers #3921
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3921 +/- ##
=======================================
Coverage 91.31% 91.31%
=======================================
Files 168 168
Lines 10673 10686 +13
Branches 3167 3077 -90
=======================================
+ Hits 9746 9758 +12
- Misses 926 927 +1
Partials 1 1 ☔ View full report in Codecov by Sentry. |
69b33a0 to
32dbe49
Compare
32dbe49 to
d52bde4
Compare
| const { raw, ...requestInit } = proxyInit ?? {} | ||
|
|
||
| const req = new Request( | ||
| input, | ||
| // @ts-expect-error `headers` in `requestInit` is not compatible with HeadersInit | ||
| { | ||
| ...buildRequestInitFromRequest(raw), | ||
| ...requestInit, | ||
| } | ||
| ) |
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.
Re: #3919, my basic understanding of the spread operator is that new objects are created and all the keys are copied. This avoids that extra overhead
| // @ts-expect-error `headers` in `requestInit` is not compatible with HeadersInit | ||
| return requestInit |
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.
This is what lines 94 and 97 were doing previously..
hono/src/helper/proxy/index.ts
Lines 92 to 99 in e7fbc3e
| const req = new Request( | |
| input, | |
| // @ts-expect-error `headers` in `requestInit` is not compatible with HeadersInit | |
| { | |
| ...buildRequestInitFromRequest(raw), | |
| ...requestInit, | |
| } | |
| ) |
| ...requestInit, | ||
| 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.
Previously the headers from buildRequestInitFromRequest(raw) were being overwritten by ...requestInit on line 97
hono/src/helper/proxy/index.ts
Lines 95 to 98 in e7fbc3e
| { | |
| ...buildRequestInitFromRequest(raw), | |
| ...requestInit, | |
| } |
| if (value === undefined) { | ||
| headers.delete(name) | ||
| } else { | ||
| headers.append(name, value) | ||
| } |
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.
Unsure if this is right..
It's deleting a header when the value is undefined, otherwise the value is appended. This means values in both the ProxyRequestInit and the request will be merged, rather than the value from the request overwriting the value from ProxyRequestInit, for example.
| }) | ||
| const req = (global.fetch as ReturnType<typeof vi.fn>).mock.calls[0][0] | ||
| expect(req.headers.get('Authorization')).toBeNull() | ||
| expect(req.headers.get('X-Request-Id')).toBe('123, 123') |
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.
Duplicate request headers using Header#append. Alternatively, using Header#set
| method: request.method, | ||
| body: request.body, | ||
| duplex: request.body ? 'half' : undefined, | ||
| ...requestInit, | ||
| 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.
Looking at this again, maybe this should actually be..
| method: request.method, | |
| body: request.body, | |
| duplex: request.body ? 'half' : undefined, | |
| ...requestInit, | |
| headers, | |
| ...requestInit, | |
| method: request.method, | |
| body: request.body, | |
| duplex: request.body ? 'half' : undefined, | |
| headers, |
..to avoid requestInit overwriting any of method, body, duplex and headers?
undefinedheaders fromProxyRequestInitRequestandProxyRequestInitFixes #3920