Skip to content
This repository has been archived by the owner on Nov 9, 2022. It is now read-only.

Commit

Permalink
Fix: No longer delete API Methods on Conflict exceptions.
Browse files Browse the repository at this point in the history
  • Loading branch information
@carlnordenfelt
carlnordenfelt committed May 23, 2016
1 parent c7d3ad1 commit 3c15cdf
Show file tree
Hide file tree
Showing 6 changed files with 133 additions and 43 deletions.
2 changes: 2 additions & 0 deletions _scripts/publish.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,8 @@ function publish() {
sed -i '.original' "s:{VERSION}:${version}:g" ${templatePath}/${templateName}
aws s3 cp ${sourceFileName} s3://${s3BucketName}.eu-west-1/${version}/${sourceFileName} --region eu-west-1
aws s3 cp ${templatePath}/${templateName} s3://${s3BucketName}.eu-west-1/${version}/${templateName} --region eu-west-1
aws s3 cp ${sourceFileName} s3://${s3BucketName}.us-east-1/${version}/${sourceFileName} --region us-east-1
aws s3 cp ${templatePath}/${templateName} s3://${s3BucketName}.us-east-1/${version}/${templateName} --region us-east-1
echo "https://s3.amazonaws.com/${s3BucketName}.eu-west-1/${version}/${templateName}"
fi

Expand Down
51 changes: 32 additions & 19 deletions lib/commands/ApiMethod.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,28 +10,48 @@ pub.getParameters = function (event) {
};

pub.createResource = function createResource(event, context, eventParams, callback) {
ApiMethodService.createMethod(eventParams, function (error) {
ApiMethodService.createMethod(eventParams, function (error, apiMethod) {
if (error) {
// If apiMethod is returned it means that the method was created but something went wrong after
// If this is the case we want the subsequent delete request to delete the method.
// Otherwise, the method should not be touched (it's either not created or it is in fact another method (Conflict error)
if (apiMethod) {
if (typeof error === 'string') { // Sanity check
error = {
message: error
};
}
error.physicalResourceId = eventParams.params.resourceId + '/' + eventParams.params.method.httpMethod;
}
return callback(error);
}
getForResponse(event, context, eventParams.params.restApiId, eventParams.params.resourceId, eventParams.params.method.httpMethod, callback);
});
};

pub.deleteResource = function deleteResource(event, _context, eventParams, callback) {
var resourceAndMethod = _parsePhysicalResourceId(event.PhysicalResourceId, eventParams.params.resourceId, eventParams.params.method.httpMethod);
ApiMethodService.deleteMethod(eventParams.params.restApiId, resourceAndMethod.resourceId, resourceAndMethod.httpMethod, function (error) {
return callback(error);
});
var resourceAndMethod = _parsePhysicalResourceId(event.PhysicalResourceId);
if (resourceAndMethod) {
ApiMethodService.deleteMethod(eventParams.params.restApiId, resourceAndMethod.resourceId, resourceAndMethod.httpMethod, function (error) {
return callback(error);
});
return;
}
callback();
};

pub.updateResource = function updateResource(event, context, eventParams, callback) {
pub.deleteResource(event, context, eventParams, function (error) {
if (error) {
return callback(error);
}
return pub.createResource(event, context, eventParams, callback);
});
var resourceAndMethod = _parsePhysicalResourceId(event.PhysicalResourceId);
if (resourceAndMethod) {
pub.deleteResource(event, context, eventParams, function (error) {
if (error) {
return callback(error);
}
return pub.createResource(event, context, eventParams, callback);
});
return;
}
callback('Resource not found');
};

module.exports = pub;
Expand All @@ -47,19 +67,12 @@ function getForResponse(_event, _context, restApiId, resourceId, httpMethod, cal
});
}

function _parsePhysicalResourceId(physicalResourceId, eventResourceId, eventHttpMethod) {
function _parsePhysicalResourceId(physicalResourceId) {
var parts = physicalResourceId.split('/');
// TODO: Candidate for much harder scrutiny. The HTTP Method can be validated to ensure that it is a proper physicalResourceId
if (parts.length === 2) {
return {
resourceId: parts[0],
httpMethod: parts[1]
};
}
// Fallback if the PhysicalResourceId appears to be incorrectly formatted.
// This happens if the create operation fails and CFN makes a subsequent delete call to clean up.
return {
resourceId: eventResourceId,
httpMethod: eventHttpMethod
};
}
13 changes: 10 additions & 3 deletions lib/service/ApiMethod/ApiMethodService.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,15 @@ pub.createMethod = function createMethod(parameters, callback) {
}
createIntegration(validParameters, function (integrationError) {
if (integrationError) {
return callback(integrationError);
return callback(integrationError, apiMethod);
}
createResponses(validParameters, function (responseError) {
if (responseError) {
return callback(responseError);
return callback(responseError, apiMethod);
}
createIntegrationResponses(validParameters, function (integrationResponseError) {
if (integrationResponseError) {
return callback(integrationResponseError);
return callback(integrationResponseError, apiMethod);
}
return callback(undefined, apiMethod);
});
Expand Down Expand Up @@ -187,7 +187,13 @@ function createIntegration(parameters, callback) {
}

function createResponses(parameters, callback) {
var createdResponses = [];
async.mapSeries(parameters.responses, function (response, asyncCallback) {
// Response already created, skip
if (createdResponses.indexOf(response.statusCode) > -1) {
return asyncCallback();
}

var params = {
restApiId: parameters.restApiId,
resourceId: parameters.resourceId,
Expand All @@ -208,6 +214,7 @@ function createResponses(parameters, callback) {
if (error) {
logger.log('Error ApiMethodService::createResponses', { error: error, params: params });
}
createdResponses.push(response.statusCode);
asyncCallback(error);
});
}, function (error) {
Expand Down
45 changes: 36 additions & 9 deletions tests/unit/commands/ApiMethodUnitTest.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,23 +69,43 @@ describe('ApiMethodCommand', function () {

describe('createMethod', function () {
it('should create resource', function (done) {
testSubject.createResource({}, {}, { params: {method: {}} }, function (error) {
testSubject.createResource({}, {}, { params: { method: {}} }, function (error) {
expect(error).to.be.undefined;
done();
});
});
it('should fail create resource', function (done) {
createMethodStub.yields('createError');
testSubject.createResource({}, {}, { params: {method: {}} }, function (error) {
testSubject.createResource({}, {}, { params: { method: {}} }, function (error) {
expect(error).to.equal('createError');
expect(createMethodStub.called).to.be.true;
expect(getForResponseStub.called).to.be.false;
done();
});
});
it('should fail create resource but with a proper physical resource id', function (done) {
createMethodStub.yields('createError', {});
testSubject.createResource({}, {}, { params: { resourceId: 'test', method: { httpMethod: 'GET' }} }, function (error) {
expect(error.message).to.equal('createError');
expect(error.physicalResourceId).to.equal('test/GET');
expect(createMethodStub.called).to.be.true;
expect(getForResponseStub.called).to.be.false;
done();
});
});
it('should fail create resource but with a proper physical resource id', function (done) {
createMethodStub.yields({ code: 'createError' }, {});
testSubject.createResource({}, {}, { params: { resourceId: 'test', method: { httpMethod: 'GET' }} }, function (error) {
expect(error.code).to.equal('createError');
expect(error.physicalResourceId).to.equal('test/GET');
expect(createMethodStub.called).to.be.true;
expect(getForResponseStub.called).to.be.false;
done();
});
});
it('should fail if get for response fails', function (done) {
getForResponseStub.yields('getForResponseError');
testSubject.createResource({}, {}, { params: {method: {}} }, function (error) {
testSubject.createResource({}, {}, { params: { method: {}} }, function (error) {
expect(error).to.equal('getForResponseError');
expect(createMethodStub.called).to.be.true;
expect(getForResponseStub.called).to.be.true;
Expand All @@ -102,6 +122,13 @@ describe('ApiMethodCommand', function () {
done();
});
});
it('should do nothingif physical resource id is not set properly', function (done) {
testSubject.deleteResource({ PhysicalResourceId: 'dummy' }, {}, { params: {method: {}} }, function (error) {
expect(error).to.be.undefined;
expect(deleteMethodStub.called).to.be.false;
done();
});
});
it('should fail delete resource', function (done) {
deleteMethodStub.yields('deleteError');
testSubject.deleteResource({ PhysicalResourceId: 'test/GET' }, {}, { params: {method: {}} }, function (error) {
Expand All @@ -123,13 +150,13 @@ describe('ApiMethodCommand', function () {
done();
});
});
it('should fallback to method from parameters if PhysicalResourceId is not properly set', function (done) {
it('should do nothing if PhysicalResourceId is not properly set', function (done) {
testSubject.updateResource({ PhysicalResourceId: 'dummy' }, {}, { params: {method: {}} }, function (error, resource) {
expect(error).to.be.undefined;
expect(resource).to.be.an('object');
expect(deleteMethodStub.called).to.be.true;
expect(createMethodStub.called).to.be.true;
expect(getForResponseStub.called).to.be.true;
expect(error).to.equal('Resource not found');
expect(resource).to.be.undefined;
expect(deleteMethodStub.called).to.be.false;
expect(createMethodStub.called).to.be.false;
expect(getForResponseStub.called).to.be.false;
done();
});
});
Expand Down
21 changes: 13 additions & 8 deletions tests/unit/service/ApiMethod/ApiMethodServiceUnitTest.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ describe('ApiMethodService', function () {
'method.response.header.Access-Control-Allow-Methods': 'GET',
'method.response.header.Access-Control-Allow-Origin': '*'
}
},
}
},
requestTemplates: {
'application/json': {}
Expand Down Expand Up @@ -131,15 +131,18 @@ describe('ApiMethodService', function () {
cacheNamespace: ''
},
responses: {
default: {
statusCode: "Response.StatusCode",
'default': {
statusCode: '200',
responseTemplates: {
ResponseTemplateObject: {},
ResponseTemplateString: ''
}
},
ResponseKey: {
statusCode: "Response.StatusCode"
'.*BadResponse1.*': {
statusCode: '500'
},
'.*BadResponse2.*': {
statusCode: '500'
}
}
}
Expand All @@ -148,6 +151,8 @@ describe('ApiMethodService', function () {
it('should create an api method', function (done) {
testSubject.createMethod(params, function (error, apiMethod) {
expect(error).to.be.undefined;
expect(putMethodResponseStub.calledTwice).to.be.true;
expect(putIntegrationResponseStub.calledThrice).to.be.true;
expect(apiMethod).to.be.an('object');
done();
});
Expand Down Expand Up @@ -235,7 +240,7 @@ describe('ApiMethodService', function () {
testSubject.createMethod(params, function (error, apiMethod) {
expect(error).to.be.an.Error;
expect(error).to.equal('putIntegrationError');
expect(apiMethod).to.be.undefined;
expect(apiMethod).to.an('object');
done();
});
});
Expand All @@ -244,7 +249,7 @@ describe('ApiMethodService', function () {
testSubject.createMethod(params, function (error, apiMethod) {
expect(error).to.be.an.Error;
expect(error).to.equal('putMethodResponseError');
expect(apiMethod).to.be.undefined;
expect(apiMethod).to.an('object');
done();
});
});
Expand All @@ -253,7 +258,7 @@ describe('ApiMethodService', function () {
testSubject.createMethod(params, function (error, apiMethod) {
expect(error).to.be.an.Error;
expect(error).to.equal('putIntegrationResponseError');
expect(apiMethod).to.be.undefined;
expect(apiMethod).to.an('object');
done();
});
});
Expand Down
Loading

0 comments on commit 3c15cdf

Please sign in to comment.