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

ApiGatewayToDynamoDB: customize resource name on REST API #848

Closed
1 of 2 tasks
fargito opened this issue Nov 17, 2022 · 7 comments · Fixed by #898
Closed
1 of 2 tasks

ApiGatewayToDynamoDB: customize resource name on REST API #848

fargito opened this issue Nov 17, 2022 · 7 comments · Fixed by #898
Assignees
Labels
feature-request A feature should be added or improved needs-triage The issue or PR still needs to be triaged

Comments

@fargito
Copy link
Contributor

fargito commented Nov 17, 2022

Hello, I would like to customize the generated resource name on my REST API when using ApiGatewayToDynamoDB.

Use Case

Currently, the resource name is generated from the DynamoDB partition key

const apiGatewayResource: api.Resource = this.apiGateway.root.addResource("{" + partitionKeyName + "}");

However, I am trying to implement a more complex use case where my integration will query on the sort key. It feel weird to have to write:

readRequestTemplate: `{ \
  "TableName": "${dynamodbTable.tableName}", \
  "KeyConditionExpression": "PK = :v2 AND SK = :v1", \
  "ExpressionAttributeValues": { \
    ":v1": { \
      "S": "$input.params('PK')" \
    }, \
    ":v2": { \
      "S": "MY_VALUE" \
    } \
  } \
}`,

I understand that this is the default use case, but it should be customizable IMO.

Proposed Solution

Add an optional resourceName string parameter on the construct and use it to define the resource name if it is passed. Otherwise, use the table's partition key like in the current implementation.

Other

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

This is a 🚀 Feature Request

@fargito fargito added feature-request A feature should be added or improved needs-triage The issue or PR still needs to be triaged labels Nov 17, 2022
@biffgaut biffgaut self-assigned this Dec 19, 2022
@biffgaut
Copy link
Contributor

We understand the concept and can see the value - but when we read the snippet it looks reasonable to us. We will take a look at what an implementation involves - but could you provide an example of what the snippet would look like after the change you're proposing? (BTW - there's a significant change to API Gateway constructs coming imminently although this item is not addressed.)

@fargito
Copy link
Contributor Author

fargito commented Feb 7, 2023

The weird thing in the example I gave is that the SK = :v1 condition in the KeyConditionExpression uses a param named PK in the path (":v1": { "S": "$input.params('PK')" }). This would kind of seem like I mixed up my partition and sort keys.

My proposed change is to:

  • add an optional resourceName argument to the construct
  • if this resourceName is not defined, default to the partitionKeyName
  • when adding the resource, use this.apiGateway.root.addResource("{" + resourceName + "}");
  • then, say that for example, I have set resourceName: 'userId' as a construct argument, I'll be able to write
      readRequestTemplate: `{ \
         "TableName": "${dynamodbTable.tableName}", \
         "KeyConditionExpression": "PK = :v2 AND SK = :v1", \
         "ExpressionAttributeValues": { \
           ":v1": { \
             "S": "$input.params('userId')" \           <--- change here
           }, \
           ":v2": { \
             "S": "MY_VALUE" \
           } \
         } \
       }`,

which would be less confusing IMO.

I'll try and drop a PR to address this, WDYT?

@biffgaut
Copy link
Contributor

biffgaut commented Feb 9, 2023

We had put this on our list to implement, but will hold off if you want to give it a try. As we see it, there's a new optional attribute on the Construct props, resourceName. If supplied, this is used in place of partitionKeyName on line 249. It's also used in every input.params('${partitionKeyName}' statement, but "KeyConditionExpression": "${partitionKeyName} = :v1", \ statements remain unchanged. Your use case will still require a custom template, as you are swapping Sort Key for Primary Key.

This is only applicable to this construct, the default resource names on the other apigateway constructs are specific to the content (Records, Message, etc.). We'd want to see at least one more unit test and one more integration test.

Let us know if you want to take it on.

(re-reading this and your post, I guess it is pretty much exactly what you proposed above :-)

@fargito
Copy link
Contributor Author

fargito commented Feb 10, 2023

That's correct! I'll still need a custom template indeed

@biffgaut
Copy link
Contributor

biffgaut commented Feb 10, 2023

So just to clarify - will you be submitting a PR, or should we add it to our todo list?

@fargito
Copy link
Contributor Author

fargito commented Feb 10, 2023

I should be able to submit a PR at the beginning of next week!

@biffgaut
Copy link
Contributor

Great - we'll watch out for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved needs-triage The issue or PR still needs to be triaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants