Merge pull request #3056 from alphagov/ministers-index-graphql-perfor…

…mance

Use GraphQL-Ruby Dataloaders to improve Ministers Index performance

app/graphql/sources/linked_to_editions_source.rb

@@ -0,0 +1,29 @@
+module Sources
+  class LinkedToEditionsSource < GraphQL::Dataloader::Source
+    # rubocop:disable Lint/MissingSuper
+    def initialize(parent_object:)
+      @object = parent_object
+      @content_store = parent_object.content_store.to_sym
+    end
+    # rubocop:enable Lint/MissingSuper
+
+    def fetch(link_types)
+      all_links = @object.document.link_set_links
+        .includes(target_documents: @content_store) # content_store is :live or :draft (a Document has_one Edition by that name)
+        .where(link_type: link_types)
+        .where(target_documents: { locale: "en" })
+
+      link_types_map = link_types.index_with { [] }
+
+      all_links.each_with_object(link_types_map) { |link, hash|
+        hash[link.link_type].concat(editions_for_link(link))
+      }.values
+    end
+
+  private
+
+    def editions_for_link(link)
+      link.target_documents.map { |document| document.send(@content_store) }
+    end
+  end
+end

app/graphql/sources/person_current_roles_source.rb

@@ -0,0 +1,51 @@
+module Sources
+  class PersonCurrentRolesSource < GraphQL::Dataloader::Source
+    def fetch(person_content_ids)
+      all_roles = Edition
+        .live
+        .includes(
+          document: {
+            reverse_links: { # role -> role_appointment
+              link_set: {
+                documents: [
+                  :editions, # role_appointment
+                  :link_set_links, # role_appointment -> person
+                ],
+              },
+            },
+          },
+        )
+        .where(
+          document_type: "ministerial_role",
+          document: { locale: "en" },
+          reverse_links: { link_type: "role" },
+          documents: { locale: "en" }, # role_appointment Documents
+          editions_documents: { document_type: "role_appointment" }, # role_appointment Editions
+          link_set_links: { target_content_id: person_content_ids, link_type: "person" },
+        )
+        .where("editions_documents.details ->> 'current' = 'true'") # editions_documents is the alias that Active Record gives to the role_appointment Editions in the SQL query
+        .order(reverse_links: { position: :asc })
+
+      ids_map = person_content_ids.index_with { [] }
+
+      all_roles.each_with_object(ids_map) { |role, hash|
+        hash[person_content_id_for_role(role)] << role
+      }.values
+    end
+
+  private
+
+    def person_content_id_for_role(role)
+      role_appointment_documents_for_role(role)
+        .flat_map(&:link_set_links)
+        .find { |link| link.link_type == "person" } # role_appointment -> person
+        .target_content_id
+    end
+
+    def role_appointment_documents_for_role(role)
+      role.document.reverse_links
+        .select { |link| link.link_type == "role" } # role -> role_appointment
+        .flat_map { |link| link.link_set.documents }
+    end
+  end
+end

app/graphql/types/base_object.rb

@@ -10,15 +10,8 @@ def self.links_field(field_name_and_link_type, graphql_field_type)
       field(field_name_and_link_type.to_sym, graphql_field_type)
 
       define_method(field_name_and_link_type.to_sym) do
-        Edition
-          .live
-          .includes(document: { reverse_links: :link_set })
-          .where(
-            document: { locale: "en" },
-            link_set: { content_id: object.content_id },
-            reverse_links: { link_type: field_name_and_link_type.to_s },
-          )
-          .order(reverse_links: { position: :asc })
+        dataloader.with(Sources::LinkedToEditionsSource, parent_object: object)
+          .load(field_name_and_link_type.to_s)
       end
     end
   end

app/graphql/types/ministers_index_type.rb

@@ -59,29 +59,8 @@ class MinistersIndexPersonLinks < Types::BaseObject
         field :role_appointments, [MinistersIndexRoleAppointment]
 
         def role_appointments
-          Edition
-            .live
-            .includes(
-              document: {
-                reverse_links: { # role -> role_appointment
-                  link_set: {
-                    documents: [
-                      :editions, # role_appointment
-                      :link_set_links, # role_appointment -> person
-                    ],
-                  },
-                },
-              },
-            )
-            .where(
-              document_type: "ministerial_role",
-              document: { locale: "en" },
-              reverse_links: { link_type: "role" },
-              editions_documents: { document_type: "role_appointment" },
-              link_set_links: { target_content_id: object.content_id, link_type: "person" },
-            )
-            .where("editions_documents.details ->> 'current' = 'true'") # editions_documents is the alias that Active Record gives to the role_appointment Editions in the SQL query
-            .order(reverse_links: { position: :asc })
+          dataloader.with(Sources::PersonCurrentRolesSource)
+            .load(object.content_id)
         end
       end
 

