bithavoc / express-winston

express.js middleware for winstonjs
https://www.npmjs.com/package/express-winston
MIT License
797 stars 187 forks source link

Setting requestWhitelist doesn't seem to work #92

Closed slooker closed 8 years ago

slooker commented 8 years ago

I can add to requestWhitelist like the docs suggest, but if I remove from requestWhitelist, it doesn't seem to work.

I've tried setting the white list like so:

expressWinston.requestWhitelist = [
  "url"
];

But the whitelist on line 216 of index.js still has the full requestWhitelist, so I'm still getting all of the data.

satyadeepk commented 8 years ago

I am facing the same issue. Have you found any workaround?

floatingLomas commented 8 years ago

That makes no sense. But you're correct. The test suite also doesn't test for being able to edit/change this.

I'll try to look at this at some point, or you're more than welcome to fork and submit a pull request.

aggied commented 8 years ago

same problem for me. Am hoping to just be able to set the array and be done w/ it.

rosston commented 8 years ago

There is a workaround. The trick is that you always need to modify the existing requestWhitelist array, without actually reassigning the variable.

Here's an example:

// what you want to do
expressWinston.requestWhitelist = ["url", "foo"];

// what will actually work
expressWinston.requestWhitelist.splice(0);
expressWinston.requestWhitelist.push.apply(expressWinston.requestWhitelist, ["url", "foo"]);

Yes, this is a dirty hack. I'm working on fixing this properly, and I should have it out in a day or two.

trvsdnn commented 8 years ago

Any reason this can't just be passed into the constructor with all of the other options?

rosston commented 8 years ago

No, the whitelists and blacklists cannot simply be options to pass to the middleware factory because there's more than one middleware factory, both of which I would expect are typically used together in the same app. And presumably the express-winston user wants the whitelists and blacklists to be consistent across their regular logs and error logs.

nwah commented 8 years ago

One way to make it a little more comfortable to replace the whitelist would be something like this:

var expressWinston = require('express-winston');
expressWinston.config.requestWhitelist = [...];
expressWinston.config.ignoredRoutes = [...];

Or

var expressWinston = require('express-winston');
expressWinston.whitelists.request = [...];
expressWinston.blacklists.body = [...];
expressWinston.ignored.routes = [...];

By exposing them under an intermediate object/property, people could replace them in a more natural way. Shouldn't be too hard to implement under the hood either, and could also easily be implemented to be backward compatible with the current implementation.

rosston commented 8 years ago

@nwah I planned on simply making the *Whitelist and *Blacklist exported properties assignable (as the OP expected) rather than needing to keep the reference intact. I feel like simple assignment to the module properties is the naturally expected behavior given the documentation in the Readme around push-ing to those arrays.

Your config object suggestion does seem a little more consistent with how other node modules work, but I would still lean toward simple assignment for backward compatibility/API consistency. However, I could be convinced otherwise. Perhaps I'm missing something?

rosston commented 8 years ago

Published in v1.3.0.