Skip to content
This repository has been archived by the owner on Dec 4, 2018. It is now read-only.

Can't set bugsnag.requestData.user in express middleware. #120

Closed
actionnick opened this issue Nov 7, 2017 · 7 comments
Closed

Can't set bugsnag.requestData.user in express middleware. #120

actionnick opened this issue Nov 7, 2017 · 7 comments

Comments

@actionnick
Copy link

I'm basically following the example in the examples directory and getting an error.

Expected behavior

bugsnag.requestData.user = { _id } should work.

Observed behavior

TypeError · Cannot set property 'user' of undefined

Steps to reproduce

In express middleware try to set requestData.

Version

bugsnag version - 2.0.1
node version - 7.9.0

Additional information

After doing some investigation it seems to process.domain is not set. This seems to make requestData not functional after looking at the getters and setters in the source code.

@bengourley
Copy link
Contributor

Hey @actionnick would you be able to put together a reduced test case? Or simply share the code that causes this issue for you? I just checked out the express example to make sure it's still correct, and by my reckoning it is.

Before seeing you code, my thoughts are leading towards the possibility that your bugsnag.requestData.user = { … } call isn't happening within the context of a req/res pair, so for example in the top level scope of the .js file rather than inside a route handler or piece of middleware. At that point process.domain would not be set like it would be in the other contexts.

Let me know!

@actionnick
Copy link
Author

From the research I've done it has to do with how process.domain is handled in native Promises. The code that I was executing is in the callback of a promise which I imagine is quite a common use case for authenticating a user. In that callback process.domain is undefined.

// Fails because executing in promise
Users.findOne(query).then(user => {
  bugsnag.requestData.user = user;
});

Related issues,
nodejs/node-v0.x-archive#8648 (comment)

Apparently it can work with bluebird, but it seems like having a core functionality of this library rely on a deprecated node feature should be changed.

@bengourley
Copy link
Contributor

Hmm, interesting. I've put together the following bare-bones example which manages to operate correctly with promises:

const express = require('express')
const app = express()
const bugsnag = require('bugsnag')

bugsnag.register("API-KEY-GOES-HERE")

const loadUser = req => new Promise((resolve, reject) => {
  let user
  switch (req.query.uid) {
    case '1':
      user = { name: 'jim' }
      break
    case '2':
      return reject(new Error('a bug when input is 2?!'))
    default:
      user = null
  }
  req.user = user
  bugsnag.requestData.user = user
  resolve()
})

const session = (req, res, next) => {
  loadUser(req)
    .then(next)
    .catch(next)
}

app.use(bugsnag.requestHandler)
app.use(session)
app.use(bugsnag.errorHandler)

app.get('/', (req, res) => {
  console.log(bugsnag.requestData)
  res.send(req.user)
})

app.listen(3000)

Hit http://localhost:3000/?uid=1, http://localhost:3000/?uid=2, http://localhost:3000/?uid=3 to make it exhibit a few different behaviours.

I noticed that I got the same error as you when I missed off the request handler middleware (app.use(bugsnag.requestHandler) which must come before any other middleware.


As an aside…

having a core functionality of this library rely on a deprecated node feature should be changed.

We very much agree. I did a bunch of research into this recently and at some point in the foreseeable future, this library will have a sizeable revamp. However, although the domain module is marked as "deprecated", there isn't yet a direct replacement. The core module async_hooks is a recent welcome addition which appears to provide decent primitives upon which to build a direct (but better) replacement for domains. But for the time being at least, domains are (sadly) the best option.

@actionnick
Copy link
Author

I'm going to look into this when I get some time, thanks for looking into this.

@Grmiade
Copy link

Grmiade commented Dec 14, 2017

I have the same problem whereas I setted app.use(bugsnag.requestHandler) in first :/

@bengourley
Copy link
Contributor

Hey @Grmiade are you able to provide a reduced case of this problem?

Let me know:

  • bugsnag version
  • node version
  • any non-native promise lib installed?

Additionally, can you try running the example I posted above and let me know if that works for you?

Cheers

@bengourley
Copy link
Contributor

Closing due to inactivity.

This module is now deprecated. Please upgrade to @bugsnag/js – our new "universal" notifier for Browser and Node JS – when you get the chance. If you're still experiencing an issue like this, feel free to write in to support or open up an issue on that repo. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants