Skip to content
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.

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

chore(amplify-graphql-searchable-transformer): refactor the expression to construct aggregate values #784

Merged
merged 1 commit into from
Sep 13, 2022

Conversation

phani-srikar
Copy link
Contributor

Description of changes

These changes refactor a step in the Request Mapping Template that is generated by the Searchable V2 transformer.
The step is responsible for constructing a Map that contains the Aggregate values that will eventually be used in the construction of the Search Query. The current refactor dynamically generates this Map instead of a raw JSON representation.
This is necessary for the Searchable Mocking to work since the mock VTL compiler does not read raw JSON representations.

Issue #, if available

Description of how you validated changes

The test snapshot updates show the change in the Request Mapping Template.
Added explicit checks as well.

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@phani-srikar phani-srikar requested a review from a team as a code owner September 7, 2022 22:31
@codecov-commenter
Copy link

Codecov Report

Merging #784 (35d9795) into main (98c021a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #784   +/-   ##
=======================================
  Coverage   63.39%   63.39%           
=======================================
  Files         278      278           
  Lines       18220    18222    +2     
  Branches     4431     4431           
=======================================
+ Hits        11550    11552    +2     
  Misses       6074     6074           
  Partials      596      596           
Impacted Files Coverage Δ
...earchable-transformer/src/generate-resolver-vtl.ts 86.36% <100.00%> (+1.36%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@phani-srikar phani-srikar changed the title chore(amplify-graphql-searchable-transformer): refactor and simplify the expression to construct aggregate values chore(amplify-graphql-searchable-transformer): refactor the expression to construct aggregate values Sep 7, 2022
AaronZyLee
AaronZyLee previously approved these changes Sep 9, 2022
Copy link
Contributor

@AaronZyLee AaronZyLee left a comment

Choose a reason for hiding this comment

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

The factor LGTM. Just one question on the raw JSON output. What the form will be after the refactor if it is not raw JSON?

@phani-srikar
Copy link
Contributor Author

phani-srikar commented Sep 9, 2022

The factor LGTM. Just one question on the raw JSON output. What the form will be after the refactor if it is not raw JSON?

It'll be the same map object but generated dynamically i.e using the map methods.

@marcvberg
Copy link
Contributor

Can we run E2E against this just for sanity?

…the expression to construct aggregate values
Copy link
Contributor

@marcvberg marcvberg left a comment

Choose a reason for hiding this comment

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

LGTM

@phani-srikar phani-srikar merged commit 0b7a1a5 into main Sep 13, 2022
@phani-srikar phani-srikar deleted the searchable-transformer-refactors branch September 13, 2022 20:11
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.

None yet

4 participants