Adding metadata to page iteration #342
Conversation
| else | ||
| chain.send(meth) | ||
| chain.send(meth, &blk) |
There was a problem hiding this comment.
without this, the Model.each et al returned an enumerable even if it was passed a block.
| expect { |b| model.where(id: '1').find_by_pages(&b) }.to yield_successive_args( | ||
| [all(be_kind_of(model)), {last_evaluated_key: an_instance_of(Hash)}], | ||
| [all(be_kind_of(model)), {last_evaluated_key: nil}], | ||
| ) |
There was a problem hiding this comment.
Actually testing the behavior here
| end | ||
| expect { |b| User.each(&b)}.to yield_successive_args( | ||
| be_kind_of(User).and(have_attributes('new_record' => false)), | ||
| be_kind_of(User).and(have_attributes('new_record' => false)) |
arjes
force-pushed
the
add_metadata_to_page_iterator
branch
from
eb94f3a
to
9198801
Compare
| match_array([{ name: 'Josh', id: '2' }, { name: 'Josh', id: '1' }]), | ||
| {last_evaluated_key: nil} | ||
| ] | ||
| ]) |
There was a problem hiding this comment.
Could you please add some specs for new behavior of query/scan - returning of non-nullable last_evaluated_key?
There was a problem hiding this comment.
I do have that expectation at a higher level for the find_by_pages https://github.com/Dynamoid/dynamoid/pull/342/files#diff-de03691a59eb936547beef74a4f01686R963
I should be able to move that down to this level too.
| first_page, first_page_meta = User.find_by_pages.first | ||
| second_page, = User.start(first_page_meta[:last_evaluated_key]).find_by_pages.first | ||
|
|
||
| expect(first_page & second_page).to be_empty |
There was a problem hiding this comment.
@andrykonchin This test paginates using the method described by the readme. The expectation is, a bit weird... Essentially ensuring we get a different set of records on each call.
Is this test also sufficient for the non-null last_evaluated_key or would you like me to write one down in the adapter too?
There was a problem hiding this comment.
It's OK. Good job. Will review the PR a little bit later
|
Looks good 👍 |
Adds the metadata for each pages as a second parameter
find_by_pages.While I was testing I also found a bug with all of the criteria delegation such as
Model.each, and theModel.find_by_pagesmethod I added. The specs didn't catch it with mine becausematch_arraycoerces it to an array. The test for criteria.eachmethod had a false positive because noexpectwas ever called.