Adr 014 remove legacy link expansion#1
Open
richardTowers wants to merge 17 commits into
Open
Conversation
Assert that when multiple links share a link_type and position, the expanded array is ordered by link_id DESC, for both direct and reverse links. This mirrors the SQL-level coverage in the (to-be-removed) spec/queries/links_spec.rb and guards the behaviour before the BFS rewrite begins. Green on the legacy implementation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move the two batch SQL queries (and their README) out of the GraphQL tree to app/queries/sql/, and introduce Queries::LinkedToEditions and Queries::ReverseLinkedToEditions as thin primitives that own the query_input JSON, sql_params, find_by_sql, dedup and grouping. They return a hash keyed by [content_id, link_type], pre-seeded with [] for every input key so missing results yield [] and ordering matches input. The GraphQL Sources::* dataloaders now delegate to these primitives and return result_hash.values, preserving the Dataloader contract. No behaviour change; the GraphQL parity suite stays green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 legacy depth-first
one-query-per-node traversal. LinkExpansion#links_with_content dispatches
to it when LINK_EXPANSION_IMPLEMENTATION=new (default: legacy).
Reproduces legacy 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: legacy 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) <noreply@anthropic.com>
DependencyResolution::BreadthFirstResolver resolves dependent content_ids breadth-first, mirroring the link expander with directions swapped. DependencyResolution#dependencies dispatches to it when LINK_EXPANSION_IMPLEMENTATION=new (default: legacy). 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 legacy 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) <noreply@anthropic.com>
Flip LINK_EXPANSION_IMPLEMENTATION's default from legacy to new, so link expansion and dependency resolution use the batch-SQL breadth-first implementation by default. The legacy depth-first LinkGraph traversal is retained for one release and can be restored with LINK_EXPANSION_IMPLEMENTATION=legacy for rollback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Delete the LinkGraph traversal now that the breadth-first batch-SQL
implementation is the default, and drop the LINK_EXPANSION_IMPLEMENTATION
flag so LinkExpansion and DependencyResolution are thin entry points to
the new expander/resolver.
Removed: app/models/link_graph{,/node,/node_collection_factory}.rb,
lib/link_expansion/{link_reference,content_cache}.rb,
lib/dependency_resolution/link_reference.rb, and their specs.
Retained (correcting the ADR's removal list): app/queries/links.rb and
app/queries/edition_links.rb, which the new dependency resolver uses at
the root; and lib/link_expansion/edition_hash.rb.
Docs updated to describe the breadth-first / two-batch-SQL model and the
new console-debugging entry points, and an amendment records the
deviations from the original ADR.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Now that the breadth-first resolver is the only caller of Queries::Links, strip the has_own_links / is_linked_to EXISTS optimisation and its next_allowed_link_types_from/to and parent_content_ids parameters. These existed solely to let the old depth-first traversal prune which nodes were worth visiting and so minimise its one-query-per-node count. The breadth-first resolver batches a whole level per query, never passes those parameters, and only reads each link's content_id, so the whole machinery was computed-or-skipped but never consumed. This takes the class from ~240 lines to ~70 and drops the EXISTS subqueries the ADR flagged as significant SQL complexity. Also reword the lingering "matches legacy" / "mirroring legacy" comments in the breadth-first expander and resolver to state the required behaviour directly, since the legacy LinkGraph implementation they referred to no longer exists. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The order of link-type keys in the expanded links hash is deliberately asymmetric: reverse-then-direct at the root, direct-then-reverse at every deeper level. This matches legacy link expansion and is observable by Content Store, but it was only implied by the order of two blocks in expand_root / expand_level and had no test. Add guards so a refactor that unifies the two methods into one shared loop (which would naturally make the orderings consistent) fails loudly rather than silently changing published output. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The expander was hard to review because a few methods each did several things at once. Make the tricky bits verifiable in isolation, with no change to output or to the O(depth) query batching: - Split the six-responsibility `attach` into `survivors_for` (the two rejects: the child_reverse edition-source drop, then per-path ancestor cycle pruning) and `attach_survivors` (build the emitted hash + push the frontier node). The empty-survivors early return stays in `attach`, so the omit-key-not-[] contract sits next to the assignment. - Give the withdrawn override a single home, `sql_edition_hash`, used by both `expand_fields` and the by_content_id branch of `root_edition_hash`; the by_edition branch deliberately keeps using the in-memory edition. - Name the per-level [node, direct_types, reverse_types] tuple as a `LevelTypes` value object, and split the child distribution into `attach_direct` / `attach_reverse` so the direct-then-reverse child key order is the literal order of two named calls rather than a comment. - Share the reverse query-input fan-out between root and child levels via `reverse_input_for` (the input-side counterpart of `reverse_editions`). expand_root and expand_level are deliberately left as two methods: their root-vs-child asymmetries (reverse-vs-direct key order, real-vs-NULL edition id, DB-discovered vs rule-derived types) are real, and unifying them would silently flip the key ordering now pinned by spec. Verified green: link expansion + edition links + the GraphQL parity suite (720 examples) + dependency resolution + presenter/downstream callers, and the <15 query-count canary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The batch SQL already LEFT JOINs unpublishings; select its `type` column (aliased "unpublishings.type") so LinkExpansion::EditionHash computes `withdrawn` itself — the field it already declares in POSSIBLE_FIELDS_FOR_LINK_EXPANSION and strips after use. The root edition, loaded via a separate path, gets the same column through the `with_unpublishing` scope. This removes the `sql_edition_hash` override and collapses the three-way `root_edition_hash` conditional (which only branched to apply that override differently) into a single EditionHash.from call. Behaviour-preserving: a published/draft edition can never carry an unpublishing row (the only caller that publishes a previously-withdrawn edition, Republish, destroys the unpublishing first; Publish only acts on drafts), so `unpublishings.type = 'withdrawal'` is identical to the old `state == 'unpublished'` proxy for every row these queries can return. The added column is additive and ignored by the GraphQL dataloaders. Verified green: link expansion + edition links + the GraphQL parity suite + dependency resolution + query-object/source specs + presenter/downstream callers (866 + 94 examples). Note: `sqlfluff lint` could not be run locally (not installed in this devcontainer); CI will lint the SQL. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
It was called exactly once (by apply_auto_reverse_links), and EditionHash.from is cheap and query-free now that root_edition is memoized — so neither a separate method nor its defined?-guarded cache earned anything. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move two self-contained responsibilities out of BreadthFirstExpander: - LinkExpansion::RootEdition resolves the root edition (the caller-supplied edition, or the locale/state fallback for the content_id — "decision 7"), loading it with its unpublishings.type column so EditionHash can derive `withdrawn` the same way the batch SQL does. - LinkExpansion::AutoReverseLinker runs the post-traversal pass that embeds the root edition back into its level-1 reverse links. This leaves the expander focused on the breadth-first traversal itself. No behaviour change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
expand_root and expand_level each distributed query results over their direct/reverse link types with near-identical loops. Introduce a small AttachTarget value object — the destination links hash, the source content_id, the parent path, and the child ancestors — so both can drive the same attach_direct/attach_reverse helpers. expand_root and expand_level stay separate: the root still supplies empty ancestors (so it can reappear one level deep) and still emits keys reverse-then-direct, while children emit direct-then-reverse and drop edition-sourced reverse rows. So cycle pruning and key ordering are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
attach_survivors had a single caller and split the "attach one link type" logic across two methods. Fold it back into attach and separate its two concerns — pushing the frontier nodes and building the emitted entries — into two loops over a shared (edition, child_links) pairing, rather than one dual-purpose map. The shared child_links hash (the top-down-mutation trick) is now created and named up front instead of mid-map. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Node and AttachTarget held nearly the same fields: parent_path was just a node's link_types_path, and child_ancestors was a node's ancestors plus the node itself. Storing the exclusion set *including* the node (renamed ancestors/child_ancestors -> excluded_content_ids) makes the node->attach- target transform the identity, so a node is its own attach target. So AttachTarget and the per-level projection in expand_level both go: the root is just the top Node (empty path, nothing excluded, root_links as its links), and child levels pass the node straight to attach_direct/attach_ reverse. link_type/terminal stay on Node as edge-metadata (nil/false for the root), read only for the level-1 nodes. Keeps the dependency resolver in sync: its Node drops `ancestors` for `excluded_content_ids` with the same include-self semantics, and add_dependency takes the parent node, so both files share one cycle-pruning idiom (check against parent.excluded_content_ids; children get + [own id]). Behaviour-preserving: the root is never added to any exclusion set, so it can still reappear deeper (A->B->A); verified by the full link-expansion + GraphQL parity + dependency-resolution suites and the O(depth) query canary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Restructure BreadthFirstExpander#initialize to resolve content_id and locale up front and build the forward/reverse queries eagerly, replacing the lazy reader methods with plain attr_readers. Also rename LevelTypes to NodeAndLinkTypes and the child_reverse: keyword to drop_edition_links: for clarity. This previously regressed link expansion for non-default locales: the eager query construction passed the `locale:` *parameter* (nil when called by_edition) instead of the resolved @Locale, so the SQL ran with a nil primary_locale and only matched default-locale targets. Build the queries from @Locale so e.g. an "fr" edition's "fr" links are expanded again. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment-only cleanup of BreadthFirstExpander: trim the verbose design-note comments down to a pointer at docs/link-expansion.md and the ADR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
richardTowers
force-pushed
the
adr-014-remove-legacy-link-expansion
branch
from
June 6, 2026 15:11
c0c56cb to
d45ce96
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Looks who's back on their bullshit 🙀
This is a vibe-coded implementation of link expansion and dependency resolution using a Breadth-First Search algorithm, leveraging the SQL queries developed for the GraphQL dataloaders.
This implementation should be a significant performance improvement over the old LinkGraph approach, as it makes
O(depth)queries instead ofO(nodes).I haven't actually reviewed the code myself yet, hence this PR targeting
richardTowers/publishing-apirather than upstream to alphagov.The tests pass though!