Skip to content

fix(appsync): add caching config to AppSync resolvers#17815

Merged
mergify[bot] merged 11 commits intoaws:masterfrom
kylevillegas93:kylevillegas93/appsync-resolver-caching-config
Dec 6, 2021
Merged

fix(appsync): add caching config to AppSync resolvers#17815
mergify[bot] merged 11 commits intoaws:masterfrom
kylevillegas93:kylevillegas93/appsync-resolver-caching-config

Conversation

@kylevillegas93
Copy link
Copy Markdown
Contributor

@kylevillegas93 kylevillegas93 commented Dec 2, 2021

While trying to add caching config to some of my application's resolvers, I discovered that the BaseResolverProps do not include caching configuration like the CfnResolver does.

This PR adds this missing caching configuration to the BaseResolverProps and adds the configuration as part of the creation of the CfnResolver.


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

@gitpod-io
Copy link
Copy Markdown

gitpod-io bot commented Dec 2, 2021

@github-actions github-actions bot added the @aws-cdk/aws-appsync Related to AWS AppSync label Dec 2, 2021
@GarrettClyde
Copy link
Copy Markdown

Awesome. Thanks for doing this! Looking forward to its release.

export interface CachingConfig {
/**
* The caching keys for a resolver that has caching enabled.
* Valid values are entries from the $context.arguments, $context.source, and $context.identity maps.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you please also add validation for this constraint?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will do!

Copy link
Copy Markdown
Contributor Author

@kylevillegas93 kylevillegas93 Dec 3, 2021

Choose a reason for hiding this comment

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

Alright - added validation for caching keys + unit test - added some constants for $context.source, $context.arguments and $context.identity to help validate that caching keys are prefixed by these keys. Technically, there's some VTL syntax that should be adhered to but I think that may be a bit much for CDK to take on.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! One last thing: Before validating you should check whether the value is encoded as a token, using the Token.isUnresolved() static method.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah cool - I'll make this change!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Alright sweet - done! Checked to see if the caching key is resolved first before validating.


/**
* The TTL in seconds for a resolver that has caching enabled.
* Valid values are between 1 and 3600 seconds.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same thing with this one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will do!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added validation + unit tests for ttl!

otaviomacedo
otaviomacedo previously approved these changes Dec 6, 2021
@mergify mergify bot dismissed otaviomacedo’s stale review December 6, 2021 18:57

Pull request has been modified.

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Dec 6, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 52b535b into aws:master Dec 6, 2021
@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Dec 6, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 7f0130f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
While trying to add caching config to some of my application's resolvers, I discovered that the BaseResolverProps do not include caching configuration like the CfnResolver does.

This PR adds this missing caching configuration to the BaseResolverProps and adds the configuration as part of the creation of the CfnResolver. 

----

*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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants