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

Use HTTP status code as error code if provided body cannot be parsed. #1380

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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