Follow-up from "GraphQL: Fix N+1 issues with RunnerGroupsResolver"
The following discussion from !106627 (merged) should be addressed:
-
@terrichu started a discussion: (+7 comments) New queries
IpRestriction Load (0.1ms) SELECT "ip_restrictions".* FROM "ip_restrictions" WHERE "ip_restrictions"."group_id" = 22 /*application:web,correlation_id:01GM3GVSADWSKFH16VDGVN1HK3,endpoint_id:GraphqlController#execute,db_config_name:main,line:/ee/lib/gitlab/ip_restriction/enforcer.rb:31:in `allows_address?'*/ ↳ ee/lib/gitlab/ip_restriction/enforcer.rb:31:in `allows_address?' IpRestriction Load (0.2ms) SELECT "ip_restrictions".* FROM "ip_restrictions" WHERE "ip_restrictions"."group_id" = 27 /*application:web,correlation_id:01GM3GVSADWSKFH16VDGVN1HK3,endpoint_id:GraphqlController#execute,db_config_name:main,line:/ee/lib/gitlab/ip_restriction/enforcer.rb:31:in `allows_address?'*/ ↳ ee/lib/gitlab/ip_restriction/enforcer.rb:31:in `allows_address?'
question: from the new queries it looks like there may be an N+1 from the ip_restrictions but I'm not sure how to get my gdk data setup to test this (I ran the query from the description and it came back empty). Do you know what kind of data I need? I'm not sure where to suggest a change without playing around with it
😄
The EE version of the query is causing an N+1 query through repeated calls to Gitlab::GraphQL::Authorize::ConnectionFilterExtension::Redactor#redact
(one per group on the page), which in turn calls Gitlab::IpRestriction::Enforcer#allows_address?, due to group&.feature_available?(:group_ip_restriction)
being true. We should find a way to perform batching of redact calls and add an EE-specific test.
Part of #384066 (closed)
This behavior can be exposed with the following test:
Patch
diff --git a/ee/spec/requests/api/graphql/ci/runner_spec.rb b/ee/spec/requests/api/graphql/ci/runner_spec.rb
index 17f41edb6482..e3729c1e0c1f 100644
--- a/ee/spec/requests/api/graphql/ci/runner_spec.rb
+++ b/ee/spec/requests/api/graphql/ci/runner_spec.rb
@@ -152,4 +152,71 @@
end
end
end
+
+ describe 'Query limits' do
+ before do
+ allow(Gitlab::IpAddressState).to receive(:current).and_return('192.168.0.2')
+ stub_licensed_features(group_ip_restriction: true)
+ end
+
+ def runner_query(runner)
+ <<~SINGLE
+ runner(id: "#{runner.to_global_id}") {
+ id
+ groups {
+ nodes {
+ id
+ path
+ fullPath
+ webUrl
+ }
+ }
+ }
+ SINGLE
+ end
+
+ let(:active_group_runner1) { create(:ci_runner, :group) }
+ let(:active_group_runner2) { create(:ci_runner, :group) }
+
+ let(:single_query) do
+ <<~QUERY
+ {
+ group_runner1: #{runner_query(active_group_runner1)}
+ }
+ QUERY
+ end
+
+ let(:double_query) do
+ <<~QUERY
+ {
+ group_runner1: #{runner_query(active_group_runner1)}
+ group_runner2: #{runner_query(active_group_runner2)}
+ }
+ QUERY
+ end
+
+ it 'does not execute more queries per runner', :aggregate_failures do
+ # warm-up license cache and so on:
+ personal_access_token = create(:personal_access_token, user: admin)
+ args = { current_user: admin, token: { personal_access_token: personal_access_token } }
+ post_graphql(double_query, **args)
+
+ control = ActiveRecord::QueryRecorder.new { post_graphql(single_query, **args) }
+
+ expect { post_graphql(double_query, **args) }.not_to exceed_query_limit(control)
+
+ expect(graphql_data.count).to eq 2
+ expect(graphql_data).to match(
+ a_hash_including(
+ 'group_runner1' => a_graphql_entity_for(
+ active_group_runner1,
+ groups: { 'nodes' => active_group_runner1.groups.map { |g| a_graphql_entity_for(g) } }
+ ),
+ 'group_runner2' => a_graphql_entity_for(
+ active_group_runner2,
+ groups: { 'nodes' => active_group_runner2.groups.map { |g| a_graphql_entity_for(g) } }
+ ),
+ ))
+ end
+ end
end
The following GraphQL query can be used to reproduce the issue with GraphiQL Explorer (assuming that there are group runners in the database):
{
runners(type: GROUP_TYPE) {
nodes {
id
groups {
nodes {
id
path
fullPath
webUrl
}
}
}
}
}