Skip to content

apigw clean up the invalid json parser #11968

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Dec 6, 2024
Merged

apigw clean up the invalid json parser #11968

merged 3 commits into from
Dec 6, 2024

Conversation

cloutierMat
Copy link
Contributor

@cloutierMat cloutierMat commented Nov 30, 2024 •
edited
Loading

Motivation

In an attempt to move fast to fix the issue, with @bentsku. we created #11967.

The naming was done hastily and we will attempt to clean it up a little in this pr.

Changes

  • clean up naming
  • simplify logic
  • adding a few tests

@cloutierMat cloutierMat added aws:apigateway Amazon API Gateway semver: patch Non-breaking changes which can be included in patch releases labels Nov 30, 2024
@cloutierMat cloutierMat added this to the 4.1 milestone Nov 30, 2024
@cloutierMat cloutierMat self-assigned this Nov 30, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 30, 2024 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±    0    2 suites  ±0   30m 47s ⏱️ - 1h 19m 4s
1 001 tests  - 2 815  943 ✅  - 2 555  58 💤  - 260  0 ❌ ±0 
1 003 runs   - 2 815  943 ✅  - 2 555  60 💤  - 260  0 ❌ ±0 

Results for commit c8b0a0a. ± Comparison against base commit d2d7409.

This pull request removes 2815 tests. 
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

♻️ This comment has been updated with latest results.

bentsku
bentsku reviewed Dec 2, 2024
View reviewed changes
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

Thanks a lot for cleaning up our... best? code! 😄 I think we still can improve the quote handling of the key, especially to match the statusCode value, but then we should be good to go! Thanks a lot again 🙏

localstack-core/localstack/services/apigateway/next_gen/execute_api/integrations/mock.py Outdated
Comment on lines 83 to 84
if key == "statusCode":
statuscode = int(value)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we had the mistake in our current implementation too, but here we directly check against the stateCode value without regards to single or double quotes it seems (except of the leftover second if check)

I think we need to still address that 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yes! I must have been tired when I wrote made the cleanup! The goal of moving it under the quotes assertions, was to solve this very issue! But it isn't removing the quotes anymore as we originally had 🤦

tests/unit/services/apigateway/test_mock_integration.py
Comment on lines +65 to +66
'{statusCode: 200, " ": " "}',
'{statusCode: 200, "": ""}',
Copy link
Contributor

Choose a reason for hiding this comment

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

we could also add a few cases with statusCode in single and double quotes, and maybe a few invalid with non closing quotes or something similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good thinking! that is indeed the most important objective 🤣

@cloutierMat cloutierMat marked this pull request as ready for review December 6, 2024 00:37
bentsku
bentsku approved these changes Dec 6, 2024
View reviewed changes
Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

LGTM! Just a minor simplification, but this is already much cleaner than before 🙏 Thanks a lot for addressing the comments and adding test cases! Really, this is such a weird case...

localstack-core/localstack/services/apigateway/next_gen/execute_api/integrations/mock.py Outdated
Comment on lines 80 to 82
key, value = [el.strip() for el in key_value]
key = convert_null_value(key)
value = convert_null_value(value)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could probably be simplified with:

Suggested change
key, value = [el.strip() for el in key_value]
key = convert_null_value(key)
value = convert_null_value(value)
key, value = [convert_null_value(el.strip()) for el in key_value]

I think we could even put the strip() call inside convert_null_value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, missed it but we could probably add type hints for convert_null_value(value: str) -> str:

@cloutierMat cloutierMat merged commit 891c13b into master Dec 6, 2024
31 checks passed
@cloutierMat cloutierMat deleted the apigw-cleanup-mock-json-parser branch December 6, 2024 02:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bentsku bentsku bentsku approved these changes

Assignees

@cloutierMat cloutierMat

Labels
aws:apigateway Amazon API Gateway semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.
2 participants
@cloutierMat @bentsku