-
Notifications
You must be signed in to change notification settings - Fork 110
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 #14 by encoding non-alpha characters in attribute keys #115
base: master
Are you sure you want to change the base?
Fix #14 by encoding non-alpha characters in attribute keys #115
Conversation
Not sure whats up with the coverage. I'll try and look into it. Any suggestions would be helpful. |
It seems that this change would also effect attributes that use AWS Naming Rules: http://docs.aws.amazon.com/amazondynamodb/latest/developerguide/HowItWorks.NamingRulesDataTypes.html |
@dangerginger No, this does not transform the key actually stored in the database. It affects the alias that is used to reference a key in a projection/query expression. Basically, it escapes key names that AWS does not permit to appear bare in an expression ( Applying this transformation to everything but ASCII letters is simple, easy to verify, and does not affect the operation nor the results of the query in any observable way, except that it will work in more cases than simply stripping out unacceptable characters (because two distinct keys could become equal by simply stripping out unacceptable characters, and then we have a name collision). |
Ok I'm good with this one. @C45tr0 any last thoughts? If not let's get this merged. |
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.
👍
Do I just need to rebase this and we are good? Sorry, didn't see the responses on here. |
@C45tr0 I left a code comment above pointing out a possible cleaned key collision scenario. |
@cdhowie are you talking about the comment from your PR? #37 (comment) Sorry, if I am just over looking it, but I do not see a code comment in this pr. Also, referring to the collisions, the test added does do some checks of the specific scenario you pointed out. Of course adding more would be good, but not sure of other scenarios at this time. |
4d292fa
to
e6e1111
Compare
Is there something I can do to help get this merged? |
Hey, this bug fix is not merged yet? |
Update to #37 to include Document paths