spec/graphql/sources/linked_to_editions_source_spec.rb

@@ -0,0 +1,18 @@
+RSpec.describe Sources::LinkedToEditionsSource do
+  it "returns the specified link set links" do
+    source_document = create(:edition)
+    target_document_1 = create(:edition)
+    target_document_2 = create(:edition)
+    target_document_3 = create(:edition)
+    link_set = create(:link_set, content_id: source_document.content_id)
+    create(:link, link_set: link_set, target_content_id: target_document_1.content_id, link_type: "test_link")
+    create(:link, link_set: link_set, target_content_id: target_document_2.content_id, link_type: "another_link_type")
+    create(:link, link_set: link_set, target_content_id: target_document_3.content_id, link_type: "test_link")
+
+    GraphQL::Dataloader.with_dataloading do |dataloader|
+      request = dataloader.with(described_class, parent_object: source_document).request("test_link")
+
+      expect(request.load).to eq([target_document_1, target_document_3])
+    end
+  end
+end

spec/graphql/sources/person_current_roles_source_spec.rb

@@ -0,0 +1,19 @@
+RSpec.describe Sources::PersonCurrentRolesSource do
+  include MinistersIndexHelpers
+
+  it "returns the current roles for a person" do
+    person_1 = create_person("Person 1")
+    role_1 = create_role("Role 1")
+    role_2 = create_role("Role 2")
+    role_3 = create_role("Role 3")
+    appoint_person_to_role(person_1, role_1)
+    appoint_person_to_role(person_1, role_2, current: false)
+    appoint_person_to_role(person_1, role_3)
+
+    GraphQL::Dataloader.with_dataloading do |dataloader|
+      request = dataloader.with(described_class).request(person_1.content_id)
+
+      expect(request.load).to eq([role_1, role_3])
+    end
+  end
+end

spec/integration/graphql/ministers_index_spec.rb

@@ -1,4 +1,6 @@
 RSpec.describe "GraphQL" do
+  include MinistersIndexHelpers
+
   describe "ministers index" do
     let(:index_page_link_set) do
       index_page = create(
@@ -169,13 +171,13 @@ def parsed_links
         person1 = create_person_with_role_appointment("Keir Starmer 1", "1st Minister")
         extra_role = create_role("First Lord of The Treasury")
         appoint_person_to_role(person1, extra_role)
-        add_link(person1, link_type: "ordered_cabinet_ministers", position: 0)
+        add_link(person1, link_type: "ordered_cabinet_ministers", link_set: index_page_link_set, position: 0)
 
         person3 = create_person_with_role_appointment("Keir Starmer 3", "3rd Minister")
-        add_link(person3, link_type: "ordered_cabinet_ministers", position: 2)
+        add_link(person3, link_type: "ordered_cabinet_ministers", link_set: index_page_link_set, position: 2)
 
         person2 = create_person_with_role_appointment("Keir Starmer 2", "2nd Minister")
-        add_link(person2, link_type: "ordered_cabinet_ministers", position: 1)
+        add_link(person2, link_type: "ordered_cabinet_ministers", link_set: index_page_link_set, position: 1)
       end
 
       it "exposes the links' fields" do
@@ -308,7 +310,7 @@ def parsed_links
           "Alan Campbell",
           "Parliamentary Secretary to the Treasury (Chief Whip)",
         )
-        add_link(person, link_type: "ordered_also_attends_cabinet")
+        add_link(person, link_type: "ordered_also_attends_cabinet", link_set: index_page_link_set)
       end
 
       it "exposes the links' fields" do
@@ -363,7 +365,7 @@ def parsed_links
           },
         )
         appoint_person_to_role(person, role)
-        add_link(person, link_type: "ordered_assistant_whips")
+        add_link(person, link_type: "ordered_assistant_whips", link_set: index_page_link_set)
       end
 
       it "exposes the links' fields" do
@@ -415,7 +417,7 @@ def parsed_links
         person = create_person("Lord Cryer")
         role = create_role("Lord in Waiting", role_payment_type: "Unpaid")
         appoint_person_to_role(person, role)
