-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Validate API Gateway Response Keys #1154
Conversation
This adds a failing test for an example of invalid API Gateway output, and proposes a fix of disallowing missing status codes. Need to investigate possible regressions of this fix before finalizing and merging, may change the fix approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of small Pythonic nits
More explicit definition of the intended behavior.
The empty set is falsy, and we're checking for a non-empty set in this method. Therefore, can return directly as boolean.
@@ -63,6 +63,10 @@ def invalid_response_returned(event, context): | |||
return "This is invalid" | |||
|
|||
|
|||
def invalid_hash_response(event, context): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe just json response?
Python 2.7 appears to treat Dictionary's keys() method as an array, and then does not support the `-` operator with a set. Adding an explicit set() casting works in my Python 2.7 local testing.
Python 2.7 incompatibility is fixed, so this is ready for review again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just minor comments, will approve after those are addressed.
# API Gateway only accepts statusCode, body, headers, and isBase64Encoded in | ||
# a response shape. | ||
if LocalApigwService._invalid_apig_response(json_output): | ||
msg = "Invalid Response: " + str(json_output) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to be more explicit in the response about accepted keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can possibly move log statement up here just before throwing an exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To do this I'd probably want to actually return the invalid keys and then perform the boolean if at this top level. Should we move to that approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that sounds good, provides more context as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in most recent change.
"headers", | ||
"isBase64Encoded" | ||
} | ||
# In Python 2.7, need to explicitly make the Dictionary keys into a set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know!
Has the checker method return keys instead of a boolean, so that the exception message can be more detailed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥇
*Issue #, if available: Resolves Issue #522
Description of changes: Per my testing, API Gateway has 4 allowable response keys:
statusCode
,body
,headers
, andisBase64Encoded
. None of these keys are required, but any extra keys will result in a 502 error from the service. Currently, the local API Gateway implementation will ignore extra keys, this change will cause those keys to raise an error, matching service behavior.Checklist:
make pr
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.