Skip to content

Commit 681d98d

Browse files
JP-EllisJP-Ellis
committed
fix: avoid N+1 cascade
This eagerly loads data, thereby avoiding an N+1 cascade. The unit tests show a reduction in the number of SQL queries from ~400 to ~20. Testing in our dev environment also showed a 10x decrease in the response. The only concern with this change is that this may result in too much data being eagerly loaded. Having said that: - The data is required _at some point_ anyway, otherwise, it wouldn't be fetching all the data. A sequential loading may mean that less data is handled at any one time though. - All endpoints which use this decorator are impacted by the same N+1 cascade, so this change will not result in excessive data in endpoints which may not need it. Signed-off-by: JP-Ellis <josh@jpellis.me>
1 parent d26daa9 commit 681d98d

File tree

4 files changed

+302
-1
lines changed

4 files changed

+302
-1
lines changed

‎lib/pact_broker/api/decorators/version_decorator.rb‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ class VersionDecorator < BaseDecorator
2525
def self.eager_load_associations
2626
[
2727
:pacticipant,
28-
:pact_publications,
28+
{ pact_publications: [:consumer, :provider, { pact_version: :latest_verification }, :tags, :head_pact_publications_for_tags] },
2929
{ branch_versions: [:version, :branch_head, { branch: :pacticipant }] },
3030
{ tags: :head_tag }
3131
]

‎spec/features/get_branch_versions_spec.rb‎

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,4 +35,113 @@
3535

3636
it_behaves_like "a paginated response"
3737
end
38+
39+
context "performance: should not generate N+1 queries with multiple versions and pacts" do
40+
# This test checks that a single GET request to
41+
# `/pacticipants/{name}/branches/{branch}/version` does not generate an
42+
# excessive number of database queries when there are many versions on the
43+
# branch, each with multiple pacts and tags.
44+
45+
before do
46+
# Create test data that triggers N+1 issue:
47+
# - 1 consumer with multiple versions on same branch
48+
# - Each version has pacts to multiple providers
49+
# - Each version has tags (triggers head_tag_names queries)
50+
td.create_consumer("PerfConsumer")
51+
.create_provider("Provider1")
52+
.create_provider("Provider2")
53+
.create_provider("Provider3")
54+
.create_provider("Provider4")
55+
56+
# Create 50 versions, each with 4 pacts and 3 tags
57+
# This should create 200 pact_publications total
58+
50.times do |i|
59+
td.create_consumer_version("#{i+1}", branch: "perf-test-branch")
60+
.create_consumer_version_tag("prod")
61+
.create_consumer_version_tag("test")
62+
.create_consumer_version_tag("staging")
63+
.use_provider("Provider1")
64+
.create_pact
65+
.use_provider("Provider2")
66+
.create_pact
67+
.use_provider("Provider3")
68+
.create_pact
69+
.use_provider("Provider4")
70+
.create_pact
71+
end
72+
73+
# Temporarily allow lazy loading for PactPublication so we can measure
74+
# query count.
75+
PactBroker::Pacts::PactPublication.class_eval do
76+
alias_method :_orig_load_associated_objects, :_load_associated_objects
77+
def _load_associated_objects(opts, *args, &block)
78+
# Call allow_lazy_load to disable the restriction for this instance
79+
allow_lazy_load
80+
_orig_load_associated_objects(opts, *args, &block)
81+
end
82+
end
83+
84+
# Setup query counter in before block so it's active for the entire test
85+
# We'll capture the count before and after the request to isolate request queries
86+
# Use a local variable that will be captured in the closure
87+
query_count = [0] # Array so it can be mutated in the closure
88+
original_log_method = PactBroker::DB.connection.method(:log_connection_yield)
89+
90+
PactBroker::DB.connection.define_singleton_method(:log_connection_yield) do |sql, conn, args=nil, &block|
91+
if sql.to_s.upcase.start_with?("SELECT") && !sql.to_s.include?("sqlite_master")
92+
query_count[0] += 1
93+
end
94+
original_log_method.call(sql, conn, args, &block)
95+
end
96+
97+
# Store in instance variables so they're accessible in after block and test
98+
@query_count = query_count
99+
@original_log_method = original_log_method
100+
end
101+
102+
after do
103+
# Restore original database logging method
104+
if @original_log_method
105+
PactBroker::DB.connection.define_singleton_method(:log_connection_yield, @original_log_method)
106+
end
107+
108+
# Restore original _load_associated_objects method
109+
PactBroker::Pacts::PactPublication.class_eval do
110+
alias_method :_load_associated_objects, :_orig_load_associated_objects
111+
remove_method :_orig_load_associated_objects
112+
end
113+
end
114+
115+
it "returns branch versions without excessive queries" do
116+
# Get the branch created by the test data
117+
perf_consumer = PactBroker::Domain::Pacticipant.find(name: "PerfConsumer")
118+
consumer_branch = PactBroker::Versions::Branch.where(
119+
name: "perf-test-branch",
120+
pacticipant_id: perf_consumer.id
121+
).first
122+
perf_path = PactBroker::Api::PactBrokerUrls.branch_versions_url(consumer_branch)
123+
124+
# Make request, capturing only queries made during this request
125+
queries_before_request = @query_count[0]
126+
response = get(perf_path, { "pageNumber" => "1", "pageSize" => "20" })
127+
queries_during_request = @query_count[0] - queries_before_request
128+
129+
# Verify response is successful
130+
expect(response.status).to eq(200)
131+
132+
body = JSON.parse(response.body)
133+
versions = body.dig("_embedded", "versions") || []
134+
pact_versions_links = versions.flat_map { |v| v.dig("_links", "pb:pact-versions") || [] }
135+
136+
# Verify we got data back
137+
expect(versions.size).to eq(20)
138+
expect(pact_versions_links.size).to eq(80) # 20 versions × 4 pacts each
139+
140+
# Without fix: 415 queries
141+
# After fix: 24 queries
142+
puts "Branch versions endpoint query count: #{queries_during_request}"
143+
expect(queries_during_request).to be < 50,
144+
"Expected fewer than 50 queries, got #{queries_during_request}"
145+
end
146+
end
38147
end

