From f7e1591e31d1ae2659a2d46ca2592d74f45b541f Mon Sep 17 00:00:00 2001 From: Richard Towers Date: Sun, 16 Nov 2025 13:24:14 +0000 Subject: [PATCH] Merge the dataloaders together The two dataloaders (linked_to_editions_source and reverse_linked_to_editions_source) were doing basically the same thing, but with slightly different sql for direct and reverse links. We can combine the SQL for both into a single giant four way UNION ALL if we take the direction of the links as query input. This is a slight performance win because of the way that the dataloaders batch up requests. If the same dataloader supports both direct and reverse links, sometimes that can save us a few SQL queries. For example, for /government/ministers/lord-in-waiting-government-whip--13 before this change we had to make a separate query to look up the reverse role link to find the role_appointment. After this change, we can combine this with the request to get the direct links (ordered_parent_organisations, organisations, taxons), making one fewer SQL query. In practice, this doesn't seem to be much of a performance win or loss, and it trades off reducing a bit of duplicated ruby and SQL code with creating one giant 163 line SQL query. The secret agenda behind it is that the resulting SQL query is pretty much what we'd need if we were to use a recursive CTE instead of using the dataloaders to batch editions. This could reduce the number of queries we're making by ~4 or 5, depending on how deep the links tree is. --- .../sources/linked_to_editions_source.rb | 10 +- .../sources/queries/linked_to_editions.sql | 107 ++++++++++++++++-- .../queries/reverse_linked_to_editions.sql | 74 ------------ .../reverse_linked_to_editions_source.rb | 40 ------- app/graphql/types/base_object.rb | 6 +- app/graphql/types/edition_type.rb | 8 +- .../sources/linked_to_editions_source_spec.rb | 30 ++--- .../reverse_linked_to_editions_source_spec.rb | 30 ++--- 8 files changed, 137 insertions(+), 168 deletions(-) delete mode 100644 app/graphql/sources/queries/reverse_linked_to_editions.sql delete mode 100644 app/graphql/sources/reverse_linked_to_editions_source.rb diff --git a/app/graphql/sources/linked_to_editions_source.rb b/app/graphql/sources/linked_to_editions_source.rb index d6a0b85c99..074fb93aaf 100644 --- a/app/graphql/sources/linked_to_editions_source.rb +++ b/app/graphql/sources/linked_to_editions_source.rb @@ -13,9 +13,9 @@ def initialize(content_store:, locale:) def fetch(editions_and_link_types) link_types_map = {} query_input = [] - editions_and_link_types.each do |edition, link_type| - query_input.push({ edition_id: edition.id, content_id: edition.content_id, link_type: }) - link_types_map[[edition.content_id, link_type]] = [] + editions_and_link_types.each do |edition, link_type, direction| + query_input.push({ edition_id: edition.id, content_id: edition.content_id, link_type:, direction: }) + link_types_map[[edition.content_id, link_type, direction.to_s]] = [] end sql_params = { @@ -30,8 +30,8 @@ def fetch(editions_and_link_types) all_editions = Edition.find_by_sql([SQL, sql_params]) all_editions.each(&:strict_loading!) all_editions.each_with_object(link_types_map) { |edition, hash| - unless hash[[edition.source_content_id, edition.link_type]].include?(edition) - hash[[edition.source_content_id, edition.link_type]] << edition + unless hash[[edition.requested_content_id, edition.link_type, edition.direction]].include?(edition) + hash[[edition.requested_content_id, edition.link_type, edition.direction]] << edition end }.values end diff --git a/app/graphql/sources/queries/linked_to_editions.sql b/app/graphql/sources/queries/linked_to_editions.sql index 9ee34f3c5b..59ebdc519b 100644 --- a/app/graphql/sources/queries/linked_to_editions.sql +++ b/app/graphql/sources/queries/linked_to_editions.sql @@ -5,12 +5,13 @@ WITH query_input AS ( json_to_recordset(:query_input::json) AS query_input ( edition_id integer, content_id uuid, - link_type varchar + link_type varchar, + direction varchar ) LIMIT (:query_input_count) -- noqa: AM09 ), -edition_linked_editions AS ( +direct_edition_linked_editions AS ( SELECT DISTINCT ON (links.id) editions.*, links.link_type, @@ -19,12 +20,18 @@ edition_linked_editions AS ( documents.content_id, documents.locale, documents.locale =:primary_locale AS is_primary_locale, - source_documents.content_id AS source_content_id + source_documents.content_id AS requested_content_id, + query_input.direction FROM editions INNER JOIN documents ON editions.document_id = documents.id INNER JOIN links ON documents.content_id = links.target_content_id INNER JOIN editions AS source_editions ON links.edition_id = source_editions.id - INNER JOIN query_input ON source_editions.id = query_input.edition_id AND links.link_type = query_input.link_type + INNER JOIN + query_input + ON + query_input.direction = 'direct' + AND source_editions.id = query_input.edition_id + AND links.link_type = query_input.link_type INNER JOIN documents AS source_documents ON source_editions.document_id = source_documents.id WHERE editions.content_store =:content_store @@ -37,7 +44,7 @@ edition_linked_editions AS ( ORDER BY links.id ASC, is_primary_locale DESC ), -link_set_linked_editions AS ( +direct_link_set_linked_editions AS ( SELECT DISTINCT ON (links.id) editions.*, links.link_type, @@ -46,13 +53,17 @@ link_set_linked_editions AS ( documents.content_id, documents.locale, documents.locale =:primary_locale AS is_primary_locale, - links.link_set_content_id AS source_content_id + links.link_set_content_id AS requested_content_id, + query_input.direction FROM editions INNER JOIN documents ON editions.document_id = documents.id INNER JOIN links ON documents.content_id = links.target_content_id INNER JOIN query_input - ON links.link_set_content_id = query_input.content_id AND links.link_type = query_input.link_type + ON + query_input.direction = 'direct' + AND links.link_set_content_id = query_input.content_id + AND links.link_type = query_input.link_type WHERE editions.content_store =:content_store AND documents.locale IN (:primary_locale,:secondary_locale) @@ -63,18 +74,90 @@ link_set_linked_editions AS ( ) -- skip any links that we already found in edition_linked_editions: AND NOT EXISTS ( - SELECT FROM edition_linked_editions + SELECT FROM direct_edition_linked_editions WHERE - edition_linked_editions.source_content_id = links.link_set_content_id - AND edition_linked_editions.link_type = links.link_type + direct_edition_linked_editions.requested_content_id = links.link_set_content_id + AND direct_edition_linked_editions.link_type = links.link_type + ) + ORDER BY links.id ASC, is_primary_locale DESC +), + +reverse_edition_linked_editions AS ( + -- NOTE: we're not using DISTINCT ON (links.id) here because the tests check that + -- if we have multiple links of the same link_type / target_content_id pointing + -- at editions of the same document with different locales, we should only get + -- the document with the best locale (rather than all of them). + -- It's not clear if the behaviour we're testing for is correct though. + -- Reverse edition links are a niche feature. + SELECT DISTINCT ON (documents.content_id, links.link_type, links.target_content_id) + editions.*, + links.link_type, + links.position, + links.id AS link_id, + documents.content_id, + documents.locale, + documents.locale =:primary_locale AS is_primary_locale, + links.target_content_id AS requested_content_id, + query_input.direction + FROM editions + INNER JOIN documents ON editions.document_id = documents.id + INNER JOIN links ON editions.id = links.edition_id + INNER JOIN + query_input + ON + query_input.direction = 'reverse' + AND links.target_content_id = query_input.content_id + AND links.link_type = query_input.link_type + WHERE + editions.content_store =:content_store + AND documents.locale IN (:primary_locale,:secondary_locale) + AND editions.document_type NOT IN (:non_renderable_formats) + AND ( + links.link_type IN (:unpublished_link_types) + OR editions.state != 'unpublished' + ) + ORDER BY documents.content_id ASC, links.link_type ASC, links.target_content_id ASC, is_primary_locale DESC +), + +reverse_link_set_linked_editions AS ( + SELECT DISTINCT ON (links.id) + editions.*, + links.link_type, + links.position, + links.id AS link_id, + documents.content_id, + documents.locale, + documents.locale =:primary_locale AS is_primary_locale, + links.target_content_id AS requested_content_id, + query_input.direction + FROM editions + INNER JOIN documents ON editions.document_id = documents.id + INNER JOIN links ON documents.content_id = links.link_set_content_id + INNER JOIN + query_input + ON + query_input.direction = 'reverse' + AND links.target_content_id = query_input.content_id + AND links.link_type = query_input.link_type + WHERE + editions.content_store =:content_store + AND documents.locale IN (:primary_locale,:secondary_locale) + AND editions.document_type NOT IN (:non_renderable_formats) + AND ( + links.link_type IN (:unpublished_link_types) + OR editions.state != 'unpublished' ) ORDER BY links.id ASC, is_primary_locale DESC ) SELECT editions.* FROM ( - SELECT * FROM link_set_linked_editions + SELECT * FROM direct_link_set_linked_editions + UNION ALL + SELECT * FROM direct_edition_linked_editions + UNION ALL + SELECT * FROM reverse_link_set_linked_editions UNION ALL - SELECT * FROM edition_linked_editions + SELECT * FROM reverse_edition_linked_editions ) AS editions ORDER BY editions.link_type ASC, editions.position ASC, editions.link_id DESC diff --git a/app/graphql/sources/queries/reverse_linked_to_editions.sql b/app/graphql/sources/queries/reverse_linked_to_editions.sql deleted file mode 100644 index 2281de1863..0000000000 --- a/app/graphql/sources/queries/reverse_linked_to_editions.sql +++ /dev/null @@ -1,74 +0,0 @@ --- reverse_linked_to_editions -WITH query_input AS ( - SELECT query_input.* - FROM - json_to_recordset(:query_input::json) AS query_input ( - content_id uuid, - link_type varchar - ) - LIMIT (:query_input_count) -- noqa: AM09 -), - -edition_linked_editions AS ( - -- NOTE: we're not using DISTINCT ON (links.id) here because the tests check that - -- if we have multiple links of the same link_type / target_content_id pointing - -- at editions of the same document with different locales, we should only get - -- the document with the best locale (rather than all of them). - -- It's not clear if the behaviour we're testing for is correct though. - -- Reverse edition links are a niche feature. - SELECT DISTINCT ON (documents.content_id, links.link_type, links.target_content_id) - editions.*, - links.link_type, - links.position, - links.id AS link_id, - documents.content_id, - documents.locale, - documents.locale =:primary_locale AS is_primary_locale, - links.target_content_id - FROM editions - INNER JOIN documents ON editions.document_id = documents.id - INNER JOIN links ON editions.id = links.edition_id - INNER JOIN query_input ON links.target_content_id = query_input.content_id AND links.link_type = query_input.link_type - WHERE - editions.content_store =:content_store - AND documents.locale IN (:primary_locale,:secondary_locale) - AND editions.document_type NOT IN (:non_renderable_formats) - AND ( - links.link_type IN (:unpublished_link_types) - OR editions.state != 'unpublished' - ) - ORDER BY documents.content_id ASC, links.link_type ASC, links.target_content_id ASC, is_primary_locale DESC -), - -link_set_linked_editions AS ( - SELECT DISTINCT ON (links.id) - editions.*, - links.link_type, - links.position, - links.id AS link_id, - documents.content_id, - documents.locale, - documents.locale =:primary_locale AS is_primary_locale, - links.target_content_id - FROM editions - INNER JOIN documents ON editions.document_id = documents.id - INNER JOIN links ON documents.content_id = links.link_set_content_id - INNER JOIN query_input ON links.target_content_id = query_input.content_id AND links.link_type = query_input.link_type - WHERE - editions.content_store =:content_store - AND documents.locale IN (:primary_locale,:secondary_locale) - AND editions.document_type NOT IN (:non_renderable_formats) - AND ( - links.link_type IN (:unpublished_link_types) - OR editions.state != 'unpublished' - ) - ORDER BY links.id ASC, is_primary_locale DESC -) - -SELECT editions.* FROM ( - SELECT * FROM link_set_linked_editions - UNION ALL - SELECT * FROM edition_linked_editions -) AS editions -ORDER BY - editions.link_type ASC, editions.position ASC, editions.link_id DESC diff --git a/app/graphql/sources/reverse_linked_to_editions_source.rb b/app/graphql/sources/reverse_linked_to_editions_source.rb deleted file mode 100644 index 2ff9177811..0000000000 --- a/app/graphql/sources/reverse_linked_to_editions_source.rb +++ /dev/null @@ -1,40 +0,0 @@ -module Sources - class ReverseLinkedToEditionsSource < GraphQL::Dataloader::Source - SQL = File.read(Rails.root.join("app/graphql/sources/queries/reverse_linked_to_editions.sql")) - - # rubocop:disable Lint/MissingSuper - def initialize(content_store:, locale:) - @content_store = content_store.to_sym - @primary_locale = locale - @secondary_locale = Edition::DEFAULT_LOCALE - end - # rubocop:enable Lint/MissingSuper - - def fetch(editions_and_link_types) - link_types_map = {} - query_input = [] - - editions_and_link_types.each do |edition, link_type| - query_input.push(content_id: edition.content_id, link_type:) - link_types_map[[edition.content_id, link_type]] = [] - end - - sql_params = { - query_input: query_input.to_json, - query_input_count: query_input.count, - secondary_locale: @secondary_locale, - primary_locale: @primary_locale, - content_store: @content_store, - unpublished_link_types: Link::PERMITTED_UNPUBLISHED_LINK_TYPES, - non_renderable_formats: Edition::NON_RENDERABLE_FORMATS, - } - all_editions = Edition.find_by_sql([SQL, sql_params]) - all_editions.each(&:strict_loading!) - all_editions.each_with_object(link_types_map) { |edition, hash| - unless hash[[edition.target_content_id, edition.link_type]].include?(edition) - hash[[edition.target_content_id, edition.link_type]] << edition - end - }.values - end - end -end diff --git a/app/graphql/types/base_object.rb b/app/graphql/types/base_object.rb index 7f8e86180f..f9ecdfdfaf 100644 --- a/app/graphql/types/base_object.rb +++ b/app/graphql/types/base_object.rb @@ -15,7 +15,7 @@ def self.links_field(link_type, graphql_field_type) content_store: object.content_store, locale: context[:root_edition].locale, ) - .load([object, link_type.to_s]) + .load([object, link_type.to_s, :direct]) end end @@ -24,11 +24,11 @@ def self.reverse_links_field(field_name, link_type, graphql_field_type) define_method(field_name.to_sym) do dataloader.with( - Sources::ReverseLinkedToEditionsSource, + Sources::LinkedToEditionsSource, content_store: object.content_store, locale: context[:root_edition].locale, ) - .load([object, link_type.to_s]) + .load([object, link_type.to_s, :reverse]) end end end diff --git a/app/graphql/types/edition_type.rb b/app/graphql/types/edition_type.rb index a247dd0725..7175eb0668 100644 --- a/app/graphql/types/edition_type.rb +++ b/app/graphql/types/edition_type.rb @@ -103,18 +103,18 @@ class EditionLinks < Types::BaseObject def role_appointments if %w[role ministerial_role].include?(object.document_type) dataloader.with( - Sources::ReverseLinkedToEditionsSource, + Sources::LinkedToEditionsSource, content_store: object.content_store, locale: context[:root_edition].locale, ) - .load([object, "role"]) + .load([object, "role", :reverse]) else dataloader.with( - Sources::ReverseLinkedToEditionsSource, + Sources::LinkedToEditionsSource, content_store: object.content_store, locale: context[:root_edition].locale, ) - .load([object, "person"]) + .load([object, "person", :reverse]) end end diff --git a/spec/graphql/sources/linked_to_editions_source_spec.rb b/spec/graphql/sources/linked_to_editions_source_spec.rb index 3bac91c3ae..3f1f13d407 100644 --- a/spec/graphql/sources/linked_to_editions_source_spec.rb +++ b/spec/graphql/sources/linked_to_editions_source_spec.rb @@ -17,7 +17,7 @@ described_class, content_store: source_edition.content_store, locale: "en", - ).request([source_edition, "test_link"]) + ).request([source_edition, "test_link", :direct]) actual_titles = request.load.map(&:title) expected_titles = [target_edition_1.title] @@ -45,7 +45,7 @@ described_class, content_store: source_edition.content_store, locale: "en", - ).request([source_edition, "test_link"]) + ).request([source_edition, "test_link", :direct]) actual_titles = request.load.map(&:title) expected_titles = [target_edition_1, target_edition_3].map(&:title) @@ -69,7 +69,7 @@ described_class, content_store: source_edition.content_store, locale: "en", - ).request([source_edition, "test_link"]) + ).request([source_edition, "test_link", :direct]) actual_titles = request.load.map(&:title) expected_titles = [target_edition_1.title] @@ -95,7 +95,7 @@ described_class, content_store: source_edition.content_store, locale: "en", - ).request([source_edition, "test_link"]) + ).request([source_edition, "test_link", :direct]) actual_titles = request.load.map(&:title) expected_titles = [target_edition_2, target_edition_0, target_edition_1].map(&:title) @@ -124,7 +124,7 @@ described_class, content_store: source_edition.content_store, locale: "en", - ).request([source_edition, "test_link"]) + ).request([source_edition, "test_link", :direct]) actual_titles = request.load.map(&:title) expected_titles = [target_edition_3, target_edition_0, target_edition_2, target_edition_1].map(&:title) @@ -150,7 +150,7 @@ described_class, content_store: source_edition.content_store, locale: "en", - ).request([source_edition, "parent"]) + ).request([source_edition, "parent", :direct]) actual_titles = request.load.map(&:title) expected_titles = [target_edition_0, target_edition_1].map(&:title) @@ -174,7 +174,7 @@ described_class, content_store: source_edition.content_store, locale: "en", - ).request([source_edition, "test_link"]) + ).request([source_edition, "test_link", :direct]) actual_titles = request.load.map(&:title) expected_titles = [target_edition_0.title] @@ -201,7 +201,7 @@ described_class, content_store: source_edition.content_store, locale: "fr", - ).request([source_edition, "test_link"]) + ).request([source_edition, "test_link", :direct]) actual_titles = request.load.map(&:title) expected_titles = [french_edition.title] @@ -226,7 +226,7 @@ described_class, content_store: source_edition.content_store, locale: "de", - ).request([source_edition, "test_link"]) + ).request([source_edition, "test_link", :direct]) actual_titles = request.load.map(&:title) expected_titles = [english_edition.title] @@ -251,7 +251,7 @@ described_class, content_store: source_edition.content_store, locale: "hu", - ).request([source_edition, "test_link"]) + ).request([source_edition, "test_link", :direct]) actual_titles = request.load.map(&:title) expected_titles = [] @@ -285,7 +285,7 @@ described_class, content_store: source_edition.content_store, locale: "fr", - ).request([source_edition, "test_link"]) + ).request([source_edition, "test_link", :direct]) actual_titles = request.load.map(&:title) expected_titles = [english_edition.title] @@ -318,7 +318,7 @@ described_class, content_store: source_edition.content_store, locale: "fr", - ).request([source_edition, "test_link"]) + ).request([source_edition, "test_link", :direct]) actual_titles = request.load.map(&:title) expected_titles = [] @@ -353,7 +353,7 @@ described_class, content_store: source_edition.content_store, locale: "fr", - ).request([source_edition, "related_statistical_data_sets"]) + ).request([source_edition, "related_statistical_data_sets", :direct]) actual_titles = request.load.map(&:title) expected_titles = [french_withdrawn_edition.title] @@ -386,7 +386,7 @@ described_class, content_store: source_edition.content_store, locale: "fr", - ).request([source_edition, "test_link"]) + ).request([source_edition, "test_link", :direct]) actual_titles = request.load.map(&:title) expected_titles = [english_edition.title] @@ -413,7 +413,7 @@ described_class, content_store: source_edition.content_store, locale: "en", - ).request([source_edition, "test_link"]) + ).request([source_edition, "test_link", :direct]) actual_titles = request.load.map(&:title) expected_titles = [renderable_edition.title] diff --git a/spec/graphql/sources/reverse_linked_to_editions_source_spec.rb b/spec/graphql/sources/reverse_linked_to_editions_source_spec.rb index 478d600e70..c132c749b0 100644 --- a/spec/graphql/sources/reverse_linked_to_editions_source_spec.rb +++ b/spec/graphql/sources/reverse_linked_to_editions_source_spec.rb @@ -1,4 +1,4 @@ -RSpec.describe Sources::ReverseLinkedToEditionsSource do +RSpec.describe Sources::LinkedToEditionsSource do context "when the same document is both a link set link and an edition link" do it "only returns the document once" do target_edition = create(:edition) @@ -16,7 +16,7 @@ described_class, content_store: target_edition.content_store, locale: "en", - ).request([target_edition, "test_link"]) + ).request([target_edition, "test_link", :reverse]) expect(request.load).to eq([source_edition]) end @@ -51,7 +51,7 @@ described_class, content_store: target_edition.content_store, locale: "en", - ).request([target_edition, "test_link"]) + ).request([target_edition, "test_link", :reverse]) actual_titles = request.load.map(&:title) expected_titles = [source_edition_1, source_edition_2].map(&:title) @@ -83,7 +83,7 @@ described_class, content_store: target_edition.content_store, locale: "en", - ).request([target_edition, "test_link"]) + ).request([target_edition, "test_link", :reverse]) actual_titles = request.load.map(&:title) expected_titles = [source_edition_2, source_edition_0, source_edition_1].map(&:title) @@ -131,7 +131,7 @@ described_class, content_store: target_edition.content_store, locale: "en", - ).request([target_edition, "test_link"]) + ).request([target_edition, "test_link", :reverse]) actual_titles = request.load.map(&:title) expected_titles = [source_edition_2, source_edition_1, source_edition_0].map(&:title) @@ -156,7 +156,7 @@ described_class, content_store: target_edition.content_store, locale: "en", - ).request([target_edition, "parent"]) + ).request([target_edition, "parent", :reverse]) actual_titles = request.load.map(&:title) expected_titles = [unpublished_edition].map(&:title) @@ -179,7 +179,7 @@ described_class, content_store: target_edition.content_store, locale: "en", - ).request([target_edition, "test_link"]) + ).request([target_edition, "test_link", :reverse]) actual_titles = request.load.map(&:title) expect(actual_titles).to eq([]) @@ -210,7 +210,7 @@ described_class, content_store: target_edition.content_store, locale: "en", - ).request([target_edition, "test_link"]) + ).request([target_edition, "test_link", :reverse]) actual_titles = request.load.map(&:title) expected_titles = [renderable_edition].map(&:title) @@ -245,7 +245,7 @@ described_class, content_store: target_edition.content_store, locale: "fr", - ).request([target_edition, "test_link"]) + ).request([target_edition, "test_link", :reverse]) actual_titles = request.load.map(&:title) expected_titles = [french_edition].map(&:title) @@ -279,7 +279,7 @@ described_class, content_store: target_edition.content_store, locale: "de", - ).request([target_edition, "test_link"]) + ).request([target_edition, "test_link", :reverse]) actual_titles = request.load.map(&:title) expected_titles = [english_edition].map(&:title) @@ -313,7 +313,7 @@ described_class, content_store: target_edition.content_store, locale: "hu", - ).request([target_edition, "test_link"]) + ).request([target_edition, "test_link", :reverse]) actual_titles = request.load.map(&:title) expect(actual_titles).to eq([]) @@ -347,7 +347,7 @@ described_class, content_store: target_edition.content_store, locale: "fr", - ).request([target_edition, "test_link"]) + ).request([target_edition, "test_link", :reverse]) actual_titles = request.load.map(&:title) expected_titles = [english_edition].map(&:title) @@ -381,7 +381,7 @@ described_class, content_store: target_edition.content_store, locale: "fr", - ).request([target_edition, "test_link"]) + ).request([target_edition, "test_link", :reverse]) actual_titles = request.load.map(&:title) expected_titles = [] @@ -417,7 +417,7 @@ described_class, content_store: target_edition.content_store, locale: "fr", - ).request([target_edition, "related_statistical_data_sets"]) + ).request([target_edition, "related_statistical_data_sets", :reverse]) actual_titles = request.load.map(&:title) expected_titles = [french_edition].map(&:title) @@ -451,7 +451,7 @@ described_class, content_store: target_edition.content_store, locale: "fr", - ).request([target_edition, "test_link"]) + ).request([target_edition, "test_link", :reverse]) actual_titles = request.load.map(&:title) expected_titles = [english_edition].map(&:title)