chjj / parted

Streaming body parser for node.js.
MIT License
63 stars 14 forks source link

consider visionmedia/node-querystring (qs in npm) for parsing #4

Closed tj closed 12 years ago

tj commented 12 years ago

tough call, but it would be nice to have a drop-in replacement for bodyParser so parted can handle both in the same manner

chjj commented 12 years ago

Yeah, the querystring parser has gotten the least love out of the three. It is my goal to have parted more or less be a streaming version of the connect body parser. I'll give it a shot soon I think.

tj commented 12 years ago

yeah understandable, I just question if it's really worth streaming those bodies, especially since they're buffered in the middleware anyway. Just like working with streaming JSON is (usually) relatively useless

chjj commented 12 years ago

Ah yeah, it's something I've thought about quite a bit. It depends on what the JSON looks like I guess. A possible benefit would be slightly less memory usage, but there's probably a certain threshold to hit before there is any gain, and I don't know how often people are passing around >100kb of json. The json parser does let people to hook into events for each value instead of using middleware, but you're right, I don't know how useful it is when it's just used as middleware. Despite all that, I do like my json parser, I think I'd want to find a place to put it before I got rid of it in parted.

parted was originally just a multipart parser. I wrote the others because I could, and because I wanted a unified interface for a body parser, where I didn't have to worry about two different middlewares messing with each other by both setting req.body.

tj commented 12 years ago

maybe those should be options too, buffer: true would just do what bodyParser does without clashing with it. yeah that's exactly it, you can DoS someone easier with bodyParser but people should limit those anyway ideally.

I agree that's a nice thing, a lot of people are confused with connect-form / using formidable (or parted streaming i suppose) directly so it's nice to have but IMO the nesting thing is necessary

chjj commented 12 years ago

Yeah, I'm thinking maybe buffer should be the default, and have the streaming json and encoded parser load lazily depending, or maybe drop them, not sure. Full compatibility with the connect bodyparser is nice, and buffering seems more practical, and generally faster. I'll create a branch for this and mess around some.

tj commented 12 years ago

awesome sounds good, I know this will help people a ton

chjj commented 12 years ago

https://github.com/chjj/parted/compare/no_stream https://github.com/chjj/parted/compare/gutted

The first defaults to non-streaming parsers, the other removes the streaming parsers altogether. It does seem cleaner, less code, less to maintain. I'm considering going that route.

codedmart commented 12 years ago

chjj I would really like this it would be huge.

chjj commented 12 years ago

b5a282ffd0ee2f07562ea603e3c5a953d7eaa98c

I'll push it to npm in a little bit.

codedmart commented 12 years ago

I just installed the latest 0.8 and I do not see a difference in the parsing. Do I need to change a config setting or something?

Thanks

chjj commented 12 years ago

Do I need to change a config setting or something?

No, it should just work. Make sure qs is installed. It will fallback to the regular node module if you don't have qs.

codedmart commented 12 years ago

I have qs installed. In bodyParser I get this:

{ _csrf: 'FsUImtPfWn2zi2sZ9D6TI17P',
  'account: {
    company_name: 'Data',
    broker: 'Data',
    address: 'Data',
  },
  user: {
    city: 'Data',
    state: 'Data',
    zip: 'Data'
  }
  'user[state]': 'WA',
  'user[zip]': '99019',
  commit: 'SUBMIT >>' }

with parted I get this:

{ _csrf: 'FsUImtPfWn2zi2sZ9D6TI17P',
  'account[company_name]': 'Data',
  'account[broker]': 'Data',
  'account[address]': 'Data',
  'user[city]': 'Data',
  'user[state]': 'Data',
  'user[zip]': 'Data',
  commit: 'SUBMIT >>' }
chjj commented 12 years ago

Are you sure? I did a fresh npm install just to check, and it works fine when I have fields named: test, user[name], and user[pass].

Output:

{ test: 'asdasd',
  user: { name: 'adasd', pass: 'asdasd' },
  send: 'Submit' }
codedmart commented 12 years ago

Ok I will check again. On Nov 10, 2011 1:32 PM, "Christopher Jeffrey" < reply@reply.github.com> wrote:

Are you sure? I did a fresh npm install just to check, and it works fine when I have fields named: test, user[name], and user[pass].

Output:

{ test: 'asdasd', user: { name: 'adasd', pass: 'asdasd' }, send: 'Submit' }


Reply to this email directly or view it on GitHub: https://github.com/chjj/parted/issues/4#issuecomment-2701441

codedmart commented 12 years ago

I did a fresh npm install and am still having the problems. Not sure what it could be.

chjj commented 12 years ago

Do you see qs in the parted/node_modules directory? If I had to guess, it's that qs isn't getting resolved correctly, and parted is falling back to node's querystring module. If you comment out the try/catch at the beginning of lib/index.js, you'll be able to see whether qs is found.

codedmart commented 12 years ago

Ok so the problem is this is a multipart form. If I do this on a normal form it works. Can I not get the same parsing on a multipart form?

Thank you

chjj commented 12 years ago

No, it won't work on multipart. The only thing that has changed in anyway meaningful for the user is the querystring parser.

That's a good idea though. I want to look into implementing it.

chjj commented 12 years ago

beaffe7

Won't be pushing that to npm right away, but it seems to work and I like the consistency.

codedmart commented 12 years ago

Awesome! I don't see any problems. This is great. Thanks

chjj commented 12 years ago

Added the streaming parsers back, but they sit behind a stream option. The streaming encoded parser also handles nested fields now.

tj commented 12 years ago

does it support all the same syntax? images[]=foo&images[]=bar etc?

chjj commented 12 years ago

Yeah, it should. One thing that was strange behavior-wise was something like: a[][]=1&a[][]=2. Is that supposed to be { a: [ [1], [2] ] } or { a: [ [1, 2] ] }? I could get it either way, but qs doesn't seem to like fields like that. I doubt anyone is doing something like that though.

tj commented 12 years ago

who knows haha