‎spec/features/get_tag_versions_spec.rb‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,4 +57,102 @@
5757
end
5858

5959
end
60+
61+
context "performance: should not generate N+1 queries with multiple versions and pacts" do
62+
before do
63+
# Create test data that triggers N+1 issue:
64+
# - 1 consumer with multiple versions
65+
# - All versions tagged with same tag
66+
# - Each version has pacts to multiple providers
67+
td.create_consumer("PerfTagConsumer")
68+
.create_provider("Provider1")
69+
.create_provider("Provider2")
70+
.create_provider("Provider3")
71+
.create_provider("Provider4")
72+
73+
50.times do |i|
74+
td.create_consumer_version("#{i+1}")
75+
.create_consumer_version_tag("perf-tag")
76+
.create_consumer_version_tag("test")
77+
.create_consumer_version_tag("staging")
78+
.use_provider("Provider1")
79+
.create_pact
80+
.use_provider("Provider2")
81+
.create_pact
82+
.use_provider("Provider3")
83+
.create_pact
84+
.use_provider("Provider4")
85+
.create_pact
86+
end
87+
88+
# Temporarily allow lazy loading for PactPublication so we can measure query count
89+
PactBroker::Pacts::PactPublication.class_eval do
90+
alias_method :_orig_load_associated_objects, :_load_associated_objects
91+
def _load_associated_objects(opts, *args, &block)
92+
allow_lazy_load
93+
_orig_load_associated_objects(opts, *args, &block)
94+
end
95+
end
96+
97+
# Setup query counter
98+
query_count = [0] # Array so it can be mutated in the closure
99+
original_log_method = PactBroker::DB.connection.method(:log_connection_yield)
100+
101+
PactBroker::DB.connection.define_singleton_method(:log_connection_yield) do |sql, conn, args=nil, &block|
102+
if sql.to_s.upcase.start_with?("SELECT") && !sql.to_s.include?("sqlite_master")
103+
query_count[0] += 1
104+
end
105+
original_log_method.call(sql, conn, args, &block)
106+
end
107+
108+
@query_count = query_count
109+
@original_log_method = original_log_method
110+
end
111+
112+
after do
113+
# Restore original database logging method
114+
if @original_log_method
115+
PactBroker::DB.connection.define_singleton_method(:log_connection_yield, @original_log_method)
116+
end
117+
118+
# Restore original _load_associated_objects method
119+
PactBroker::Pacts::PactPublication.class_eval do
120+
alias_method :_load_associated_objects, :_orig_load_associated_objects
121+
remove_method :_orig_load_associated_objects
122+
end
123+
end
124+
125+
it "returns tag versions without excessive queries" do
126+
perf_consumer = PactBroker::Domain::Pacticipant.find(name: "PerfTagConsumer")
127+
perf_tag = PactBroker::Domain::Tag.where(
128+
name: "perf-tag",
129+
pacticipant_id: perf_consumer.id
130+
).first
131+
perf_path = PactBroker::Api::PactBrokerUrls.tag_versions_url(perf_tag)
132+
133+
# Query counting is done inside the test (not in before/after hooks) because:
134+
# 1. We only want to count queries during the actual HTTP request
135+
# 2. We don't want to count queries from test setup (finding consumer, tag, etc.)
136+
# 3. The counter must be reset for each test run
137+
138+
queries_before_request = @query_count[0]
139+
response = get(perf_path, { "pageNumber" => "1", "pageSize" => "20" })
140+
queries_during_request = @query_count[0] - queries_before_request
141+
142+
expect(response.status).to eq(200)
143+
144+
body = JSON.parse(response.body)
145+
versions = body.dig("_embedded", "versions") || []
146+
pact_versions_links = versions.flat_map { |v| v.dig("_links", "pb:pact-versions") || [] }
147+
148+
expect(versions.size).to eq(20)
149+
expect(pact_versions_links.size).to eq(80)
150+
151+
# Without fix: 411 queries
152+
# After fix: 20 queries
153+
puts "Tag versions endpoint query count: #{queries_during_request}"
154+
expect(queries_during_request).to be < 50,
155+
"Expected fewer than 50 queries, got #{queries_during_request}"
156+
end
157+
end
60158
end

