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

A (little) more dynamic [DynamoDbProperty] attribute #239

Open
devtekve opened this issue May 10, 2024 · 4 comments
Open

A (little) more dynamic [DynamoDbProperty] attribute #239

devtekve opened this issue May 10, 2024 · 4 comments

Comments

@devtekve
Copy link

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.

@firenero
Copy link
Contributor

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.

@devtekve
Copy link
Author

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!!

@Dreamescaper
Copy link

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.

@firenero
Copy link
Contributor

firenero commented Jun 8, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants