villadora / express-bunyan-logger

bunyan logger middleware for express
139 stars 74 forks source link

fix req.headers from always becoming "[CIRCULAR]" #39

Open andyto opened 8 years ago

andyto commented 8 years ago

req-headers is always "[CIRCULAR]" due to Bunyan's safeCycle() during JSON.stringify. Deep clone req.headers to req-header to overcome the same object check in safeCycle()

andyto commented 8 years ago

Right, that seems to be a better function choice.

marcbachmann commented 8 years ago

Can you show an example how to reproduce that issue? Is it really the responsibility of this library? If so, we'll also need a test for it.

andyto commented 8 years ago

The following will produces a "[CIRCULAR]" in req.header in the output in console.

I am not sure what test is necessary in this case, thank you.

// express-bunyan-logger v1.3.1
// bunyan v1.8.1

const express = require('express')
const expressBunyanLogger = require('express-bunyan-logger')

const app = express()

app.use(expressBunyanLogger())

app.get('*', function (req, res) {
  res.send('test')
})

app.listen(8080)
jquatier commented 7 years ago

Ran into this as well. Looks like the headers are already output as req-headers so req.headers is just a duplicate anyway. It was a bigger problem for me because it broke some downstream log indexing that expects headers to be an object and not a string. Ended up using a different middleware to accomplish the same goal.

thefotios commented 7 years ago

This should test the PR appropriately (it fails before the PR and passes afterwards).

describe('test req.headers', function() {
  var app, output;

  beforeEach(function() {
    app = express();
    output = st();
  });

  it('is not circular', function(done) {
    app.use(bunyanLogger({
      stream: output
    }));

    app.get('*', function(req, res) {
       res.send('test');
    });

    request(app)
      .get('/')
      .expect('GET /', function(err, res) {
        var json = JSON.parse(output.content.toString());
        assert.notEqual(json.req.headers, '[Circular]');
        done();
      });
   });
});
thefotios commented 7 years ago

This brings up another question/problem (potentially outside the scope of this PR).

After this PR, it appears as thought the req-headers object is empty (unless there is something I'm missing). res-headers does appear to be a proper object.

However, both req and res have a headers and header property (respectively). res.header is the exact string returned, whereas req.headers is the parsed object. Should these be aligned/consolidated in some way?

{
  ...
  // NOTE: req-headers is empty object
  'req-headers': {},
  'res-headers':
   { 'x-powered-by': 'Express',
     'content-type': 'text/html; charset=utf-8',
     'content-length': '4',
     etag: 'W/"4-CY9rzUYh03PK3k6DJie09g"' },
  req:
   { method: 'GET',
     url: '/',
     headers:
      // NOTE: This is a parsed object
      { host: '127.0.0.1:43795',
        'accept-encoding': 'gzip, deflate',
        'user-agent': 'node-superagent/2.3.0',
        connection: 'close' },
  },
  res:
   { statusCode: 200,
     // NOTE: This is a raw string
     header: 'HTTP/1.1 200 OK\r\nX-Powered-By: Express\r\nContent-Type: text/html; charset=utf-8\r\nContent-Length: 4\r\nETag: W/"4-CY9rzUYh03PK3k6DJie09g"\r\nDate: Tue, 03 Jan 2017 16:11:43 GMT\r\nConnection: close\r\n\r\n'},
}
snowyu commented 7 years ago

No need to change if you use the bunyan-log,