MONGOID-5105: Mongoid::Association::EmbedsMany::Proxy#count should allow block form #5000

johnnyshields · 2021-05-27T14:26:38Z

Mongoid::Association::EmbedsMany::Proxy#count should allow block form to make it congruent with #any?, #none?, #all?, etc.

Interestingly this already works for normal relations but not embedded ones.

Example:

person.addresses.create(country: "FR")
person.addresses.create(country: "US")
person.addresses.build(country: "FR")

person.addresses.count {|a| a.country == "FR" }  #=> 2

To fully be congruent with the method_missing behavior, I've also added support for the args form of #count, although it's not super useful in practice:

addr = person.addresses.first

person.addresses.count(addr)  #=> 1
person.addresses.count("foobar")  #=> 0

I've also added additional tests for other methods #any?, #none?, #all?

One of the reasons for doing this is that Rubocop/Performance corrects the following:

person.addresses.select {|a| a.country == "FR" }.size  # BAD

person.addresses.count {|a| a.country == "FR" }  # GOOD

So this will help avoid accidental errors.

…count to make it congruent with #any?, #none?, #all?, etc. Example: Person.addresses.count {|a| a.country == "FR" } I've also added additional tests for other methods.

p-mongo · 2021-05-27T15:30:02Z

Nice PR number.

johnnyshields · 2021-05-27T16:11:12Z

Haha I was wondering if you'd notice that!

johnnyshields · 2021-07-27T11:08:18Z

Can this be merged?

comandeo · 2021-07-27T15:06:45Z

Hi @johnnyshields,

Could you please merge the current master into your branch? I see there are test failures on evergreen, some of them should be fixed by merging the current master. I will check others then.

Thank you!

…-proxy-count-block

johnnyshields · 2021-07-27T15:30:07Z

@comandeo done

comandeo · 2021-07-28T10:09:43Z

