[BUG] Cannot add multiple attribute decorators for same property

lnovelli commented 4 months ago

Summary:

Hi @blazejkustra ! BIG kudos for developing this library, I truly enjoy using it.

I'm bumping into an issue that I think is related to the recently closed #27 . The bug appears when I try to add 2 attribute decorators (one main attribute, one GSI attribute) to a single property. This is normally possible when using CDK or the AWS console.

Code sample:

export class TableUser extends Entity {

  @attribute.partitionKey.string()
  @attribute.gsi.partitionKey.string({ indexName: 'index-1' })
  domain: string;

  @attribute.sortKey.string()
  @attribute.gsi.sortKey.string({ indexName: 'index-1' })
  email: string;

}

My TableManager is defined as follows:

export const UserdataTableManager = new TableManager(TableUser, {
  tableName: USERS_TABLE_NAME,
  partitionKey: 'domain',
  sortKey: 'email',
  indexes: {
    'index-1': {
      partitionKey: 'domain',
      sortKey: 'email',
    },
  },
  createdAt: 'createdAt',
  updatedAt: 'updatedAt',
});

Action performed:

If I remove the secondary attribute decorators, the code compiles. Is there anything I'm doing wrong? Let me know if you need any other information to help debug it!

Expected result:

I'd expect to be able to add multiple attribute decorators to a single property, no matter if they belong to GSI, LSI or main ones. Unfortunately this is blocking me from using the GSIs, but I LOVE this package and I hope we can find a fix.

Actual result:

I get this compile error:

DynamodeError [DynamodeStorageError]: Attribute "domain" was already decorated in entity "TableUser"

Environment:

MacOS Sonoma 14.4.1 Node v20.14

Other:

[ X] I have read through the Dynamode documentation before posting this issue
[ X] I have searched through the GitHub issues (including closed issues) and pull requests to ensure this issue has not already been raised before
[ X] I have searched the internet to ensure this issue hasn't been raised or answered before

blazejkustra commented 4 months ago

Hey, thank you! 🙌 Tbh I haven't thought about a case where primary key would be also a GSI at the same time.

Can you describe what would be the purpose for such index?

edit: Either way I'll look for a fix this or next week

lnovelli commented 4 months ago

Yes, I know it might sound a bit strange. I have a DB where some attributes are VERY heavy, so I'm creating a secondary index where I only project the lighter attributes, but I keep the same access modality (same partition/sort key). Does it make sense?

blazejkustra commented 4 months ago

Ah yes, I forgot about this use case, it makes sense. Thank you!

afronski commented 4 months ago

@blazejkustra This is also the case I missed here https://github.com/build-on-aws/aws-lambda-and-cqrs-a-winning-combination/commit/17439a4f3d456f021f2d13b58be49331ea426094#r136008382, but I did not describe it correctly there (plus I did not catch so far up as I promised). So to make it clearer - this was the 1st thing I missed and #27 (already fixed) was the 2nd thing.

Long story short: being able to add multiple decorators to the same property allows for a technique called index overloading, where you utilise an existing attribute multiple times. It is usually used to save on WCU (it may sound like penny pinching, but in case of tables with a lot of writes it adds up) and for easier way to preserve consistency without redundant values across multiple attributes - and it can be particularly useful when those attributes are already used as key(s).

EDOT: I can help with implementation if you need - just let me know! ✋

blazejkustra commented 4 months ago

I did a little research and know how to code it but didn't have the time last week. Thank you for your proposition @afronski, any contributions are welcome! In this case I would also be glad if you could review the changes once I create a PR, wdyt?

afronski commented 4 months ago

@blazejkustra sounds good to me - happy to help, feel free to ping me when it's ready!

blazejkustra commented 4 months ago

https://github.com/blazejkustra/dynamode/pull/31 is up @afronski, please have a look!

afronski commented 4 months ago

@blazejkustra thank you so much! I will post my remarks in #31.

blazejkustra commented 3 months ago

https://github.com/blazejkustra/dynamode/pull/31 was just merged 😄 The issue should be fixed in v1.4.0! Let me know if it works as you expected @lnovelli. Thanks for your help again @afronski

lnovelli commented 3 months ago

It's perfect! Many thanks @blazejkustra and @afronski 🙌

blazejkustra / dynamode