Skip to content
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.

Sign up for GitHub

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

Fix single rel caching issue. #1712

Closed
wants to merge 2 commits into from
Closed

Fix single rel caching issue. #1712

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6668d79
Fix single rel caching issue.
huseynov206 Dec 22, 2023
1443ca8
fix failed specs
huseynov206 Dec 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/active_graph/node/has_n.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,13 +111,17 @@ def init_cache

def add_to_cache(object, rel = nil)
(@cached_rels ||= []) << rel if rel
(@cached_result ||= []).tap { |results| results << object if object && !results.include?(object) }
(@cached_result ||= []).tap { |results| results << object if !results.include?(object) } if object
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you realize that this change is not equivalent? If object is nil then @cached_result will not be initialized if nil, while it was before. However this will make a difference only in case of a call with only nil parametesr add_to_cache(nil, nil).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a reason why I changed this code. After adding the rel function to AssociationProxy, I noticed that the following case is working incorrectly.

role = Role.find(xxx)
role.manager.source_rel
role.manager.source

Even though the manager has a source, if we call source after calling source_rel, it returns nil. Because we used the add_to_cache method in line has_n.rb:118, @cached_result was initialized as an empty array then in line has_n.rb:81 we check @cached_result, since it is initialized we use it, i.e. the empty array. To avoid this, I made @cached_result initialize only when the object exists. All specs and manual tests passed successfully.

  • Object parameter is nil only when we use the add_to_cache method for rels.

end

def rels
@cached_rels || super.tap { |rels| rels.each { |rel| add_to_cache(nil, rel) } }
end

def rel
rels.first
end

def cache_query_proxy_result
(result_cache_proc_cache || @query_proxy).to_a.tap { |result| cache_result(result) }
end
Expand Down
24 changes: 22 additions & 2 deletions spec/e2e/association_proxy_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,23 @@
has_one :out, :favorite_lesson, type: nil, model_class: :Lesson
has_many :out, :homework, type: :HOMEWORK, model_class: %w[Lesson Exam]
has_many :out, :friends, type: :friend, model_class: :Student
has_one :out, :school, rel_class: :SchoolRel
end

stub_relationship_class('LessonEnrollment') do
from_class :Student
to_class :Lesson
type :has_studet
type :has_student

property :grade
end

stub_relationship_class('SchoolRel') do
from_class :Student
to_class :School
type :has_student
end

stub_node_class('Lesson') do
property :subject
property :level, type: Integer
Expand All @@ -34,6 +41,11 @@
has_many :in, :lessons, model_class: :Lesson, origin: :exams_given
has_many :out, :students, type: :has_student, model_class: :Student
end

stub_node_class('School') do
property :name
has_many :out, :students, type: :has_student, model_class: :School
end
end

let(:billy) { Student.create(name: 'Billy') }
Expand All @@ -44,6 +56,7 @@
let(:science_exam2) { Exam.create(name: 'Science Exam 2') }
let(:leszek) { Student.create(name: 'Leszek', friends: [zinto]) }
let(:zinto) { Student.create(name: 'Zinto') }
let(:code_academy) { School.create(name: 'Code Academy') }

before do
[math, science].each { |lesson| billy.lessons << lesson }
Expand All @@ -52,6 +65,7 @@
science.exams_given << science_exam
science.exams_given << science_exam2
billy.favorite_lesson = math
billy.school = code_academy
end

context 'self referencing relationships' do
Expand Down Expand Up @@ -314,11 +328,17 @@
end

describe '#rels' do
it 'caches results for consecutive calls' do
it 'caches multi rels for consecutive calls' do
expect_queries(1) do
2.times { billy.lessons.rels }
end
end

it 'caches single rel for consecutive calls' do
expect_queries(1) do
2.times { billy.school.rel }
end
end
end

describe '#inspect' do
Expand Down
Loading