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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

andyto
Copy link

@andyto andyto commented Aug 9, 2016

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()

@@ -126,7 +126,7 @@ module.exports.errorLogger = function (opts) {
'response-time': responseTime,
"response-hrtime": hrtime,
"status-code": status,
'req-headers': req.headers,
'req-headers': JSON.parse(JSON.stringify(req.headers)),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh. I don't like to merge that. This has a huge performance impact.
Can you try to use Object.create(req.headers) or Object.assign({}, req.headers)` instead?

@andyto
Copy link
Author

andyto commented Aug 9, 2016

Right, that seems to be a better function choice.

@marcbachmann
Copy link
Collaborator

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
Copy link
Author

andyto commented Aug 10, 2016

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
Copy link

jquatier commented Dec 1, 2016

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
Copy link

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
Copy link

thefotios commented Jan 3, 2017

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
Copy link

snowyu commented Jan 6, 2017

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

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

Successfully merging this pull request may close these issues.

6 participants