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.

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

feat(appsync): add support for mapping DynamoDB queries #5940

Merged
merged 2 commits into from
Feb 4, 2020
Merged

feat(appsync): add support for mapping DynamoDB queries #5940

merged 2 commits into from
Feb 4, 2020

Conversation

duarten
Copy link
Contributor

@duarten duarten commented Jan 23, 2020

Add support to the L2 AppSync constructs for mapping DynamoDB queries.

Fixes #5861

Signed-off-by: Duarte Nunes duarte@uma.ni

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@duarten
Copy link
Contributor Author

duarten commented Jan 29, 2020

ping

@MrArnoldPalmer
Copy link
Contributor

@duarten Started to review this but most likely will be getting back to you tomorrow. Catching up a bit with the current state of AppSync. @shivlaks is also taking a look.

Didn't want to leave you hanging another day so just letting you know.

@duarten
Copy link
Contributor Author

duarten commented Jan 30, 2020

Thanks for the heads up, @MrArnoldPalmer! :)

MrArnoldPalmer
MrArnoldPalmer previously requested changes Feb 1, 2020
View reviewed changes
Copy link
Contributor

@MrArnoldPalmer MrArnoldPalmer left a comment

Choose a reason for hiding this comment

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

This feels like a great addition. I think we should add integ coverage of the other KeyConditions. Otherwise good to go.

I wonder if we can keep building on top of this to abstract away some of the dynamo specifics. Separate discussion though.

packages/@aws-cdk/aws-appsync/test/integ.graphql.ts Outdated
customerDS.createResolver({
typeName: 'Query',
fieldName: 'getCustomerOrders',
requestMappingTemplate: MappingTemplate.dynamoDbQuery(KeyCondition.eq('id', 'id')),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add coverage for the other KeyConditions we have defined. It's a bit tedious but feels like it will payoff in the long run.

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! Thanks for reviewing :)

@duarten
Copy link
Contributor Author

duarten commented Feb 1, 2020

I wonder if we can keep building on top of this to abstract away some of the dynamo specifics. Separate discussion though.

Yeah, I think that would be a good direction. The current mappings only support basic usage (e.g., dynamoDbGetItem only supports setting the partition key but not the sort key).

@duarten
feat(appsync): add support for mapping DynamoDB queries
c92200e 
Add support to the L2 AppSync constructs for mapping DynamoDB queries.

Fixes aws#5861

Signed-off-by: Duarte Nunes <duarte@uma.ni>
@duarten duarten force-pushed the feature/appsync-dynamo-query branch from 7e11115 to c92200e Compare February 1, 2020 02:48
@mergify mergify bot dismissed MrArnoldPalmer’s stale review February 1, 2020 02:49

Pull request has been modified.

@duarten
Copy link
Contributor Author

duarten commented Feb 1, 2020

PR updated with the remaining tests.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

MrArnoldPalmer
MrArnoldPalmer previously approved these changes Feb 3, 2020
View reviewed changes
@mergify
Copy link
Contributor

mergify bot commented Feb 3, 2020

Thank you for contributing! Your pull request is now being automatically merged.

@mergify mergify bot dismissed MrArnoldPalmer’s stale review February 4, 2020 21:34

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Feb 4, 2020

Thank you for contributing! Your pull request is now being automatically merged.

@mergify mergify bot merged commit 2240e97 into aws:master Feb 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrArnoldPalmer MrArnoldPalmer MrArnoldPalmer approved these changes

Assignees

@MrArnoldPalmer MrArnoldPalmer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for DynamoDB queries in AppSync mapping templates
3 participants
@duarten @aws-cdk-automation @MrArnoldPalmer