From 2b259dfc53612e34bcb7148094d137a6cc0cdd43 Mon Sep 17 00:00:00 2001 From: Richard Towers Date: Fri, 5 Jun 2026 15:19:33 +0000 Subject: [PATCH 1/2] Add breadth-first link expander behind a flag LinkExpansion::BreadthFirstExpander reproduces links_with_content using the shared batch SQL queries, walking the link graph breadth-first (one forward + one reverse query per level) instead of the current depth-first one-query-per-node traversal. LinkExpansion#links_with_content dispatches to it when LINK_EXPANSION_IMPLEMENTATION=breadth-first (default: depth-first). Reproduces existing behaviour exactly: per-path ancestor cycle pruning (not a global visited set), root link-type discovery, edition links only at the root (edition_id: NULL at child levels), reverse re-keying including the role_appointments -> person/role fan-out, auto_reverse_link, and the withdrawn override for SQL-sourced editions. Adds a link_source discriminator column to both SQL queries so the expander can identify editions reached via edition links: existing code never expands their children ("no nested edition links") and excludes edition-sourced reverse links at child levels. The column is additive and ignored by the GraphQL dataloaders. Co-Authored-By: Claude Opus 4.8 (1M context) --- app/queries/sql/linked_to_editions.sql | 8 +- .../sql/reverse_linked_to_editions.sql | 8 +- lib/link_expansion.rb | 19 ++ lib/link_expansion/auto_reverse_linker.rb | 40 ++++ lib/link_expansion/breadth_first_expander.rb | 196 ++++++++++++++++++ lib/link_expansion/root_edition_resolver.rb | 38 ++++ .../auto_reverse_linker_spec.rb | 75 +++++++ .../breadth_first_expander_spec.rb | 193 +++++++++++++++++ .../root_edition_resolver_spec.rb | 62 ++++++ 9 files changed, 635 insertions(+), 4 deletions(-) create mode 100644 lib/link_expansion/auto_reverse_linker.rb create mode 100644 lib/link_expansion/breadth_first_expander.rb create mode 100644 lib/link_expansion/root_edition_resolver.rb create mode 100644 spec/lib/link_expansion/auto_reverse_linker_spec.rb create mode 100644 spec/lib/link_expansion/breadth_first_expander_spec.rb create mode 100644 spec/lib/link_expansion/root_edition_resolver_spec.rb diff --git a/app/queries/sql/linked_to_editions.sql b/app/queries/sql/linked_to_editions.sql index abac2d6ed3..facafb67b2 100644 --- a/app/queries/sql/linked_to_editions.sql +++ b/app/queries/sql/linked_to_editions.sql @@ -19,7 +19,9 @@ 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 source_content_id, + 'edition' AS link_source, + unpublishings.type AS "unpublishings.type" FROM editions INNER JOIN documents ON editions.document_id = documents.id INNER JOIN links ON documents.content_id = links.target_content_id @@ -60,7 +62,9 @@ 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 source_content_id, + 'link_set' AS link_source, + unpublishings.type AS "unpublishings.type" FROM editions INNER JOIN documents ON editions.document_id = documents.id INNER JOIN links ON documents.content_id = links.target_content_id diff --git a/app/queries/sql/reverse_linked_to_editions.sql b/app/queries/sql/reverse_linked_to_editions.sql index f4fb11d6e3..9b2ec4918b 100644 --- a/app/queries/sql/reverse_linked_to_editions.sql +++ b/app/queries/sql/reverse_linked_to_editions.sql @@ -18,7 +18,9 @@ edition_linked_editions AS ( documents.content_id, documents.locale, TRUE AS is_primary_locale, - links.target_content_id + links.target_content_id, + 'edition' AS link_source, + unpublishings.type AS "unpublishings.type" FROM editions INNER JOIN documents ON editions.document_id = documents.id INNER JOIN links ON editions.id = links.edition_id @@ -49,7 +51,9 @@ link_set_linked_editions AS ( documents.content_id, documents.locale, documents.locale =:primary_locale AS is_primary_locale, - links.target_content_id + links.target_content_id, + 'link_set' AS link_source, + unpublishings.type AS "unpublishings.type" FROM editions INNER JOIN documents ON editions.document_id = documents.id INNER JOIN links ON documents.content_id = links.link_set_content_id diff --git a/lib/link_expansion.rb b/lib/link_expansion.rb index f868b33592..1de1a209e7 100644 --- a/lib/link_expansion.rb +++ b/lib/link_expansion.rb @@ -5,6 +5,12 @@ # The concept is documented in /docs/link-expansion.md # class LinkExpansion + # Selects the link expansion implementation. Defaults to the existing depth-first LinkGraph traversal; + # set LINK_EXPANSION_IMPLEMENTATION=breadth-first to use the new breadth-first batch-SQL expander. + def self.implementation + ENV.fetch("LINK_EXPANSION_IMPLEMENTATION", "depth-first").to_sym + end + def self.by_edition(edition, with_drafts: false) new(edition:, with_drafts:) end @@ -19,9 +25,22 @@ def initialize(options) end def links_with_content + if self.class.implementation == :"breadth-first" + return breadth_first_expander.links_with_content + end + populate_links(link_graph.links) end + def breadth_first_expander + @breadth_first_expander ||= LinkExpansion::BreadthFirstExpander.new( + edition: options[:edition], + content_id: options[:content_id], + locale: options[:edition] ? nil : options[:locale], + with_drafts:, + ) + end + def link_graph @link_graph ||= LinkGraph.new( root_content_id: content_id, diff --git a/lib/link_expansion/auto_reverse_linker.rb b/lib/link_expansion/auto_reverse_linker.rb new file mode 100644 index 0000000000..72892409ee --- /dev/null +++ b/lib/link_expansion/auto_reverse_linker.rb @@ -0,0 +1,40 @@ +class LinkExpansion::AutoReverseLinker + def initialize(root_edition:, with_drafts:) + @root_edition = root_edition + @with_drafts = with_drafts + end + + def apply(level_one_nodes) + return unless root_edition_hash + + level_one_nodes.each do |node| + reverse_type = node.link_type + next unless rules.is_reverse_link_type?(reverse_type) + next unless should_link?(reverse_type) + + rules.reverse_to_direct_link_type(reverse_type).each do |direct| + expanded = rules.expand_fields(root_edition_hash, link_type: direct, draft: with_drafts) + node.links[direct] = [expanded.merge(links: {})] + end + end + end + +private + + attr_reader :root_edition, :with_drafts + + def rules + ExpansionRules + end + + def root_edition_hash + return @root_edition_hash if defined?(@root_edition_hash) + + @root_edition_hash = LinkExpansion::EditionHash.from(root_edition) + end + + def should_link?(reverse_type) + Link::PERMITTED_UNPUBLISHED_LINK_TYPES.include?(reverse_type.to_s) || + root_edition_hash[:state] != "unpublished" + end +end diff --git a/lib/link_expansion/breadth_first_expander.rb b/lib/link_expansion/breadth_first_expander.rb new file mode 100644 index 0000000000..eb0a203ad5 --- /dev/null +++ b/lib/link_expansion/breadth_first_expander.rb @@ -0,0 +1,196 @@ +# Breadth-first link expansion. +# +# Builds the `links_with_content` tree using the two batch SQL queries shared +# with the GraphQL API (Queries::LinkedToEditions / ReverseLinkedToEditions). +# It walks the link graph one level at a time, issuing a small fixed number of +# queries per level (O(depth)) rather than one query per node (O(nodes)). +# +# See docs/link-expansion.md and the design notes in +# docs/arch/adr-014-batch-link-expansion-and-dependency-resolution.md for the +# tricky bits. +class LinkExpansion::BreadthFirstExpander + EditionAndContentId = Data.define(:id, :content_id) + Node = Data.define(:content_id, :link_type, :link_types_path, :excluded_content_ids, :links, :terminal) + NodeAndLinkTypes = Data.define(:node, :direct_types, :reverse_types) + + def initialize(edition: nil, content_id: nil, locale: nil, with_drafts: false) + @edition = edition + @content_id = edition&.content_id || content_id + @locale = edition&.locale || locale + @with_drafts = with_drafts + @forward_query = Queries::LinkedToEditions.new(locale: @locale, with_drafts:) + @reverse_query = Queries::ReverseLinkedToEditions.new(locale: @locale, with_drafts:) + @root_edition_resolver = LinkExpansion::RootEditionResolver.new( + edition: @edition, + content_id: @content_id, + locale: @locale, + with_drafts: @with_drafts, + ) + @rules = ExpansionRules + end + + def links_with_content + root_links = {} + level_one_nodes = expand_root(root_links) + LinkExpansion::AutoReverseLinker.new(root_edition:, with_drafts:).apply(level_one_nodes) + + frontier = level_one_nodes.reject(&:terminal) + frontier = expand_level(frontier) until frontier.empty? + + root_links + end + +private + + attr_reader :edition, :content_id, :locale, :with_drafts, :forward_query, :reverse_query, :root_edition_resolver, :rules + + def expand_root(root_links) + root_ids = EditionAndContentId.new(root_edition_id, content_id) + + reverse_types = rules.reverse_links + direct_types = discover_root_direct_link_types + + forward_input = direct_types.map { |type| [root_ids, type] } + reverse_input = reverse_input_for(root_ids, reverse_types) + + forward_results = forward_query.call(forward_input) + reverse_results = reverse_query.call(reverse_input) + + next_frontier = [] + + root = Node.new( + content_id:, + link_type: nil, + link_types_path: [], + excluded_content_ids: [], + links: root_links, + terminal: false, + ) + + # Root key order: reverse links, then direct links. Edition links are + # followed at the root, so reverse attachment here keeps edition-sourced rows. + attach_reverse(root, reverse_types, reverse_results, next_frontier, drop_edition_links: false) + attach_direct(root, direct_types, forward_results, next_frontier) + + next_frontier + end + + # The batch SQL needs explicit link types - for the root edition we expand all link types, + # so we need to query the DB for all link types from the root edition / content id. + def discover_root_direct_link_types + scope = Link.where(link_set_content_id: content_id) + scope = scope.or(Link.where(edition_id: root_edition_id)) if root_edition_id + scope.distinct.order(:link_type).pluck(:link_type) + end + + def expand_level(frontier) + forward_input = [] + reverse_input = [] + + nodes_and_link_types = frontier.map do |node| + direct_types = rules.link_expansion.allowed_direct_link_types(node.link_types_path) + reverse_types = rules.link_expansion.allowed_reverse_link_types(node.link_types_path) + + # edition_id is NULL for non-root nodes: edition links are only followed + # at the root (we don't support nested edition links). + child_ids = EditionAndContentId.new(nil, node.content_id) + direct_types.each { |type| forward_input << [child_ids, type.to_s] } + reverse_input.concat(reverse_input_for(child_ids, reverse_types)) + + NodeAndLinkTypes.new(node:, direct_types:, reverse_types:) + end + + forward_results = forward_query.call(forward_input) + reverse_results = reverse_query.call(reverse_input) + + next_frontier = [] + nodes_and_link_types.each do |node_and_types| + node = node_and_types.node + # Child key order: direct links, then reverse links. + # Edition links are not followed below root level. + attach_direct(node, node_and_types.direct_types, forward_results, next_frontier) + attach_reverse(node, node_and_types.reverse_types, reverse_results, next_frontier, drop_edition_links: true) + end + + next_frontier + end + + def attach_direct(parent, direct_types, forward_results, next_frontier) + direct_types.each do |type| + editions = forward_results.fetch([parent.content_id, type.to_s], []) + attach(parent, next_frontier, type.to_sym, editions) + end + end + + def attach_reverse(parent, reverse_types, reverse_results, next_frontier, drop_edition_links:) + reverse_types.each do |reverse_type| + editions = reverse_editions(reverse_results, parent.content_id, reverse_type) + attach(parent, next_frontier, reverse_type, editions, drop_edition_links:) + end + end + + # A reverse link type can fan out to several direct query types + # (e.g. :role_appointments => [:person, :role]) + # Emit one input row per direct type, in that order. + def reverse_input_for(ids, reverse_types) + reverse_types.flat_map do |reverse_type| + rules.reverse_to_direct_link_type(reverse_type).map { |direct| [ids, direct.to_s] } + end + end + + # Gather and re-key the reverse results for one reverse link type, the + # result-side counterpart of reverse_input_for. + def reverse_editions(reverse_results, source_content_id, reverse_type) + rules.reverse_to_direct_link_type(reverse_type).flat_map do |direct| + reverse_results.fetch([source_content_id, direct.to_s], []) + end + end + + # Attach the surviving editions for `link_type` into the parent's `links` hash + # and push a frontier node for each. Non-terminal nodes have their own links + # expanded at the next level; terminal (edition-link-sourced, root-level) nodes + # are pushed only so AutoReverseLinker can decorate them - the caller drops + # them from the frontier. + def attach(parent, next_frontier, link_type, editions, drop_edition_links: false) + survivors = survivors_for(editions, parent.excluded_content_ids, drop_edition_links:) + return if survivors.empty? + + survivors_with_links = survivors.map { |edition| [edition, {}] } + + survivors_with_links.each do |edition, child_links| + next_frontier << Node.new( + content_id: edition.content_id, + link_type:, + link_types_path: parent.link_types_path + [link_type], + excluded_content_ids: parent.excluded_content_ids + [edition.content_id], + links: child_links, + terminal: edition_link_sourced?(edition), + ) + end + + parent.links[link_type] = survivors_with_links.map do |edition, child_links| + expand_fields(edition, link_type).merge(links: child_links) + end + end + + def survivors_for(editions, excluded_content_ids, drop_edition_links:) + editions = editions.reject { |edition| edition_link_sourced?(edition) } if drop_edition_links + editions.reject { |edition| excluded_content_ids.include?(edition.content_id) } + end + + def edition_link_sourced?(edition) + edition.link_source == "edition" + end + + def expand_fields(edition, link_type) + rules.expand_fields(LinkExpansion::EditionHash.from(edition), link_type:, draft: with_drafts) + end + + def root_edition + root_edition_resolver.edition + end + + def root_edition_id + root_edition_resolver.id + end +end diff --git a/lib/link_expansion/root_edition_resolver.rb b/lib/link_expansion/root_edition_resolver.rb new file mode 100644 index 0000000000..ce4e995ef2 --- /dev/null +++ b/lib/link_expansion/root_edition_resolver.rb @@ -0,0 +1,38 @@ +# Resolves the root edition for link expansion. +# This will either be a caller-supplied edition or the best edition for the content_id +class LinkExpansion::RootEditionResolver + def initialize(edition: nil, content_id: nil, locale: nil, with_drafts: false) + @explicit_edition = edition + @content_id = content_id + @locale = locale + @with_drafts = with_drafts + end + + def edition + return @edition if defined?(@edition) + + @edition = @explicit_edition || load + end + + def id + edition&.id + end + +private + + attr_reader :content_id, :locale, :with_drafts + + def load + edition_ids = Queries::GetEditionIdsWithFallbacks.call( + [content_id], + locale_fallback_order: [locale, Edition::DEFAULT_LOCALE].uniq, + state_fallback_order: with_drafts ? %i[draft published withdrawn] : %i[published withdrawn], + ) + return nil if edition_ids.blank? + + # NOTE: we join unpublishing here so that we can work out if the edition is withdrawn, see EditionHash#withdrawn? + Edition.with_document.with_unpublishing + .select("editions.*", 'unpublishings.type AS "unpublishings.type"') + .find_by(id: edition_ids.first) + end +end diff --git a/spec/lib/link_expansion/auto_reverse_linker_spec.rb b/spec/lib/link_expansion/auto_reverse_linker_spec.rb new file mode 100644 index 0000000000..5e0d8ef330 --- /dev/null +++ b/spec/lib/link_expansion/auto_reverse_linker_spec.rb @@ -0,0 +1,75 @@ +RSpec.describe LinkExpansion::AutoReverseLinker do + include DependencyResolutionHelper + + # The linker only reads a node's link_type and mutates its links hash, so the + # spec uses a minimal stand-in for the expander's Node. + let(:node_class) { Data.define(:link_type, :links) } + + def node(link_type) + node_class.new(link_type:, links: {}) + end + + def apply(root_edition, nodes, with_drafts: false) + described_class.new(root_edition:, with_drafts:).apply(nodes) + end + + it "back-links reverse-type nodes to the root under the direct link type" do + root = create_edition(SecureRandom.uuid, "/root") + child = node(:children) + + apply(root, [child]) + + expect(child.links).to match( + parent: [a_hash_including(base_path: "/root", links: {})], + ) + end + + it "fans out to every direct type behind the reverse type (role_appointments => person / role)" do + root = create_edition(SecureRandom.uuid, "/appointment") + appointment = node(:role_appointments) + + apply(root, [appointment]) + + expect(appointment.links.keys).to eq(%i[person role]) + expect(appointment.links.values).to all( + match([a_hash_including(base_path: "/appointment", links: {})]), + ) + end + + it "leaves nodes of non-reverse link types alone" do + root = create_edition(SecureRandom.uuid, "/root") + organisation = node(:organisation) + + apply(root, [organisation]) + + expect(organisation.links).to eq({}) + end + + it "does nothing when there is no root edition" do + child = node(:children) + + apply(nil, [child]) + + expect(child.links).to eq({}) + end + + describe "unpublished root editions" do + it "still back-links the permitted unpublished link types" do + root = create_edition(SecureRandom.uuid, "/root", factory: :withdrawn_unpublished_edition) + child = node(:children) + + apply(root, [child]) + + expect(child.links.keys).to eq([:parent]) + end + + it "does not back-link other link types" do + root = create_edition(SecureRandom.uuid, "/root", factory: :withdrawn_unpublished_edition) + appointment = node(:role_appointments) + + apply(root, [appointment]) + + expect(appointment.links).to eq({}) + end + end +end diff --git a/spec/lib/link_expansion/breadth_first_expander_spec.rb b/spec/lib/link_expansion/breadth_first_expander_spec.rb new file mode 100644 index 0000000000..18255d2adf --- /dev/null +++ b/spec/lib/link_expansion/breadth_first_expander_spec.rb @@ -0,0 +1,193 @@ +RSpec.describe LinkExpansion::BreadthFirstExpander do + include DependencyResolutionHelper + + let(:a) { create_link_set } + let(:b) { create_link_set } + let(:c) { create_link_set } + let(:d) { create_link_set } + + def expand(content_id, with_drafts: true) + described_class.new(content_id:, locale: "en", with_drafts:).links_with_content + end + + describe "traversal structure" do + before do + create_edition(a, "/a", factory: :draft_edition) + create_edition(b, "/b", factory: :draft_edition) + create_edition(c, "/c", factory: :draft_edition) + create_edition(d, "/d", factory: :draft_edition) + end + + it "expands a multi-level recursive chain to its full depth" do + create_link(a, b, "parent") + create_link(b, c, "parent") + create_link(c, d, "parent") + + expect(expand(a)[:parent]).to match([ + a_hash_including(base_path: "/b", links: { + parent: [a_hash_including(base_path: "/c", links: { + parent: [a_hash_including(base_path: "/d", links: {})], + })], + }), + ]) + end + + it "prunes cycles per-path, not with a global visited set" do + create_link(a, b, "parent") + create_link(b, a, "parent") + + # the root reappears one level deep as b's parent, with its own children + # pruned (a global visited set would have excluded it entirely) + expect(expand(a)[:parent]).to match([ + a_hash_including(base_path: "/b", links: { + parent: [a_hash_including(base_path: "/a", links: {})], + }), + ]) + end + end + + # The order of link-type keys in the output hash is deliberately asymmetric: + # reverse-then-direct at the root, direct-then-reverse at every deeper level. + # This matches the way link expansion worked historically, and is observable + # by Content Store, but it is otherwise only implied by the order of two + # blocks in expand_root / expand_level. These tests pin it so a refactor that + # "tidies" the two methods into one shared loop (which would naturally make + # the orderings consistent) fails loudly rather than silently changing published output. + describe "output key ordering" do + it "orders root link types reverse-then-direct" do + org = create_link_set + child = create_link_set + create_edition(a, "/a", factory: :draft_edition) + create_edition(org, "/org", factory: :draft_edition) + create_edition(child, "/child", factory: :draft_edition) + + create_link(a, org, "organisation") # a direct (link set) link from the root + create_link(child, a, "parent") # child is a child of a => root has :children + + expect(expand(a).keys).to eq(%i[children organisation]) + end + + it "orders child link types direct-then-reverse" do + taxon = create_link_set # the root; left without an edition so the + child_taxon = create_link_set # auto_reverse_link pass is skipped and the + associated = create_link_set # child's keys reflect only expand_level + grandchild_taxon = create_link_set + create_edition(child_taxon, "/child-taxon", factory: :draft_edition) + create_edition(associated, "/associated", factory: :draft_edition) + create_edition(grandchild_taxon, "/grandchild-taxon", factory: :draft_edition) + + # taxon -child_taxons-> child_taxon, reached via the reverse of parent_taxons + create_link(child_taxon, taxon, "parent_taxons") + # at [:child_taxons] both a direct (:associated_taxons) and a reverse + # (:child_taxons) link type are allowed; give the child_taxon one of each + create_link(child_taxon, associated, "associated_taxons") + create_link(grandchild_taxon, child_taxon, "parent_taxons") + + child = expand(taxon).fetch(:child_taxons).first + expect(child[:links].keys).to eq(%i[associated_taxons child_taxons]) + end + end + + describe "query count" do + before do + create_edition(a, "/a", factory: :draft_edition) + create_edition(b, "/b", factory: :draft_edition) + create_edition(c, "/c", factory: :draft_edition) + create_edition(d, "/d", factory: :draft_edition) + create_link(a, b, "parent") + create_link(b, c, "parent") + create_link(c, d, "parent") + end + + it "issues a bounded number of queries (O(depth), not O(nodes))" do + queries = [] + counter = lambda do |_name, _start, _finish, _id, payload| + queries << payload[:sql] unless payload[:name] == "SCHEMA" + end + + ActiveSupport::Notifications.subscribed(counter, "sql.active_record") do + expand(a) + end + + expect(queries.count).to be < 15 + end + end + + describe "reverse re-keying with fan-out (role_appointments => person / role)" do + it "buckets both the person and role reverse links under :role_appointments" do + person = create_link_set + role = create_link_set + appointment = create_link_set + + create_edition(appointment, "/appointment", factory: :draft_edition, document_type: "role_appointment", schema_name: "role_appointment") + create_edition(person, "/person", factory: :draft_edition, document_type: "person", schema_name: "person") + create_edition(role, "/role", factory: :draft_edition, document_type: "ministerial_role", schema_name: "role") + + # The appointment links to both a person and a role (the stored, "direct" + # link types). From the person's / role's point of view these are + # role_appointments (the reverse name). + create_link(appointment, person, "person") + create_link(appointment, role, "role") + + person_links = expand(person) + role_links = expand(role) + + expect(person_links[:role_appointments].map { _1[:base_path] }).to eq(["/appointment"]) + expect(role_links[:role_appointments].map { _1[:base_path] }).to eq(["/appointment"]) + end + end + + # Edition links are followed at the root but treated as leaf nodes, and are + # ignored entirely below the root (see the "edition links are only followed + # at root level" edge case in ADR-014). + describe "edition links as terminal leaf nodes" do + it "does not expand the links of an edition reached via a root edition link" do + create_edition(a, "/a", factory: :draft_edition, links_hash: { parent: [b] }) + create_edition(b, "/b", factory: :draft_edition) + create_edition(c, "/c", factory: :draft_edition) + create_link(b, c, "parent") # would expand to /c if /b were not terminal + + expect(expand(a)[:parent]).to match([ + a_hash_including(base_path: "/b", links: {}), + ]) + end + + it "keeps edition-sourced reverse links at the root" do + create_edition(b, "/b", factory: :draft_edition) + # c's *edition* links to b, so b sees c among its child_taxons + create_edition(c, "/c", factory: :draft_edition, links_hash: { parent_taxons: [b] }) + + expect(expand(b)[:child_taxons].map { _1[:base_path] }).to eq(["/c"]) + end + + it "drops edition-sourced reverse links below the root" do + # No root edition for `a`, so the auto-reverse pass is skipped and the + # child's links reflect only the traversal. + create_edition(b, "/b", factory: :draft_edition) + # the same edition link as above, but b is now one level down + create_edition(c, "/c", factory: :draft_edition, links_hash: { parent_taxons: [b] }) + create_link(b, a, "parent_taxons") # b is a child taxon of the root + + child = expand(a).fetch(:child_taxons).first + expect(child[:base_path]).to eq("/b") + expect(child[:links]).to eq({}) + end + end + + describe "no renderable root edition" do + it "still expands link set and reverse links without auto_reverse_link" do + # No edition exists for `a`, but it has link set links and is linked to. + create_edition(b, "/b", factory: :draft_edition) + create_edition(c, "/c", factory: :draft_edition) + create_link(a, b, "organisation") # link set link from a + create_link(c, a, "parent") # c is a child of a + + result = expand(a) + + expect(result[:organisation].map { _1[:base_path] }).to eq(["/b"]) + expect(result[:children].map { _1[:base_path] }).to eq(["/c"]) + # auto_reverse_link is skipped (no root edition to reverse-link back to) + expect(result[:children][0][:links]).to eq({}) + end + end +end diff --git a/spec/lib/link_expansion/root_edition_resolver_spec.rb b/spec/lib/link_expansion/root_edition_resolver_spec.rb new file mode 100644 index 0000000000..5c6372b9e0 --- /dev/null +++ b/spec/lib/link_expansion/root_edition_resolver_spec.rb @@ -0,0 +1,62 @@ +RSpec.describe LinkExpansion::RootEditionResolver do + include DependencyResolutionHelper + + let(:content_id) { SecureRandom.uuid } + + def resolver(locale: "en", with_drafts: false, edition: nil) + described_class.new(edition:, content_id:, locale:, with_drafts:) + end + + it "returns nil when no edition exists for the content_id" do + expect(resolver.edition).to be_nil + expect(resolver.id).to be_nil + end + + it "returns a caller-supplied edition in preference to anything in the database" do + create_edition(content_id, "/published") + explicit = create_edition(SecureRandom.uuid, "/explicit") + + expect(resolver(edition: explicit).edition).to equal(explicit) + end + + describe "state fallbacks" do + it "finds a draft edition when with_drafts is true" do + draft = create_edition(content_id, "/draft", factory: :draft_edition) + + expect(resolver(with_drafts: true).edition.id).to eq(draft.id) + end + + it "ignores draft editions when with_drafts is false" do + create_edition(content_id, "/draft", factory: :draft_edition) + + expect(resolver.edition).to be_nil + end + + it "finds a published edition" do + published = create_edition(content_id, "/published") + + expect(resolver.edition.id).to eq(published.id) + end + + it "falls back to a withdrawn edition when nothing is published" do + withdrawn = create_edition(content_id, "/withdrawn", factory: :withdrawn_unpublished_edition) + + expect(resolver.edition.id).to eq(withdrawn.id) + end + end + + describe "locale fallbacks" do + it "falls back to the default locale when the requested locale has no edition" do + en = create_edition(content_id, "/en", locale: "en") + + expect(resolver(locale: "ar").edition.id).to eq(en.id) + end + + it "prefers the requested locale when it exists" do + create_edition(content_id, "/en", locale: "en") + ar = create_edition(content_id, "/ar", locale: "ar") + + expect(resolver(locale: "ar").edition.id).to eq(ar.id) + end + end +end From 8faf1acba16aeadb5a73c6753239c4a5b8662cce Mon Sep 17 00:00:00 2001 From: Richard Towers Date: Fri, 5 Jun 2026 15:19:45 +0000 Subject: [PATCH 2/2] Add breadth-first dependency resolver behind a flag DependencyResolution::BreadthFirstResolver resolves dependent content_ids breadth-first, mirroring the link expander with directions swapped. DependencyResolution#dependencies dispatches to it when LINK_EXPANSION_IMPLEMENTATION=breadth-first (default: depth-first), the same flag and values the expander uses. Unlike link expansion, dependency resolution returns a dependent's content_id whether or not it has a renderable edition, so the resolver reads the links table directly rather than the edition-joining batch SQL. Edition links only matter at the root (child levels follow link set links only, as the depth-first implementation does), so the root reuses the existing Queries::Links / Queries::EditionLinks and the recursive levels batch plain links-table reads (one incoming + one outgoing query per level). This keeps spec/integration/dependency_resolution_spec.rb passing unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) Co-Authored-By: Claude Fable 5 --- lib/dependency_resolution.rb | 8 ++ .../breadth_first_resolver.rb | 135 ++++++++++++++++++ .../breadth_first_resolver_spec.rb | 114 +++++++++++++++ 3 files changed, 257 insertions(+) create mode 100644 lib/dependency_resolution/breadth_first_resolver.rb create mode 100644 spec/lib/dependency_resolution/breadth_first_resolver_spec.rb diff --git a/lib/dependency_resolution.rb b/lib/dependency_resolution.rb index 75e34f217f..e9dc6a142e 100644 --- a/lib/dependency_resolution.rb +++ b/lib/dependency_resolution.rb @@ -14,6 +14,14 @@ def initialize(content_id, locale: Edition::DEFAULT_LOCALE, with_drafts: false) end def dependencies + if LinkExpansion.implementation == :"breadth-first" + return DependencyResolution::BreadthFirstResolver.new( + content_id, + locale:, + with_drafts:, + ).dependencies + end + link_graph.links_content_ids end diff --git a/lib/dependency_resolution/breadth_first_resolver.rb b/lib/dependency_resolution/breadth_first_resolver.rb new file mode 100644 index 0000000000..5fc9fc80e4 --- /dev/null +++ b/lib/dependency_resolution/breadth_first_resolver.rb @@ -0,0 +1,135 @@ +# Breadth-first dependency resolution ("link expansion in reverse"). Returns a +# flat Array of dependent content_ids. +# +# Unlike link expansion, dependency resolution works purely on the links graph: +# a dependent's content_id is returned whether or not it has a renderable +# edition, so this resolver reads the `links` table directly rather than the +# edition-joining batch SQL the expander uses. Edition links are only relevant +# at the root. +class DependencyResolution::BreadthFirstResolver + Node = Data.define(:content_id, :link_types_path, :excluded_content_ids, :terminal) + + def initialize(content_id, locale:, with_drafts: false) + @content_id = content_id + @locale = locale + @with_drafts = with_drafts + @rules = ExpansionRules + end + + def dependencies + nodes = expand_root + # Level-1 nodes reached via edition links are "terminal": their children are + # never expanded (we don't support nested edition links). + frontier = nodes.reject(&:terminal) + until frontier.empty? + frontier = expand_level(frontier) + nodes.concat(frontier) + end + nodes.map(&:content_id).uniq + end + +private + + attr_reader :content_id, :locale, :with_drafts, :rules + + def dependency_resolution_rules + rules.dependency_resolution + end + + def expand_root + outgoing_link_types = rules.reverse_to_direct_link_types(rules.reverse_links) + + incoming_link_set = Queries::Links.to(content_id) + outgoing_link_set = rules.reverse_link_types_hash( + Queries::Links.from(content_id, allowed_link_types: outgoing_link_types), + ) + incoming_edition = Queries::EditionLinks.to( + content_id, locale:, with_drafts:, allowed_link_types: nil + ) + outgoing_edition = rules.reverse_link_types_hash( + Queries::EditionLinks.from( + content_id, locale:, with_drafts:, allowed_link_types: outgoing_link_types + ), + ) + + frontier = [] + # Link set sources can be expanded further; edition sources are terminal. + collect_root(frontier, incoming_link_set, terminal: false) + collect_root(frontier, outgoing_link_set, terminal: false) + collect_root(frontier, incoming_edition, terminal: true) + collect_root(frontier, outgoing_edition, terminal: true) + frontier + end + + def collect_root(frontier, links_by_link_type, terminal:) + links_by_link_type.each do |link_type, links| + links.each do |link| + dependency = link[:content_id] + # A level-1 node excludes only itself; the root's content_id is never + # added to any exclusion set, so it can legitimately reappear deeper in + # the graph. + frontier << Node.new( + content_id: dependency, + link_types_path: [link_type], + excluded_content_ids: [dependency], + terminal:, + ) + end + end + end + + def expand_level(frontier) + content_ids = frontier.map(&:content_id).uniq + + incoming = Link.link_set_links + .where(target_content_id: content_ids) + .pluck(:target_content_id, :link_type, :link_set_content_id) + .group_by(&:first) + + outgoing = Link.link_set_links + .where(link_set_content_id: content_ids) + .pluck(:link_set_content_id, :link_type, :target_content_id) + .group_by(&:first) + + next_frontier = [] + frontier.each do |node| + # "Direct" dependencies: things linking to this node. + allowed_direct = dependency_resolution_rules.allowed_direct_link_types(node.link_types_path).map(&:to_s) + incoming.fetch(node.content_id, []).each do |(_target, link_type, source)| + next unless allowed_direct.include?(link_type) + + add_dependency(next_frontier, source, node, link_type.to_sym) + end + + # "Reverse" dependencies: things this node links to that have reverse link types. + reverse_name_for = reverse_name_lookup(node.link_types_path) + outgoing.fetch(node.content_id, []).each do |(_source, link_type, target)| + reverse_name = reverse_name_for[link_type] + next unless reverse_name + + add_dependency(next_frontier, target, node, reverse_name) + end + end + + next_frontier + end + + def reverse_name_lookup(link_types_path) + dependency_resolution_rules.allowed_reverse_link_types(link_types_path).each_with_object({}) do |reverse_type, memo| + rules.reverse_to_direct_link_type(reverse_type).each do |direct| + memo[direct.to_s] = reverse_type + end + end + end + + def add_dependency(frontier, dependency, parent, link_type) + return if parent.excluded_content_ids.include?(dependency) + + frontier << Node.new( + content_id: dependency, + link_types_path: parent.link_types_path + [link_type], + excluded_content_ids: parent.excluded_content_ids + [dependency], + terminal: false, + ) + end +end diff --git a/spec/lib/dependency_resolution/breadth_first_resolver_spec.rb b/spec/lib/dependency_resolution/breadth_first_resolver_spec.rb new file mode 100644 index 0000000000..32b783f725 --- /dev/null +++ b/spec/lib/dependency_resolution/breadth_first_resolver_spec.rb @@ -0,0 +1,114 @@ +RSpec.describe DependencyResolution::BreadthFirstResolver do + include DependencyResolutionHelper + + let(:content_id) { SecureRandom.uuid } + + def resolve(with_drafts: true) + described_class.new(content_id, locale: "en", with_drafts:).dependencies + end + + it "resolves a multi-level reverse chain (child_taxons) to its full depth" do + a = SecureRandom.uuid + b = SecureRandom.uuid + c = SecureRandom.uuid + + create_link_set(content_id, links_hash: { parent_taxons: [a] }) + create_link_set(a, links_hash: { parent_taxons: [b] }) + create_link_set(b, links_hash: { parent_taxons: [c] }) + + expect(resolve).to match_array([a, b, c]) + end + + it "does not require the dependent content to have an edition" do + links_to = SecureRandom.uuid + create_link_set(links_to, links_hash: { organisation: [content_id] }) + + expect(resolve).to match_array([links_to]) + end + + it "bounds traversal through cycles via per-path ancestors" do + a = SecureRandom.uuid + b = SecureRandom.uuid + + create_link_set(a, links_hash: { parent_taxons: [content_id, b] }) + create_link_set(b, links_hash: { parent_taxons: [a] }) + + expect(resolve).to match_array([a, b]) + end + + it "follows only valid multi-level link paths" do + a = SecureRandom.uuid + b = SecureRandom.uuid + c = SecureRandom.uuid + + # Invalid ordering: mainstream_browse_pages then ordered_related_items is not + # a valid path, so only the direct parent dependency is found. + create_link_set(a, links_hash: { mainstream_browse_pages: [b] }) + create_link_set(b, links_hash: { ordered_related_items: [c] }) + create_link_set(c, links_hash: { parent: [content_id] }) + + expect(resolve).to match_array([c]) + end + + it "resolves the root's outgoing edition links of reverse types without following them" do + x = SecureRandom.uuid + y = SecureRandom.uuid + + # The root's *edition* links to x via parent_taxons, so x presents the root + # among its child_taxons and must be re-presented. Edition-sourced nodes + # are terminal, so x's own links are not followed. + create_edition(content_id, "/root", links_hash: { parent_taxons: [x] }) + create_link_set(x, links_hash: { parent_taxons: [y] }) + + expect(resolve).to match_array([x]) + end + + it "does not follow edition links recursively (nested edition links unsupported)" do + link_content_id = SecureRandom.uuid + edition_content_id = SecureRandom.uuid + + create_link_set(link_content_id, links_hash: { parent_taxons: [content_id] }) + create_edition( + edition_content_id, + "/edition-links", + links_hash: { parent_taxons: [link_content_id] }, + ) + + expect(resolve).to match_array([link_content_id]) + end + + describe "role_appointments fan-out (person / role)" do + it "resolves both the person and role reverse dependencies" do + person = SecureRandom.uuid + role = SecureRandom.uuid + + create_link_set(content_id, links_hash: { person: [person], role: [role] }) + + expect(resolve).to match_array([person, role]) + end + end + + describe "query count" do + it "issues a bounded number of queries (O(depth), not O(nodes))" do + # A wide, shallow graph: many dependents at a single level. A per-node + # implementation would issue queries proportional to the node count and + # blow past the bound; the batched resolver stays flat at O(depth). + level_1 = Array.new(40) { SecureRandom.uuid } + create_link_set(content_id, links_hash: { parent_taxons: level_1 }) + # A little depth on top of the width, so the bound covers both. + level_2 = SecureRandom.uuid + create_link_set(level_1.first, links_hash: { parent_taxons: [level_2] }) + + queries = [] + counter = lambda do |_name, _start, _finish, _id, payload| + queries << payload[:sql] unless payload[:name] == "SCHEMA" + end + + ActiveSupport::Notifications.subscribed(counter, "sql.active_record") do + resolve + end + + expect(queries.count).to be < 15 + end + end +end