ADR-014 - Batch link expansion and dependency resolution at publish time#4097
ADR-014 - Batch link expansion and dependency resolution at publish time#4097richardTowers wants to merge 3 commits into
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>
|
Hi Richard, good to hear for you. Thanks for the PR, which we've added to the backlog. We're currently thinking about how to solve the edition links recursion issue outside of GraphQL, and will look into implementation options in Q2, so these changes could help us with those decisions. |
|
Cheers Bruce! Feel free to take whatever you find valuable from these PRs and discard anything that isn't without needing to check with me first. I'm starting a new job in July, so I won't have quite so much time to kill. Note that in these PRs we go to some length to avoid recursive edition links (to keep parity with the current implementation). The link expansion code would actually be simpler if edition links were treated like link sets I think. Dependency Resolution would be more complicated though - at the moment it only looks at link sets, so it might get tricky. I found that current state-of-the-art LLMs were pretty useful for understanding the code and exploring options - obviously you still have to understand the code yourself to make decisions, but it's worth trying them out if it's permitted. Your mileage may vary. Good luck wrangling it! |
2 participants
Look who's back on their nonsense! 😅 Hope you're all having fun at GDS ❤️
There was one bit of publishing-api refactoring which I really wanted to do before I left, but sadly I never managed it while I worked at GDS. Namely consolidating the two approaches to link expansion. The implementation we used for GraphQL is much more efficient, because of the way it batches up SQL queries, and we're pretty confident it has parity with the existing approach by virtue of the extensive testing we've done. If we could use the same implementation for both publish-time and request-time link expansion, we'd get those efficiency benefits and avoid any risk of drift between the two implementations.
The thing that dissuaded me from getting this done in my time at GDS was the perceived difficulty - I'd have to understand both implementations fully, and I'd need to work out how to handle dependency resolution, which goes in the other direction. This felt like too much for me at the time, so I basically chickened out.
I've had a few months to sleep on it now, and in roughly that time AI coding assistants have also improved to the point that they're very helpful for refactoring tasks like this.
I'm about to drop a stack of 5 PRs on you, of which this is the first. A significant proportion of the code was written by Claude, but this is an area of the codebase I'm still very familiar with, and I've worked through the changes closely enough that I stand behind them - the code in these PRs is at least as good as I could have managed on my own. The prose in the ADR is human authored, typed out by my own fair hands - I'm still of the view that asking people to review a bunch of AI slop prose is disrespectful, even if I have somehow rationalised it's okay to do the same thing with code.
You may want to consider GDS' policy on external contributions that use AI before getting too far through reviewing this. I'm not sure what the state of play was when I left, and it could well have changed since then.
Anyway, this first PR introduces the context as a new ADR - ADR-014, and makes some initial refactors (adding some tests, moving the SQL queries out of the graphql directory and wrapping them with Query classes).
I'll raise the other PRs as a sequence on top of this one.