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

@aws-cdk/aws-appsync: Add ability to create resolvers with "operation": "BatchInvoke" #14079

Closed
2 tasks
asnaseer-resilient opened this issue Apr 9, 2021 · 9 comments · Fixed by #15283
Closed
2 tasks
Labels
@aws-cdk/aws-appsync Related to AWS AppSync effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2

Comments

@asnaseer-resilient
Copy link

I want to setup an AppSync resolver using the CDK to use "operation": "BatchInvoke" as described here https://docs.aws.amazon.com/appsync/latest/devguide/tutorial-lambda-resolvers.html#advanced-use-case-batching

I am currently using the createResolver function to create my resolvers but cannot see a way to specify this type of operation - see here: https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-appsync.LambdaDataSource.html#createwbrresolverpropsspan-[…]y-change-without-noticespan

Use Case

The link above describes my use case.

Proposed Solution

None

Other

N/A

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change

This is a 🚀 Feature Request

@asnaseer-resilient asnaseer-resilient added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Apr 9, 2021
@github-actions github-actions bot added the @aws-cdk/aws-appsync Related to AWS AppSync label Apr 9, 2021
@MrArnoldPalmer MrArnoldPalmer added effort/small Small work item – less than a day of effort p2 and removed needs-triage This issue or PR still needs to be triaged. labels Apr 9, 2021
@asnaseer-resilient
Copy link
Author

Any chance of this getting implemented in the near future?

@MrArnoldPalmer
Copy link
Contributor

@asnaseer-resilient the core team is not actively working on adding features to appsync right now. PRs from the community are always welcome or you may be able to implement this by extending the BaseDataSource.

I don't have any experience using BatchInvoke in CFN, @BryanPan342 you have any guidance to add here for implementing?

@BryanPan342
Copy link
Contributor

I also don't have experience with BatchInvoke, but it looks like a Mapping Template issue that I honestly don't have that good of an answer for 😔

I know @duarten created an RFC for mapping templates and I think that would be nice to have even though JS is looking like an alternative.. would definitely require more thought

@duarten
Copy link
Contributor

duarten commented May 6, 2021

Most of that effort is in https://github.com/umani/ribosome, which works but isn't pretty (the API was designed to be compatible with the constraints of jsii, in case it would ever be merged into the cdk). But yeah, looking forward to JS resolvers :)

@asnaseer-resilient
Copy link
Author

I have had a look around the code base and I think the change would need to be made in mapping-template.ts in this method:

public static lambdaRequest(payload: string = '$util.toJson($ctx)'): MappingTemplate {

That method currently seems to hardwire the operation to "Invoke" as follows:

  public static lambdaRequest(payload: string = '$util.toJson($ctx)'): MappingTemplate {
    return this.fromString(`{"version": "2017-02-28", "operation": "Invoke", "payload": ${payload}}`);
  }

So maybe you could introduce an additional optional parameter as follows for backwards compatibility:

  public static lambdaRequest(payload: string = '$util.toJson($ctx)', operation: string = 'Invoke'): MappingTemplate {
    return this.fromString(`{"version": "2017-02-28", "operation": "${operation}", "payload": ${payload}}`);
  }

and then expose the ability to set this in the CDK. I am not really familiar enough with any of the CDK codebase to attempt to make any changes to it myself.

@BryanPan342
Copy link
Contributor

@asnaseer-resilient I'm currently working on the schema validation PR, but ill see what I can do. It does seem pretty straightforward but I want to make sure I read more about mapping templates before I implement anything that will change the API.

@asnaseer-resilient
Copy link
Author

@BryanPan342 - Any updates on this?

@BryanPan342
Copy link
Contributor

@BryanPan342 - Any updates on this?

I'm finishing up my school quarter atm so don't have the bandwidth atm, but will look to implement it sometime in the summer

@MrArnoldPalmer MrArnoldPalmer removed their assignment Jun 21, 2021
@mergify mergify bot closed this as completed in #15283 Jul 19, 2021
mergify bot pushed a commit that referenced this issue Jul 19, 2021
… template (#15283)

**[CORE CHANGES]**
Add optional `operation` parameter to `lamdaRequest` mapping template.
- Defaults to `"Invoke"`
- Allows for `"BatchInvoke"` operations directly through the static `lambdaRequest` function

**[MISC]**
* Add integration test w/ a verification script to test mapping template
* preliminary mapping template unit tests (created an issue to create more testing #15274 
* Use `path.resolve()` to resolve testing integration test in different directories

Fixes: #14079 

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this issue Aug 3, 2021
… template (aws#15283)

**[CORE CHANGES]**
Add optional `operation` parameter to `lamdaRequest` mapping template.
- Defaults to `"Invoke"`
- Allows for `"BatchInvoke"` operations directly through the static `lambdaRequest` function

**[MISC]**
* Add integration test w/ a verification script to test mapping template
* preliminary mapping template unit tests (created an issue to create more testing aws#15274 
* Use `path.resolve()` to resolve testing integration test in different directories

Fixes: aws#14079 

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this issue Aug 26, 2021
… template (aws#15283)

**[CORE CHANGES]**
Add optional `operation` parameter to `lamdaRequest` mapping template.
- Defaults to `"Invoke"`
- Allows for `"BatchInvoke"` operations directly through the static `lambdaRequest` function

**[MISC]**
* Add integration test w/ a verification script to test mapping template
* preliminary mapping template unit tests (created an issue to create more testing aws#15274 
* Use `path.resolve()` to resolve testing integration test in different directories

Fixes: aws#14079 

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-appsync Related to AWS AppSync effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants