Update batchPut to work with timestamps #449

dolsem · 2018-10-17T15:49:08Z

Summary:

Second take on #447.

Currently batchPut fails with error Cannot read property 'updateTimestamps' of undefined if a model uses timestamps. The error results from this line, because toDynamo is called without options here.

This PR fixes the issue and adds two options to batchPut: updateTimestamps (defaults to false, because currently batchPut does not update timestamps), and updateExpires (also defaults to false).

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 · 2018-10-17T15:52:23Z

Pull Request Test Coverage Report for Build 685

5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.07%) to 85.156%

Files with Coverage Reduction	New Missed Lines	%
lib/Table.js	2	77.93%

Totals
Change from base Build 682:	0.07%
Covered Lines:	1880
Relevant Lines:	2137

💛 - Coveralls

coveralls · 2018-10-17T15:52:30Z

Pull Request Test Coverage Report for Build 685

5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.07%) to 85.156%

Files with Coverage Reduction	New Missed Lines	%
lib/Table.js	2	77.93%

Totals
Change from base Build 682:	0.07%
Covered Lines:	1880
Relevant Lines:	2137

💛 - Coveralls

coveralls · 2018-10-17T15:52:30Z

Pull Request Test Coverage Report for Build 689

1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.08%) to 85.168%

Totals
Change from base Build 682:	0.08%
Covered Lines:	1876
Relevant Lines:	2133

💛 - Coveralls

fishcharlie

Really good. Just a few suggestions.

fishcharlie · 2018-10-17T16:03:34Z

docs/_docs/model.md

+
+**updateTimestamps**: boolean
+
+Should update the `updatedAt` timestamp if exists. Defaults to true.


Shouldn't this be Defaults to false for the documentation?

Oh, that's right. Oops!

fishcharlie · 2018-10-17T16:05:10Z

lib/Model.js

