Skip to content

APIGW: implement UpdateMethodResponse and add lifecycle and negative tests for MethodResponse #12821

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
Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

APIGW: implement UpdateMethodResponse and add lifecycle and negative tests for MethodResponse #12821

wants to merge 5 commits into from
+1,053 −7

Conversation

ArthurAkh
Copy link
Contributor

@ArthurAkh ArthurAkh commented Jun 30, 2025 •
edited
Loading

Motivation

Add the implementation of the UpdateMethodResponse operation in the API Gateway v1 provider, and improve the test coverage for MethodResponse.

Changes

  • Implemented UpdateMethodResponse in the API Gateway v1 provider.

  • Added validation logic for:

    • Invalid patch operations
    • Invalid statusCode values
    • Combined validation errors to match AWS’s multi-error response format
    • Added the following test cases:
      • test_lifecycle_method_response
      • test_update_method_response
      • test_update_method_response_negative_tests
      • test_update_method_response_wrong_operations
      • test_update_method_wrong_param_names
      • test_update_method_lack_response_parameters_and_models

@ArthurAkh ArthurAkh added this to the 4.6 milestone Jun 30, 2025
@ArthurAkh ArthurAkh self-assigned this Jun 30, 2025
@ArthurAkh ArthurAkh added the semver: patch Non-breaking changes which can be included in patch releases label Jun 30, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 30, 2025 •
edited
Loading

Test Results - Preflight, Unit

21 795 tests  ±0   20 138 ✅ ±0   6m 15s ⏱️ -13s
     1 suites ±0    1 657 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit f41a5cb. ± Comparison against base commit a6ec498.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 30, 2025 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   18m 8s ⏱️ - 1h 28m 10s
1 150 tests  - 3 756  1 084 ✅  - 3 046  66 💤  - 710  0 ❌ ±0 
1 152 runs   - 3 756  1 084 ✅  - 3 046  68 💤  - 710  0 ❌ ±0 

Results for commit f41a5cb. ± Comparison against base commit a6ec498.

This pull request removes 3762 and adds 6 tests. Note that renamed tests count towards both. 
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]
…

tests.aws.services.apigateway.test_apigateway_api.TestApigatewayIntegration ‑ test_lifecycle_method_response
tests.aws.services.apigateway.test_apigateway_api.TestApigatewayIntegration ‑ test_update_method_lack_response_parameters_and_models
tests.aws.services.apigateway.test_apigateway_api.TestApigatewayIntegration ‑ test_update_method_response
tests.aws.services.apigateway.test_apigateway_api.TestApigatewayIntegration ‑ test_update_method_response_negative_tests
tests.aws.services.apigateway.test_apigateway_api.TestApigatewayIntegration ‑ test_update_method_response_wrong_operations
tests.aws.services.apigateway.test_apigateway_api.TestApigatewayIntegration ‑ test_update_method_wrong_param_names

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 30, 2025 •
edited
Loading

Test Results (amd64) - Acceptance

7 tests  ±0   5 ✅ ±0   3m 11s ⏱️ +2s
1 suites ±0   2 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit f41a5cb. ± Comparison against base commit a6ec498.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 30, 2025 •
edited
Loading

Test Results (amd64) - Integration, Bootstrap

    5 files      5 suites   35m 45s ⏱️
1 174 tests 1 108 ✅ 66 💤 0 ❌
1 180 runs  1 108 ✅ 72 💤 0 ❌

Results for commit f41a5cb.

♻️ This comment has been updated with latest results.

@ArthurAkh ArthurAkh marked this pull request as ready for review July 1, 2025 08:58
bentsku
bentsku reviewed Jul 1, 2025
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.

This is great work! Very nice, and very nice implementation of the update_method_response operation. This is looking really good!

I only have a few questions and comments, one is unrelated to your PR directly but is something that was set before but that you can update to make your life easier 👌

Only have one question regarding one case where the parameter didn't exist before and where we could maybe raise an unhandled KeyError, we could cover this with an additional test case.

Once this is addressed, we're good to merge! 🚀 Nice work

@baermat
Copy link
Member

baermat commented Jul 1, 2025

Since this PR was opened very recently, if it is really going into tomorrow's 4.6 release, please make sure to do so before the freeze, and to create a highlight and a docs board entry if applicable :)

bentsku
bentsku approved these changes Jul 1, 2025
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! Thanks for addressing the comments super fast, and adding even more validation, this looks great! Thank you so much for adding this previously unsupported operation! 🙏

localstack-core/localstack/services/apigateway/legacy/provider.py
Comment on lines +1034 to +1037
if method_response.response_models is None:
method_response.response_models = {}
if method_response.response_parameters is None:
method_response.response_parameters = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

nice catch! great thinking 👌

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

@cloutierMat cloutierMat Awaiting requested review from cloutierMat cloutierMat is a code owner

Assignees

@ArthurAkh ArthurAkh

Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.6
Development

Successfully merging this pull request may close these issues.
3 participants
@ArthurAkh @baermat @bentsku