Allow querying local indexes and resolve problem with using updatedAt as range key in a query.

[dolsem]

Summary:

This PR fixes the issue discussed in #439, and one more bug:
toDynamo method of Attribute is called without updateTimestamps option set to false during Query.exec(), so when updatedAt timestamp is used as a range key in a query, supplied value gets substituted with current timestamp.

I also moved the debug statement for global secondary index, because local secondary index passes that outer check too.

GitHub linked issue:

Closes #439

Other information:

I took the liberty of updating existing tests that were doing nothing meaningful instead of creating new ones. Let me know if you want me to resort to a more conservative option and just define my own tests. However, please note that test Query with Local Global Index as range (I assumed there's a typo in the name) currently queries a global index, not a local one, while test Should allow multiple local indexes and query correctly in its current state should fail as it doesn't even make sense conceptually (local indexes can't have their own range keys and it doesn't make sense for the same attribute to be the key of multiple local indexes). Moreover, the schema defined in that test most likely ends up being unused (which might be why it doesn't fail), because instead of creating a new table it tries to overwrite the one created by the previous test (Log-1) and likely fails silently, because you cannot add / remove local indexes for an existing table, you have to create a new one.

I marked this as a breaking change, however, if we assume that no one used the fact that updatedAt value was changed during a query as a "feature", it shouldn't break any code.

Type (select 1):

• Bug fix
• Feature implementation
• Documentation improvement
• Testing improvement

• Test added to report bug (GitHub issue #--- @---)
• Something not listed here

Is this a breaking change? (select 1):

• 🚨 YES 🚨
• No
• I'm not sure

Is this ready to be merged into Dynamoose? (select 1):

• Yes
• No

Are all the tests currently passing on this PR? (select 1):

• Yes
• No

Other:

• I have searched through the GitHub pull requests to ensure this PR has not already been submitted
• I have updated the Dynamoose documentation (if required) given the changes I made
• I have added/updated the Dynamoose test cases (if required) given the changes I made
• I have run npm test from the root of the project directory to ensure all tests continue to pass
• I agree that all changes made in this pull request may be distributed and are made available in accordance with the Dynamoose license
• All of my commits and commit messages are detailed, explain what changes were made, and are easy to follow and understand
• I have confirmed that all my code changes are indented properly using 2 spaces
• I have filled out all fields above

[coveralls]

Pull Request Test Coverage Report for Build 676

• 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.5%) to 85.087%

---

Totals
Change from base Build 669:	0.5%
Covered Lines:	1875
Relevant Lines:	2132

---

💛 - Coveralls

[fishcharlie]

Mainly questions here. I'd like to get this approved once we verify everything, and once we do, I'll submit a release shortly after.

[fishcharlie]

@yokkoyokko Mind taking a look at this and submitting a GitHub review?

[fishcharlie]

@dolsem In terms of the following.

I marked this as a breaking change, however, if we assume that no one used the fact that updatedAt value was changed during a query as a "feature", it shouldn't break any code.

I don't think this is an issue or a breaking change. Did the updatedAt value really change during a query??? 😬 That is kinda bad. So yes, if that was happening and that is the only breaking change, I consider that a major bug. updatedAt to me, means that the item was actually updated and changed.

If you could, it'd be amazing if you could write a test to ensure that updatedAt won't be updated on a query, and if you want even a scan or get for that matter. I think based on what you said, that test should fail on the current version, and should pass on this PR. Just to ensure we are covering that so we don't break it in the future.

---

I really appreciate your work on this. Thank you so much!!!

[dolsem]

@fishcharlie This last test you had questions about already checks for that (perhaps somewhat implicitly), that's why I use updatedAt as a local index. The test currently fails. If you change that to createdAt (which doesn't get updated with updateTimestamps set to true), nothing should change, as their values in this case are the same and set during creation, but the test will then pass. I could add an expectation that explicitly checks that the value of updatedAt stays the same after the query is formed, but I haven't seen a way to get the actual DynamoDB query after it gets generated by Query.exec() (other than output to debug), so I am not sure about the best way to go about it.

[fishcharlie]

@dolsem Got it. Looks like there are still some comments from my review that I don't think have been addressed yet.

[fishcharlie]

@dolsem What do you think? Think we are ready to merge in and get a release out?

[dolsem]

@fishcharlie yes, I believe so.

[fishcharlie]

@dolsem Awesome. I'll get that done and hopefully get a release out before the end of the day. Again thanks so much for your help! Was hoping #434 was going to be further along before we do a release, but looks like that won't happen. Thanks again!

[fishcharlie]

@dolsem Thanks again for your work on this. My goal is since version 1.0.0 was such a massive update to provide bug updates ASAP for the next little bit for things that broke in version 1.0.0 that were working before version 1.0.0. Therefor version 1.0.1 has been released that includes this fix!!

Please keep filing issues if you notice any other problems and we will work to get them out as soon as possible.

[dolsem]

No problem, glad I could help!