+  if (options.updateTimestamps === true) {
+    toDynamoOptions.updateTimestamps = true;
+  }
+  if (options.updateExpires === true) {


Instead of doing these two if statements why not do something like the following?

let toDynamoOptions = { updateTimestamps: options.updateTimestamps || false, updateExpires: options.updateExpires || false };

We might even be able to reduce it to something like the following since it'd be falsey if it doesn't exist.

let toDynamoOptions = { updateTimestamps: options.updateTimestamps, updateExpires: options.updateExpires };

Yes, I personally like this more (your first version). I was trying to copy the existing code style as much as possible. But since you suggested it yourself, I'll definitely change it. Not a fan of the second version, because it's slightly less telling and not as safe. So I'll go with the first one.

@dolsem I'd like to refactor a lot of that code and fix up a lot of things to use more things like the versions I had above. It'd be a massive project but at some point I'd like to go through and clean up a lot of the Dynamoose code to make it more clean.

Not sure when that will happen tho. Until then I fully support new code using those type of clean coding practices.

Okay, got you.

fishcharlie · 2018-10-17T16:10:16Z

test/Model.js

+                    should.exist(result3);
+                    result3.length.should.eql(cats.length);
+
+                    result3.forEach(cat => cat.myLittleUpdatedAt.should.not.eql(timestamps[cat.id]));  


I'm thinking we should set this to check to ensure the new updated at is greater than the old value. Just to make this test more thorough.

fishcharlie · 2018-10-17T16:11:46Z

test/Model.js

+
+                    result3.forEach((cat) => {
+                      cat.array.length.should.eql(2);
+                      cat.expires.should.not.eql(originalExpires[cat.id]);


Same thing here, this should probably check to see if it's greater than the old value.

fishcharlie · 2018-10-17T16:17:30Z

test/Model.js

+              var timestamps = {};
+              for (var i=0 ; i<10 ; ++i) {
+                cats[i].name = 'John_'+i;
+                Object.assign(timestamps, { [cats[i].id]: new Date(cats[i].myLittleUpdatedAt) });


I'm not a huge fan of Object.assign if we can avoid it. Is there a better method for this? Something like the following maybe?

timestamps[cats[i].id] = new Date(cats[i].myLittleUpdatedAt);

To me that is much simpler code and easier to understand. I don't think Object.assign makes a lot of sense in this context.

fishcharlie · 2018-10-17T16:19:35Z

test/Model.js

+              var timestamps = {};
+              for (var i=0 ; i<10 ; ++i) {
+                cats[i].name = 'John_'+i;
+                Object.assign(timestamps, { [cats[i].id]: new Date(cats[i].myLittleUpdatedAt) });


Same thing here. Something like the following maybe?

timestamps[cats[i].id] = new Date(cats[i].myLittleUpdatedAt);

fishcharlie · 2018-10-17T16:20:10Z

test/Model.js

+              var timestamps = {};
+              for (var i=0 ; i<10 ; ++i) {
+                cats[i].name = 'John_'+i;
+                Object.assign(timestamps, { [cats[i].id]: new Date(cats[i].myLittleUpdatedAt) });


Same thing here. Something like the following maybe?

timestamps[cats[i].id] = new Date(cats[i].myLittleUpdatedAt);

fishcharlie · 2018-10-17T16:24:03Z

test/Model.js

+                result3.length.should.eql(cats.length);
+
+                originalExpires = result3.reduce(
+                  (acc, cat) => Object.assign(acc, { [cat.id]: cat.expires }),


Object.assign makes sense to me here. Here is another option, but totally up to you if you want to use this or not. I think your code is completely fine here, but just another option to keep it consistent in terms of not using Object.assign.

Change the code above to:

var originalExpires = {};

Then here do:

result3.forEach(cat => { acc[cat.id] = cat.expires; });

You could also make that a for loop since it seems like you are using that a lot throughout these tests.

Yeah I think it's always best to stick to either good old procedural style or functional, not combining the two. I lean towards functional with JavaScript, but I was reusing some of the code from other tests in the same file, I didn't want my tests to look radically different from everything around. I guess I'll make things more consistent.

@dolsem If Object.assign is used a lot throughout the tests then that is fine. I'm not sure what is currently being used for these cases.

dolsem · 2018-10-18T04:58:55Z

I've just made the updates you requested, as well as a few other updates to code style. Let me know if there's anything else.

fishcharlie

One more thing

fishcharlie · 2018-10-18T05:02:21Z

test/Model.js

+              timestamps[id] = new Date(myLittleUpdatedAt);
+            });
+
+            await new Promise(resolve => setTimeout(resolve, 1000));


I just realized one more thing I forgot to mention. Why not make this something much shorter? Like 5 or even 1? Is there a specific reason why we have to wait a full second here? Waiting just a few milliseconds should ensure that the timestamps are later than the original value, correct?

If we decide to edit it we have to make sure we do it throughout these new tests.

Yes, I tried making it shorter, and the tests would sometimes fail. Seems like when timestamps get updated milliseconds get stripped away either on the original objects after they saved or on the results of batchGet, not sure which one right now. I didn't have time to investigate this, so I don't know if this is the result of floor, ceiling, or round, so to be safe I made it wait a full second. This made the tests deterministic, no chance of failure anymore. I might be mistaken, but I think the same timeout is used for other tests around timestamps in the test suit too.

fishcharlie

LGTM

fishcharlie · 2018-10-18T05:11:21Z

@dolsem Thanks for your hard work on this! I'll get this merged in soon!!

dolsem added 4 commits October 17, 2018 11:38

Added tests for batchPut with timestamps

32a6866

Updated batchPut to work with timestamps

5c79d34

Added tests for batchPut with updateExpires option

6e7781e

Added updateExpires to batchPut options

f7c8b0b

fishcharlie requested changes Oct 17, 2018

View reviewed changes

dolsem force-pushed the batchput-timestamps branch from 21498d7 to 03b1f44 Compare October 18, 2018 03:42

Updated documentation with newly added options for batchPut

6c359e7

dolsem force-pushed the batchput-timestamps branch from 03b1f44 to 6c359e7 Compare October 18, 2018 03:43

fishcharlie added the status:do-not-merge label Oct 18, 2018

Updated style for dynamoose#449

edf897b

fishcharlie requested changes Oct 18, 2018

View reviewed changes

fishcharlie approved these changes Oct 18, 2018

View reviewed changes

fishcharlie removed the status:do-not-merge label Oct 18, 2018

fishcharlie mentioned this pull request Oct 18, 2018

Version 1.1.0 #450

Merged

fishcharlie merged commit 43773c3 into dynamoose:master Oct 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update batchPut to work with timestamps #449

Update batchPut to work with timestamps #449

dolsem commented Oct 17, 2018

coveralls commented Oct 17, 2018

coveralls commented Oct 17, 2018

coveralls commented Oct 17, 2018 •

edited

Loading

fishcharlie left a comment

fishcharlie Oct 17, 2018

dolsem Oct 17, 2018

fishcharlie Oct 17, 2018

dolsem Oct 17, 2018

fishcharlie Oct 17, 2018

dolsem Oct 17, 2018

fishcharlie Oct 17, 2018

fishcharlie Oct 17, 2018

fishcharlie Oct 17, 2018

dolsem Oct 17, 2018

fishcharlie Oct 17, 2018

fishcharlie Oct 17, 2018

fishcharlie Oct 17, 2018

dolsem Oct 17, 2018

fishcharlie Oct 17, 2018

dolsem commented Oct 18, 2018

fishcharlie left a comment

fishcharlie Oct 18, 2018

dolsem Oct 18, 2018

fishcharlie left a comment

fishcharlie commented Oct 18, 2018


		updateTimestamps: boolean

		Should update the `updatedAt` timestamp if exists. Defaults to true.

Update batchPut to work with timestamps #449

Update batchPut to work with timestamps #449

Conversation

dolsem commented Oct 17, 2018

Summary:

Type (select 1):

Is this a breaking change? (select 1):

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

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

Other:

coveralls commented Oct 17, 2018

Pull Request Test Coverage Report for Build 685

💛 - Coveralls

coveralls commented Oct 17, 2018

Pull Request Test Coverage Report for Build 685

💛 - Coveralls

coveralls commented Oct 17, 2018 • edited Loading

Pull Request Test Coverage Report for Build 689

💛 - Coveralls

fishcharlie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dolsem commented Oct 18, 2018

fishcharlie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fishcharlie left a comment

Choose a reason for hiding this comment

fishcharlie commented Oct 18, 2018

coveralls commented Oct 17, 2018 •

edited

Loading