Set Content-type based on HTTP method type #402

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
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.

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

This comment has been minimized.

Show comment Hide comment
@tchak

tchak Oct 2, 2012

Member

@olawiberg wow, what kind of server is it?

Member

tchak commented Oct 2, 2012

@olawiberg wow, what kind of server is it?

@wagenet

This comment has been minimized.

Show comment Hide comment
@wagenet

wagenet Oct 18, 2012

Owner

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

Owner

wagenet commented Oct 18, 2012

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

@tchak

This comment has been minimized.

Show comment Hide comment
@tchak

tchak Oct 18, 2012

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.

Member

tchak commented Oct 18, 2012

@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

This comment has been minimized.

Show comment Hide comment
@wagenet

wagenet Oct 18, 2012

Owner

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.

Owner

wagenet commented Oct 18, 2012

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 Oct 18, 2012

@olawiberg

This comment has been minimized.

Show comment Hide comment
@olawiberg

olawiberg Oct 18, 2012

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';
}

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

This comment has been minimized.

Show comment Hide comment
@wagenet

wagenet Oct 19, 2012

Owner

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

Owner

wagenet commented Oct 19, 2012

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

@wagenet wagenet reopened this Oct 19, 2012

@tchak

This comment has been minimized.

Show comment Hide comment
@tchak

tchak Oct 19, 2012

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';
}
Member

tchak commented Oct 19, 2012

@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

This comment has been minimized.

Show comment Hide comment
@olawiberg

olawiberg Oct 19, 2012

@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.

@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 comment has been minimized.

Show comment Hide comment
@bsphere

bsphere Dec 24, 2012

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)
    }
});

bsphere commented Dec 24, 2012

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

This comment has been minimized.

Show comment Hide comment
@epall

epall Jan 3, 2013

Thanks for the workaround, @bsphere!

epall commented Jan 3, 2013

Thanks for the workaround, @bsphere!

@wagenet

This comment has been minimized.

Show comment Hide comment
@wagenet

wagenet Jan 24, 2013

Owner

Looks like #601 is a fix for this.

Owner

wagenet commented Jan 24, 2013

Looks like #601 is a fix for this.

@tomdale

This comment has been minimized.

Show comment Hide comment
@tomdale

tomdale Apr 6, 2013

Owner

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.

Owner

tomdale commented Apr 6, 2013

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 Apr 6, 2013

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