Skip to content
This repository

Request failure event with 204 status code #2254

Closed
davidfoley opened this Issue January 25, 2012 · 6 comments

5 participants

David Foley Arian Stolwijk Olmo Maldonado Sean McArthur Marvin Tam
David Foley

Hi, I'm defining my requests like this, explicitly adding the isSuccess option method:

var request= new Request.JSON({
    url: endpoint, 
    method:'DELETE',
    emulation:false,
    async:true,
    isSuccess: function (status) {
          return 199 < status && 300 > status;
    }
});

request.addEvent('failure', function (xhr) {
    // the status code is 204- WTF is the failure callback being called?
});

The endpoint I send the request to returns (correctly) a 204. The isSuccess method correctly determines that 204 is an acceptable status to be considered 'successul'. However, the failure event is being dispatched. This surely has to be a bug, no?
I am using MooTools 1.4.1

Edit1: Noted that Request.JSON is being used (as this is the default request type used in the app). Perhaps this is a limitation or grey area of Request.JSON thats not present in Request?

Edit2: Found the issue- in Request.JSON-

success: function(text){
    var json;
    try {
        json = this.response.json = JSON.decode(text, this.options.secure);
    } catch (error){
        this.fireEvent('error', [text, error]);
        return;
    }
    if (json == null) this.onFailure();
    else this.onSuccess(json, text);
}

As the 204 no-content response carries no body, json is null, so the method assumes onFailure. This is admittedly an edge case (normally you would use a regular request for this kind of operation), but I would be inclined to think that an outright null response is actually not enough to determine that a request failed. There should also be some kind of correlation with the status code, no?

Olmo Maldonado
Owner

Well.. your isSuccess is being called and the success method of Request.JSON is being called. So I think the 204 related arguments are working as expected.

Whether the success method should treat null as an error I think the behavior is as expected. Keep in mind Request.JSON is used to request: JSON hehe. Returning null in any kind of API is a bad API. Most RESTful APIs return an object with:

{},
// or
{
 "data": {},
},
// or
{
  "error": '...',
  "code": 400
}
David Foley

Well.... I'm not so sure...

What if I do a PUT, sending a JSON resource to a REST API- an appropriate response after that can actually be a 204 No Content, which by definition, must have no body, therefore there would be no JSON response. The code as it stands does not support that and similar (valid REST) scenarios, so I still think its an issue.

Arian Stolwijk
Owner

In that case you can just use Request. Request.JSON is to retrieve a JSON response.

Sean McArthur

Indeed, just as Request.HTML is when you want to retrieve HTML, not when you plan to send it.

Arian Stolwijk arian closed this January 25, 2012
David Foley

Hey guys, I get your perspectives on this. But I disagree.

A JSON Request should not be limited strictly to the retrieval of JSON data, as in a GET and in some cases, from a POST response.

The convenience of Request.JSON is also the encoding of objects for sending to the server and having the Content-Type set up for you. It is actually valid to have a JSON Request that sends JSON data, but does not expect JSON as the response. In terms of RPC, instead of REST, yes you would, but even then, the Request implementation deciding that a null JSON response is an error is to take too much ownership of the business logic- thats the servers job, or the client application's job, right? Request should be agnostic to business logic as its a generic library class. The Request should not carry the interpretation that a null JSON response is an error, when in fact, this is not always the case.

Hope you change your mind on this, even if it is a narrow point.

Marvin Tam

I agree with @davidfoley. The Content-type of the request should not influence the semantics of the HTTP verbs.

Here's what W3C says about DELETE (my use case):

A successful response SHOULD be 200 (OK) if the response includes an entity describing the status, 202 (Accepted) if the action has not yet been enacted, or 204 (No Content) if the action has been enacted but the response does not include an entity.

Besides, it's simply unexpected behavior for a 2xx class response code to invoke onFailure.

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.