Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

removal of bodyParser() middleware seems bothersome #46

Closed
searls opened this Issue · 13 comments

4 participants

@searls
Owner

cc/ @davemo

I struggled to write a stub endpoint that could read POST params. req.body was undefined b/c the bodyParser() middleware had been removed. I had to add this to my lineman project:

var express = require('./../node_modules/lineman/node_modules/express');

module.exports = {
  drawRoutes: function(app) {
    app.use(express.bodyParser());

    app.post('/accounts', function(req, res){
      console.log("Creating a user for", req.body);
      res.send(201);
    });
  }
};

Is there another way to do this? Requiring users know about this middleware in order to get at POST params seems way too difficult.

@davemo
Owner

Well, we ended up removing this because the way we have our proxy configured it wasn't forwarding HTTP PUT properly. What i'm struggling to remember if this was a bug in express, or the http-proxy module (I'm leaning towards the latter).

I think this piece of functionality is pretty advanced and I don't think many people are using it (yet); it doesn't strike me as odd that you would need to know about how express works in order to use this feature in the first place, which leads me to think your solution seems reasonable.

@kbaribeau
Collaborator

The bodyParser() bug is described in some detail here: https://github.com/kbaribeau/node-proxy-timeout-test

I've been meaning to submit a fix to the http-proxy module, but haven't got around to it.

@davemo
Owner

Ah yes, I forgot about that test repo, thanks for chiming in @kbaribeau :D

@kbaribeau
Collaborator

... and by 'submit a fix to the http-proxy module' I mean ' submit a fix to wherever the urlencoded middleware is defined'.

@searls
Owner

I'm not really satisfied with leaving this up to the user. I think reading in POST params is, from a user's perspective, one of the first things they'll run into.

Not to mention that my workaround only works b/c I happen to not be using the proxy feature.

Sad face.

@kbaribeau
Collaborator

Is your post JSON? That's probably the most common case, and we should be able to re-add the json middleware (which sets req.body) without breaking anything.

This would mean are proxy isn't completely dumb anymore, but the JSON middleware is both reasonably unintrusive and stable. You could make a similar argument for some other middlewares as well (so long as they don't do request buffering) but let's not go crazy adding a bunch at once.

The catch is that we're leaving out the multipart and urlencoded middlewares, which means if you're using one of those content types req.body will still be unset. So we'll still have this issue in those cases.

edit: Of these three middlewares, it's only the urlencoded one that seems to do the buffering that's causing us problems. I should really ping someone who knows more about them and ask about this. I'll see if I can jump onto IRC tomorrow and ask around.

@swalke16

Has there been any more progress on this issue? I've been banging my head on this one for a couple hours before I found this.

@kbaribeau
Collaborator

I don't think we ever agreed on an answer that everyone liked. I haven't been using lineman much in the past 5 months, so I let it slip.

I think my comment about re-adding the JSON middleware still stands though. What happens if you add app.use(express.json()) to your config/server.js ?

Btw, the relevant commit in lineman is 73e3efb. Also see my node-proxy-timeout-test repo that shows why we made the change in the first place (if you haven't already).

@searls
Owner
@swalke16

FYI per @kbaribeau's comment above just adding the express.json did work in my case as I'm sending JSON requests.

@kbaribeau
Collaborator

Actually now I'm a bit confused.

I'm looking at 73e3efb and seeing that I did still included the JSON middleware in that commit, but somehow it got removed later.

I say we re-add it, but leave out bodyParser(), since it seems to cause timeout issues with POSTs when proxying at least rails, sinatra and python/flask.

@searls
Owner

Hey @kbaribeau & @davemo it looks like the nodejitsu folks caught onto this (see also their linked issue). https://github.com/nodejitsu/node-http-proxy#post-requests-and-buffering

First I've seen of this.

@davemo
Owner

So it seems like this can be solved with a workaround by simply defining the middleware before the bodyParser: nodejitsu/node-http-proxy#180 (comment)

@searls searls closed this issue from a commit
@searls searls fixes #46 (finally!)
Reproduced this with a super simple POST request,
was much easier to reproduce once we knew that
ordering was the issue.

``` ruby
require 'sinatra'

post '/hi' do
  puts params
end
```

Ran that server, connected the bodyParser at the
top of the express setup, then tried this from
console:

``` javascript
$.post('/hi', {wtf: true})
```

And sure enough, it hung forever.

Moved the bodyParser beneath the proxying setup
and it worked fine.

yay!
eb8b6c7
@searls searls closed this in eb8b6c7
@jasonkarns jasonkarns referenced this issue from a commit
@jasonkarns jasonkarns Merge branch 'master' into grunt-contrib-watch
* master: (23 commits)
  0.13.0
  add quotes around paths
  Upgrade all the things. See #117
  0.12.5
  Upgrade jasmine-given
  Add link to 'Using Ember.js with Qunit' to README.md
  0.12.4
  update jasmine-stealth
  0.12.3
  Augments app.routes to insert bodyParser - this approach leaves apiProxy'd endpoints alone, and prevents issues with the bodyParser messing up proxy'd endpoints - this allows both proxy'd and stubbed endpoints to exist and operate in harmony :)
  0.12.2
  Fix bodyParser() by drawing routes in configure()
  wtfdebuggershowdotheyworklolbbq
  fixes #46 (finally!)
  0.12.1
  adds js+css to the default pages template context
  Fix the note in the readme
  0.12.0
  Implements a nifty `config` reader
  Print out help if the user only enters `lineman`
  ...

Conflicts:
	package.json
a484581
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.