EfficientDynamoDb icon indicating copy to clipboard operation
EfficientDynamoDb copied to clipboard

A (little) more dynamic [DynamoDbProperty] attribute

Open devtekve opened this issue 1 year ago • 4 comments

Currently, the [DynamoDbProperty(name)] attribute necessitates specifying a property name, which could be more streamlined. An overload that automatically derives the property name from the class property it’s mapped to would greatly simplify the implementation. This would allow developers to maintain clean code while still ensuring that properties are correctly mapped to DynamoDB attributes.

devtekve avatar May 10 '24 11:05 devtekve

I have mixed feelings about this feature. It's the same request as we had before #180. The reasons why we didn't implement it originally are still valid. So far I'm not sure that providing a way to make an accidental mistake worth the reward.

For example, this workaround

[DynamoDbProperty(nameof(Value))]
public int Value { get; set; }

doesn't look much worse for me than this code:

[DynamoDbProperty]
public int Value { get; set; }

That said, I am open to discussion. Maybe I'm missing something and there are more benefits to it rather than saving a few chars.

firenero avatar May 11 '24 01:05 firenero

I'll have to take a look at #180 (I'm on the phone right now) but I can argue that your suggestion and the suggestion I added both are still supported, it's more a matter of choice. In fact, your suggestion and mine both translate to the same CLR code. The main benefit is much easier copy paste or quick provisioning of multiple properties. If I need to be explicit then I can use the overload to provide the custom name I need tho. Right now I can't see what mistake could the user make by not providing the name explicitly, but I will take a closer look tomorrow at #180, I might be missing something...

Thanks for taking a look!!

devtekve avatar May 11 '24 02:05 devtekve

Personally I prefer the approach of AWS's SDK when attribute is not required by default. The reasons outlined in #180 ("Renaming a property will impact your DDB attribute name and likely cause bugs.", "RCU and WCU consumption depends on item size that includes not only data but attribute name.") are not applicable for me - this is my DB model, I want names to match in 99% of cases.

Dreamescaper avatar Jun 03 '24 13:06 Dreamescaper

At this point we can't remove attributes requirement even if we wanted to because it will break all existing users. We can add the change from this PR to make attributes to infer the name of the property though. I don't like this change personally but it's one of the common requests so I might give up soon :)

this is my DB model, I want names to match in 99% of cases.

to be fair, nothing prevents you from using [DynamoDbProperty(nameof(YourProperty))] to achieve that. Sure, it's more verbose that not everyone likes but it makes the intention clear. However, I agree that it's something you can decide on within your team so I'll give it a thought again and maybe merge the PR.

firenero avatar Jun 08 '24 08:06 firenero