@johnnyshields Unfortunately, tests for this PR fail on Ruby 2.3 and 2.4:

  1) Mongoid::Association::Embedded::EmbedsMany::Proxy#any? when documents are persisted argument form is supported
      Failure/Error: expect(person.addresses.any?(person.addresses.first)).to be true
      
      ArgumentError:
        wrong number of arguments (given 1, expected 0)
      # ./spec/mongoid/association/embedded/embeds_many/proxy_spec.rb:1439:in `block (4 levels) in <top (required)>'
      # ./spec/lite_spec_helper.rb:68:in `block (3 levels) in <top (required)>'
      # ./spec/lite_spec_helper.rb:67:in `block (2 levels) in <top (required)>'

    2) Mongoid::Association::Embedded::EmbedsMany::Proxy#any? when documents are not persisted argument form is supported
      Failure/Error: expect(person.addresses.any?(person.addresses.first)).to be true
      
      ArgumentError:
        wrong number of arguments (given 1, expected 0)
      # ./spec/mongoid/association/embedded/embeds_many/proxy_spec.rb:1459:in `block (4 levels) in <top (required)>'
      # ./spec/lite_spec_helper.rb:68:in `block (3 levels) in <top (required)>'
      # ./spec/lite_spec_helper.rb:67:in `block (2 levels) in <top (required)>'
     3) Mongoid::Association::Embedded::EmbedsMany::Proxy#any? when documents are not present argument form is supported
      Failure/Error: expect(person.addresses.any?(1)).to be false
      
      ArgumentError:
        wrong number of arguments (given 1, expected 0)
      # ./spec/mongoid/association/embedded/embeds_many/proxy_spec.rb:1474:in `block (4 levels) in <top (required)>'
      # ./spec/lite_spec_helper.rb:68:in `block (3 levels) in <top (required)>'
      # ./spec/lite_spec_helper.rb:67:in `block (2 levels) in <top (required)>'
     4) Mongoid::Association::Embedded::EmbedsMany::Proxy#all? when documents are persisted argument form is supported
      Failure/Error: expect(person.addresses.all?(person.addresses.first)).to be true
      
      ArgumentError:
        wrong number of arguments (given 1, expected 0)
      # ./spec/mongoid/association/embedded/embeds_many/proxy_spec.rb:1500:in `all?'
      # ./spec/mongoid/association/embedded/embeds_many/proxy_spec.rb:1500:in `block (4 levels) in <top (required)>'
      # ./spec/lite_spec_helper.rb:68:in `block (3 levels) in <top (required)>'
      # ./spec/lite_spec_helper.rb:67:in `block (2 levels) in <top (required)>'
     5) Mongoid::Association::Embedded::EmbedsMany::Proxy#all? when documents are not persisted argument form is supported
      Failure/Error: expect(person.addresses.all?(person.addresses.first)).to be true
      
      ArgumentError:
        wrong number of arguments (given 1, expected 0)
      # ./spec/mongoid/association/embedded/embeds_many/proxy_spec.rb:1520:in `all?'
      # ./spec/mongoid/association/embedded/embeds_many/proxy_spec.rb:1520:in `block (4 levels) in <top (required)>'
      # ./spec/lite_spec_helper.rb:68:in `block (3 levels) in <top (required)>'
      # ./spec/lite_spec_helper.rb:67:in `block (2 levels) in <top (required)>'
     6) Mongoid::Association::Embedded::EmbedsMany::Proxy#all? when documents are not present argument form is supported
      Failure/Error: expect(person.addresses.all?(nil)).to be true
      
      ArgumentError:
        wrong number of arguments (given 1, expected 0)
      # ./spec/mongoid/association/embedded/embeds_many/proxy_spec.rb:1535:in `all?'
      # ./spec/mongoid/association/embedded/embeds_many/proxy_spec.rb:1535:in `block (4 levels) in <top (required)>'
      # ./spec/lite_spec_helper.rb:68:in `block (3 levels) in <top (required)>'
      # ./spec/lite_spec_helper.rb:67:in `block (2 levels) in <top (required)>'
     7) Mongoid::Association::Embedded::EmbedsMany::Proxy#none? when documents are persisted argument form is supported
      Failure/Error: expect(person.addresses.none?(person.addresses.first)).to be false
      
      ArgumentError:
        wrong number of arguments (given 1, expected 0)
      # ./spec/mongoid/association/embedded/embeds_many/proxy_spec.rb:1562:in `none?'
      # ./spec/mongoid/association/embedded/embeds_many/proxy_spec.rb:1562:in `block (4 levels) in <top (required)>'
      # ./spec/lite_spec_helper.rb:68:in `block (3 levels) in <top (required)>'
      # ./spec/lite_spec_helper.rb:67:in `block (2 levels) in <top (required)>'

    8) Mongoid::Association::Embedded::EmbedsMany::Proxy#none? when documents are not persisted argument form is supported
      Failure/Error: expect(person.addresses.none?(person.addresses.first)).to be false
      
      ArgumentError:
        wrong number of arguments (given 1, expected 0)
      # ./spec/mongoid/association/embedded/embeds_many/proxy_spec.rb:1582:in `none?'
      # ./spec/mongoid/association/embedded/embeds_many/proxy_spec.rb:1582:in `block (4 levels) in <top (required)>'
      # ./spec/lite_spec_helper.rb:68:in `block (3 levels) in <top (required)>'
      # ./spec/lite_spec_helper.rb:67:in `block (2 levels) in <top (required)>'
     9) Mongoid::Association::Embedded::EmbedsMany::Proxy#none? when documents are not present argument form is supported
      Failure/Error: expect(person.addresses.none?(nil)).to be true
      
      ArgumentError:
        wrong number of arguments (given 1, expected 0)
      # ./spec/mongoid/association/embedded/embeds_many/proxy_spec.rb:1597:in `none?'
      # ./spec/mongoid/association/embedded/embeds_many/proxy_spec.rb:1597:in `block (4 levels) in <top (required)>'
      # ./spec/lite_spec_helper.rb:68:in `block (3 levels) in <top (required)>'
      # ./spec/lite_spec_helper.rb:67:in `block (2 levels) in <top (required)>'

