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

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

Conversation

kylevillegas93
Copy link
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

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

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
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
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
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
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
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
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
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
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
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
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
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
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.

None yet

4 participants