Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Follow standard convention for Arrays #33

Open
coolaj86 opened this Issue · 25 comments

10 participants

@coolaj86

According to common convention (i.e. php, python, ruby) named fields ending with square brackets [] should be treated as arrays.

This example HTTP POST should produce a parsed object like the one shown here below:

Shorter snippet from link above:

-----------------------------114772229410704779042051621609
Content-Disposition: form-data; name="attachments[]"; filename="file1.txt"
Content-Type: text/plain

This is file 1

-----------------------------114772229410704779042051621609
Content-Disposition: form-data; name="attachments[]"; filename="file2.txt"
Content-Type: text/plain

This is file 2
It has three lines
And was created on OS X

Parsed Object:

{
    name: "AJ ONeal",
    email: "coolaj86@email.com",
    // <Object HTML5::FileAPI::File>
    avatar: {
        name: "smiley-cool.png",
        size: 869,
        type: "image/png",
        path: "/tmp/ab489fe1d9a4df.png"
    },
    attachments: [
        // <Object HTML5::FileAPI::File>
        {
            name: "file1.txt",
            size: 15,
            type: "text/plain",
            path: "/tmp/c3e29fa4de1d9f.png"
        },
        {
            name: "file2.txt",
            size: 58,
            type: "text/plain",
            path: "/tmp/9a4ab48dffe1d9.png"
        }
    ]
}
@felixge
Owner

I'm not sure if I want this to be part of formdiable yet. I can see how it's nice, but at the same time it feels like sugar that can easily build on top of formidable. Let me know why you think it's important.

@coolaj86

Perhaps my understanding is incorrect (I'm using express, not formidable directly, so correct me if I'm mistaken), but formidable can't currently accept uploads from multiple input fields

So either using the common convention []

<input type="file" name="attachments[]" multiple=multiple>

Or not

<input type="file" name="attachments" multiple=multiple>

I can't get access to form.attachments[0]. Is this not a bug?

The HTML spec treats all form fields as Arrays. However, that's the same ugly way that xml does it.

I agree that we should not encourage that mistake -- especially since web frameworks have traditionally used the [] convention, and it works well, is simple to implement, and intuitive to understand.

Although it may not be the best solution, but it's what end-developers are accustomed to.

That said, I would be okay with any solution to the bug

  • using the convention of [] to treat the object as an array - this works generally well and I support the web framework community in its use
  • guessing at when an object should be an array - I don't like this because it is inconsistent.
  • making everything an array whether it was intended to be or not - perhaps the best solution, certainly standards compliant, albeit somewhat ugly.

The issue is that when I use a multiple file input form, I should be able to get at all the files.

@tj
tj commented

yeah I dont personally care what the solution is but this is pretty important, [] was the first thing I tried

@felixge
Owner

The issue is that when I use a multiple file input form, I should be able to get at all the files.

The API supports that. You just need to listen for the 'file' event. If express/connect does not expose that, a patch there might be the first thing to consider?

@tj
tj commented

connect-form is just a really thin layer around formidable, I think it is the final callback wit the field hash that ends up being "invalid" with multiples

@felixge
Owner

connect-form is just a really thin layer around formidable, I think it is the final callback wit the field hash that ends up being "invalid" with multiples

The callback is meant as a simplified API which doesn't handle multiple fields with the same name. But I guess connect-form is not meant to "emit" individual file events anyway?

--fg

@coolaj86

So which of the two should change? Express or Formidable?

I would really appreciate formidable handling it appropriately since I won't always be using express when I handle uploads, but I would pretty much always like to use formidable when I handle uploads.

I think that the change will benefit more projects in general if it happens in the formidable layer.

@tj
tj commented

you can still use formidable directly.. connect-form is just a convenience middleware

@ghost

This select tab with multiple fields problem is definitely a bug. I use formidable without express or connect and it is impossible for me to accept same-named fields. I will try the patch provided a few months ago but I'd prefer to use unpatched formidable.

I also vote for adding "[]" to the name as a de-facto standard.

@felixge
Owner

This select tab with multiple fields problem is definitely a bug.

No, you are misunderstanding the current API.

I use formidable without express or connect and it is impossible for me to accept same-named fields.

That's not true. You can listen to the 'file' event of the form, and you will receive one event per file, regardless of the field name.

This doesn't mean that I won't change the API, apparently most people prefer magic over predictable outcomes, but please don't spread FUD.

@ghost

I'm sorry, I didn't know about the event solution. i missed that in the docs. One would like to just have the fields in an obj, not go to the trouble of setting event handlers. It isn't a bug, just a pretty big inconvienence.

@fideloper

