-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
APIGW: fix TestInvokeMethod path logic #13030
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
Conversation
|
||
# we do not need a true HTTP request for the context, as we are skipping all the parsing steps and using the | ||
# provider data | ||
invocation_context = RestApiInvocationContext( | ||
request=Request(method=http_method), | ||
) | ||
path_query = test_request.get("pathWithQueryString", "/").split("?") | ||
test_request_path = test_request.get("pathWithQueryString") or resource_path |
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.
this single line is effectively the fix for it
try: | ||
# this is AWS behavior, it will accept any value for the `pathWithQueryString`, even if it doesn't match | ||
# the expected format. It will just fall back to no value if it cannot parse the path parameters out of it | ||
_, path_parameters = RestAPIResourceRouter(deployment).match(invocation_context) | ||
except Exception: | ||
path_parameters = {} |
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.
this is also part of the fix, due to the ability to pass invalid values, but it was not strictly needed for the sample to work
LocalStack Community integration with Pro 2 files ± 0 2 suites ±0 18m 16s ⏱️ - 1h 25m 44s Results for commit 12c143c. ± Comparison against base commit 3f463b5. This pull request removes 3474 tests.
♻️ This comment has been updated with latest results. |
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 35m 13s ⏱️ Results for commit 12c143c. ♻️ This comment has been updated with latest results. |
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.
Thank you for the thorough parity improvements 🚀
# this is AWS behavior, it will accept any value for the `pathWithQueryString`, even if it doesn't match | ||
# the expected format. It will just fall back to no value if it cannot parse the path parameters out of it | ||
_, path_parameters = RestAPIResourceRouter(deployment).match(invocation_context) | ||
except 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.
Are we ok swalling the exception here, or are there scenarios where debug logs would be necessary to understand a bug in .match
?
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.
Yes, I thought we're fine, the routing step is already done and we're using it only to extract the path parameters. As the input can be anything, any error could surface. I was thinking about adding debug logs, but I wasn't sure it would be very useful. But maybe just a warning for user could still be useful somehow 🤔 I'll add something
@@ -45,6 +45,19 @@ | |||
{formatted_date} : Method completed with status: {method_response_status} | |||
""" | |||
|
|||
TEST_INVOKE_TEMPLATE_MOCK = """Execution log for request {request_id} |
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.
praise: neat detail considering these verbose mock logs ✨
|
||
# we want to do very precise matching on the log, and splitting on new lines will help in case the snapshot | ||
# fails | ||
# the snapshot library does not allow to ignore an array index as the last node, so we need to put it in a dict |
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.
FYI: @tiurin snapshot library limitation to consider tracking
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.
This was copy-pasted from the previous utility, I actually fixed that issue with localstack/localstack-snapshot#16 but we don't yet have a release of localstack-snapshot
😅 soon I'll update these!
target_resource_id=resource_id, | ||
method="GET", | ||
) | ||
# assert "HTTP Method: GET, Resource Path: /pets/123" in response["log"] |
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: Is this comment intentional (same below)?
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.
Ah, good catch! I'll update them, I think I had the case once that the snapshots were not exactly what I was expecting from AWS, so those made sure that we got what we wanted. Will update them 👍
Motivation
We got a report that the
TestInvokeMethod
would not work when thepathWithQueryString
was not provided. After testing in AWS, it showed that this parameter was not required and this was an assumption that stayed from the previous implementation.This PR implements more tests around this parameter and the behavior of AWS when it is not provided.
It also showed AWS is not sending the same
log
response when the integration is of typeMOCK
.I took the opportunity to refactor a bit the utilities we have around
TestInvokeMethod
, because it is quite tricky to snapshot match as it returns a big string with newlines that we would like to snapshot match more precisely.Changes
TestInvokeMethod
whenpathWithQueryString
is not provided: it will fallback to theResource
path
valueMOCK
integration, this should probably be revisited at some point