Optional custom json replacer/spaces per response

[rubenstolk]

In some cases it is useful to be able to overwrite the json replacer at response level, in the end it is something related to the response.

This change also makes it possible to simply say: res.jsonReplacer = null; res.json(data); to prevent the jsonReplacer from kicking in.

[dougwilson]

Please come up with a different API, as adding a magical property to res is not what we want.

Please also read through a previous request at #2098 and, if you can, provide a sample use-case for what the per-request replacer function you are trying to use.

[rlidwka]

Here is a better idea:

app.get('/', function() {
  res.body = { '...': 'any json you want' } // use that instead of res.json()
  res.jsonReplacer = function(){ /* whatever */ }
  next()
})

app.use(function(req, res, next) {
  if (!res.body) return next()
  res.send(JSON.stringify(res.body, res.jsonReplacer))
})

Which doesn't require any changes to the core at all.

[rubenstolk]

I don't think we want to use res.send for json.

[dougwilson]

Hi @rubenstolk , have you had time to come up with a different API yet?

[rubenstolk]

Not yet, but I will, hopefully have some time this weekend.

Ruben Stolk

CHANGER, Raising the bar in Online Experience!

+91 95 6105 0034
+31 6 46 11 80 37

INDIA OFFICE
308 Sky Max, Datta Mandir Chowk, Viman Nagar, Pune – 411014, India (
http://goo.gl/maps/muZw7 http://g.co/maps/g9y7n)
+91 20 65 7373 33

NETHERLANDS OFFICE
Maerten Trompstraat 25, 2628 RC Delft, Netherlands (http://goo.gl/QQHfWE
http://g.co/maps/tcpwn)
+31 88 1001300

http://changer.nl

On Fri, Oct 31, 2014 at 7:06 PM, Douglas Christopher Wilson <
notifications@github.com> wrote:

Hi @rubenstolk https://github.com/rubenstolk , have you had time to
come up with a different API yet?

—
Reply to this email directly or view it on GitHub
#2422 (comment).

[rlidwka]

If you really think it is a good idea to add it to express itself, the best api as of now would be this:

res.set('json replacer', function(){})
res.json(data)

Thus, allowing to override any app.set() with res.set() for an individual request.

I'd totally went for it in express@3. But express becomes more modular nowadays, and now I doubt that replacer is even worth having here.

[Fishrock123]

I don't like the idea of res.set('json replacer', function(){})

Mainly because set/get probably will not not exist in 5 and this is additional overhead.

Can we do res.json(data, replacer)?

[dougwilson]

Can we do res.json(data, replacer)?

res.type('json').send(JSON.stringify(data, replacer))

Otherwise res.json(data, replacer) is ambiguous, because data could be a number and replacer could be null, which is why they form has been turned down before.

[Fishrock123]

Otherwise res.json(data, replacer) is ambiguous, because data could be a number and replacer could be null, which is why they form has been turned down before.

em, pardon?

Isn't json(num, data) being removed in 5.x?

[dougwilson]

I was under the impression this PR was a feature request for 4, since that is where is PR is based. @rubenstolk what version are you requesting this feature in?

[Fishrock123]

@dougwilson I'm not sure it matters.

I'm wondering where we can implement the most reasonable interface for the feature.

[dougwilson]

@Fishrock123 I was just saying your suggestion is impossible to implemented in 3/4 is all, as it will break current (deprecated) uses of res.json.

What is wrong with res.type('json').send(JSON.stringify(data, replacer))?

[dougwilson]

Plus I'm still waiting the hear what the use-case is for a per-request json replacer. The provided use-case from the last time this was requested was fundamentally flawed and made no sense, which contributed to why it wasn't added.

[Fishrock123]

What is wrong with res.type('json').send(JSON.stringify(data, replacer))?

Nothing actually. I forgot that this was actually stringifying it. >_<

This is the most reasonable approach yeah. I don't think we should add it regardless of use case.

[rubenstolk]

Guys, I wasn't aware of the fact that we could use .get and .set on res as well, so I've changed that at least. I think current solution is really good and pragmatic. You can now either set the json replacer on app level or on response level.

My use case:

• We have an app that has a 'global' json replacer that strips out certain unwanted properties from all of our models (for example, mongoose models icw toObject always results in __v and id properties which we do not want in our json), so we strip them out.
• Now, on one of our api's (a legacy one used by and older production version of a client) needs to have the id property. Since we currently only have the option to define json replacer at app level, we can't override this. (Except using the custom res.send as suggested by some people above, but I really don't like that because our framework is fully convention based and our generic api-layer takes care of the res.json).
• My preferred suggestion: accept this pull request, it doesn't harm anything but allows more flexibility.
• My alternative suggestion: remove json replacer from app level, but come up with an alternative to make the behavior of res.json overridable. Currently the res.json method does not allow for customizing the behavior at all, which is limiting imho.

[dougwilson]

Guys, I wasn't aware of the fact that we could use .get and .set on res as well, so I've changed that at least.

You... can't. It was a suggestion for a possible implementation, but you'd have to fully implement res.set and res.get...

[dougwilson]

res.get is an alias to res.getHeader. This wouldn't be stored as a header value, besides the fact that Node.js will also ignore your request to res.set() a function...

[rubenstolk]

OK got it, I'll come up with something else then, give me more time.

[dougwilson]

Uhm, why is it working then?

Because res.get('json replacer') calls res.getHeader('json replacer') which is just returning undefined, since there is no json replacer header set. You never tried to add tests for the feature you added to notice?

[dougwilson]

Using your code right now, the following app:

var express = require('./index')
var app = express()

app.set('json replacer', function (key, value) {
  if (key === 'id') return
  return value
})

app.use(function (req, res) {
  res.set('json replacer', function (key, value) {
    if (key === 'secret') return
    return value
  })

  res.json({
    id: 3,
    secret: 'keyboardcat',
    name: 'loki'
  })
})

app.listen(3000)

Results in the following HTTP response:

$ curl -i http://localhost:3000/
HTTP/1.1 200 OK
X-Powered-By: Express
json replacer: function (key, value) {if (key === 'secret') returnreturn value}
Content-Type: application/json; charset=utf-8
Content-Length: 38
ETag: W/"26-3a6fc2e6"
Date: Sat, 01 Nov 2014 03:55:17 GMT
Connection: keep-alive

{"secret":"keyboardcat","name":"loki"}

Notice how the json replace I tried to set with res.set not only didn't apply to the body (because the "secret" key is still there), but the body of the function appears as a response header.

[dougwilson]

Oh, sorry @rubenstolk I didn't realized you replied. You can just make a new comment rather than replying by completely replacing the content of an old comment, which is really confusing and doesn't let me know you made a reply.

[dougwilson]

Oh, and FYI, to me the use-case for adding this I guess sounds valid, though using express to do your DTO -> DTO transformations seems a little heavy-handed to me.

[rubenstolk]

Well, I'm not using express per se for it, but express has a json replacer setting which is meant for this purpose right?

[rubenstolk]

@dougwilson What do you think about the current approach? A separate 'createJSON' method which is used by both res.json and res.jsonp. Now we can just override createJSON on res if needed which will solve my need.

[rubenstolk]

Interested to hear if this is going to work out or not. FYI, currently I'm living with this patch in my code:

  // TODO: this is a non desired override because it might not survive express updates
  // Hopefully https://github.com/strongloop/express/pull/2422 works out
  app.response.__proto__.json = function (obj) { // jshint ignore:line
    var replacer = typeof this.jsonReplacer !== 'undefined' ?
      this.jsonReplacer : this.app.get('json replacer');
    var spaces = typeof this.jsonSpaces !== 'undefined' ?
      this.jsonSpaces : this.app.get('json spaces');
    var body = JSON.stringify(obj, replacer, spaces);
    if (!this.get('Content-Type')) {
      this.set('Content-Type', 'application/json');
    }
    return this.send(body);
  };

[dougwilson]

So, it's still open because we need to address this (the issue about the replacer/spaces scope) in some way, but I'm not sure the current implementation in the PR would work out, i.m.o.

[jczaplew]

While I don't have a use for setting json replacer on the response level, I do have a use for setting json spaces. I'm not sure there is a great way to implement this without messing up the API but, I'm thinking about something along the lines of

res
    .config({
        "json replacer" : function() {...},
        "json spaces" : 0
    })
    .json(data);

Basically, .config could be used to set properties of the response that are not headers. Do you think something like that would be consistent enough with the API?

[jczaplew]

Likewise, I could see something like

res
    .config("json spaces", 0)
    .json(data);

[Radagaisus]

+1. This would be very helpful when escaping output from certain requests, and not from others.