[DO NOT MERGE] - ADR-014 end state#4098
Draft
richardTowers wants to merge 9 commits into
Draft
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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 depth-first implementation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <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 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) <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=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) <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Flip LINK_EXPANSION_IMPLEMENTATION's default from depth-first to breadth-first, so link expansion and dependency resolution use the batch-SQL breadth-first implementation by default. The depth-first LinkGraph traversal can still be restored with LINK_EXPANSION_IMPLEMENTATION=depth-first; it is removed in the next commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <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: 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.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <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. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
LinkExpansion and DependencyResolution had become effectively delegation shells around LinkExpansion::BreadthFirstExpander and DependencyResolution::BreadthFirstResolver. Now that the depth-first implementation is gone there is nothing to select between, and the "breadth-first" class names encoded migration history rather than information, so the implementations become the entry-point classes themselves. LinkExpansion is now the expander (still namespacing AutoReverseLinker, RootEditionResolver, EditionHash and EditionDiff); DependencyResolution is now the resolver, absorbing the wrapper's default locale. Callers are unchanged: LinkExpansion.by_edition / by_content_id and DependencyResolution.new(...).dependencies were already the public API. The expander unit spec merges into the existing spec/lib/ link_expansion_spec.rb; the resolver spec moves to spec/lib/dependency_resolution_spec.rb. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Because I can't push branches to alphagov anymore (😭), I've had to create the intermediate PRs on my fork.
We have:
This represents the entire stack of changes as one PR, so you can see overall summaries in terms of lines added / removed, or look through the commits one by one if you prefer. It would be reckless to merge this in one go though - you should pause after the second PR and do some manual testing that the implementations behave the same as each other.