search

clarkie / dynogels

DynamoDB data mapper for node.js. Originally forked from https://github.com/ryanfitz/vogels
Other
490 stars 110 forks source link

Fix #14 by encoding non-alpha characters in attribute keys #115

Open C45tr0 opened 6 years ago

C45tr0 commented 6 years ago

Update to #37 to include Document paths

coveralls commented 6 years ago

Coverage Status

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

C45tr0 commented 6 years ago

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

coveralls commented 6 years ago

Coverage Status

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

jkav77 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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

C45tr0 commented 6 years ago

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

cdhowie commented 6 years ago

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

C45tr0 commented 6 years ago

@cdhowie are you talking about the comment from your PR? https://github.com/clarkie/dynogels/pull/37#issuecomment-331222837

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 commented 5 years ago

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

sonagrigoryan commented 5 years ago

Hey, this bug fix is not merged yet?