campaigns: Cache external changeset diffstats
Created by: LawnGnome
Fixes #11075.
The general approach here is to cache the changeset diffstat if the base or head OID has changed when we sync the changeset, then use that cached changeset when a GraphQL query is made that requires a campaign or changeset diffstat.
There is one observable behaviour change: closed changesets previously returned zero diffstats, but now return their actual diffstat. I think this is OK, and it simplifies the implementation, but we could force the syncer to cache zeroes for closed changesets to return to the old behaviour. I've kept the relevant test change in a separate commit for now to make it easier to change this if necessary.
Since campaign diffstats require knowledge of which changesets are accessible to the user, I've elected to use the simple approach for now of continuing to use the GraphQL connection. (See the discussion below between @mrnugget and I for more detail.) This is technically suboptimal, as we're pulling in sync data that we don't need to, but I think that's likely OK pending further work on being able to resolve permissions at the store level. I'm also happy to attempt @mrnugget's suggested optimisation in a future PR.
Merge request reports
Merged by
(Jul 26, 2025 10:16am UTC)
Activity
Created by: codecov[bot]
Codecov Report
Merging #11279 into master will increase coverage by
0.35%. The diff coverage is40.41%.@@ Coverage Diff @@ ## master #11279 +/- ## ========================================== + Coverage 47.13% 47.49% +0.35% ========================================== Files 1402 1401 -1 Lines 79484 79583 +99 Branches 6775 6665 -110 ========================================== + Hits 37467 37798 +331 + Misses 38443 38217 -226 + Partials 3574 3568 -6Flag Coverage Δ #go 51.66% <40.00%> (-0.07%)
#storybook 10.12% <ø> (?)#typescript 36.42% <100.00%> (+1.45%)
#unit 47.09% <40.41%> (-0.05%)Impacted Files Coverage Δ cmd/frontend/graphqlbackend/campaigns.go 0.00% <ø> (ø)web/src/enterprise/campaigns/detail/backend.ts 10.71% <ø> (ø)enterprise/internal/campaigns/state.go 61.16% <12.00%> (-17.18%)internal/campaigns/types.go 11.49% <42.85%> (+0.92%)...terprise/internal/campaigns/resolvers/campaigns.go 63.13% <75.00%> (-6.07%)...erprise/internal/campaigns/resolvers/changesets.go 64.77% <75.00%> (-0.51%)enterprise/internal/campaigns/store.go 85.52% <80.00%> (-0.11%)enterprise/internal/campaigns/syncer.go 78.62% <85.71%> (+0.19%)enterprise/internal/campaigns/webhooks.go 37.10% <100.00%> (ø)enterprise/internal/campaigns/workers.go 47.28% <100.00%> (ø)... and 31 more Created by: LawnGnome
One "Hmm"-thought I had that I wanted to mention (and I'm curious to hear your thoughts): I'm not 100% sure about the name
ChangesetSyncStateandsync_state. I think I get what it's used for, something like an internal cache, but at the same time I'm wondering: "why isn't ExternalState and ExternalReviewState etc. insync_data?" I'm curious to hear how you think about that column and what should go in and what not.To me, this column is for state that needs to be persisted between syncs that is internal to
enterprise/internal/campaigns: fields that we're not surfacing via GraphQL, but rather fields that we need to compute the fields that are surfaced.I'm open to better naming, of course.
Technically, we could move the external fields into this JSON blob, but I think having that separation between "here are things we know we need, and we're calling them out in the schema as explicit fields" and "here is a blob of state that is hidden behind the curtain" helps with understanding the data model.