Mongoid still supports 2.3 and 2.4 even in the latest version - https://docs.mongodb.com/mongoid/master/reference/compatibility/#ruby-compatibility
So, these failures should be fixed to get the PR merged.

johnnyshields · 2021-07-28T13:13:07Z

@comandeo thanks. The correct solution here is just to ignore those tests on older Ruby. The mongoid code calls super and the older ruby versions don't support some of the syntax like .any?(1)

johnnyshields · 2021-07-30T10:47:48Z

@comandeo may I ask why the CI is still failing?

comandeo · 2021-07-30T13:52:20Z

@johnnyshields CI is pretty green now, actually. The only configuration that fails is jruby-app-test; however, this configuration fails everywhere last couple of days. I created a ticked to investigate - https://jira.mongodb.org/browse/MONGOID-5134 - and disabled this configuration temporarily.

So, if you merge the current master into your branch, everything should pass.

…-proxy-count-block

johnnyshields · 2021-07-30T14:46:12Z

OK done

johnnyshields · 2021-07-31T15:45:06Z

Now Ruby 3.0 failing?

comandeo · 2021-08-02T11:43:36Z

Actually, not, everything pass. There was a problem with a runner, some connectivity issues. Now all good.

p-mongo

I believe the behavior of Mongoid on all supported Ruby versions should be the same, if practical. I believe this generally how Rails works.

Therefore I believe either this PR would need to implement the missing block form behavior on Ruby 2.3 and 2.4, or Mongoid should increase the minimum required version to 2.5 at which point the 2.5+ checks in this PR would be unneeded.

In mongodb/mongo-ruby-driver#2298 we increased the minimum Ruby version to 2.4 for the driver, but per the docs in https://docs.mongodb.com/mongoid/master/reference/compatibility/#ruby-compatibility 2.3 and 2.4 support has already been deprecated for two Mongoid releases and perhaps the minimum can be increased to 2.5 to save work in this PR.

lib/mongoid/association/embedded/embeds_many/proxy.rb

spec/mongoid/association/embedded/embeds_many/proxy_spec.rb

johnnyshields · 2021-08-03T07:57:19Z

I believe the behavior of Mongoid on all supported Ruby versions should be the same, if practical.

The behavior on all versions is the same: we delegate count etc. to the underlying object. The underlying object behavior may differ depending on the Ruby version. It is inadvisable to add a monkey patch here on Ruby kernel methods; I'm not going to change this aspect of my PR.

Regardless, Ruby 2.3 and 2.4 are both EOL (2019-03-31 and 2020-03-31).

p-mongo · 2021-08-03T07:59:20Z

No patching of standard library methods is needed, the needed code can be added to Mongoid methods based on active Ruby version.

johnnyshields · 2021-08-03T08:00:11Z

I'm not going to add EOL Ruby 2.3 and 2.4 specific hacks in the Mongoid codebase.

If any user wishes to emulate Ruby 2.5+ kernel behavior in Ruby 2.3/2.4, they are free to add such patches into their own project.

- Update docs

johnnyshields · 2021-08-05T04:48:26Z

Shall we merge this? If Mongoid team would like to add the EOL Ruby hacks it can be done as a separate PR; be aware they would apply to many methods (#any?, #all?, #none?, etc.) which are not being touched in this PR. This PR only affects #count and I have implemented it in a way which is consistent with the current codebase.

dblock · 2021-08-13T20:30:20Z

I am confused how the build is green but more work is required to get this PR merged?

johnnyshields · 2021-08-24T04:50:50Z

@comandeo looks like this needs your review?

johnnyshields · 2021-08-27T15:50:42Z

Ruby 2.5 is now the min version, so the issues raised around Ruby 2.3/2.4 are no longer a concern.

p-mongo · 2021-08-28T17:37:48Z

#5060

johnnyshields · 2021-08-29T16:25:47Z

Closing in favor of #5060

Mongoid::Association::EmbedsMany::Proxy should allow block form for #…

701958d

…count to make it congruent with #any?, #none?, #all?, etc. Example: Person.addresses.count {|a| a.country == "FR" } I've also added additional tests for other methods.

johnnyshields changed the title ~~Mongoid::Association::EmbedsMany::Proxy#count should allow block form~~ MONGOID-5105: Mongoid::Association::EmbedsMany::Proxy#count should allow block form May 27, 2021

Update docs

55e52f9

alcaeus requested a review from comandeo July 27, 2021 11:57

Merge remote-tracking branch 'remotes/origin/master' into embeds-many…

40b7025

…-proxy-count-block

Fix specs on Ruby 2.3 and 2.4

af28e43

Merge remote-tracking branch 'remotes/origin/master' into embeds-many…

c127641

…-proxy-count-block

p-mongo reviewed Aug 3, 2021

View reviewed changes

lib/mongoid/association/embedded/embeds_many/proxy.rb Outdated Show resolved Hide resolved

spec/mongoid/association/embedded/embeds_many/proxy_spec.rb Outdated Show resolved Hide resolved

- .create --> .create! in spec

c050e98

- Update docs

p-mongo added the oleg-backlog label Aug 23, 2021

comandeo requested a review from p-mongo August 27, 2021 16:18

p-mongo added oleg-done and removed oleg-backlog labels Aug 28, 2021

johnnyshields closed this Aug 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MONGOID-5105: Mongoid::Association::EmbedsMany::Proxy#count should allow block form #5000

MONGOID-5105: Mongoid::Association::EmbedsMany::Proxy#count should allow block form #5000

johnnyshields commented May 27, 2021 •

edited

Loading

p-mongo commented May 27, 2021

johnnyshields commented May 27, 2021

johnnyshields commented Jul 27, 2021

comandeo commented Jul 27, 2021

johnnyshields commented Jul 27, 2021

comandeo commented Jul 28, 2021

johnnyshields commented Jul 28, 2021

johnnyshields commented Jul 30, 2021

comandeo commented Jul 30, 2021

johnnyshields commented Jul 30, 2021

johnnyshields commented Jul 31, 2021

comandeo commented Aug 2, 2021

p-mongo left a comment

johnnyshields commented Aug 3, 2021 •

edited

Loading

p-mongo commented Aug 3, 2021

johnnyshields commented Aug 3, 2021 •

edited

Loading

johnnyshields commented Aug 5, 2021 •

edited

Loading

dblock commented Aug 13, 2021

johnnyshields commented Aug 24, 2021

johnnyshields commented Aug 27, 2021

p-mongo commented Aug 28, 2021

johnnyshields commented Aug 29, 2021

MONGOID-5105: Mongoid::Association::EmbedsMany::Proxy#count should allow block form #5000

MONGOID-5105: Mongoid::Association::EmbedsMany::Proxy#count should allow block form #5000

Conversation

johnnyshields commented May 27, 2021 • edited Loading

p-mongo commented May 27, 2021

johnnyshields commented May 27, 2021

johnnyshields commented Jul 27, 2021

comandeo commented Jul 27, 2021

johnnyshields commented Jul 27, 2021

comandeo commented Jul 28, 2021

johnnyshields commented Jul 28, 2021

johnnyshields commented Jul 30, 2021

comandeo commented Jul 30, 2021

johnnyshields commented Jul 30, 2021

johnnyshields commented Jul 31, 2021

comandeo commented Aug 2, 2021

p-mongo left a comment

Choose a reason for hiding this comment

johnnyshields commented Aug 3, 2021 • edited Loading

p-mongo commented Aug 3, 2021

johnnyshields commented Aug 3, 2021 • edited Loading

johnnyshields commented Aug 5, 2021 • edited Loading

dblock commented Aug 13, 2021

johnnyshields commented Aug 24, 2021

johnnyshields commented Aug 27, 2021

p-mongo commented Aug 28, 2021

johnnyshields commented Aug 29, 2021

johnnyshields commented May 27, 2021 •

edited

Loading

johnnyshields commented Aug 3, 2021 •

edited

Loading

johnnyshields commented Aug 3, 2021 •

edited

Loading

johnnyshields commented Aug 5, 2021 •

edited

Loading