‎spec/features/get_versions_spec.rb‎

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,4 +47,98 @@
4747
expect(subject).to be_a_404_response
4848
end
4949
end
50+
51+
context "performance: should not generate N+1 queries with multiple versions and pacts" do
52+
before do
53+
# Create test data that triggers N+1 issue:
54+
# - 1 consumer with multiple versions
55+
# - Each version has pacts to multiple providers
56+
# - Each version has tags (triggers head_tag_names queries)
57+
td.create_consumer("PerfConsumer")
58+
.create_provider("Provider1")
59+
.create_provider("Provider2")
60+
.create_provider("Provider3")
61+
.create_provider("Provider4")
62+
63+
50.times do |i|
64+
td.create_consumer_version("#{i+1}")
65+
.create_consumer_version_tag("prod")
66+
.create_consumer_version_tag("test")
67+
.create_consumer_version_tag("staging")
68+
.use_provider("Provider1")
69+
.create_pact
70+
.use_provider("Provider2")
71+
.create_pact
72+
.use_provider("Provider3")
73+
.create_pact
74+
.use_provider("Provider4")
75+
.create_pact
76+
end
77+
78+
# Temporarily allow lazy loading for PactPublication so we can measure query count
79+
PactBroker::Pacts::PactPublication.class_eval do
80+
alias_method :_orig_load_associated_objects, :_load_associated_objects
81+
def _load_associated_objects(opts, *args, &block)
82+
allow_lazy_load
83+
_orig_load_associated_objects(opts, *args, &block)
84+
end
85+
end
86+
87+
# Setup query counter
88+
query_count = [0] # Array so it can be mutated in the closure
89+
original_log_method = PactBroker::DB.connection.method(:log_connection_yield)
90+
91+
PactBroker::DB.connection.define_singleton_method(:log_connection_yield) do |sql, conn, args=nil, &block|
92+
if sql.to_s.upcase.start_with?("SELECT") && !sql.to_s.include?("sqlite_master")
93+
query_count[0] += 1
94+
end
95+
original_log_method.call(sql, conn, args, &block)
96+
end
97+
98+
@query_count = query_count
99+
@original_log_method = original_log_method
100+
end
101+
102+
after do
103+
# Restore original database logging method
104+
if @original_log_method
105+
PactBroker::DB.connection.define_singleton_method(:log_connection_yield, @original_log_method)
106+
end
107+
108+
# Restore original _load_associated_objects method
109+
PactBroker::Pacts::PactPublication.class_eval do
110+
alias_method :_load_associated_objects, :_orig_load_associated_objects
111+
remove_method :_orig_load_associated_objects
112+
end
113+
end
114+
115+
it "returns versions without excessive queries" do
116+
perf_consumer = PactBroker::Domain::Pacticipant.find(name: "PerfConsumer")
117+
perf_path = "/pacticipants/#{perf_consumer.name}/versions"
118+
119+
# Query counting is done inside the test (not in before/after hooks) because:
120+
# 1. We only want to count queries during the actual HTTP request
121+
# 2. We don't want to count queries from test setup (finding consumer, etc.)
122+
# 3. The counter must be reset for each test run
123+
124+
queries_before_request = @query_count[0]
125+
response = get(perf_path, { "pageNumber" => "1", "pageSize" => "20" })
126+
queries_during_request = @query_count[0] - queries_before_request
127+
128+
expect(response.status).to eq(200)
129+
130+
body = JSON.parse(response.body)
131+
versions = body.dig("_embedded", "versions") || []
132+
pact_versions_links = versions.flat_map { |v| v.dig("_links", "pb:pact-versions") || [] }
133+
134+
expect(versions.size).to eq(20)
135+
expect(pact_versions_links.size).to eq(80)
136+
137+
# Without fix: 410 queries
138+
# After fix: 19 queries
139+
puts "Versions endpoint query count: #{queries_during_request}"
140+
expect(queries_during_request).to be < 50,
141+
"Expected fewer than 50 queries, got #{queries_during_request}"
142+
end
143+
end
50144
end

0 commit comments

Comments
 (0)