MONGOID-5029 #empty method should use an #exists? rather than a #count query #5029

johnnyshields · 2021-08-17T17:17:45Z

#empty? method should use an exists? rather than a count query, which is more efficient (single document existence check vs. counting total docs)
#empty? (and #any?) should short-circuit avoid making a query if _added is set
#any? should call !empty? since empty is its exact inverse.

Specs DONE -- Ready for merge

verify that an exists query is used instead of count (both empty/any)
verify that if _added is set no DB query is performed (both empty/any)

- `#empty?` method should use an exists rather than a count query. - `#empty?` and `#any?` should short-circuit avoid making a query if _added is set - Make `#empty?` congruent with `#any?` code-wide

…ty-any-methods

p-mongo · 2021-08-25T02:09:32Z

lib/mongoid/association/referenced/has_many/enumerable.rb

@@ -209,9 +209,9 @@ def each
          # @since 2.1.0
          def empty?
            if _loaded?
-              in_memory.count == 0
+              in_memory.length == 0


Can this simply call in_memory.empty? ?

Yes, fixed.

p-mongo · 2021-08-25T02:10:14Z

spec/mongoid/association/referenced/has_many/enumerable_spec.rb

+  describe "#empty?" do
+
+    let(:person) do
+      Person.create


create! please, and elsewhere.

I'll do it here, but we really should just create a PR which does this across the entire repo. The reason I didn't use create! is because I copy-pasted from existing specs.

I can raise the PR once I have CI visibility. It will break many tests.

Fixed in this PR.

p-mongo · 2021-08-25T02:10:52Z

spec/mongoid/association/referenced/has_many/enumerable_spec.rb

+        described_class.new(criteria)
+      end
+
+      let!(:empty) do


Why does this use let! instead of let?

Because I copied the spec from #any and it was done that way there. I've changed it for both.

p-mongo · 2021-08-25T02:12:22Z

spec/mongoid/association/referenced/has_many/enumerable_spec.rb

+      end
+
+      it "retains the correct length" do
+        expect(enumerable.length).to eq(2)


If you intended to call enumerable.empty? and then perform additional assertions, these examples should be moved under their own context which has a before block which calls .empty?, and the context should be labeled something like "after .empty? is called".

OK, I've changed it. Again I copied an existing spec.

p-mongo · 2021-08-25T02:13:23Z

spec/mongoid/association/referenced/has_many/enumerable_spec.rb

+        expect(enumerable.to_a.length).to eq(2)
+      end
+
+      context "when iterating over the relation a second time" do


We use "association" because that is the correct term. "Relation" (per the original relational DB model) refers to values in the columns being related into a single table. For BC reasons the method names retain "relation" in a number of places, but all comments, local variables, prose etc. should use "association".

Again, I've just copy-pasted from existing specs. I will do the change here, but I recommend you raise a PR which does this change uniformly across the entire repo.

Changed in this PR.

johnnyshields · 2021-08-25T14:23:47Z

@p-mongo all requested changes done, let's merge.

p-mongo · 2021-08-27T03:16:37Z

I appreciate you repairing the issues that existed in the codebase previously. When reviewing I look at the diff so it's often not obvious that the code was copy-pasted from existing code.

johnnyshields · 2021-08-27T04:45:24Z

It would be great if you can raise PRs to fix some of the issues you highlighted uniformly across the codebase.

As a general guideline, if something is done a certain way 1000 times in the codebase, we shouldn't make it a requirement to change for specific PRs in order to merge them--the fix should be done uniformly in a separate PR. Contributors shouldn't be penalized for following apparently established conventions of the repo.

p-mongo · 2021-08-28T17:47:47Z

Hi Johnny,

It would be great if you can raise PRs to fix some of the issues you highlighted uniformly across the codebase.

I'm not sure this is realistic. Changing existing tests would require us to review all of them to ensure the tests continue to work as intended. When the tests are otherwise being changed, or new tests are being added, those get reviewed, and at this point it is relatively clear whether create! can be used in place of create for example. So, to take this PR as an example, I reviewed about 350 lines of changes, but our entire codebase is almost 3 million lines of spec tests.

As a general guideline, if something is done a certain way 1000 times in the codebase, we shouldn't make it a requirement to change for specific PRs in order to merge them--the fix should be done uniformly in a separate PR.

There was never a requirement to change existing code (unless it's being changed already for some other reason). The request was for new code to follow the current requirements.

Contributors shouldn't be penalized for following apparently established conventions of the repo.

If by "penalized" you refer to changing existing code, again, you don't have to do that. With any established project there is some code that isn't written according to current practices, if this code happens to get touched by a change it may be necessary to bring it into compliance with the current requirements. Same thing for new code, it is helpful to copy-paste existing specs for example instead of coming up with new specs from scratch but if the existing specs don't meet current requirements they may need to be revised. You still obtain the benefit of referercing/copying existing specs.

johnnyshields · 2021-08-29T05:04:16Z

I'm not sure this is realistic.

It's completely realistic. I just did it here: #5061 it took me 10 minutes. If the specs pass, then it's "working as intended".

p-mongo · 2021-08-29T14:31:58Z

If the specs pass, then it's "working as intended".

Unfortunately this isn't true. For example if you replace 10 specs with the same spec copied 10 times, the specs would pass but they wouldn't work "as intended".

johnnyshields · 2021-09-02T09:32:52Z

Looks like 2 approvals are done. Let's merge?

…t query (#5029) * Fixes MONGOID-5029 - `#empty?` method should use an exists rather than a count query. - `#empty?` and `#any?` should short-circuit avoid making a query if _added is set - Make `#empty?` congruent with `#any?` code-wide * #any? is exactly the inverse of #empty? * Specs for empty? * Add spec for any * Fix all feedback from oleg * adjust the tests since there are now fewer queries Co-authored-by: shields <shields@tablecheck.com> Co-authored-by: Oleg Pudeyev <code@olegp.name>

johnnyshields · 2021-09-05T19:19:21Z

Thanks!

* master: MONGOID-5029 #empty method should use an #exists? rather than a #count query (mongodb#5029)

@SInCE

* upstream/master: (27 commits) Create security policy following github's template (mongodb#5070) MONGOID-5131 Use proper mdb <-> ubuntu versions (mongodb#5067) MONGOID-5131: Set up build pipeline for outside contributors (mongodb#5043) MONGOID-5165 Fix documentation link (mongodb#5065) Fix indentation MONGOID-5029 #empty method should use an #exists? rather than a #count query (mongodb#5029) MONGOID-5177 Fix flaky contextual spec (mongodb#5064) MONGOID-5170 - Fix more typos mostly in code docs and code comments (mongodb#5056) MONGOID-5105 Allow block form in Mongoid::Association::EmbedsMany::Proxy#count (mongodb#5060) MONGOID-5162 Add a release note for the planned changes in ObjectId#as_json (mongodb#5059) MONGOID-5171 - Make the minimum Ruby version 2.5 (mongodb#5058) MONGOID-5130 Remove conn count from client spec (mongodb#5055) Add driver 2.15 to compat table Fix MONGOID-5136 .touch with custom field is broken for embedded documents (mongodb#5039) MONGOID-5144 Add a deprecation warning when using the legacy query cache (mongodb#5042) MONGOID-???? Replace field[0] == ?$ with field.start_with?('$') -- easier to read and 30%+ faster (mongodb#5052) MONGOID-???? - Remove BSON::ObjectId#as_json as we will defer to the bson-ruby gem implementation. (mongodb#5057) MONGOID-4737 Deprecate the :compact option on Document#as_json (mongodb#5040) MONGOID-5143 [Code Comments Only] Remove magic comment "# encoding: utf-8" (mongodb#5041) Remove all @SInCE comments. These are generally not useful as: (mongodb#5037) ...

* master: (26 commits) MONGOID-5128 Scoped associations (mongodb#5017) MONGOID-5005 - .sum, .count, and similar aggregables now ignore sort if not limiting/skipping (mongodb#5049) MONGOID-5098 Improve specs for timezone handling (specs only; no behavior change) (mongodb#5023) MONGOID-5151 Respect aliased fields in pluck/distinct by having Document.database_field_name recursively consider embedded docs (mongodb#5047) RUBY-2675 test coverage on the Mongoid side (mongodb#5030) MONGOID-4592 Add examples of using read_attribute and write_attribute to implement custom field behavior (mongodb#5082) MONGOId-5185 Remove tests for _id serialization (mongodb#5081) MONGOID-5103 Implement eq symbol operator (mongodb#5076) Fix MONGOID-5006 Link default auth source documentation to driver instead of incorrectly claiming "admin" is always the default (mongodb#5048) Create security policy following github's template (mongodb#5070) MONGOID-5131 Use proper mdb <-> ubuntu versions (mongodb#5067) MONGOID-5131: Set up build pipeline for outside contributors (mongodb#5043) MONGOID-5165 Fix documentation link (mongodb#5065) Fix indentation MONGOID-5029 #empty method should use an #exists? rather than a #count query (mongodb#5029) MONGOID-5177 Fix flaky contextual spec (mongodb#5064) MONGOID-5170 - Fix more typos mostly in code docs and code comments (mongodb#5056) MONGOID-5105 Allow block form in Mongoid::Association::EmbedsMany::Proxy#count (mongodb#5060) MONGOID-5162 Add a release note for the planned changes in ObjectId#as_json (mongodb#5059) MONGOID-5171 - Make the minimum Ruby version 2.5 (mongodb#5058) ...

Fixes MONGOID-5029

3a37177

- `#empty?` method should use an exists rather than a count query. - `#empty?` and `#any?` should short-circuit avoid making a query if _added is set - Make `#empty?` congruent with `#any?` code-wide

johnnyshields changed the title ~~Fixes MONGOID-5029~~ Fixes MONGOID-5029: #empty method should use an #exists? rather than a #count query Aug 17, 2021

johnnyshields changed the title ~~Fixes MONGOID-5029: #empty method should use an #exists? rather than a #count query~~ MONGOID-5029 #empty method should use an #exists? rather than a #count query Aug 17, 2021

johnnyshields added 4 commits August 18, 2021 02:26

#any? is exactly the inverse of #empty?

59b5281

Specs for empty?

1fd8fa5

Add spec for any

2773427

Merge remote-tracking branch 'remotes/origin/master' into improve-emp…

330257e

…ty-any-methods

p-mongo reviewed Aug 25, 2021

View reviewed changes

Fix all feedback from oleg

f8b3526

p-mongo added the oleg-check label Aug 27, 2021

adjust the tests since there are now fewer queries

4eb21ae

johnnyshields mentioned this pull request Aug 29, 2021

MONGOID-5173 Specs should use bang (!) methods as much as possible #5061

Merged

p-mongo requested a review from comandeo August 29, 2021 14:33

p-mongo approved these changes Aug 29, 2021

View reviewed changes

p-mongo added oleg-done and removed oleg-check labels Aug 29, 2021

comandeo approved these changes Sep 2, 2021

View reviewed changes

p-mongo merged commit d5b64c1 into mongodb:master Sep 5, 2021

johnnyshields deleted the improve-empty-any-methods branch September 5, 2021 19:19

p-mongo pushed a commit to johnnyshields/mongoid that referenced this pull request Sep 6, 2021

Merge branch 'master' into pr-5047

96bc691

* master: MONGOID-5029 #empty method should use an #exists? rather than a #count query (mongodb#5029)

MONGOID-5029 #empty method should use an #exists? rather than a #count query #5029

MONGOID-5029 #empty method should use an #exists? rather than a #count query #5029

Uh oh!

Conversation

johnnyshields commented Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnnyshields commented Aug 25, 2021

Uh oh!

p-mongo commented Aug 27, 2021

Uh oh!

johnnyshields commented Aug 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

p-mongo commented Aug 28, 2021

Uh oh!

johnnyshields commented Aug 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

p-mongo commented Aug 29, 2021

Uh oh!

johnnyshields commented Sep 2, 2021

Uh oh!

johnnyshields commented Sep 5, 2021

Uh oh!

Uh oh!

johnnyshields commented Aug 17, 2021 •

edited

Loading

johnnyshields commented Aug 27, 2021 •

edited

Loading

johnnyshields commented Aug 29, 2021 •

edited

Loading