MONGOID-5863 #last overrides #skip#5975
Conversation
This was being set even if n==0, which overrode any previously declared skip.
|
Assigned |
jamis
requested review from
alcaeus
and removed request for
comandeo-mongo and
tom-selander
alcaeus
left a comment
alcaeus
left a comment
There was a problem hiding this comment.
For the record, I'm unsure why this is expected to return the nth-from-last element, but I agree with fixing the inadvertent BC break and look at this separately.
let's take this opportunity to simplify the GHA test workflow
|
By the way, I still haven't upgraded from Mongoid 7 due to concern over bugs like this (as well as performance issues). I am thinking to try upgrading soon, I will likely find more things. |
|
Regression found, as expected: https://jira.mongodb.org/browse/MONGOID-5865 |
| it "returns the nth from last element" do | ||
| expect(context.skip(1).last).to eq(depeche_mode) | ||
| end | ||
| end |
There was a problem hiding this comment.
The method has sort(inverse_sorting) and then later v.to_a.reverse, which ensure the ordering in the case that multiple docs are returned.
Is this covered by tests? i.e. ensuring correct document ordering the result for multiple docs?
- combining
skip(n)withlimit(m)(where n, m > 1) last(n)(where n > 1)
* remove redundant skip() This was being set even if n==0, which overrode any previously declared skip. * GHA is retiring ubuntu-20.04 let's take this opportunity to simplify the GHA test workflow * try and fix syntax error * try again
* remove redundant skip() This was being set even if n==0, which overrode any previously declared skip. * GHA is retiring ubuntu-20.04 let's take this opportunity to simplify the GHA test workflow * try and fix syntax error * try again
Prior to Mongoid 8.1,
Model.skip(1).lastwould return the next-to-last document. A refactoring in 8.1 resulted in this breaking, due to an extraneous call to#skipthat would override any other invocation of#skip.Summary
Fixes
Model.skip(1).lastso it again returns the next-to-last document, instead of just the last document.