Added test cases and fixed issues for RequestValidator apigateway #7917
Conversation
There was a problem hiding this comment.
Congratulations on your first contribution!
It seems you've forked LocalStack, could you please push your branch directly to the localstack/localstack repo, so that the integration-tests-against-pro action could run? (It can run only for branches inside the repo). Because of the nature of API Gateway, it would be safer to run those as we have quite a lot of integrations.
Otherwise, I only have some minor comments about the test structure and a nit about a logic condition. Great use of all the snapshots, this is really great to see! Thanks a lot for tackling this and getting RequestValidator in parity with AWS! 🎉
| if key in ["validateRequestParameters", "validateRequestBody"]: | ||
| if value.lower() == "true": | ||
| value = True | ||
| else: | ||
| value = False |
There was a problem hiding this comment.
| if key in ["validateRequestParameters", "validateRequestBody"]: | |
| if value.lower() == "true": | |
| value = True | |
| else: | |
| value = False | |
| elif key in ("validateRequestParameters", "validateRequestBody"): | |
| value = value and value.lower() == "true" |
There was a problem hiding this comment.
You could simplify the logic a bit here, and also guard if value is not in the dictionary. Not sure what would actually happen here, I don't think we've covered this case either in our other tests.
There was a problem hiding this comment.
Thanks for the review @bentsku 🙏🚀🙂 So I had already checked this case before writing the logic above; if we don't specify the value in this case it takes default False value, which was covered in the above logic. Let me even write a test case for this.
There was a problem hiding this comment.
@bentsku - I have pushed the branch directly to localstack/localstack repo: https://github.com/localstack/localstack/pull/7926
| assert [ | ||
| { | ||
| "id": validator_id, | ||
| "name": name, | ||
| "validateRequestBody": False, | ||
| "validateRequestParameters": False, | ||
| } | ||
| ] == result["items"] |
There was a problem hiding this comment.
I think you can delete this test, I believe your tests are covering much more ground than this one. Our goal would be to basically progressively delete those in favour on the new ones.
| response = apigateway_client.get_request_validator( | ||
| restApiId=api_id, requestValidatorId=validator_id | ||
| ) | ||
| snapshot.match("get-request-validators", response) |
There was a problem hiding this comment.
We could add this to test_create_request_validator, as it is basically the same test extended with only one operation? Same for test_get_request_validators.
I feel like we could condense some of the tests with the flow:
- create a validator
- get the validator
- get all validators
- delete the validator
- try getting the deleted validator
- get all validators
This would look a lot liketest_delete_request_validator, which cover the same ground astest_create_request_validatorandtest_get_request_validators.
With snapshot, we can take advantage of the fact that we can setup the resource one and follow its lifecycle.
We have good examples of this flow in the new lambda tests (see here:)
Otherwise, it's impeccable and I like how you structured the tests, and it's very clear, thanks a lot for tackling this!
| @@ -1083,11 +1081,36 @@ def update_request_validator( | |||
| f"Validator {request_validator_id} for API Gateway {rest_api_id} not found" | |||
| ) | |||
|
|
|||
| patched_validator = apply_json_patch_safe(validator, patch_operations) | |||
| rest_api_container.validators[request_validator_id] = patched_validator | |||
| for operation in patch_operations: | |||
There was a problem hiding this comment.
nit: what if we instead of operation use patch_operation and after instead of operation_performed we can use operation
There was a problem hiding this comment.
Thanks @calvernaz for the review 🚀🙏🙂
For
RequestValidatorAPI in apigateway :test_request_validator