Skip to content

Add ability to include the current record to the cursor #29

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

Merged
merged 4 commits into from
Feb 25, 2023

Conversation

FabienChaynes
Copy link
Collaborator

Currently, a Mongoid::Scroll::Cursor points to the next record.

In some situation, it can be useful to have a cursor including the current record.
For example, it can be the case when building a cursor based pagination in an API to provide the ability to the user to replay a request from a certain record. Such an API response could look like this:

{
  "records": [
   [...]
  ],
  "paging": {
    "current_page_token": "eyJ2YWx1ZSI6IjU0MTAxNGUxN2FhNThkOGNjZjAwMDAyNTo1NDEwMTRlMTdhYTU4ZDhjY2YwMDAwMjUiLCJmaWVsZF90eXBlIjoiQlNPTjo6T2JqZWN0SWQiLCJmaWVsZF9uYW1lIjoiX2lkIiwiZGlyZWN0aW9uIjoxfQ=="
  }
}

(with the current_page_token being used to rebuild the Mongoid::Scroll::Cursor, cf. #28 for example)

This is what is made possible in this PR by adding the include_current option to Mongoid::Scroll::Cursor. By default, it'll be false to preserve the current behavior. But when set to true, the cursor will include the current record.
Here's an usage example:

[1] pry(main)> Feed::Item.limit(4).scroll.pluck(:id)
=> [BSON::ObjectId('541014e17aa58d8ccf000025'),
 BSON::ObjectId('541015010f4ca111df0000b0'),
 BSON::ObjectId('54101d4d0f4ca1fd04000056'),
 BSON::ObjectId('54101d960f4ca115b700001e')]

[2] pry(main)> cursor = nil; Feed::Item.all.limit(2).scroll {|_, c| cursor = c}; cursor
=> #<Mongoid::Scroll::Cursor:0x000055cf20472440
 @direction=1,
 @field_name="_id",
 @field_type="BSON::ObjectId",
 @include_current=false,
 @tiebreak_id=BSON::ObjectId('541015010f4ca111df0000b0'),
 @value=BSON::ObjectId('541015010f4ca111df0000b0')>

# Starts at 54101d4d0f4ca1fd04000056, third Feed::Item
[3] pry(main)> Feed::Item.limit(2).scroll(cursor).pluck(:id)
=> [BSON::ObjectId('54101d4d0f4ca1fd04000056'), BSON::ObjectId('54101d960f4ca115b700001e')]

[4] pry(main)> cursor.include_current = true
=> true

# Starts at 541015010f4ca111df0000b0, second Feed::Item
[5] pry(main)> Feed::Item.limit(2).scroll(cursor).pluck(:id)
=> [BSON::ObjectId('541015010f4ca111df0000b0'), BSON::ObjectId('54101d4d0f4ca1fd04000056')]

Copy link
Collaborator

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Isn't the scrolling giving you the last record (aka not the current record)?

Page 1: records 1 - 10, cursor is on 10
Page 2: records 11 - 20

Are you saying you still want page 2 to be 10 - 19? If so I think it's not include_current, but include_last?

@FabienChaynes
Copy link
Collaborator Author

Are you saying you still want page 2 to be 10 - 19?

Exactly. I'll try to explain my thinking about the naming.

When we get a cursor from a specific record, the cursor points to this record (it becomes the tiebreak), but this record will not be included if we scroll using this cursor. We'll return the first record after this "current" record. This is why I think that the current record is not included.

[1] pry(main)> Feed::Item.limit(3).scroll.pluck(:id)
=> [BSON::ObjectId('541014e17aa58d8ccf000025'),
 BSON::ObjectId('541015010f4ca111df0000b0'),
 BSON::ObjectId('54101d4d0f4ca1fd04000056')]

[2] pry(main)> first_item = Feed::Item.limit(3).scroll.first
=> # Item 541014e17aa58d8ccf000025

[3] pry(main)> cursor = Mongoid::Scroll::Cursor.from_record(first_item, { field: Feed::Item.fields['_id'] })
=> #<Mongoid::Scroll::Cursor:0x000055e734db0b20
 @direction=1,
 @field_name="_id",
 @field_type="BSON::ObjectId",
 @include_current=false,
 @tiebreak_id=BSON::ObjectId('541014e17aa58d8ccf000025'),
 @value=BSON::ObjectId('541014e17aa58d8ccf000025')>

# Starts at second item
[4] pry(main)> Feed::Item.limit(2).scroll(cursor).pluck(:id)
=> [BSON::ObjectId('541015010f4ca111df0000b0'), BSON::ObjectId('54101d4d0f4ca1fd04000056')]

Similarly, when we pass a block to scroll and iterate over the records. At every iteration we get a "current record" and a cursor pointing to it. But if we use this cursor, the "current record" is not included.

[1] pry(main)> current_item = nil; cursor = nil; Feed::Item.limit(1).scroll {|i, c| current_item = i; cursor = c}
=> [...]

# First item
[2] pry(main)> current_item.id
=> BSON::ObjectId('541014e17aa58d8ccf000025')

[3] pry(main)> cursor
=> #<Mongoid::Scroll::Cursor:0x000055ace275fd70
 @direction=1,
 @field_name="_id",
 @field_type="BSON::ObjectId",
 @include_current=false,
 @tiebreak_id=BSON::ObjectId('541014e17aa58d8ccf000025'),
 @value=BSON::ObjectId('541014e17aa58d8ccf000025')>

# Starts at second item
[4] pry(main)> Feed::Item.limit(2).scroll(cursor).pluck(:id)
=> [BSON::ObjectId('541015010f4ca111df0000b0'), BSON::ObjectId('54101d4d0f4ca1fd04000056')]

If so I think it's not include_current, but include_last?

IMO there's no notion of "last" in a cursor because a cursor does not represent a list of records (it doesn't know what the limit set to the criteria was) but a link pointing to a specific record.
Not including the current record as it's currently done totally makes sense as a default. But it'd be nice to have the option to include it.

@dblock
Copy link
Collaborator

dblock commented Feb 23, 2023

I understand why you chose include_current now. Good with me. Finish it up!

@FabienChaynes FabienChaynes changed the title [WIP] Add ability to include the current record to the cursor Add ability to include the current record to the cursor Feb 23, 2023
@FabienChaynes FabienChaynes requested a review from dblock February 23, 2023 23:47
Copy link
Collaborator

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Great. Needs to be documented in README, please.

@FabienChaynes FabienChaynes requested a review from dblock February 24, 2023 10:42
@dblock dblock merged commit bd51f4c into mongoid:master Feb 25, 2023
@dblock
Copy link
Collaborator

dblock commented Feb 25, 2023

Merged.

@FabienChaynes any interest in helping co-maintain this gem? Maybe make the next release? Email me your rubygems username to dblock at dblock dot org if you're interested.

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.

None yet

2 participants