Skip to content

Loading…

Set Content-type based on HTTP method type #402

Closed
wants to merge 1 commit into from

6 participants

@olawiberg

Currently the Content-type is set to "application/json" for all HTTP method types. This causes problems with some servers for GET and DELETE methods as they do not contain a request body.

This update uses the default JQuery ajax content type "application/x-www-form-urlencoded; charset=UTF-8" for GET and DELETE and "application/json; charset=utf-8" for PUT and POST.

@olawiberg olawiberg Setting Content-type based on HTTP method type. Use default JQuery ty…
…pe for GET and DELETE and application/json for PUT and POST.
a285303
@tchak
Ember.js member

@olawiberg wow, what kind of server is it?

@wagenet
Ember.js member

This is probably fine, but I'd like to do some verification first.

@tchak
Ember.js member

@olawiberg @wagenet actually I disagree with this change, because it will send inconsistent http requests to people with "real" http servers. We need to check the spec. Sending "application/x-www-form-urlencoded" is an ugly hack.

@wagenet
Ember.js member

On second thought, I agree with @tchak. If your server doesn't conform to spec, then you should just extend RESTAdapter and overwrite the ajax method with your own implementation.

@wagenet wagenet closed this
@olawiberg

Extending the RESTAdapter will work. However, to make a consistent implementation I was thinking it would be a good solution to use the default JQuery Content-type since JQuery is used for the ajax calls. I do not see a reason to override the default GET and DELETE Content-type to something different than the default as there is no entity-body sent for GET and DELETE requests. So, specifying "application/x-www-form-urlencoded" is what JQuery does.

So a better approach is probably to not set the content-type at all unless it is a PUT or POST request, like this.

if(type === 'PUT' || type === 'POST') {
hash.contentType = 'application/json; charset=utf-8';
}

@wagenet
Ember.js member

@olawiberg Ok, I'll reopen for more discussion.

@wagenet wagenet reopened this
@tchak
Ember.js member

@olawiberg this is what jQuery is doing :
https://github.com/jquery/jquery/blob/master/src/ajax.js#L633

It definitely do not set "application/x-www-form-urlencoded" as default for GET/HEAD requests.

In case of GET/HEAD it do not set contentType. Except if contentType is given in options, in this case it will set no matter what. In my opinion this is wrong. Especially given the fact you can always force it via headers option.

We should probably do something like this :

if (type === 'POST' || type === 'PUT') {
  hash.contentType = 'application/json; charset=utf-8';
}
@olawiberg

@tchak if I understand this correctly JQuery sets the default values here:
https://github.com/jquery/jquery/blob/master/src/ajax.js#L256

Then load those settings here, unless you provide options:
https://github.com/jquery/jquery/blob/master/src/ajax.js#L354

And override them if the request has data:
https://github.com/jquery/jquery/blob/master/src/ajax.js#L633

So, I agree we only need to set the content type for POST/PUT.

@bsphere

this issue is affecting express.js servers with express.bodyParser() middleware,
it expects JSON body if the content type is set to application/json

workaround:

app.use(express.bodyParser());

app.use(function(err, req, res, next) {
    if (err.message == 'invalid json')  {
        next()
    } else {
        next(err)
    }
});
@epall

Thanks for the workaround, @bsphere!

@wagenet
Ember.js member

Looks like #601 is a fix for this.

@tomdale
Ember.js member

I think the approach taken in #601 is the correct behavior here. I'll close this PR and try to get that one merged in.

@tomdale tomdale closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 2, 2012
  1. @olawiberg

    Setting Content-type based on HTTP method type. Use default JQuery ty…

    olawiberg committed
    …pe for GET and DELETE and application/json for PUT and POST.
This page is out of date. Refresh to see the latest.
View
6 packages/ember-data/lib/adapters/rest_adapter.js
@@ -246,7 +246,11 @@ DS.RESTAdapter = DS.Adapter.extend({
hash.url = url;
hash.type = type;
hash.dataType = 'json';
- hash.contentType = 'application/json; charset=utf-8';
+ if(type === 'PUT' || type === 'POST') {
+ hash.contentType = 'application/json; charset=utf-8';
+ } else {
+ hash.contentType = 'application/x-www-form-urlencoded; charset=UTF-8';
+ }
hash.context = this;
if (hash.data && type !== 'GET') {
View
31 packages/ember-data/tests/unit/rest_adapter_test.js
@@ -1,6 +1,6 @@
var get = Ember.get, set = Ember.set;
-var adapter, store, ajaxUrl, ajaxType, ajaxHash;
+var adapter, store, ajaxUrl, ajaxType, ajaxHash, ajaxContentType;
var Person, person, people;
var Role, role, roles;
var Group, group;
@@ -10,6 +10,7 @@ module("the REST adapter", {
ajaxUrl = undefined;
ajaxType = undefined;
ajaxHash = undefined;
+ ajaxContentType = "application/x-www-form-urlencoded; charset=UTF-8";
adapter = DS.RESTAdapter.create({
ajax: function(url, type, hash) {
@@ -18,6 +19,11 @@ module("the REST adapter", {
ajaxUrl = url;
ajaxType = type;
ajaxHash = hash;
+ if(ajaxType === 'PUT' || ajaxType === 'POST') {
+ ajaxContentType = "application/json; charset=utf-8";
+ } else {
+ ajaxContentType = "application/x-www-form-urlencoded; charset=UTF-8";
+ }
if (success) {
hash.success = function(json) {
@@ -85,6 +91,10 @@ var expectType = function(type) {
equal(type, ajaxType, "the HTTP method is " + type);
};
+var expectContentType = function(contentType) {
+ equal(contentType, ajaxContentType, "the Content-type is " + contentType);
+};
+
var expectData = function(hash) {
deepEqual(hash, ajaxHash.data, "the hash was passed along");
};
@@ -113,6 +123,7 @@ test("creating a person makes a POST to /people, with the data hash", function()
expectUrl("/people", "the collection at the plural of the model name");
expectType("POST");
+ expectContentType("application/json; charset=utf-8");
expectData({ person: { name: "Tom Dale" } });
ajaxHash.success({ person: { id: 1, name: "Tom Dale" } });
@@ -134,6 +145,7 @@ test("singular creations can sideload data", function() {
expectUrl("/people", "the collection at the plural of the model name");
expectType("POST");
+ expectContentType("application/json; charset=utf-8");
expectData({ person: { name: "Tom Dale" } });
ajaxHash.success({
@@ -166,6 +178,7 @@ test("updating a person makes a PUT to /people/:id with the data hash", function
expectUrl("/people/1", "the plural of the model name with its ID");
expectType("PUT");
+ expectContentType("application/json; charset=utf-8");
ajaxHash.success({ person: { id: 1, name: "Brohuda Brokatz" } });
expectState('saving', false);
@@ -191,6 +204,7 @@ test("updates are not required to return data", function() {
expectUrl("/people/1", "the plural of the model name with its ID");
expectType("PUT");
+ expectContentType("application/json; charset=utf-8");
ajaxHash.success();
expectState('saving', false);
@@ -220,6 +234,7 @@ test("singular updates can sideload data", function() {
expectUrl("/people/1", "the plural of the model name with its ID");
expectType("PUT");
+ expectContentType("application/json; charset=utf-8");
ajaxHash.success({
person: { id: 1, name: "Brohuda Brokatz" },
@@ -266,6 +281,7 @@ test("deleting a person makes a DELETE to /people/:id", function() {
expectUrl("/people/1", "the plural of the model name with its ID");
expectType("DELETE");
+ expectContentType("application/x-www-form-urlencoded; charset=UTF-8");
ajaxHash.success();
expectState('deleted');
@@ -293,6 +309,7 @@ test("singular deletes can sideload data", function() {
expectUrl("/people/1", "the plural of the model name with its ID");
expectType("DELETE");
+ expectContentType("application/x-www-form-urlencoded; charset=UTF-8");
ajaxHash.success({
groups: [{ id: 1, name: "Group 1" }]
@@ -340,6 +357,7 @@ test("finding all can sideload data", function() {
expectUrl("/groups", "the plural of the model name");
expectType("GET");
+ expectContentType("application/x-www-form-urlencoded; charset=UTF-8");
ajaxHash.success({
groups: [{ id: 1, name: "Group 1", people: [ 1 ] }],
@@ -361,6 +379,7 @@ test("finding a person by ID makes a GET to /people/:id", function() {
expectState('loaded', false);
expectUrl("/people/1", "the plural of the model name with the ID requested");
expectType("GET");
+ expectContentType("application/x-www-form-urlencoded; charset=UTF-8");
ajaxHash.success({ person: { id: 1, name: "Yehuda Katz" } });
@@ -403,6 +422,7 @@ test("finding many people by a list of IDs", function() {
expectUrl("/people");
expectType("GET");
+ expectContentType("application/x-www-form-urlencoded; charset=UTF-8");
expectData({ ids: [ 1, 2, 3 ] });
ajaxHash.success({
@@ -473,6 +493,7 @@ test("additional data can be sideloaded in a GET with many IDs", function() {
expectUrl("/groups");
expectType("GET");
+ expectContentType("application/x-www-form-urlencoded; charset=UTF-8");
expectData({ ids: [ 1 ] });
ajaxHash.success({
@@ -513,6 +534,7 @@ test("finding people by a query", function() {
expectUrl("/people", "the collection at the plural of the model name");
expectType("GET");
+ expectContentType("application/x-www-form-urlencoded; charset=UTF-8");
expectData({ page: 1 });
ajaxHash.success({
@@ -549,6 +571,7 @@ test("finding people by a query can sideload data", function() {
expectUrl("/groups", "the collection at the plural of the model name");
expectType("GET");
+ expectContentType("application/x-www-form-urlencoded; charset=UTF-8");
expectData({ page: 1 });
ajaxHash.success({
@@ -598,6 +621,7 @@ test("creating several people (with bulkCommit) makes a POST to /people, with a
expectUrl("/people", "the collection at the plural of the model name");
expectType("POST");
+ expectContentType("application/json; charset=utf-8");
expectData({ people: [ { name: "Tom Dale" }, { name: "Yehuda Katz" } ] });
ajaxHash.success({ people: [ { id: 1, name: "Tom Dale" }, { id: 2, name: "Yehuda Katz" } ] });
@@ -623,6 +647,7 @@ test("bulk commits can sideload data", function() {
expectUrl("/people", "the collection at the plural of the model name");
expectType("POST");
+ expectContentType("application/json; charset=utf-8");
expectData({ people: [ { name: "Tom Dale" }, { name: "Yehuda Katz" } ] });
ajaxHash.success({
@@ -665,6 +690,7 @@ test("updating several people (with bulkCommit) makes a PUT to /people/bulk with
expectUrl("/people/bulk", "the collection at the plural of the model name");
expectType("PUT");
+ expectContentType("application/json; charset=utf-8");
ajaxHash.success({ people: [
{ id: 1, name: "Brohuda Brokatz" },
@@ -707,6 +733,7 @@ test("bulk updates can sideload data", function() {
expectUrl("/people/bulk", "the collection at the plural of the model name");
expectType("PUT");
+ expectContentType("application/json; charset=utf-8");
ajaxHash.success({
people: [
@@ -752,6 +779,7 @@ test("deleting several people (with bulkCommit) makes a PUT to /people/bulk", fu
expectUrl("/people/bulk", "the collection at the plural of the model name with 'delete'");
expectType("DELETE");
+ expectContentType("application/x-www-form-urlencoded; charset=UTF-8");
ajaxHash.success();
@@ -791,6 +819,7 @@ test("bulk deletes can sideload data", function() {
expectUrl("/people/bulk", "the collection at the plural of the model name with 'delete'");
expectType("DELETE");
+ expectContentType("application/x-www-form-urlencoded; charset=UTF-8");
ajaxHash.success({
groups: [{ id: 1, name: "Group 1" }]
Something went wrong with that request. Please try again.