fix(projects): bound project query connections to avoid complexity limit - #284
Conversation
✅ knip — no dead codeNo unused files, exports, types, or dependencies detected. |
There was a problem hiding this comment.
Thanks @jackhutson — this is excellent work, and the investigation is the part I want to dwell on.
The insight carrying the whole fix is that Linear's estimator prices page-size math, not rows. That's precisely why #276 reproduced on a one-project workspace and why nobody could pin it on data volume. I re-derived your numbers and they're exactly consistent: (2 × 50 × 1.3 + 9.5) × 100 = 13,950, (2 × 25 × 1.3 + 9.5) × 100 = 7,450, and 10,000/74.5 = 134 matches your observed --limit ceiling — as does 4 × 50 × 1.3 ≈ 260 against your pass-at-35 / fail-at-40 boundary. Numbers with a stated method that reproduce under someone else's arithmetic are rare in a bug report; thank you for that.
You also didn't stop at the reported symptom. #283 pointed at the mutation payload, you measured it, found the real culprit was the full-detail pre-read in --label-mode add|remove, and corrected the issue rather than quietly fixing something adjacent. The nil-UUID projectUpdate probe — confirming the payload clears complexity validation by failing afterwards on "Entity not found", so nothing mutates — is a genuinely clever way to test a mutation you don't want to run.
On layering: I checked getProjectLabelIds against the invariants and services is the right home, so please don't let anyone move it. It takes a UUID and returns UUIDs — there is no human identifier to resolve, which is what the resolver layer is defined for — and putting it in resolvers would additionally need an ARCHITECTURAL EXCEPTION docstring it doesn't warrant. Your PR body undersells this as "per the layer rules"; it's the correct call for a specific reason.
One ask before merge: a getFragment-based assertion locking the first: 25 bounds (inline). Right now the fix for two P0 bugs is the only part of the diff without a test, and the helper is already sitting in that file. Three other notes inline — the pageInfo truncation signal, the first: 250 ceiling, and the question of whether issues list still carries this defect at its default limit. That last one I've split out as #286 rather than leave it in your PR description, so it isn't scope creep on you here.
On my own earlier instinct to ask for a --limit clamp: I talked myself out of it. parseLimit is shared across ten command families and the safe ceiling is a property of the fragment, not the domain — and silently clamping an explicit -l 200 would misreport the page size to a JSON consumer, which is worse than an error. The better follow-up is that src/client/graphql-client.ts:22 types GraphQL errors as { message: string } and never reads extensions, so the userPresentableMessage you mined the complexity figures from is thrown away before the user ever sees it. That's a global fix, and firmly out of scope here.
The truncation trade-offs in your "Notes for reviewers" are the right ones, and flagging the user-visible --issues-first default change plainly is exactly what I want to see. Approving.
… in tests Review follow-ups for linearis-oss#284: - assert in tests that every bounded fragment connection carries a literal first: argument, so a future tidy-up that unbounds them fails CI instead of reintroducing linearis-oss#276 - select pageInfo.hasNextPage on the five bounded connections in the detail fragment so truncated pages are detectable in read and mutation responses; measured live, pageInfo prices at ~23 complexity per parent row, so the signal lives only where the parent count is one — selecting it in ProjectListFields put the default list back over budget (12120) and is deliberately omitted there - getProjectLabelIds now selects hasNextPage on its first: 250 read (Linear's per-connection maximum) and throws on truncation instead of letting a partial label set be written back as complete via the full-replacement labelIds input Refs linearis-oss#284
|
Thanks for the thorough review — all three asks are in as of 141ab05, with one measured deviation and one answered question: Bounds test (the merge blocker):
Your And thanks for the layering confirmation on |
iamfj
left a comment
iamfj
left a comment
There was a problem hiding this comment.
Re-approving on 141ab05 so the approval sits on the current head.
Checked the full checklist locally: tsc clean, check:ci clean, 903 tests passing, knip clean. I also mutation-tested the new bounds assertions rather than trusting them, and they fail the way they should when a bound or a pageInfo goes missing. The measured pushback on my pageInfo suggestion was the right response to a suggestion I hadn't costed.
Two small things left, both in threads and neither blocking: projects read still can't signal truncation on projectMilestones and issues, and the Project-specific caveat on the cost model didn't make it into the comment at project-service.ts:100. Merge whenever you're happy.
Separately, I'd be glad if you picked up #286. I've put your measurements in there and rescoped it, but you're the one who understands the estimator's behaviour now, and it'd be a shame for that to sit in a closed PR thread. No obligation.
Linear's complexity estimator charges an unbounded connection at its default page size (50) per parent row. ProjectListFields selected unbounded teams and labels connections, pricing the default `projects list` (first: 100) at ~13950 against a budget of 10000 — so default-limit lists failed with "Query too complex" even on near-empty workspaces, since the estimator prices page size, not data. - bound teams/labels (25) in ProjectListFields, members/initiatives (25) in ProjectDetailFields, and projectMilestones (25) in the default-connections fragment; the same list query now prices ~7450 - lower the projects read issues default from 50 to 25: each issue in the response costs ~260 (CompleteIssueFields carries four unbounded connections), so the old default read exceeded the budget on real workspaces - replace the update --label-mode add|remove pre-read (full project detail including milestones and issues) with a lean label-IDs-only query Closes linearis-oss#276 Closes linearis-oss#283
… in tests Review follow-ups for linearis-oss#284: - assert in tests that every bounded fragment connection carries a literal first: argument, so a future tidy-up that unbounds them fails CI instead of reintroducing linearis-oss#276 - select pageInfo.hasNextPage on the five bounded connections in the detail fragment so truncated pages are detectable in read and mutation responses; measured live, pageInfo prices at ~23 complexity per parent row, so the signal lives only where the parent count is one — selecting it in ProjectListFields put the default list back over budget (12120) and is deliberately omitted there - getProjectLabelIds now selects hasNextPage on its first: 250 read (Linear's per-connection maximum) and throws on truncation instead of letting a partial label set be written back as complete via the full-replacement labelIds input Refs linearis-oss#284
141ab05 to
a3145c1
Compare
|
Heads up, I rebased this onto current You were 34 commits behind, including the graphql v17 major and the new Your changes came through untouched. One thing did need fixing: biome on current The checklist passes on the new head: generate, tsc, check:ci, 935 tests, knip. If you have local work in flight, We're holding the merge to |
|
Thanks for handling the rebase — synced clean, nothing lost, and the biome fold-in looks right. Since Actions is down, here's the verification your CI can't run right now, from a clean |
2 participants
What does this PR do?
Fixes the "Query too complex" failures on the
projectscommands by bounding the nested connections Linear's complexity estimator charges at default page size (50) per parent row — the query cost was determined by page-size math, not actual data, which is why even a fresh one-project workspace failed (#276).Closes #276
Closes #283
Type of change
Changes
ProjectListFields:teams(first: 25),labels(first: 25). Measured against the live API: the defaultprojects list(first: 100) priced at 13,950 vs the 10,000 budget; with these bounds it prices at ~7,450 (works up to--limit 134). Each connection node with a 3-field selection costs 1.3; unbounded is charged identically tofirst: 50.ProjectDetailFields/...WithDefaultConnections:members,initiatives,projectMilestonesbounded at 25 for the same reason (these ride along in every project mutation payload).projects readissues default 50 → 25: each returned issue prices at ~260 (CompleteIssueFieldscarries four connections charged at 50 each), so the old default read exceeded the budget on real workspaces (empirically passes at--issues-first 35, fails at 40). I deliberately did not touchCompleteIssueFieldsitself to keep this fix scoped toprojects— bounding its inner connections would roughly halve per-issue cost across the whole issues domain if you'd prefer that direction instead; happy to follow up.update --label-mode add|remove: the pre-read fetched the full project detail (milestones + default issues) just to read current label IDs — that pre-read, not the mutation payload, is what actually made label updates fail (projects updatehits "Query too complex" with no possible workaround flag (mutation payload selects five unbounded connections) #283 originally blamed the payload; correction noted there). Replaced with a leanGetProjectLabelIdsquery per the resolver/service layer rules.Checklist
npm run check:cipasses (lint + format)npx tsc --noEmitpasses (type check)npm testpasses (unit tests)Testing
knipclean; newgetProjectLabelIdsservice tests (happy path + not-found), label-mode command tests updated to the lean call.projects list(default limit) andprojects read <p>(default milestones/issues) both previously returned{"error":"Query too complex"}and now succeed;projects read --issues-first 35passes /40fails, confirming the ~260/issue pricing.projectUpdateprobe (fails with "Entity not found" after passing complexity validation, so nothing mutates).Notes for reviewers
--issues-firstdefault change (50 → 25) is user-visible;--issues-first <n>still overrides.extensions.userPresentableMessage(e.g. "Complexity: 13119.99… Maximum allowed complexity: 10000"), reproducible with any workspace since the estimator ignores actual row counts.