-
Notifications
You must be signed in to change notification settings - Fork 18
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
Fix handling of key attributes with CustomName attributes #78
base: master
Are you sure you want to change the base?
Conversation
## What? Fixes an issue where you have an inverted key for a GSI and you are using the CustomName attribute at the same time
| Some av -> values.Add(prop.Name, av) | ||
|
||
| Some av -> | ||
let exists, value = values.TryGetValue prop.Name |
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.
match
to avoid the allocation?
@@ -51,6 +61,31 @@ type ``Inverse GSI Table Operation Tests``(fixture: TableFixture) = | |||
|
|||
interface IClassFixture<TableFixture> | |||
|
|||
type ``Inverse GSI Table with Custom Names Operation Tests``(fixture: TableFixture) = | |||
|
|||
let rand = let r = Random.Shared in fun () -> int64 <| r.Next() |
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.
let rand = let r = Random.Shared in fun () -> int64 <| r.Next() | |
let rand = let r = Random.Shared in fun () -> r.Next() |> int64 |
Unsure if this is already common library usage but duplicating attributes in a record via |
@samritchie I agree, the CustomName attribute does invite some strange usage patterns and the error handling isn't as robust as it would have to be so that the end user falls into a pit of success. Ideally there would just be better validation e.g. if a user sets the CustomName attribute to the same name as another property we need to validate that the values of those properties are equal. I'm fine with putting this on ice, or maybe just adding a better error messages when this happens. We originally started using this pattern due to not being able to put the main key attributes and the GSI attributes on the same properties but that has been fixed. |
What?
Fixes an issue where you have an inverted key for a GSI and you are using the CustomName attribute at the same time