probot
Fix error response
The error field is not valid for an API Gateway response. As noted in the AWS docs below, when an error response contains invalid fields, it will always return a 502 error code with a body of {"message": "Internal server error"}. This PR updates the error response fields to contain valid fields and a more sensible response message.
- https://docs.aws.amazon.com/lambda/latest/dg/services-apigateway.html#services-apigateway-errors
Actually, I have some additional thoughts/questions on this. @gr2m, can we remove the try/catch block in this file? Here's my reasoning:
According to the docs linked in my original post:
API Gateway treats all invocation and function errors as internal errors. If the Lambda API rejects the invocation request, API Gateway returns a 500 error code. If the function runs but returns an error, or returns a response in the wrong format, API Gateway returns a 502. In both cases, the body of the response from API Gateway is {"message": "Internal server error"}.
When the serverless.yaml file has a value of async: false (the default value), the try/catch works fine because API Gateway waits for either the 200 or 500 response that is returned by the Lambda function and returns that to the client.
However, when a value of async: true is used, where API Gateway does not wait for a response from and instead returns a 200 response to the client immediately, the try/catch block makes it hard to track down errors (as in #78) since the Lambda function does not currently throw any errors. So in this case, we have both an API Gateway response returning a 200 and also the Lambda function not throwing any errors despite errors having actually occurred.
The impact of removing the try/catch block would be this:
When async: false (the default), if an error occurs, the Lambda will correctly throw an error, and API Gateway will return a 502 response with a JSON response of {"message": "Internal server error"}.
When async: true, if an error occurs, the Lambda will correctly throw an error, but API Gateway will still continue to return a 200 response as expected.
Hopefully that all makes sense. I can elaborate more if necessary.
Hopefully that all makes sense. I can elaborate more if necessary.
No I trust you on this one :)
No I trust you on this one :)
Awesome! This all sounds good in theory, but I'd like to test it locally before pushing those changes to this PR. I will try to test later today and update this PR with my findings.
@gr2m, we should be all set to merge this. I just pushed some changes and tested them locally.
The tables below show how Probot errors are interpreted by the respective AWS services on the master branch vs. my fix-error-response PR branch when async is true or false.
It was the Lambda column that was messed up prior to these changes.
master Branch
| - | Lambda | API Gateway |
|---|---|---|
async: false |
does not report errors | does report errors |
async: true |
does not report errors | does not report errors |
fix-error-response Branch
| - | Lambda | API Gateway |
|---|---|---|
async: false |
does report errors | does report errors |
async: true |
does report errors | does not report errors |
@gr2m, any chance we can get this, #111, and #112 merged today?
@gr2m, just checking in. can these PRs be merged?
I closed this to prevent the deletion from getting merged.
These changes were included as a part of #111 (see my note in the PR body)
sweet, thank you! I was just confused about the merge conflicts, now it makes sense