use parted instead of formidable for multipart parsing #673

Closed
andrewrk opened this Issue Oct 14, 2012 · 21 comments

Projects

None yet

4 participants

@andrewrk
Contributor

It looks a lot better. What went into the choice to use formidable, and what would it take to get you to switch to parted?

What if I ported formidable's test suite over to parted and verified that it is as fast or faster in benchmarks? Would that be enough?

@langpavel
Contributor

Is parted maintained? Last commit 9 months ago..
What is benefit of parted over formidable?
Nobody force you to use formidable, do you agree?

@langpavel
Contributor

Oh, you are maintainer of formidable, this changes situation dramatically :-)

@andrewrk
Contributor

I'd rather maintain parted than formidable.

Nobody force you to use formidable, do you agree?

Connect uses formidable for multipart parsing. So yes, connect forces you to use formidable, given that you want to use connect for multipart parsing.

@tj
Member
tj commented Oct 14, 2012

well it's easy to just not use .multipart() and swap in your own middleware, that's the whole point of middleware really. formidable is robust and well tested through felix's transloadit, I'm fine with seeing some numbers but faster != better, I think parted even includes a connect middleware IIRC

@andrewrk
Contributor

Let me put this a different way:

I was just made maintainer of formidable. Upon auditing the code, I made several discoveries:

  1. There are multiple security issues not caught by test cases.
  2. The code is sloppy and poorly thought out.
  3. I planned out what changes it would take to shape up formidable into something respectable, and then I looked at parted, and realized that parted is exactly where I would end up.
  4. I am not going to maintain formidable when an alternative exists which already fixes 80% of the issues.
@langpavel
Contributor

That is reasonable. What about API, are there incompatibilities?
Can be provided small backward compatibility API?

@tj
Member
tj commented Oct 14, 2012

hmm seems odd if he's not going to maintain it, I think that's a pretty crucial portion of his business unless he's moved to something else, im not sure, but yeah let's maybe do a patch and see how things go. I helped a tiny bit on parted a long time ago but ended up going with formidable

@sjonnet19

My only requirement would be that parted handle nested JSON as formidable.

@tj
Member
tj commented Oct 15, 2012

@sjonnet19 it's actually connect that does that part, formidable just gives you part events or the "shallow" object

@sjonnet19

That makes sense though I remember when I last used parted the deep objects where not converting properly. Maybe it was just a connect talking to parted issue?

@tj
Member
tj commented Oct 15, 2012

no clue, maybe parted isn't doing it correctly at the moment, haven't tried it in ages

@andrewrk
Contributor

OK. Here's what I'll do:

  1. Fork parted. Import formidable's test and benchmark suite.
  2. Work out any bugs.
  3. Create a pull request to connect using parted instead of formidable for multipart middleware.
@tj
Member
tj commented Oct 15, 2012

sounds good

@langpavel
Contributor

+1 formidable is relatively complex

@andrewrk
Contributor

Initial results:

parted has some issues too but I've worked them out. I'm in the process of porting over formidable's test suite.

I ported over the benchmarking script, and here are some results of before I do anything to parted:

formidable averages 1219.51 mb/sec
parted averages 497.51 mb/sec

I still want to go with parted's codebase due to the code cleanliness, but it looks like my fork has diverged enough that I might have to call it a new project.

I'll do the optimizations to the parser that make sense, and if it's still slower than formidable then I'll look into using formidable's parser, or maybe even look into using felixge's faster but also also unmaintained multipart parser.

@langpavel
Contributor

Is there a chance to merge with parted (raise up minor version or something..)? If parted has issues, should be fixed there...

@andrewrk
Contributor

By all means, I'm going to make this a pull request to parted, but in my experience when you change too much stuff the original author gets defensive.

@langpavel
Contributor

Yes, It's hard to review..

@andrewrk
Contributor
andrewrk commented Apr 4, 2013

https://github.com/superjoe30/node-multiparty

I took the time to rewrite formidable.

  • use the Node.js v0.10 streams properly, even in Node.js v0.8
  • It will not create a temp file for you unless you want it to.
  • simpler, more maintainable code. less baggage
@andrewrk
Contributor
andrewrk commented Sep 7, 2013

solved with 296398a

@andrewrk andrewrk closed this Sep 7, 2013
@tj
Member
tj commented Sep 7, 2013

sweet thanks for finding the issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment