Automattic / knox

S3 Lib
MIT License
1.74k stars 285 forks source link

Wish: improved docs for putBuffer error handling #275

Closed markstos closed 9 years ago

markstos commented 9 years ago

From reading the putBuffer docs, one might assume that if there is no err returned, then the uploaded succeeded. However, this is not necessarily the case. The upload may have failed with 403 or another error.

Either the logic should be updated to treat non 2XX responses also as an err, or the doc should be updated to clarify that in attention to checking for err, you also need to check res.statusCode to make sure an HTTP success code was returned a well.

markstos commented 9 years ago

I suggest following the pattern used by node-ses for interacting with the AWS API: Treat HTTP response codes < 200 and >= 400 as errors. In those cases, parse the XML errors returned by AWS into a JavaScript object, and return that:

https://github.com/aheckmann/node-ses/blob/master/lib/ses.js#L276

Here's the whole related function from node-ses:

  request(options, function (err, res, data) {
    debug('received: %s %d %j', err, res && res.statusCode || 0, data);

    if (err) return callback(err, data, res);

    if (res.statusCode < 200 || res.statusCode >= 400) {
      var msg = xmlParser.toJson(data,  {object:true});
      if(!msg.ErrorResponse){
        return callback("Unknown error: " + data);
      }
      return callback(msg["ErrorResponse"]["Error"]);
    }

    callback(null, data, res);
  });
}
domenic commented 9 years ago

Dupe of #114. Pull request, either to change the functionality or improve the docs, welcome. (I was pretty sure the docs do contain examples of checking the status code though.)