Skip to content

fix vtl resolver patch #11886

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 2 commits into from
Nov 20, 2024
Merged

fix vtl resolver patch #11886

merged 2 commits into from
Nov 20, 2024

Conversation

cloutierMat
Copy link
Contributor

Motivation

While investigating the issue reported by #11843, I noticed that the default returned value of unfound variables differs between apigateway and appsync. We had this patch working for apigateway, but it would unfortunately fail some appsync functions that are expecting to receive a null value.

Changes

  • Augmented the signature of prepare_namespace to allow setting a source.
  • Patch only if the source is apigateway
  • Reimplemented the template used by the previous test reffered in the comment

fixes #11843

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

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 43m 38s ⏱️ +5s
3 563 tests +1  3 227 ✅ +1  336 💤 ±0  0 ❌ ±0 
3 565 runs  +1  3 227 ✅ +1  338 💤 ±0  0 ❌ ±0 

Results for commit 92d8bd5. ± Comparison against base commit ad1af78.

This pull request removes 1 and adds 2 tests. Note that renamed tests count towards both. 
tests.aws.services.apigateway.test_apigateway_kinesis ‑ test_apigateway_to_kinesis

tests.aws.services.apigateway.test_apigateway_kinesis ‑ test_apigateway_to_kinesis[PutRecord]
tests.aws.services.apigateway.test_apigateway_kinesis ‑ test_apigateway_to_kinesis[PutRecords]

@cloutierMat cloutierMat marked this pull request as ready for review November 20, 2024 21:10
@cloutierMat cloutierMat requested a review from bentsku as a code owner November 20, 2024 21:10
@cloutierMat cloutierMat requested a review from simonrw November 20, 2024 21:11
@cloutierMat
Copy link
Contributor Author

@simonrw I added you here as it directly impacts appsync 😉

simonrw
simonrw approved these changes Nov 20, 2024
View reviewed changes
Copy link
Contributor

@simonrw simonrw left a comment

Choose a reason for hiding this comment

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

Great change, thank you for making a clean implementation on being able to dispatch between AppSync and API Gateway behaviour!

bentsku
bentsku approved these changes Nov 20, 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! Neat fix, thanks for updating and validating the test! Glad that it still caught a possible regression... 😅

@cloutierMat cloutierMat merged commit 77b14d7 into master Nov 20, 2024
45 checks passed
@cloutierMat cloutierMat deleted the appsync-fix-isNull branch November 20, 2024 22:44
@cloutierMat cloutierMat modified the milestones: 4.1, 4.0 Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonrw simonrw simonrw approved these changes

@bentsku bentsku bentsku approved these changes

Assignees

@cloutierMat cloutierMat

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

Successfully merging this pull request may close these issues.

bug: Appsync graphql errors when including multiple fields in query
3 participants
@cloutierMat @simonrw @bentsku