Skip to content

Commit

Permalink
Use HTTP status code as error code if provided body cannot be parsed. (
Browse files Browse the repository at this point in the history
…#1380)

* Use HTTP status code as error message if provided body cannot be parsed.

* Replace "GLOBAL" with "global" in tests to get rid of deprecation notices

* Use HTTP status message as error message when body unparsable

* Update request type definition and documentation to mention statusMessage argument to httpHeaders event
  • Loading branch information
jeskew authored Mar 10, 2017
1 parent 2286004 commit fb2fef7
Show file tree
Hide file tree
Showing 15 changed files with 97 additions and 32 deletions.
2 changes: 1 addition & 1 deletion dist-tools/test/helpers.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ fs = require('fs')
evalCode = (code, preamble) ->
eval """
(function() {
var window = GLOBAL;
var window = global;
#{preamble};
return #{code};
})();
Expand Down
10 changes: 7 additions & 3 deletions lib/event_listeners.js
Original file line number Diff line number Diff line change
Expand Up @@ -206,8 +206,11 @@ AWS.EventListeners = {
function callback(httpResp) {
resp.httpResponse.stream = httpResp;

httpResp.on('headers', function onHeaders(statusCode, headers) {
resp.request.emit('httpHeaders', [statusCode, headers, resp]);
httpResp.on('headers', function onHeaders(statusCode, headers, statusMessage) {
resp.request.emit(
'httpHeaders',
[statusCode, headers, resp, statusMessage]
);

if (!resp.httpResponse.streaming) {
if (AWS.HttpClient.streamsApiVersion === 2) { // streams2 API check
Expand Down Expand Up @@ -277,8 +280,9 @@ AWS.EventListeners = {
});

add('HTTP_HEADERS', 'httpHeaders',
function HTTP_HEADERS(statusCode, headers, resp) {
function HTTP_HEADERS(statusCode, headers, resp, statusMessage) {
resp.httpResponse.statusCode = statusCode;
resp.httpResponse.statusMessage = statusMessage;
resp.httpResponse.headers = headers;
resp.httpResponse.body = new AWS.util.Buffer('');
resp.httpResponse.buffers = [];
Expand Down
7 changes: 6 additions & 1 deletion lib/http/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,12 @@ AWS.NodeHttpClient = AWS.util.inherit({
if (cbAlreadyCalled) return; cbAlreadyCalled = true;

callback(httpResp);
httpResp.emit('headers', httpResp.statusCode, httpResp.headers);
httpResp.emit(
'headers',
httpResp.statusCode,
httpResp.headers,
httpResp.statusMessage
);
});
httpRequest.stream = stream; // attach stream to httpRequest

Expand Down
7 changes: 6 additions & 1 deletion lib/http/xhr.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,12 @@ AWS.XHRClient = AWS.util.inherit({
try { xhr.responseType = 'arraybuffer'; } catch (e) {}
emitter.statusCode = xhr.status;
emitter.headers = self.parseHeaders(xhr.getAllResponseHeaders());
emitter.emit('headers', emitter.statusCode, emitter.headers);
emitter.emit(
'headers',
emitter.statusCode,
emitter.headers,
xhr.statusText
);
headersEmitted = true;
}
if (this.readyState === this.DONE) {
Expand Down
6 changes: 5 additions & 1 deletion lib/http_response.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,12 @@ export class HttpResponse {
* The HTTP status code of the response (e.g., 200, 404).
*/
statusCode: number;
/**
* The HTTP status message of the response (e.g., 'Bad Request', 'Not Found')
*/
statusMessage: string;
/**
* Whether this response is being streamed at a low-level.
*/
streaming: boolean;
}
}
21 changes: 13 additions & 8 deletions lib/protocol/json.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,19 @@ function extractError(resp) {
}

if (httpResponse.body.length > 0) {
var e = JSON.parse(httpResponse.body.toString());
if (e.__type || e.code) {
error.code = (e.__type || e.code).split('#').pop();
}
if (error.code === 'RequestEntityTooLarge') {
error.message = 'Request body must be less than 1 MB';
} else {
error.message = (e.message || e.Message || null);
try {
var e = JSON.parse(httpResponse.body.toString());
if (e.__type || e.code) {
error.code = (e.__type || e.code).split('#').pop();
}
if (error.code === 'RequestEntityTooLarge') {
error.message = 'Request body must be less than 1 MB';
} else {
error.message = (e.message || e.Message || null);
}
} catch (e) {
error.statusCode = httpResponse.statusCode;
error.message = httpResponse.statusMessage;
}
} else {
error.statusCode = httpResponse.statusCode;
Expand Down
9 changes: 8 additions & 1 deletion lib/protocol/query.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,14 @@ function extractError(resp) {
Message: 'Unknown operation ' + resp.request.operation
};
} else {
data = new AWS.XML.Parser().parse(body);
try {
data = new AWS.XML.Parser().parse(body);
} catch (e) {
data = {
Code: resp.httpResponse.statusCode,
Message: resp.httpResponse.statusMessage
};
}
}

if (data.requestId && !resp.requestId) resp.requestId = data.requestId;
Expand Down
11 changes: 10 additions & 1 deletion lib/protocol/rest_xml.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,16 @@ function buildRequest(req) {
function extractError(resp) {
Rest.extractError(resp);

var data = new AWS.XML.Parser().parse(resp.httpResponse.body.toString());
var data;
try {
data = new AWS.XML.Parser().parse(resp.httpResponse.body.toString());
} catch (e) {
data = {
Code: resp.httpResponse.statusCode,
Message: resp.httpResponse.statusMessage
};
}

if (data.Errors) data = data.Errors;
if (data.Error) data = data.Error;
if (data.Code) {
Expand Down
4 changes: 2 additions & 2 deletions lib/request.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ export class Request<D, E> {
* @param {string} event - httpHeaders: triggered when headers are sent by the remote server.
* @param {function} listener - Callback to run when the headers are sent by the remote server.
*/
on(event: "httpHeaders", listener: (statusCode: number, headers: {[key: string]: string}, response: Response<D, E>) => void): Request<D, E>;
on(event: "httpHeaders", listener: (statusCode: number, headers: {[key: string]: string}, response: Response<D, E>, statusMessage: string) => void): Request<D, E>;
/**
* Adds a listener that is triggered when data is sent by the remote server.
*
Expand Down Expand Up @@ -176,4 +176,4 @@ export class Request<D, E> {
export interface Progress {
loaded: number;
total: number;
}
}
4 changes: 3 additions & 1 deletion lib/request.js
Original file line number Diff line number Diff line change
Expand Up @@ -246,11 +246,13 @@ fsm.setupStates();
*
* @!group HTTP Events
*
* @!event httpHeaders(statusCode, headers, response)
* @!event httpHeaders(statusCode, headers, response, statusMessage)
* Triggered when headers are sent by the remote server
* @param statusCode [Integer] the HTTP response code
* @param headers [map<String,String>] the response headers
* @param (see AWS.Request~send)
* @param statusMessage [String] A status message corresponding to the HTTP
* response code
* @context (see AWS.Request~send)
*
* @!event httpData(chunk, response)
Expand Down
18 changes: 9 additions & 9 deletions test/helpers.coffee
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
AWS = null
global = null
topLevelScope = null
ignoreRequire = require
if typeof window == 'undefined'
AWS = ignoreRequire('../lib/aws')
global = GLOBAL
topLevelScope = global
else
AWS = window.AWS
global = window
topLevelScope = window

if global.jasmine
global.jasmine.DEFAULT_TIMEOUT_INTERVAL = 30000
if topLevelScope.jasmine
topLevelScope.jasmine.DEFAULT_TIMEOUT_INTERVAL = 30000

_it = it
global.it = (label, fn) ->
topLevelScope.it = (label, fn) ->
if label.match(/\(no phantomjs\)/) and navigator and navigator.userAgent.match(/phantomjs/i)
return
_it(label, fn)
Expand Down Expand Up @@ -76,10 +76,10 @@ _spyOn = (obj, methodName) ->
# Disable setTimeout for tests, but keep original in case test needs to use it
# Warning: this might cause unpredictable results
# TODO: refactor this out.
global.setTimeoutOrig = global.setTimeout
global.setTimeout = (fn) -> fn()
topLevelScope.setTimeoutOrig = topLevelScope.setTimeout
topLevelScope.setTimeout = (fn) -> fn()

global.expect = require('chai').expect
topLevelScope.expect = require('chai').expect

matchXML = (xml1, xml2) ->
results = []
Expand Down
11 changes: 10 additions & 1 deletion test/protocol/json.spec.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ describe 'AWS.Protocol.Json', ->
describe 'extractError', ->
extractError = (body) ->
response.httpResponse.statusCode = 500
response.httpResponse.statusMessage = 'Internal Server Error'
response.httpResponse.body = new Buffer(body)
svc.extractError(response)

Expand All @@ -102,6 +103,14 @@ describe 'AWS.Protocol.Json', ->
expect(response.error.message).to.equal('500')
expect(response.data).to.equal(null)

it 'returns the status code when the body is not valid JSON', ->
extractError '<html><body><b>Http/1.1 Service Unavailable</b></body> </html>'
expect(response.error ).to.be.instanceOf(Error)
expect(response.error.code).to.equal('UnknownError')
expect(response.error.statusCode).to.equal(500)
expect(response.error.message).to.equal('Internal Server Error')
expect(response.data).to.equal(null)

it 'returns UnknownError when the error type is not set', ->
extractError '{"message":"Error Message" }'
expect(response.error ).to.be.instanceOf(Error)
Expand Down Expand Up @@ -171,4 +180,4 @@ describe 'AWS.Protocol.Json', ->
extractData '{"i":1, "b": null}'
expect(response.error).to.equal(null)
expect(response.data.i).to.equal(1)
expect(response.data.b).to.equal(null)
expect(response.data.b).to.equal(null)
7 changes: 7 additions & 0 deletions test/protocol/query.spec.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ describe 'AWS.Protocol.Query', ->
</Error>
"""
response.httpResponse.statusCode = 400
response.httpResponse.statusMessage = 'Bad Request'
response.httpResponse.body = new Buffer(body)
svc.extractError(response)

Expand All @@ -122,6 +123,12 @@ describe 'AWS.Protocol.Query', ->
expect(response.error.message).to.equal(null)
expect(response.data).to.equal(null)

it 'returns an empty error when the body cannot be parsed', ->
extractError JSON.stringify({"foo":"bar","fizz":["buzz", "pop"]})
expect(response.error.code).to.equal(400)
expect(response.error.message).to.equal('Bad Request')
expect(response.data).to.equal(null)

it 'extracts error when inside <Errors>', ->
extractError """
<SomeResponse>
Expand Down
8 changes: 8 additions & 0 deletions test/protocol/rest_xml.spec.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ describe 'AWS.Protocol.RestXml', ->
</Error>
"""
response.httpResponse.statusCode = 400
response.httpResponse.statusMessage = 'Bad Request'
response.httpResponse.body = new Buffer(body)
svc.extractError(response)

Expand All @@ -234,6 +235,13 @@ describe 'AWS.Protocol.RestXml', ->
expect(response.error.message).to.equal(null)
expect(response.data).to.equal(null)

it 'returns an empty error when the body cannot be parsed', ->
extractError JSON.stringify({"foo":"bar","fizz":["buzz", "pop"]})
expect(response.error ).to.be.instanceOf(Error)
expect(response.error.code).to.equal(400)
expect(response.error.message).to.equal('Bad Request')
expect(response.data).to.equal(null)

it 'extracts error when inside <Errors>', ->
extractError """
<SomeResponse>
Expand Down
4 changes: 2 additions & 2 deletions test/util.spec.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ describe 'AWS.util.date', ->
describe 'getDate', ->
it 'should return current date by default', ->
now = {}
obj = if AWS.util.isNode() then GLOBAL else window
obj = if AWS.util.isNode() then global else window
helpers.spyOn(obj, 'Date').andCallFake -> now
expect(util.getDate()).to.equal(now)

Expand All @@ -95,7 +95,7 @@ describe 'AWS.util.date', ->

beforeEach ->
[date, mocked, config] = [Date, false, AWS.config]
obj = if AWS.util.isNode() then GLOBAL else window
obj = if AWS.util.isNode() then global else window
helpers.spyOn(obj, 'Date').andCallFake (t) ->
if mocked
new date(t)
Expand Down

0 comments on commit fb2fef7

Please sign in to comment.