req.params undefined with unspecified res.format default

[CullenShane]

Hello everybody,

I'm using Express 4.17.1 and I ran into a problem where req.params is undefined in an onFinished call back that I recently added for error logging.

I have a middleware which listens to onFinished and inspects the req.params for error logging. I also use res.format to choose between two different supported Accept: headers, but I don't specify a default since the documentation says it will return a 406 if unhandled, which is what I want. I have a test that ensures that my server returns a 406 according to the documentation.

My 406 test fails because req.params is undefined so I can't navigate further into the object. When I was trying to sort that out I added a "default" handler to res.format and then the req.params object was valid and parsed correctly.

Additionally, I discovered that req.params is parsed properly when in my controller (before the call to res.format), but the version of the req.params that is passed into onFinished later in that same request is undefined.

Is the the expected behavior? The documentation and behavior makes me think this is unexpected.

I can provide more information if needed

Thanks

[dougwilson]

Hi @CullenShane , the req.params property is only populated in the middleware/handler that has those parameters defined on the path. I assume you defined your error handler with no parameters (app.use((err, res, res, next) => ...) vs app.use('/:param1', (err, req, res, next) => ...) which is why the req.params is empty, due to the error handler having no parameters defined.

But ultimately, without providing the full code and instructions for how to reproduce the issue, we're just making guesses, so not 100% sure if the behavior you are experiencing is expected vs unexpected. If you can provide a reproduceable test-case we can run to see the issue demonstrated, we can make an accurate evaluation.

[busticated]

hi @dougwilson - thanks for taking a look and thanks for all the work you do (and have done over the years) maintaining express and friends 🙏:pray::pray:

@CullenShane and i were pairing on this one earlier so i figured i'd chime in w/ more detail.

basically, we have a fairly simple app that runs some middleware for all routes to capture data after a response is finished:

const onFinished = require('on-finished');
module.exports = ({ logFoo }) => {
    return (req, res, next) => {
        onFinished(res, () => logFoo(req.params.foo));
        next();
    };
};

...which hosts an endpoint like:

app.get('/v1/stuff/:foo/config', configFooStuff);

...which ultimately calls res.format() like:

configFooStuff(req, res) {
    res.format({
        'application/json': async () => {
            //...
        },
        'application/schema+json': () => {
            //...
        }
    });
}

when the Accept header specifies something we aren't handling (e.g. text/html), we end up with Uncaught TypeError: Cannot read property 'params' of undefined. when handling a format we do support (e.g. application/json), req.params is an object w/ a valid .foo property and everything works as expected. further, if we add an explicit default handler to our res.format() call - e.g.:

'default': () => {
    res.status(406).json({ error: 'Not Acceptable' });
}

...things work but req.params is an empty object.

so the main question is why would req.params be undefined when res.format() doesn't use a default handler? willing to bet we're just missing something given how exercised this code must be but 🤷‍♂️

thanks again!

edit: fix typos and minor derpage 🤦

[dougwilson]

Ah, very interesting. Even more so given you are seeing the error `Uncaught TypeError: Cannot read property 'params' of undefined occurring... I am not sure how you can possibly seeing such an error with the code you gave above. I would love it if I could run it and try it out, to get that same condition to see what is happening. Is it possible you can write up a really simple app that I can call that demonstrates the issue (reproduces the error)?

[busticated]

Is it possible you can write up a really simple app that I can call that demonstrates the issue (reproduces the error)?

cool, yeah that's probably the best next-step. we'll try to pull something together in the next day or two.

thanks again 🙏:+1:

[busticated]

re-reading this with fresh eyes, i see i messed up the error detail 🤦‍♂️🙄

it's actually:

Uncaught TypeError: Cannot read property 'foo' of undefined

iow, req.params is undefined when we expected it always at least be an empty object.

regardless, we'll put together a simplified demo that reproduces the issue :+1:

[dougwilson]

req.params is undefined when we expected it always at least be an empty object.

Ah, thank you, that does help 👍 . I can say, at least, that req.params will always be undefined when the request cycle is not currently "in" a middleware or handler in Express. This means that when the request first comes in from Node.js it will be undefined, and also during any "in between time" between middlewares. When a request enters a middleware/handler, before user code is executed, Express will set up req.params property, but once that middleware/handler calls next() or throws an error, the property is removed so the next middleware/handler can populate it, if needed.

It will always be an object if the request is actively "in" a middleware/handler, even if it's an empty object. Since it is undefined for you, it means (assuming there is not some code actually setting it to undefined somewhere) that the request is not inside a middleware/handler of Express at all... which would be strange based on your description above.

[busticated]

hm. well, after all that all i can say with confidence is that this does not reproduce the issue:

// filename: repro.js
const onFinished = require('on-finished');
const express = require('express');
const app = express();


app.use((req, res, next) => {
	onFinished(res, () => console.log('FOO IS:', req.params.foo));
	next();
});

app.get('/v1/stuff/:foo/config', (req, res) => {
	res.format({
		'application/json': () => {
			res.status(200).json({ ok: true });
		},
		'application/schema+json': () => {
			res.status(200).type('application/schema+json').send('ok');
		}
	});
});

app.listen(9001);

module.exports = app;

start the server:

./node repro.js

hit the endpoint:

curl http://localhost:9001/v1/stuff/12345/config -H "Accept: text/html"

so, i think we have more digging to do :pray::wave:

[dougwilson]

Thanks for that update @busticated . I just wanted to post here to see if you found anything additional at all or not so far.

[busticated]

thanks for the ping @dougwilson :pray: no, we haven't been able to dig much further into this one yet. feel free to close it if that helps improve your signal-to-noise ratio 😁

the one thing i did turn up is that - in our app at least - we get the same error / issue when invoking Express' default error handler (docs) - that is, req.params is not an object and we get Uncaught TypeError: Cannot read property 'foo' of undefined.

[dougwilson]

the one thing i did turn up is that - in our app at least - we get the same error / issue when invoking Express' default error handler (docs) - that is, req.params is not an object and we get Uncaught TypeError: Cannot read property 'foo' of undefined.

As for this, that is definately expected, as req.params is only populated while the request/response lifecycle is within a handler that defined parameters. Once it leaves the handler and re-enters into the routing system, req.params is cleared in order to populate again for the next route. In order for a request/response to have reached the default error handler, it had to have left a handler (or never entered one) and gone back into the routing system.