bower / registry

The Bower registry
https://registry.bower.io/packages
MIT License
292 stars 66 forks source link

regain curl support after express.bodyParser() deprecation #83

Closed kentwills closed 10 years ago

kentwills commented 10 years ago

This is an expedited request to get functionality back. Tests to follow.

https://github.com/bower/registry/commit/f894b576da046d3d508b17b88672e152db4a39b7

kentwills commented 10 years ago

@wibblymat

rayshan commented 10 years ago

Hi @kentwills, thanks for the PR. I replied to the commit you referenced, but here it is again:

What's your expected output?

Here's what I got:

❯❯❯ curl http://bower.herokuapp.com/packages -v -F 'name=jquery' -F 'url=git://g                          ithub.com/jquery/jquery.git'
* Hostname was NOT found in DNS cache
*   Trying 54.235.95.213...
* Connected to bower.herokuapp.com (54.235.95.213) port 80 (#0)
> POST /packages HTTP/1.1
> User-Agent: curl/7.35.0
> Host: bower.herokuapp.com
> Accept: */*
> Content-Length: 271
> Expect: 100-continue
> Content-Type: multipart/form-data; boundary=------------------------4835d158c6                          35496d
>
< HTTP/1.1 100 Continue
< HTTP/1.1 403 Forbidden
* Server Cowboy is not blacklisted
< Server: Cowboy
< Connection: keep-alive
< X-Powered-By: Express
< Content-Type: text/html; charset=utf-8
< Content-Length: 26
< Vary: Accept-Encoding
< Date: Tue, 29 Jul 2014 17:06:05 GMT
< Via: 1.1 vegur
* HTTP error before end of send, stop sending
<
* Closing connection 0
Package already registered%                 

Also I believe we're duplicating middleware with your patch, see: http://stackoverflow.com/a/22145007

kentwills commented 10 years ago

Hey @rayshan @wibblymat, I expect this server logging message: http://bpaste.net/show/ErOiY8dP2h6aS3PPyvnS/ I get this server message: http://bpaste.net/show/Ey6zNddszOpvAiCKtFKo/

I have updated the PR to not duplicate middleware and added a test to ensure this problem does not happen again. Furthermore, I re-structured the current tests.

Also, http://stackoverflow.com/a/22145007 is exactly our problem:

"If you only specify json and urlencoded middlewares, the form data won't be parsed by any middleware, thus req.body won't be defined. You then need to add a middleware that is able to parse form data such as formidable, busboy or multiparty"

https://github.com/kentwills/registry/blob/master/lib/routes/packages.js#L114-L115 has a dependency on req.body

The more technical answer is curl -F sends a multipart request (execute curl with --data and the command succeeds on the current version):

 -F, --form <name=content>

This causes curl to POST data using the Content-Type multipart/form-data according to RFC 2388. 

Not having request.multipart() means node does not know how to process this curl request.

rayshan commented 10 years ago

Thanks @kentwills. It kinda makes sense to me. So this hasn't been a problem because curl method wasn't used frequently? I'm not sure why bodyParser was removed in the first place. I'll have to defer to other maintainers.

Tests are failing btw, I think it's something w/ the way timeout is used.

kentwills commented 10 years ago

@rayshan,

Since the curl method is listed as the first option in the readme (https://github.com/bower/registry/blob/master/README.md) for creating Bower packages, it should be supported in the code base. bodyParser is apparently deprecated, so this is why they probably were attempting to remove it. I replaced bodyParser with the three packages it represents.

Timeout issue is now fixed.

rayshan commented 10 years ago

@kentwills I see why you need timeout now, I had to do the same thing.

Changes make sense. I recommend merging this.

rayshan commented 10 years ago

@kentwills sorry to bother you with this, but a few things had to happen before this PR. I'd like to merge it. Could you be so kind to rebase your PR?

kentwills commented 10 years ago

@rayshan done

kentwills commented 10 years ago

@rayshan I just updated the structure for the tests, it is not exactly what you had commented above, but I think it meets the standard.

rayshan commented 10 years ago

Thank you @kentwills. Sorry for the delay on this. FYI I do plan to swap express out with hapi. We're very behind on express anyway.

kentwills commented 10 years ago

@rayshan np, thanks for getting this merged!