-
Notifications
You must be signed in to change notification settings - Fork 12
Refactor cursors to inherit from BaseCursor, scroll with a Mongoid::Scroll::Base64EncodedCursor #33
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
Conversation
82b1c84
to
364683d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Base64EncodedCursors
are not working properly when the sort field is not a String one. Cf. comment below.
b6be2b7
to
b9606aa
Compare
Specs passing. There's probably a more methodical way to approach writing these tests than the lots of copy-paste. |
raise Mongoid::Scroll::Errors::InvalidBase64CursorError.new(cursor: value) | ||
end | ||
super BaseCursor.parse_field_value(parsed['field_type'], parsed['field_name'], parsed['value']), { | ||
field_type: BaseCursor.parse_field_type(parsed['field_type'], parsed['field_name']), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BaseCursor.parse_field_type
returns the class (e.g. Date
) while BaseCursor.extract_field_options
used in Cursor
return the class as a string (e.g. "Date"
).
With the class, the scroll
method will raise a MismatchedSortFieldsError
because the class will be compared with a string in Scrollable::Fields#different_sort_fields?
. We could either use to_s
in BaseCursor#sort_options
or not use BaseCursor.parse_field_type
here to do things like in Cursor
(if so, the method will have to be deleted):
field_type: BaseCursor.parse_field_type(parsed['field_type'], parsed['field_name']), | |
field_type: parsed['field_type'], |
[3] pry(main)> c=nil; Feed::Item.asc(:name).limit(2).scroll(Mongoid::Scroll::Base64EncodedCursor) {|_, cr| c = cr}; c
[7] pry(main)> Feed::Item.asc(:name).limit(2).scroll(c).pluck(:id)
=> [BSON::ObjectId('6075ad853298c919807b4b52'), BSON::ObjectId('5e8d90cf5bd5441f2894f2e7')]
[8] pry(main)> Feed::Item.asc(:name).limit(2).scroll(Mongoid::Scroll::Base64EncodedCursor.new(c.to_s)).pluck(:id)
Mongoid::Scroll::Errors::MismatchedSortFieldsError:
@FabienChaynes good eyes, not sure how you saw this 😅 fixed, added specs to reuse both the cursor returned and the serialized cursor for all types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, I wouldn't have noticed it without a manual test 🙂
LGTM, just one question about a new dependency that I don't understand.
when /3/ then gem 'mongoid', '~> 3.1' | ||
else gem 'mongoid', version | ||
when /5/ then | ||
gem 'bigdecimal', '1.3.5' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran into https://stackoverflow.com/questions/60226893/rails-nomethoderror-undefined-method-new-for-bigdecimalclass with a newer ruby version. We don't use that in CI, maybe I'll add it.
Thanks for a thorough code review! |
@FabienChaynes take a look, this lets you scroll normally.
I removed support for Mongoid 3, 4 and moped, I think it's time.