Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added support for arrays in 'data' #40

Closed
wants to merge 2 commits into from

Conversation

stask
Copy link

@stask stask commented Nov 8, 2013

Support for sending multiple values for a field (for example, tags).

@stask
Copy link
Author

stask commented Nov 8, 2013

I saw the #38 pull request and agree that it's better to use JSON.stringify for more complex objects. I still think that arrays needs to be supported (and it's a very simple fix).

@danialfarid
Copy link
Owner

An array is not a valid JSON object. It is better to send your data to the server in a valid JSON format.
I am gonna add a check to do JSON.stringify() for the values that are not string when adding to formData

@stask
Copy link
Author

stask commented Nov 9, 2013

Not sure I understood your comment.
As far as I know, FormData uses multipart/form-data as encoding.
It's possible to have multiple entries with the same name in the form data.
All I did is support for that particular case: fields with multiple values.

/stask

On 9 בנוב 2013, at 00:46, danialfarid [email protected] wrote:

An array is not a valid JSON object. It is better to send your data to the server in a valid JSON format.
I am gonna add a check to do JSON.stringify() for the values that are not string when adding to formData


Reply to this email directly or view it on GitHub.

@elgs
Copy link

elgs commented Nov 10, 2013

@stask why not just use JSON and keep the protocol semantically consistent? JSON is very easy to build and parse and it saves you a lot of time building and parsing the strings.

@stask
Copy link
Author

stask commented Nov 10, 2013

@elgs I'm not trying to change the protocol. What i'm suggesting is part of HTTP. FormData object allows it. Checkboxes work like that, combo boxes with multiple selection work like that. Field with multiple values gets encoded into something like this: fieldName=value0&fieldName=value1.

@vvo
Copy link
Contributor

vvo commented Nov 10, 2013

fieldName=value0&fieldName=value1.

Yes this is standard way to send data when you have checkboxes with multiple choices. We must support it.

@danialfarid
Copy link
Owner

That is only the case for GET requests. Angular by default converts the config.data to JSON if it is an object.
It is better to send data as a JSON object like

fieldName=[value0, value1].

rather than

fieldName=value0&fieldName=value1

This will save you a lot of url query space for big objects and would be better API on the server side if you are designing a service API with the JSON object as an input param.

@stask
Copy link
Author

stask commented Nov 10, 2013

@danialfarid You're cancelling default Angular behaviour by changing transformRequest here: config.transformRequest = angular.identity.
You can of course encode non-binary parameters as different 'part' in FormData with encoding 'application/json'. Not sure it's simpler/more standard though.

@danialfarid
Copy link
Owner

Yes you are right. I just realize that before reading this. I have put a fix in the code, gonna release a new version with a fix tomorrow.
It will apply all the transformRequest functions or defaults on the data before changing it to angular.identity.

@danialfarid
Copy link
Owner

hmm I realized that what I do is a bit different from angular. Angular sends the whole json data as encoded request body, but here I go over each data key and add them to formData. The thing is that here I need a formData name for the actual data. A solution is to convert the whole data to json and send it as "data" form field.
I need to look into it more.
I don't quiet get your suggestion "encode non-binary parameters as different 'part' in FormData with encoding 'application/json'" can you elaborate more on that.

@elgs
Copy link

elgs commented Nov 10, 2013

@danialfarid an array is a valid JSON object, based on the definition from this website: http://www.json.org, I'm not sure if this website is the official definition of JSON, however, I tested [1,2,3] in http://jsonlint.com, and it's said to be valid.

@danialfarid
Copy link
Owner

Yes you are right, thanks for letting me know.
For the purpose of sending data to the client we need a formData name or key to retrieve that data. If you just put a plain array in the data there is no "key" associated with it. I am thinking it's better to send all the data serialized as json with one formData named "data".

@elgs
Copy link

elgs commented Nov 10, 2013

I think the current situation already has given a great flexibility to transfer data in whatever format we want, thanks to JSON, or am I missing anything? @stask can you please share your use case about this requirement, or make it a little bit more specific by showing some code which you expect the data's format would be?

@stask
Copy link
Author

stask commented Nov 10, 2013

@elgs Here is my use case: I have a form with few fields (name, description, tags) and a file field. Tags field is composed of several fields with the same name 'tags'.
The problem has nothing to do with JSON. FormData doesn't specify content type for string 'parts'. Here is a snippet from tcpdump of the POST request:

........
------WebKitFormBoundary7mflflo0NxFZDGlQ
Content-Disposition: form-data; name="name"

broca-phrase-extractor-worker
------WebKitFormBoundary7mflflo0NxFZDGlQ
Content-Disposition: form-data; name="description"

