Skip to content

Commit cddda5f

Browse files
MONGOID-5740 Fix performance regression on belongs_to validation (backport to 8.0-stable) (#5794)
* MONGOID-5704 Fix performance regression on belongs_to validation (#5787) * MONGOID-5704 don't try to validate persisted, unchanged associations * we just need the attribute name * fix failing specs * tread very carefully so we don't trigger a load while validating * hopefully fix has-many behavior * documentation * simplify * make sure attributes getter is defined * Fix typo -- records are not loaded *from the database* Co-authored-by: Dmitry Rybakov <[email protected]> --------- Co-authored-by: Dmitry Rybakov <[email protected]> * use `rake ci` instead of `rake spec` to avoid side effects --------- Co-authored-by: Dmitry Rybakov <[email protected]>
1 parent ee91be5 commit cddda5f

File tree

5 files changed

+128
-49
lines changed

5 files changed

+128
-49
lines changed

Diff for: .github/workflows/test.yml

+1-1
Original file line numberDiff line numberDiff line change
@@ -313,7 +313,7 @@ jobs:
313313
- name: test
314314
timeout-minutes: 60
315315
continue-on-error: "${{matrix.experimental}}"
316-
run: bundle exec rake spec
316+
run: bundle exec rake ci
317317
env:
318318
BUNDLE_GEMFILE: "${{matrix.gemfile}}"
319319
MONGODB_URI: "${{ steps.start-mongodb.outputs.cluster-uri }}"

Diff for: lib/mongoid/validatable.rb

+8
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,14 @@ def exit_validate
3737
Threaded.exit_validate(self)
3838
end
3939

40+
# Perform a validation within the associated block.
41+
def validating
42+
begin_validate
43+
yield
44+
ensure
45+
exit_validate
46+
end
47+
4048
# Given the provided options, are we performing validations?
4149
#
4250
# @example Are we performing validations?

Diff for: lib/mongoid/validatable/associated.rb

+96-18
Original file line numberDiff line numberDiff line change
@@ -15,32 +15,110 @@ module Validatable
1515
#
1616
# validates_associated :name, :addresses
1717
# end
18-
class AssociatedValidator < ActiveModel::EachValidator
18+
class AssociatedValidator < ActiveModel::Validator
19+
# Required by `validates_with` so that the validator
20+
# gets added to the correct attributes.
21+
def attributes
22+
options[:attributes]
23+
end
1924

20-
# Validates that the associations provided are either all nil or all
21-
# valid. If neither is true then the appropriate errors will be added to
22-
# the parent document.
25+
# Checks that the named associations of the given record
26+
# (`attributes`) are valid. This does NOT load the associations
27+
# from the database, and will only validate records that are dirty
28+
# or unpersisted.
2329
#
24-
# @example Validate the association.
25-
# validator.validate_each(document, :name, name)
30+
# If anything is not valid, appropriate errors will be added to
31+
# the `document` parameter.
32+
#
33+
# @param [ Mongoid::Document ] document the document with the
34+
# associations to validate.
35+
def validate(document)
36+
options[:attributes].each do |attr_name|
37+
validate_association(document, attr_name)
38+
end
39+
end
40+
41+
private
42+
43+
# Validates that the given association provided is either nil,
44+
# persisted and unchanged, or invalid. Otherwise, the appropriate errors
45+
# will be added to the parent document.
2646
#
2747
# @param [ Document ] document The document to validate.
2848
# @param [ Symbol ] attribute The association to validate.
29-
# @param [ Object ] value The value of the association.
30-
def validate_each(document, attribute, value)
31-
begin
32-
document.begin_validate
33-
valid = Array.wrap(value).collect do |doc|
34-
if doc.nil? || doc.flagged_for_destroy?
35-
true
49+
def validate_association(document, attribute)
50+
# grab the proxy from the instance variable directly; we don't want
51+
# any loading logic to run; we just want to see if it's already
52+
# been loaded.
53+
proxy = document.ivar(attribute)
54+
return unless proxy
55+
56+
# if the variable exists, now we see if it is a proxy, or an actual
57+
# document. It might be a literal document instead of a proxy if this
58+
# document was created with a Document instance as a provided attribute,
59+
# e.g. "Post.new(message: Message.new)".
60+
target = proxy.respond_to?(:_target) ? proxy._target : proxy
61+
62+
# Now, fetch the list of documents from the target. Target may be a
63+
# single value, or a list of values, and in the case of HasMany,
64+
# might be a rather complex collection. We need to do this without
65+
# triggering a load, so it's a bit of a delicate dance.
66+
list = get_target_documents(target)
67+
68+
valid = document.validating do
69+
# Now, treating the target as an array, look at each element
70+
# and see if it is valid, but only if it has already been
71+
# persisted, or changed, and hasn't been flagged for destroy.
72+
list.all? do |value|
73+
if value && !value.flagged_for_destroy? && (!value.persisted? || value.changed?)
74+
value.validated? ? true : value.valid?
3675
else
37-
doc.validated? ? true : doc.valid?
76+
true
3877
end
39-
end.all?
40-
ensure
41-
document.exit_validate
78+
end
79+
end
80+
81+
document.errors.add(attribute, :invalid) unless valid
82+
end
83+
84+
private
85+
86+
# Examine the given target object and return an array of
87+
# documents (possibly empty) that the target represents.
88+
#
89+
# @param [ Array | Mongoid::Document | Mongoid::Association::Proxy | HasMany::Enumerable ] target
90+
# the target object to examine.
91+
#
92+
# @return [ Array<Mongoid::Document> ] the list of documents
93+
def get_target_documents(target)
94+
if target.respond_to?(:_loaded?)
95+
get_target_documents_for_has_many(target)
96+
else
97+
get_target_documents_for_other(target)
4298
end
43-
document.errors.add(attribute, :invalid, **options) unless valid
99+
end
100+
101+
# Returns the list of all currently in-memory values held by
102+
# the target. The target will not be loaded.
103+
#
104+
# @param [ HasMany::Enumerable ] target the target that will
105+
# be examined for in-memory documents.
106+
#
107+
# @return [ Array<Mongoid::Document> ] the in-memory documents
108+
# held by the target.
109+
def get_target_documents_for_has_many(target)
110+
[ *target._loaded.values, *target._added.values ]
111+
end
112+
113+
# Returns the target as an array. If the target represents a single
114+
# value, it is wrapped in an array.
115+
#
116+
# @param [ Array | Mongoid::Document | Mongoid::Association::Proxy ] target
117+
# the target to return.
118+
#
119+
# @return [ Array<Mongoid::Document> ] the target, as an array.
120+
def get_target_documents_for_other(target)
121+
Array.wrap(target)
44122
end
45123
end
46124
end

Diff for: spec/mongoid/validatable/associated_spec.rb

+13-30
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,6 @@
7575
end
7676

7777
it "does not run validation on them" do
78-
expect(description).to receive(:valid?).never
7978
expect(user).to be_valid
8079
end
8180

@@ -84,22 +83,22 @@
8483
end
8584
end
8685

87-
describe "#validate_each" do
86+
describe "#validate" do
8887

8988
let(:person) do
9089
Person.new
9190
end
9291

9392
let(:validator) do
94-
described_class.new(attributes: person.attributes)
93+
described_class.new(attributes: person.relations.keys)
9594
end
9695

9796
context "when the association is a one to one" do
9897

9998
context "when the association is nil" do
10099

101100
before do
102-
validator.validate_each(person, :name, nil)
101+
validator.validate(person)
103102
end
104103

105104
it "adds no errors" do
@@ -108,14 +107,9 @@
108107
end
109108

110109
context "when the association is valid" do
111-
112-
let(:associated) do
113-
double(valid?: true, flagged_for_destroy?: false)
114-
end
115-
116110
before do
117-
expect(associated).to receive(:validated?).and_return(false)
118-
validator.validate_each(person, :name, associated)
111+
person.name = Name.new(first_name: 'A', last_name: 'B')
112+
validator.validate(person)
119113
end
120114

121115
it "adds no errors" do
@@ -125,13 +119,9 @@
125119

126120
context "when the association is invalid" do
127121

128-
let(:associated) do
129-
double(valid?: false, flagged_for_destroy?: false)
130-
end
131-
132122
before do
133-
expect(associated).to receive(:validated?).and_return(false)
134-
validator.validate_each(person, :name, associated)
123+
person.name = Name.new(first_name: 'Jamis', last_name: 'Buck')
124+
validator.validate(person)
135125
end
136126

137127
it "adds errors to the parent document" do
@@ -149,7 +139,7 @@
149139
context "when the association is empty" do
150140

151141
before do
152-
validator.validate_each(person, :addresses, [])
142+
validator.validate(person)
153143
end
154144

155145
it "adds no errors" do
@@ -159,13 +149,9 @@
159149

160150
context "when the association has invalid documents" do
161151

162-
let(:associated) do
163-
double(valid?: false, flagged_for_destroy?: false)
164-
end
165-
166152
before do
167-
expect(associated).to receive(:validated?).and_return(false)
168-
validator.validate_each(person, :addresses, [ associated ])
153+
person.addresses << Address.new(street: '123')
154+
validator.validate(person)
169155
end
170156

171157
it "adds errors to the parent document" do
@@ -175,13 +161,10 @@
175161

176162
context "when the association has all valid documents" do
177163

178-
let(:associated) do
179-
double(valid?: true, flagged_for_destroy?: false)
180-
end
181-
182164
before do
183-
expect(associated).to receive(:validated?).and_return(false)
184-
validator.validate_each(person, :addresses, [ associated ])
165+
person.addresses << Address.new(street: '123 First St')
166+
person.addresses << Address.new(street: '456 Second St')
167+
validator.validate(person)
185168
end
186169

187170
it "adds no errors" do

Diff for: spec/support/models/name.rb

+10
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ class Name
44
include Mongoid::Document
55
include Mongoid::Attributes::Dynamic
66

7+
validate :is_not_jamis
8+
79
field :_id, type: String, overwrite: true, default: ->{
810
"#{first_name}-#{last_name}"
911
}
@@ -23,4 +25,12 @@ class Name
2325
def set_parent=(set = false)
2426
self.parent_title = namable.title if set
2527
end
28+
29+
private
30+
31+
def is_not_jamis
32+
if first_name == 'Jamis' && last_name == 'Buck'
33+
errors.add(:base, :invalid)
34+
end
35+
end
2636
end

0 commit comments

Comments
 (0)