-        add_link(person, link_type: "ordered_baronesses_and_lords_in_waiting_whips")
+        add_link(person, link_type: "ordered_baronesses_and_lords_in_waiting_whips", link_set: index_page_link_set)
       end
 
       it "exposes the links' fields" do
@@ -465,7 +467,7 @@ def parsed_links
           "Baroness Wheeler MBE",
           "Captain of The King’s Bodyguard of the Yeoman of the Guard",
         )
-        add_link(person, link_type: "ordered_house_lords_whips")
+        add_link(person, link_type: "ordered_house_lords_whips", link_set: index_page_link_set)
       end
 
       it "exposes the links' fields" do
@@ -515,7 +517,7 @@ def parsed_links
           "Samantha Dixon MP",
           "Vice Chamberlain of HM Household",
         )
-        add_link(person, link_type: "ordered_house_of_commons_whips")
+        add_link(person, link_type: "ordered_house_of_commons_whips", link_set: index_page_link_set)
       end
 
       it "exposes the links' fields" do
@@ -565,7 +567,7 @@ def parsed_links
           "Vicky Foxcroft MP",
           "Junior Lord of the Treasury (Government Whip)",
         )
-        add_link(person, link_type: "ordered_junior_lords_of_the_treasury_whips")
+        add_link(person, link_type: "ordered_junior_lords_of_the_treasury_whips", link_set: index_page_link_set)
       end
 
       it "exposes the links' fields" do
@@ -612,7 +614,7 @@ def parsed_links
     describe "orderedMinisterialDepartments links" do
       before do
         cabinet_office = create_organisation("Cabinet Office")
-        add_link(cabinet_office, link_type: "ordered_ministerial_departments")
+        add_link(cabinet_office, link_type: "ordered_ministerial_departments", link_set: index_page_link_set)
 
         person = create_person("Keir Starmer")
         role = create_role("Prime Minister")
@@ -705,102 +707,4 @@ def parsed_links
   def create_index_page
     index_page_link_set # invoke `let`
   end
-
-  def create_organisation(title)
-    create(
-      :live_edition,
-      title:,
-      document_type: "organisation",
-      schema_name: "organisation",
-      base_path: "/government/organisations/#{title.parameterize}",
-    )
-  end
-
-  def create_role(title, role_payment_type: nil, whip_organisation: nil)
-    create(
-      :live_edition,
-      title: title,
-      document_type: "ministerial_role",
-      base_path: "/government/ministers/#{title.parameterize}",
-      details: {
-        attends_cabinet_type: nil,
-        body: [{
-          content: "# #{title}\nThe #{title} is the #{title} of His Majesty's Government",
-          content_type: "text/govspeak",
-        }],
-        supports_historical_accounts: true,
-        role_payment_type:,
-        seniority: 100,
-        whip_organisation:,
-      },
-    )
-  end
-
-  def appoint_person_to_role(person, role)
-    role_appointment = create(
-      :live_edition,
-      title: "#{person.title} - #{role.title}",
-      document_type: "role_appointment",
-      schema_name: "role_appointment",
-      details: {
-        current: true,
-        started_on: Time.zone.local(2024, 7, 5),
-      },
-    )
-
-    create(
-      :link_set,
-      content_id: role_appointment.content_id,
-      links_hash: { person: [person.content_id], role: [role.content_id] },
-    )
-  end
-
-  def create_person(title)
-    create(
-      :live_edition,
-      title: title,
-      document_type: "person",
-      schema_name: "person",
-      base_path: "/government/people/#{title.parameterize}",
-      details: {
-        body: [{
-          content: "#{title} A Role on 5 July 2024.",
-          content_type: "text/govspeak",
-        }],
-        image: {
-          url: "http://assets.dev.gov.uk/media/#{title.parameterize}.jpg",
-          alt_text: title,
-        },
-      },
-    )
-  end
-
-  def create_person_with_role_appointment(person_title, role_title)
-    person = create_person(person_title)
-    role = create_role(role_title)
-    appoint_person_to_role(person, role)
-
-    person
-  end
-
-  def add_link(target_content, link_type:, position: 0)
-    create(
-      :link,
-      position:,
-      link_type:,
-      link_set: index_page_link_set,
-      target_content_id: target_content.content_id,
-    )
-  end
-
-  def add_department_link(department, target_content, link_type:)
-    link_set = LinkSet.find_or_create_by!(content_id: department.content_id)
-
-    create(
-      :link,
-      link_type:,
-      link_set:,
-      target_content_id: target_content.content_id,
-    )
-  end
 end