foo
------WebKitFormBoundary7mflflo0NxFZDGlQ
Content-Disposition: form-data; name="tags"

["bar0","bar1","bar2"]
------WebKitFormBoundary7mflflo0NxFZDGlQ
Content-Disposition: form-data; name="package"; filename="broca-phrase-extractor-worker.tar.gz"
Content-Type: application/x-gzip
.........

As you can see, the file 'part' has Content-Type application/x-gzip. While other 'parts' don't have Content-Type.
On server side, the fields are parsed as strings, so the 'tags' field has string value of ["bar0","bar1","bar2"], which means that i have to manually parse it as JSON (it isn't parsed automatically because there is no Content-Type).
If i use the version i suggested earlier (append each value of 'tags' array separately), i'm getting following in tcpdump:

......
------WebKitFormBoundaryuXR8giQff8JYjBMI
Content-Disposition: form-data; name="name"

broca-phrase-extractor-worker
------WebKitFormBoundaryuXR8giQff8JYjBMI
Content-Disposition: form-data; name="description"

foo
------WebKitFormBoundaryuXR8giQff8JYjBMI
Content-Disposition: form-data; name="tags"

bar0
------WebKitFormBoundaryuXR8giQff8JYjBMI
Content-Disposition: form-data; name="tags"

bar1
------WebKitFormBoundaryuXR8giQff8JYjBMI
Content-Disposition: form-data; name="tags"

bar2
------WebKitFormBoundaryuXR8giQff8JYjBMI
Content-Disposition: form-data; name="package"; filename="broca-phrase-extractor-worker.tar.gz"
Content-Type: application/x-gzip

........

And on the server side the field 'tags' is properly parsed into an array by middleware.

@danialfarid When i said "encode non-binary parameters as different 'part' in FormData with encoding 'application/json'", i meant that we can try sending non-file parts as Blob, where Content-Type can be specified. I'm not sure if standard middlewares will be able to parse it properly though, will try it later today and will let you know.

@stask
Copy link
Author

stask commented Nov 10, 2013

Nope, seems like using Blob doesn't solve the issue. It's parsed as regular file on server side.
Here is what i did:

if (config.data) {
  formData.append("data", new Blob([angular.toJson(config.data)], {type: "application/json"}));
}

It produced following in tcpdump:

......
------WebKitFormBoundaryny5kBReF3SiUIqwH
Content-Disposition: form-data; name="data"; filename="blob"
Content-Type: application/json

{"name":"broca-phrase-extractor-worker","description":"foo","tags":["bar0","bar1","bar2"]}
------WebKitFormBoundaryny5kBReF3SiUIqwH
Content-Disposition: form-data; name="package"; filename="broca-phrase-extractor-worker.tar.gz"
Content-Type: application/x-gzip

.......

And on server side, i got two files, one for the actual file i sent, another one for the JSON Blob:

2013-Nov-10 21:42:39 +0200 stask-rmbp.local DEBUG [panopticon.server] - {:package {:size 11826846, :tempfile #<File /var/folders/2_/mbsyq3k17yl5hj8ykndw3z5c0000gn/T/ring-multipart-7067552405863570940.tmp>, :content-type application/x-gzip, :filename broca-phrase-extractor-worker.tar.gz}, :data {:size 90, :tempfile #<File /var/folders/2_/mbsyq3k17yl5hj8ykndw3z5c0000gn/T/ring-multipart-4911199280822317527.tmp>, :content-type application/json, :filename blob}}

I'll try to dig a little more, but not sure there is any option besides the one i suggested originally.

@danialfarid
Copy link
Owner

Can you try sending your data with $http.post() without the file and see if you can receive it on the server side any better. I assume $http.post() does the same thing and you need to convert string to json on your server side.

@danialfarid
Copy link
Owner

by the way, I think for your situation it is better to send the file alone and upload it and once it is uploaded completely then send the other related data with a $http.post() along with the file id that the server returns. There have been some known issues with Amazon S3 when uploading file along with data. I personally like to use upload just for the file since it is inherently a slow process, and then once the whole slow upload is complete do the other things and send the small data in a separate request and associate them with the uploaded file using an id.

This is not an answer to this issue but just a design suggestion.

@stask
Copy link
Author

stask commented Nov 11, 2013

@danialfarid Yes, i tried using $http.post without the file. It works because in this case the request is not multi-part and the whole request content type is application/json, so server side knows how to parse it.
Regarding splitting the upload in two parts (one for the file and another for the meta data) -- yes, i'll probably do that, thanks.
So, to sum up, here is the situation as i understand it, please correct me if i'm wrong:

  • When using FormData object, the request has Content-Type "multi-part/form-data", each "part" (each call to FormData.append) is encoded as separate part, each with it's own Content-Type.
  • FormData.append has three versions, one accepting File object, one accepting Blob object, and one accepting String. File and Blob versions allow specifying Content-Type, String version doesn't, and has implicit Content-Type of "form-data". It's not possible (or at least i didn't find a way) to specify Content-Type of string "parts", so server-side middleware parses the string "parts" as plain strings.
  • The only way that i see to support multiple form fields with same name is to use FormData.append for each value separately without encoding it as JSON because we cannot tell server that it's JSON.

@danialfarid
Copy link
Owner

I see,
It is correct. For the third option it won't work if you have a nested structure like an array of values inside another object.
Also could you let me know what library do you use to convert data to your model objects on the server? I need to test if we do this change for the first level data objects it would work for most common server technologies and won't break.
For now I believe the most common guideline would be to parse the json string on the server side which would work for nested arrays and others.

@stask
Copy link
Author

stask commented Nov 12, 2013

@danialfarid Well, to support nested structures you can use something similar to what was suggested in #38 (i would suggest refactoring it to support unlimited depth). It does require special handling on server side. Rails does it automatically, but other frameworks might not (in Clojure you have to add another middleware for instance).
I was talking about most common use case, fields with multiple values. It doesn't require anything special on server side as far as i know.
Right now i use ring on server side. It uses Apache's commons-fileupload underneath to parse multipart requests.

Regarding your last suggestion -- primitive values (strings, numbers) are encoded as plain strings, objects and arrays are encoded as JSON. Since it's not possible to specify content type for string parts of the multipart request, the only way to determine whether the value is just string or JSON string is by field name (either hard-coded logic or some kind of convention for field names). So it's not the most convenient option.

@danialfarid
Copy link
Owner

@stask Could you verify if #38 works with Rail or Clojure or any framework you are working with. So if you have the nested array like this

{ array: [ [1,2,3], [5,6,7] ] }

and they are sent as separate form data element like this:

array[0][0] = 1, array[0][1] = 2, ...

What would server create for the data.

Also for this example:

{ model: {array: [1,2,3]} }

I can add an option to the config to convert the arrays to the equivalent indexed keys in the form data to make it easier for these frameworks but not all frameworks would support that so by default it would convert them to JSON.

@stask
Copy link
Author

stask commented Nov 14, 2013

@danialfarid i'm not sure i'm explaining myself good enough. I'll try again.
#38 uses non-standard convention that uses square brackets in field names to denote nesting and arrays. It is supported by Rails by default (I think PHP supports it as well). In ring (clojure) you have to use special middleware to support that.

My pull request adds support for standard HTTP feature -- fields with multiple values. Forms with multiple selection (<select multiple...) and checkboxes use that.
If we weren't talking about file upload, and weren't using FormData object, you would be able to specify the Content-Type of the request as "application/json" and then it would be properly decoded by any web framework. That's what AngularJS does.
But we are talking about file upload, which means that the Content-Type of the request is "multipart/form-data".
Each call to FormData.append adds new part to the request (see the tcpdump snippets i posted earlier). File parts have Content-Type. String parts don't.
You cannot just encode input as JSON without explicitly specifying it in the request. FormData doesn't allow you to explicitly specify Content-Type for string parts (FormData.append(key, val) doesn't have an option to specify Content-Type). Which means that server side doesn't know that the string you send is JSON-encoded.

So, bottom line -- i see two options (besides just saying that fields with multiple values aren't supported):

  • implement something similar to Added advanced variable parse. #38 that uses non-standard convention to encode nested data structures and arrays in form field names.
  • Implement standard HTTP feature as i suggested in this pull request.

danialfarid added a commit that referenced this pull request Nov 15, 2013
@danialfarid
Copy link
Owner

I added an option to upload formDataAppender to handle adding data to the formData.
In your upload options you can add this:

$upload.upload({
    url : 'upload',
    headers: {'myHeaderKey': 'myHeaderVal'},
    data : {
        myModel : $scope.myModel,
        array: [1,2,3]
    },
    formDataAppender: function(fd, key, val) {
        if (angular.isArray(val)) {
            angular.forEach(val, function(v) {
                fd.append(key, v);
            });
        } else {
            fd.append(key, val);
        }
    },
    file : $file,
    fileFormDataName: 'myFile'
})

Which could take care of appending arrays to the formData as you expect.

Since it is server specific implementation by default it would convert the data to JSON format but you can specify a formDataAppender to change this behaviour and add your data in your way to the formData for your server.

@cressie176
Copy link

Had the same problem. Your formDataAppender fix works for me (using NodeJS & Express on the server side)

@danialfarid
Copy link
Owner

ok cool, I added some notes about this to the REAME file.

@danialfarid danialfarid mentioned this pull request Dec 1, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants