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

Fix handling of key attributes with CustomName attributes #78

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

purkhusid
Copy link
Contributor

What?

Fixes an issue where you have an inverted key for a GSI and you are using the CustomName attribute at the same time

## 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
Copy link
Member

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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let rand = let r = Random.Shared in fun () -> int64 <| r.Next()
let rand = let r = Random.Shared in fun () -> r.Next() |> int64

@samritchie
Copy link
Collaborator

Unsure if this is already common library usage but duplicating attributes in a record via CustomName seems odd (especially key attributes) - aside from anything else I don’t know what the expected update behaviour should be. Are you able to go into a bit more detail about where/why you use this?

@purkhusid
Copy link
Contributor Author

@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.
Doing this validation proved harder that anticipated since comparing AttributeValue correctly is a bit painful.

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.

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

Successfully merging this pull request may close these issues.

3 participants