-
Notifications
You must be signed in to change notification settings - Fork 246
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
feat(aws-apigateway-dynamodb): add optional resourceName parameter #898
feat(aws-apigateway-dynamodb): add optional resourceName parameter #898
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
a2f8e5b
to
416173b
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least one of the integration tests is failing -
@aws-solutions-constructs/aws-apigateway-dynamodb: ├─ [~] AttributeDefinitions
@aws-solutions-constructs/aws-apigateway-dynamodb: │ └─ @@ -1,6 +1,10 @@
@aws-solutions-constructs/aws-apigateway-dynamodb: │ [ ] [
@aws-solutions-constructs/aws-apigateway-dynamodb: │ [ ] {
@aws-solutions-constructs/aws-apigateway-dynamodb: │ [-] "AttributeName": "id",
@aws-solutions-constructs/aws-apigateway-dynamodb: │ [+] "AttributeName": "PK",
@aws-solutions-constructs/aws-apigateway-dynamodb: │ [ ] "AttributeType": "S"
@aws-solutions-constructs/aws-apigateway-dynamodb: │ [+] },
@aws-solutions-constructs/aws-apigateway-dynamodb: │ [+] {
@aws-solutions-constructs/aws-apigateway-dynamodb: │ [+] "AttributeName": "SK",
@aws-solutions-constructs/aws-apigateway-dynamodb: │ [+] "AttributeType": "S"
@aws-solutions-constructs/aws-apigateway-dynamodb: │ [ ] }
@aws-solutions-constructs/aws-apigateway-dynamodb: │ [ ] ]
@aws-solutions-constructs/aws-apigateway-dynamodb: └─ [~] KeySchema (requires replacement)
@aws-solutions-constructs/aws-apigateway-dynamodb: └─ @@ -1,6 +1,10 @@
@aws-solutions-constructs/aws-apigateway-dynamodb: [ ] [
@aws-solutions-constructs/aws-apigateway-dynamodb: [ ] {
@aws-solutions-constructs/aws-apigateway-dynamodb: [-] "AttributeName": "id",
@aws-solutions-constructs/aws-apigateway-dynamodb: [+] "AttributeName": "PK",
@aws-solutions-constructs/aws-apigateway-dynamodb: [ ] "KeyType": "HASH"
@aws-solutions-constructs/aws-apigateway-dynamodb: [+] },
@aws-solutions-constructs/aws-apigateway-dynamodb: [+] {
@aws-solutions-constructs/aws-apigateway-dynamodb: [+] "AttributeName": "SK",
@aws-solutions-constructs/aws-apigateway-dynamodb: [+] "KeyType": "RANGE"
@aws-solutions-constructs/aws-apigateway-dynamodb: [ ] }
@aws-solutions-constructs/aws-apigateway-dynamodb: [ ] ]
@aws-solutions-constructs/aws-apigateway-dynamodb: Outputs
From a quick perusal, it looks like the PR changes the output for integ.additional-request-templates.ts.
I added an integration test, I'll check |
1c847b4
to
cf28cbd
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
4f8ad15
to
4272c67
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Ok, I managed to get it working! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the submission! One thing to clear up - the initial issue we set out to fix was that the resourceName was tightly coupled to the partitionKey name. In this iteration they are still tightly coupled in the code for the readRequestTemplate at line 269, but now the partitionKeyName is assumed to be the same as the resourceName. Since the client must provide a partitionKeyName in the TableProps, this sets up a situation where a client could provide different values - which I think would lead to the read request not working as the partitionKeyName at line 281 would be incorrect.
We expected the readRequestTemplate to look like this:
const readRequestTemplate = props.readRequestTemplate ??
`{ \
"TableName": "${this.dynamoTable.tableName}", \
"KeyConditionExpression": "${partitionKeyName} = :v1", \
"ExpressionAttributeValues": { \
":v1": { \
"S": "$input.params('${resourceName}')" \
} \
} \
}`;
That decouples the partitionKeyName and resourceName.
Integration test looks good, but we need an unit test or two to confirm the correctness of the Construct output (unit tests are in apigateway-dynamodb.test.ts)
687ceff
to
7980c93
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
7980c93
to
a794aa1
Compare
@biffgaut you're absolutely right, I edited this, and also added a unit test |
a794aa1
to
aa078d4
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Looks like this is ready for review, but just want to confirm before diving in? |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
That is correct @biffgaut 🙂 |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Great contribution - Thanks! |
This will go out with the next release - since we just dropped a release yesterday the next one will probably in in the next week. |
Thanks a lot! 🚀 |
Fixes #848
Description of changes:
ApiGatewayToDynamoDB
constructBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.