For clarity (And I'm aware this is an old thread), you can add and event listen for the "file" event, emitted by formidable, to capture every file rather than use the callback with the formidable.parse() method:
(And please correct me if I'm mistaken)

var form = new formidable.IncomingForm();
form.uploadDir = '/some/path/to/directory';

form.addListener('file', function(file) {
  console.log('File Fired');
  console.log(file);
});

form.parse(req, function(err, fields, files) {
//Code here
}
@FluffyJack

I'm wanting to use formidable with normal arrays of fields (not file fields), so I'm working on a fork now, I found the perfect spot for the code to go. I'll do a pull request in a second when I'm done. It will let you do things like:

<input name="tags[]" value="first tag" />
<input name="tags[]" value="second tag" />
<input name="tags[]" value="third tag" />

And then in the fields parameter of the cb we'll get tags:["first tag","second tag","third tag"] instead of what we're currently getting which is "tags[]":"third tag"

I'll post a comment once I've finished and made the pull request

@FluffyJack

I've submitted the pull request: #77

@shanebo

@felixge, any news on this? I agree with others here that foo[] should return an array...

@felixge
Owner

@shanebo No news yet, but I'll keep you posted. I'm getting to this soon I think

@shanebo

@felixge thank you!

@atheken

I am not exactly, sure, but I think that this is adding to the problems: tj/node-querystring#30

@OrangeDog

@atheken I don't think this ever landed.

@rcprior

@felixge, excuse me for bringing back this old issue, but I think you should take into consideration the following aspects:

(1) Having a form submission (without file fields) yield different results when the form is submitted with different enctype is a big no-no.

(2) The argument for simplicity is weak in this case. If you would return an array only for multi-valued fields, the code for processing forms without multi-valued fields (i.e., the kind of forms that are supported right now) would be exactly the same as now.

(3) The way multi-valued fields are handled right now is also bad for files. If Joe Attacker submits a form with many files having the same field name, the callback only sees the last one, and is unaware of the others. Repeat such submission many times, and the disk could fill up, opening a vector for a DoS attack.

The fix is trivial, and I have already implemented it on my system. The reason I am taking the time to discuss this is that I find formidable to be a very nice piece of work, and this is a simple fix that would make many a programmer's life easier, avoiding the need to reimplement this stuff. Everyone would benefit from having a single form processing module in node that handles all cases well.

@felixge
Owner

if somebody volunteers to make the change and deal with any problems / tickets that result from it, I'm happy to give him commit access

@rcprior

@felixge, thank you very much for your interest. I will happily deal with any tickets resulting from this change -- I am pretty sure there will be few (if any), since the change is trivial and does not break anything on forms without multiple-valued fields.

I have never used git before, but here is a tested patch (to apply with -p1 inside the formidable directory):

--- formidable/lib/incoming_form.js     2013-11-04 17:47:08.407799172 +0000
+++ formidable/lib/incoming_form.js     2013-11-05 22:48:10.272473546 +0000
@@ -80,10 +80,24 @@
     var fields = {}, files = {};
     this
       .on('field', function(name, value) {
-        fields[name] = value;
+        if (fields.hasOwnProperty(name)) {
+          if (!(fields[name] instanceof Array)) {
+            fields[name] = [ fields[name] ];
+          }
+          fields[name].push(value);
+        } else {
+          fields[name] = value;
+        }
       })
       .on('file', function(name, file) {
-        files[name] = file;
+        if (files.hasOwnProperty(name)) {
+          if (!(files[name] instanceof Array)) {
+            files[name] = [ files[name] ];
+          }
+          files[name].push(file);
+        } else {
+          files[name] = file;
+        }
       })
       .on('error', function(err) {
         cb(err, fields, files);

If you prefer that I commit the change myself, please let me know and I will learn how to do it.

@felixge
Owner

@rcprior thanks. The patch looks good. Additionally this change will require:

  • Making sure the existing test suite passes
  • Add some new tests for the new behavior
  • Documenting the new behavior
  • Creating a new npm release (I can handle this)

Given that you're new to git I'll not give you commit access right away to avoid accidents : ). So could you please go ahead and send your change as a pull request? It will require "forking" the repo via the fork button, committing / pushing the change into your forked repo, and then creating the pull request. These guides may help:

Thanks

@rcprior

@felixge, I documented the new behavior (and also fixed some issues in Readme.md) and updated test/legacy/simple/test-incoming-form.js to reflect the new behavior (since it was testing for the behavior of dropping all values of a name but the last), but now I'm stuck with testing errors that are unrelated to my changes (they also occur on a fresh clone of your code):

$ node run.js
[0:00:00 0 0/15 0.0% node integration/test-fixtures.js]

  /home/rprior/git/tmp/node-formidable/test/integration/test-fixtures.js:18
      .sync(common.dir.fixture + '/js')
       ^
  TypeError: Object function walk(dir, opts, emitter, dstat) {
      if (!opts) opts = {};
      var fdir = opts._original || dir;
      opts._original = undefined;

      if (!emitter) {
          emitter = new EventEmitter;
          emitter.stop = function () {
              emitter._stopped = true;
              emitter.emit('stop');
          };
          emitter._pending = 0;
          emitter._seen = {};
      }
      emitter._pending ++;

      if (dstat) {
          var stopped = false;
          emitter.emit('directory', fdir, dstat, function stop () {
              stopped = true;
          });
          emitter.emit('path', fdir, dstat);
          if (!stopped) fs.readdir(dir, onreaddir);
          else check()
      }
      else fs.lstat(dir, function onstat (err, stat) {
          if (emitter._stopped) return;
          if (err) return finish();
          emitter._seen[stat.ino || dir] = true;

          if (stat.isSymbolicLink() && opts.followSymlinks) {
              emitter.emit('link', fdir, stat);
              fs.readlink(dir, function (err, rfile) {
                  if (emitter._stopped) return;
                  if (err) return finish();
                  var file_ = path.resolve(dir, rfile);
                  emitter.emit('readlink', fdir, file_);
                  fs.lstat(file_, onstat);
              });
          }
          else if (stat.isSymbolicLink()) {
              emitter.emit('link', fdir, stat);
              emitter.emit('path', fdir, stat);
              finish();
          }
          else if (stat.isDirectory()) {
              var stopped = false;
              emitter.emit('directory', fdir, stat, function stop () {
                  stopped = true;
              });
              emitter.emit('path', fdir, stat);
              if (!stopped) fs.readdir(dir, onreaddir);
              else check()
          }
          else {
              emitter.emit('file', fdir, stat);
              emitter.emit('path', fdir, stat);
              finish();
          }
      });

      return emitter;

      function check () {
          if (-- emitter._pending === 0) finish();
      }

      function finish () {
          emitter.emit('end');
          emitter._seen = null;
      }

      function onreaddir (err, files) {
          if (emitter._stopped) return;
          emitter._pending --;
          if (err) return check();

          files.forEach(function (rfile) {
              emitter._pending ++;
              var file = path.join(fdir, rfile);

              fs.lstat(file, function (err, stat) {
                  if (emitter._stopped) return;
                  if (err) check()
                  else onstat(file, stat)
              });
          });
      }

      function onstat (file, stat, original) {
          if (emitter._seen[stat.ino || file]) return check();
          emitter._seen[stat.ino || file] = true;

          if (stat.isDirectory()) {
              if (original) opts._original = original;
              walk(file, opts, emitter, stat);
              check();
          }
          else if (stat.isSymbolicLink() && opts.followSymlinks) {
              emitter.emit('link', file, stat);

              fs.readlink(file, function (err, rfile) {
                  if (emitter._stopped) return;
                  if (err) return check();
                  var file_ = path.resolve(path.dirname(file), rfile);

                  emitter.emit('readlink', file, file_);
                  fs.lstat(file_, function (err, stat_) {
                      if (emitter._stopped) return;
                      if (err) return check();

                      emitter._pending ++;
                      onstat(file_, stat_, file);
                      check();
                  });
              });
          }
          else if (stat.isSymbolicLink()) {
              emitter.emit('link', file, stat);
              emitter.emit('path', file, stat);
              check();
          }
          else {
              emitter.emit('file', file, stat);
              emitter.emit('path', file, stat);
              check();
          }
      }
  } has no method 'sync'
      at Server.findFixtures (/home/rprior/git/tmp/node-formidable/test/integration/test-fixtures.js:18:6)
      at Server.g (events.js:175:14)
      at Server.EventEmitter.emit (events.js:92:17)
      at net.js:1052:10
      at process._tickCallback (node.js:415:13)
      at Function.Module.runMain (module.js:499:11)
      at startup (node.js:119:16)
      at node.js:901:3

[0:00:00 1 10/15 73.3% node legacy/simple/test-incoming-form.js]

  /home/rprior/node_modules/gently/lib/gently/gently.js:99
      throw new Error(this._name(obj, method)+' is not gently stubbed');
            ^
  Error: [Object].extname() is not gently stubbed
      at Gently.restore (/home/rprior/node_modules/gently/lib/gently/gently.js:99:11)
      at Object.<anonymous> (/home/rprior/git/tmp/node-formidable/test/legacy/simple/test-incoming-form.js:728:14)
      at Gently._stubFn (/home/rprior/node_modules/gently/lib/gently/gently.js:166:31)
      at Object.delegate (/home/rprior/node_modules/gently/lib/gently/gently.js:82:17)
      at IncomingForm._uploadPath (/home/rprior/git/tmp/node-formidable/lib/incoming_form.js:523:20)
      at testFileExtension (/home/rprior/git/tmp/node-formidable/test/legacy/simple/test-incoming-form.js:736:10)
      at _uploadPath (/home/rprior/git/tmp/node-formidable/test/legacy/simple/test-incoming-form.js:737:5)
      at test (/home/rprior/git/tmp/node-formidable/test/legacy/simple/test-incoming-form.js:22:3)
      at Object.<anonymous> (/home/rprior/git/tmp/node-formidable/test/legacy/simple/test-incoming-form.js:704:1)
      at Module._compile (module.js:456:26)

[0:00:01 2 13/15 100.0% node legacy/system/test-multi-video-upload.js]
@felixge
Owner

@rcprior not sure what the problem is, the master tests seem ok on node 0.8, 0.10 and 0.11 on Linux. https://travis-ci.org/felixge/node-formidable

@jonlil

Why don't make a joint effort to get this trivial code merged, 3 years and still open?
Cocopods AFNetworking module sends their data as comments[][text]

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.