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

Headers are null when using .inject #47

Closed
2 tasks done
SimenB opened this issue Aug 17, 2021 · 14 comments · Fixed by fastify/light-my-request#154 or fastify/light-my-request#224
Closed
2 tasks done

Headers are null when using .inject #47

SimenB opened this issue Aug 17, 2021 · 14 comments · Fixed by fastify/light-my-request#154 or fastify/light-my-request#224
Labels
bug Something isn't working

Comments

@SimenB
Copy link
Member

SimenB commented Aug 17, 2021

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure it has not already been reported

Fastify version

3.20.2

Plugin version

0.3.3

Node.js version

14.17.5

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

11.5.1

Description

When using fastify.inject and this plugin, res.headers is always null. The headers are in res.raw.res.getHeaders() FWIW.

Debugging, https://github.com/fastify/light-my-request/blob/014c1c8016f8605b5ca4c7906f527d5ab0b17267/lib/response.js#L66-L81 is never called

Steps to Reproduce

In this repo, change fastify.listen to fastify.inject

diff --git i/test/application.test.js w/test/application.test.js
index a8e1915..a28e4dd 100644
--- i/test/application.test.js
+++ w/test/application.test.js
@@ -166,14 +166,11 @@ test('Should flush headers if express handles request', t => {
     .register(expressPlugin)
     .after(() => { fastify.use(router) })
 
-  fastify.listen(0, (err, address) => {
+  fastify.inject({
+    method: 'GET',
+    url: '/'
+  }, (err, res) => {
     t.error(err)
-    sget({
-      method: 'GET',
-      url: address
-    }, (err, res) => {
-      t.error(err)
-      t.strictEqual(res.headers.foo, 'bar')
-    })
+    t.strictEqual(res.headers.foo, 'bar')
   })
 })

image

Expected Behavior

headers should be populated

@mcollina
Copy link
Member

Would you like to send a Pull Request to address this issue? Remember to add unit tests.

@mcollina mcollina added bug Something isn't working good first issue Good for newcomers labels Aug 17, 2021
@SimenB
Copy link
Member Author

SimenB commented Aug 17, 2021

Happy to, but I don't know what the fix is. Any pointers?

I can send a PR with what I have locally which I don't think is the correct fix, but that's in find-my-way and not here (essentially forcing headers to be copied in https://github.com/fastify/light-my-request/blob/014c1c8016f8605b5ca4c7906f527d5ab0b17267/lib/response.js#L118 if they're not). I assume the fix should be in this plugin?

@mcollina
Copy link
Member

The fix should be here.

@SimenB
Copy link
Member Author

SimenB commented Aug 18, 2021

Right, makes sense. I don't think I'll be able to send a PR though as I don't know what the fix is 😅

@mcollina
Copy link
Member

My understanding is that light-my-request cannot work with Express as the latter does a lot of prototype chain manipulation that light-my-request is not really liking.

I think the fix should/would be in light-my-request.

@mcollina mcollina removed the good first issue Good for newcomers label Aug 18, 2021
@SimenB
Copy link
Member Author

SimenB commented Aug 18, 2021

Yeah, that seems reasonable. I can send the PR I was talking about above, and we can figure out if it's an ok way to go?

@mcollina
Copy link
Member

let's see!

@SimenB
Copy link
Member Author

SimenB commented Aug 18, 2021

Opened up fastify/light-my-request#154

@SimenB
Copy link
Member Author

SimenB commented Sep 10, 2022

This has regressed as of fastify/light-my-request#221

apply this diff to get a failure (same as OP, but that doesn't apply cleanly anymore):

diff --git i/test/application.test.js w/test/application.test.js
index efc0e4c..a8a44c3 100644
--- i/test/application.test.js
+++ w/test/application.test.js
@@ -145,7 +145,7 @@ test('Should expose the express app on the fastify instance', t => {
 })
 
 test('Should flush headers if express handles request', t => {
-  t.plan(3)
+  t.plan(2)
   const fastify = Fastify()
   t.teardown(fastify.close)
 
@@ -166,14 +166,11 @@ test('Should flush headers if express handles request', t => {
     .register(expressPlugin)
     .after(() => { fastify.use(router) })
 
-  fastify.listen({ port: 0 }, (err, address) => {
+  fastify.inject({
+    method: 'GET',
+    url: '/'
+  }, (err, res) => {
     t.error(err)
-    sget({
-      method: 'GET',
-      url: address
-    }, (err, res) => {
-      t.error(err)
-      t.equal(res.headers.foo, 'bar')
-    })
+    t.equal(res.headers.foo, 'bar')
   })
 })

@Eomm
Copy link
Member

Eomm commented Sep 10, 2022

I found the issue: the writehead method is not called because of this line in fastify:

Reply.prototype.status = Reply.prototype.code

I think the best solution would be to that light-my-request support both the cases instead that this module care about the fastify internals

@Eomm
Copy link
Member

Eomm commented Sep 10, 2022

Ok, after a lot of debugging, I found that the reply.raw.end function is overwritten during the code flow.

If you log reply.raw.end.toString(), you will see that it changes from:

  • fastify-express/index.js
  • router.get('/', (req, res) => { console.log(res.end.toString())

The issue comes from here:

https://github.com/expressjs/express/blob/33e8dc303af9277f8a7e4f46abfdcb5e72f6797b/lib/application.js#L237-L239

This code is executed after three express layers:

  • function query(req, res, next)
  • function expressInit(req, res, next) * that changes the proto
  • finally the router function router(req, res, next) that receives the wrong object

This prototype switching simply does not execute the light-my-request code.

So, now I think that we should find how to configure/fix this project instead or, restoring the previous code in lmr that copies the headers.
In the latter case, I was unable to replicate this issue/configuration on that repository 😞

What I tried but failed to do on this repo:

Object.assign(fastify.express.response, LmrResponse)

Object.setPrototypeOf(Object.getPrototypeOf(reply.raw), LmrRequest.prototype)

@SimenB
Copy link
Member Author

SimenB commented Sep 16, 2022

@mcollina should we reintroduce the header code removed from light-my-request up until a cleaner solution is available? Even though we seem unable to reproduce it properly outside of fastify-express.

@mcollina
Copy link
Member

I have no time to dig into the problem atm (I'll be very busy until mid-october), I'm ok with a full revert or whatever fix you think it's appropriate.

@SimenB
Copy link
Member Author

SimenB commented Sep 16, 2022

Cool, I'll send a PR 👍

I don't think we need to revert, just re-add the somewhat hacky header code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants