Skip to content

fix APIGW request validation with empty request body#7916

Merged
bentsku merged 2 commits intomasterfrom
fix-apigw-body-validation
Mar 20, 2023
Merged

fix APIGW request validation with empty request body#7916
bentsku merged 2 commits intomasterfrom
fix-apigw-body-validation

Conversation

@bentsku
Copy link
Contributor

@bentsku bentsku commented Mar 20, 2023

Following #7842, we were missing a step in the validation: if the request body was empty, we should consider the request body as an empty JSON object to allow the validation to take effect, and not throw an error for invalid JSON.

I've extended the existing test for the request validator integration to add the GET method, and test with a request with no request body.

@bentsku bentsku requested a review from calvernaz as a code owner March 20, 2023 16:24
@bentsku bentsku temporarily deployed to localstack-ext-tests March 20, 2023 16:24 — with GitHub Actions Inactive
@bentsku bentsku requested a review from whummer March 20, 2023 16:24
Copy link
Member

@whummer whummer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great extension for the previous fix, kudos for jumping on this one so quickly! 🚀

"httpMethod": "POST",
"methodIntegration": {
"cacheKeyParameters": [],
"cacheNamespace": "5l4d79",
Copy link
Member

@whummer whummer Mar 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we could add a transformer (e.g., key_value(..)) to store a placeholder for this value - to make the test repeatable against AWS (without updating the snapshot) as well. 👍

Copy link
Contributor Author

@bentsku bentsku Mar 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, yes! Good catch. It is repeatable at the moment because we're skipping the verification of the snapshot because methodIntegration format is very not in parity, but I'll add the transformer right now, because the git diff is ugly. Thanks for pointing this out!
edit: I actually missed that one and it was correct, we were returning the field, so thanks again.

@bentsku bentsku temporarily deployed to localstack-ext-tests March 20, 2023 16:54 — with GitHub Actions Inactive
@coveralls
Copy link

Coverage Status

Coverage: 85.13% (-0.004%) from 85.135% when pulling 03f32c0 on fix-apigw-body-validation into 10e77c0 on master.

@github-actions
Copy link

LocalStack integration with Pro

       3 files  ±0         3 suites  ±0   1h 45m 2s ⏱️ + 9m 29s
1 828 tests ±0  1 440 ✔️ ±0  388 💤 ±0  0 ±0 
2 550 runs  ±0  1 806 ✔️ ±0  744 💤 ±0  0 ±0 

Results for commit 03f32c0. ± Comparison against base commit 10e77c0.

@bentsku bentsku merged commit d28e4c3 into master Mar 20, 2023
@bentsku bentsku deleted the fix-apigw-body-validation branch March 20, 2023 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants