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

Using JSON responses rather then HTTP response status for testChunk #160

Closed
relector-tuxnix opened this issue Mar 10, 2014 · 5 comments
Closed

Comments

@relector-tuxnix
Copy link

Enabling 'testChunk' means if a chunk is not found a 404 (or non 200 HTTP status) is returned. Although this is perfectly valid API design its quite annoying to see lots of 404's in my logs etc. Can I propose the the following change:

If a chunk is found return the following JSON:

{"success":true,"message":"Chunk found."}

If a chunk is not found return the following JSON:

{"success":false,"message":"Chunk not found."}

The following code changes can be made in resumable.js:

                var testHandler = function(e) {

                        $.tested = true;

                        var status = $.status();

                        if(status=='success') {

                                var response = JSON.parse($.xhr.responseText);

                                if(response.success == false) {
                                        $.send();
                                } else {
                                        $.callback(status, $.message());
                                        $.resumableObj.uploadNextChunk();
                                }
                        } else {
                                console.log("HTTP Error.");
                        }
                };
@steffentchr
Copy link
Member

Hi @neonnds

You're right that the console warnings are extremely annoying; but I do really like the extreme simplicity of only communicating through HTTP codes -- since it makes no assumption about preferred messaging formats on the server side.

Any idea for a semantically nice HTTP status code to replace 404. One that would cause console logs to be made, and preferable one that doesn't break existing implementations?

@relector-tuxnix
Copy link
Author

I would allow for non-200 (for backwards compatibility) and a json response.

Major players using REST APIs (Vimeo, Facebook, Twitter) return 200 status codes for every request. Status code 200 means the request itself completed with no errors (the act of accessing the API and returning a result).

Within a specific call, an error may occur, and this is where a specific error code can be returned.

Returning something similar to below would do the job for most situations:

Status - Ok or Fail
Data (optional) - Any data returned
Code (optional) - HTTP code associated with the error
Message (optional) - Message associated with the error
Explanation (optional) - Additional error info

{status: "Fail", data: {}, code: 404, message: "Chunk not found", explanation: ""}

{status: "Ok", data: {}, code: 200, message: "Chunk found.", explaination: ""}

Also in the future its probably a good idea to allow for communication in XML and not just JSON.

                var testHandler = function(e) {

                        $.tested = true;

                        var status = $.status();

                        if(status == 'success') {

                                var response = JSON.parse($.xhr.responseText);

                                if(response.status == "Ok") {
                                        $.send();
                                } else {
                                        $.callback(status, $.message());
                                        $.resumableObj.uploadNextChunk();
                                }

                        //NON-200 Returned so did not find chunk
                        } else {
                                console.log("HTTP Error.");

                                $.callback(status, $.message());
                                $.resumableObj.uploadNextChunk();
                        }
                };

@steffentchr
Copy link
Member

I agree with you almost entirely: Settling for a JSON transport for messaging instead of flat HTTP status code could be helpful in cases. Avoiding red console log entries for something that's by-design would be welcome as well. I even tend to agree on the everything-is-200 approach for API design; even if there are compelling arguments against it.

The reason I'm holding back on this suggestion is that I'm worries about having two different supported ways of doing the exact same thing. I makes implementation, debugging and maintenance harder.

If the main intent here is to avoid console logs, we can find a way of doing so (for example by allow the check to return 204 No Content to signify the same thing as a 404 Not Found). If this is a wider argument for more detailed messaging between the server- and client-side, that's a welcome discussion to have as well ;-)

@relector-tuxnix
Copy link
Author

Well 204 will do the job to remove the errors, so that solves my problem. And your right that supporting two different methods will be problematic. Probably best just to use HTTP codes for now and just keep the other approach for future releases / branches. Also if people really need more detailed responses, hopefully our conversation above will help them implement their own changes. Cheers.

@meden
Copy link

meden commented Jan 30, 2015

While this bug has already been closed, here are my 2cents: to be semantically correct, any 2xx status codes should be interpreted as a successful upload. The difference between success codes are only to notify the client about particular conditions, so that it could behave accordingly (i.e. not trying to build a DOM if it receives 204 No Content).
Maybe a good response for a missing chunk test could be 100 Continue. But it has the limitation of being a HTTP/1.1-only response code (it is not defined in HTTP/1.0).
For further informations, read https://en.wikipedia.org/wiki/List_of_HTTP_status_codes.

UPDATE: I read the RFC2116 Section 8.2.3 and I was wrong. So, to be semantically correct, the only way would be a JSON response. Sorry for the noise...

Thanks for you work!

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

No branches or pull requests

3 participants