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)