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 #14 by encoding non-alpha characters in attribute keys #115

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

Conversation

C45tr0
Copy link
Contributor

@C45tr0 C45tr0 commented Oct 20, 2017

Update to #37 to include Document paths

@coveralls
Copy link

coveralls commented Oct 20, 2017

Coverage Status

Coverage decreased (-73.9%) to 24.784% when pulling 65180d5 on C45tr0:encode-expression-attribute-names-and-values into f0b023d on clarkie:master.

@C45tr0
Copy link
Contributor Author

C45tr0 commented Oct 20, 2017

Not sure whats up with the coverage. I'll try and look into it. Any suggestions would be helpful.

@coveralls
Copy link

coveralls commented Oct 20, 2017

Coverage Status

Coverage increased (+0.002%) to 98.544% when pulling e6e1111 on C45tr0:encode-expression-attribute-names-and-values into df7acfe on clarkie:master.

@jkav77
Copy link
Collaborator

jkav77 commented Nov 30, 2017

It seems that this change would also effect attributes that use -, _, ., and0-9 which are otherwise permissible keys. I think this is an unintended consequence that would effect people using keys like user-name who do not want to have the - encoded this way. Am I reading this right?

AWS Naming Rules: http://docs.aws.amazon.com/amazondynamodb/latest/developerguide/HowItWorks.NamingRulesDataTypes.html

@cdhowie
Copy link
Collaborator

cdhowie commented Nov 30, 2017

@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 (ProjectionExpression, FilterExpression, etc.). Note that we apply this process to every key already, because some words are reserved in DynamoDB expressions, and quoting every key prevents reserved word errors without us having to maintain a matching set of reserved words.

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).

@jkav77
Copy link
Collaborator

jkav77 commented Jan 2, 2018

Ok I'm good with this one. @C45tr0 any last thoughts? If not let's get this merged.

@jkav77 jkav77 requested review from cdhowie and clarkie January 2, 2018 20:45
Copy link
Owner

@clarkie clarkie left a comment

Choose a reason for hiding this comment

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

👍

@C45tr0
Copy link
Contributor Author

C45tr0 commented Jun 15, 2018

Do I just need to rebase this and we are good? Sorry, didn't see the responses on here.

@cdhowie
Copy link
Collaborator

cdhowie commented Jun 15, 2018

@C45tr0 I left a code comment above pointing out a possible cleaned key collision scenario.

@C45tr0
Copy link
Contributor Author

C45tr0 commented Jul 27, 2018

@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.

@C45tr0 C45tr0 force-pushed the encode-expression-attribute-names-and-values branch from 4d292fa to e6e1111 Compare July 27, 2018 21:13
@C45tr0
Copy link
Contributor Author

C45tr0 commented Oct 23, 2018

Is there something I can do to help get this merged?

@sonagrigoryan
Copy link

Hey, this bug fix is not merged yet?

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.

7 participants