Skip to content
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

Add a cursor for the current page. #43

Merged
merged 11 commits into from
Sep 4, 2024

Conversation

GCorbel
Copy link
Contributor

@GCorbel GCorbel commented Sep 3, 2024

I added current_cursor to Mongoid::Scroll::Scrollable::Iterator.

current_cursor = nil

cursor = cursor_type.from_record Feed::Item.find_by(name: '7'), field_name: field_name, field_type: field_type

Feed::Item.asc(field_name).limit(2).scroll(cursor) do |_, iterator|
  current_cursor = iterator.current_cursor
end

Feed::Item.asc(field_name).limit(2).scroll(current_cursor) do |record|
  # loop over the same records
end

This allows to store the cursor and have the same result.

We use it to have this kind of payload for an API response :

GET {{baseUrl}}/attachments?pageToken=eyJ2YWx1ZSI6bnVsbCwiZmllbGRfdHlwZSI6IlRpbWUiLCJmaWVsZF9uYW1lIjoiY3JlYXRlZF9hdCIsImRpcmVjdGlvbiI6MSwiaW5jbHVkZV9jdXJyZW50IjpmYWxzZSwidGllYnJlYWtfaWQiOm51bGwsInR5cGUiOiJuZXh0In0=
{
    "records": [
        // ....
    ],
    "paging": {
        "pageToken": "eyJ2YWx1ZSI6MTM0OTM2MTI4MS4wMjcsImZpZWxkX3R5cGUiOiJUaW1lIiwiZmllbGRfbmFtZSI6ImNyZWF0ZWRfYXQiLCJkaXJlY3Rpb24iOjEsImluY2x1ZGVfY3VycmVudCI6dHJ1ZSwidGllYnJlYWtfaWQiOiI1MDZkOWU4MTdhYTU4ZDEyNTkwMDBmMTIiLCJ0eXBlIjoibmV4dCJ9",
        "perPage": 1,
        "firstPageToken": "eyJ2YWx1ZSI6bnVsbCwiZmllbGRfdHlwZSI6IlRpbWUiLCJmaWVsZF9uYW1lIjoiY3JlYXRlZF9hdCIsImRpcmVjdGlvbiI6MSwiaW5jbHVkZV9jdXJyZW50IjpmYWxzZSwidGllYnJlYWtfaWQiOm51bGwsInR5cGUiOiJuZXh0In0=",
        "nextPageToken": "eyJ2YWx1ZSI6MTM0OTM2MTI4MS4wMjcsImZpZWxkX3R5cGUiOiJUaW1lIiwiZmllbGRfbmFtZSI6ImNyZWF0ZWRfYXQiLCJkaXJlY3Rpb24iOjEsImluY2x1ZGVfY3VycmVudCI6ZmFsc2UsInRpZWJyZWFrX2lkIjoiNTA2ZDllODE3YWE1OGQxMjU5MDAwZjEyIiwidHlwZSI6Im5leHQifQ=="
    }
}

Given that, we can find the same records if we do a call with the token related to the current cursor.

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.

Add to README.

Not for this PR, but I wonder whether we can make some of the cursor construction lazy to avoid paying the cost of the cursors being generated without being used.

CHANGELOG.md Outdated Show resolved Hide resolved
iterator = Mongoid::Criteria::Scrollable::Iterator.new(
previous_cursor: previous_cursor,
next_cursor: cursor_type.from_record(record, cursor_options)
next_cursor: cursor_type.from_record(record, cursor_options),
current_cursor: current_cursor
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some rubocop will be needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed, thanks.

Rubocop show me 138 offenses with the config in .rubocop.yml 😥

lib/mongo/scrollable.rb Show resolved Hide resolved
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.

Looks like CI failed for something legit.

README.md Outdated Show resolved Hide resolved
@GCorbel
Copy link
Contributor Author

GCorbel commented Sep 4, 2024

Not for this PR, but I wonder whether we can make some of the cursor construction lazy to avoid paying the cost of the cursors being generated without being used.

The problem is that we have to build cursors with the first and the last record. We don't know how many records there are without an extra query.

We can do :

records = records.to_a

iterator = Mongoid::Criteria::Scrollable::Iterator.new(
  previous_cursor: cursor_type.from_record(records.first, cursor_options.merge(type: :previous)),
  next_cursor: cursor_type.from_record(records.last, cursor_options),
  current_cursor: cursor_type.from_record(records.first, cursor_options.merge(include_current: true))
)

records.each do |record|
  yield record, iterator
end

But it means we converts records to an array, so we fetch documents first and I guess we will have other errors.

@GCorbel
Copy link
Contributor Author

GCorbel commented Sep 4, 2024

Actually, we can have data about the records with records.view.cursor.entries.

GCorbel and others added 2 commits September 4, 2024 10:04
Co-authored-by: Daniel (dB.) Doubrovkine <[email protected]>
@GCorbel
Copy link
Contributor Author

GCorbel commented Sep 4, 2024

Looks like CI failed for something legit.

I don't know why but I fixed it.

@GCorbel
Copy link
Contributor Author

GCorbel commented Sep 4, 2024

#42 have to be merged first so I will change the Changelog and Readme in this PR.

@dblock
Copy link
Collaborator

dblock commented Sep 4, 2024

Thanks! Rebase?

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.

Something's odd in the README.

README.md Outdated Show resolved Hide resolved
@dblock dblock merged commit 5f41285 into mongoid:master Sep 4, 2024
12 checks passed
@dblock
Copy link
Collaborator

dblock commented Sep 7, 2024

FYI, 2.0 has been released, https://twitter.com/dblockdotorg/status/1832420865847259391/

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.

2 participants