Skip to content

Commit b8cd6fd

Browse files
authored
refactor(change): persist typed change details from the change provider (#195)
## Summary ### Why? The change provider already produces rich per-URI facts (author, changed files, line counts), but its value types lived in the extension layer and the data was thrown away — validate fetched ChangeInfo only to log a file count, and ChangeRecord stored an opaque Metadata JSON string that was never written. Nothing downstream could read typed change facts. ### What? - Move the change value types into entities: entity.User, entity.ChangedFile (now with LinesModified), entity.ChangeDetails (the facts), and entity.ChangeInfo (URI -> Details), with aggregation helpers. The changeprovider extension and GitHub impl now produce these. - Replace ChangeRecord.Metadata (opaque string) with typed Details (ChangeDetails); the change table's metadata JSON column becomes details. - Add ChangeStore.UpdateDetails — a version-guarded conditional write, following the optimistic-locking contract (arithmetic in the controller). - validate now persists each fetched ChangeInfo onto the request's change records (per-URI, idempotent; ErrVersionMismatch is a benign no-op). This is the producer half: typed details now exist and are persisted. The score controller consumes them in a follow-up. ## Test Plan - ✅ `make build`, `make test`, `make lint`, `make check-mocks/gazelle/tidy` - ✅ `make integration-test` (storage contract suite round-trips Details and covers UpdateDetails create/update/version-mismatch) ## Issues ## Stack 1. @ #195 1. #196 1. #197 1. #193 1. #202
1 parent b5b24db commit b8cd6fd

19 files changed

Lines changed: 294 additions & 234 deletions

File tree

submitqueue/entity/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ go_library(
77
"batch_dependent.go",
88
"build.go",
99
"cancel_request.go",
10+
"change_provider.go",
1011
"change_record.go",
1112
"land_request.go",
1213
"queue_config.go",
Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
// Copyright (c) 2025 Uber Technologies, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package entity
16+
17+
// Author represents the author of a change.
18+
type Author struct {
19+
// Name is the display name of the author.
20+
Name string `json:"name"`
21+
// Email is the email address of the author.
22+
Email string `json:"email"`
23+
}
24+
25+
// ChangedFile represents a single file modification in a change.
26+
type ChangedFile struct {
27+
// Path is the file path relative to the repository root.
28+
Path string `json:"path"`
29+
// LinesAdded is the number of lines added in this file.
30+
LinesAdded int `json:"lines_added"`
31+
// LinesDeleted is the number of lines deleted in this file.
32+
LinesDeleted int `json:"lines_deleted"`
33+
// LinesModified is the number of lines modified in this file. Some providers
34+
// (e.g. GitHub) report only additions and deletions and leave this zero.
35+
LinesModified int `json:"lines_modified"`
36+
}
37+
38+
// TotalLines returns the total number of lines touched in this file.
39+
func (f ChangedFile) TotalLines() int {
40+
return f.LinesAdded + f.LinesDeleted + f.LinesModified
41+
}
42+
43+
// ChangeDetails holds the provider-supplied facts about a single change (author,
44+
// modified files, line counts). It carries no identity — the owning URI lives on
45+
// ChangeInfo (provider correlation) and ChangeRecord (persisted claim).
46+
type ChangeDetails struct {
47+
// Author is the author of the change.
48+
Author Author `json:"author"`
49+
// ChangedFiles is the list of files modified in this change. Order is unspecified.
50+
ChangedFiles []ChangedFile `json:"changed_files,omitempty"`
51+
}
52+
53+
// TotalLinesChanged returns the total number of lines touched across all files in the change.
54+
func (d ChangeDetails) TotalLinesChanged() int {
55+
total := 0
56+
for _, f := range d.ChangedFiles {
57+
total += f.TotalLines()
58+
}
59+
return total
60+
}
61+
62+
// FileCount returns the number of files touched in the change.
63+
func (d ChangeDetails) FileCount() int {
64+
return len(d.ChangedFiles)
65+
}
66+
67+
// ChangeInfo maps a change URI to its details. It is the change provider's return
68+
// type: for a Change with multiple URIs (e.g. a stacked PR set), the provider
69+
// returns one ChangeInfo per URI so callers can correlate results to inputs by URI.
70+
type ChangeInfo struct {
71+
// URI is the full change URI for correlation with the input request
72+
// (e.g., "github://uber/repo/pull/98/c3a4d5e6f7890123456789abcdef0123456789ab").
73+
URI string `json:"uri"`
74+
// Details is the provider-supplied facts for this URI.
75+
Details ChangeDetails `json:"details"`
76+
}

submitqueue/entity/change_record.go

Lines changed: 14 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,9 @@
1515
package entity
1616

1717
// ChangeRecord represents a single URI's claim by a request, persisted in the change store.
18-
// The (Queue, URI, RequestID) triple is the identity and is immutable; Metadata may be
19-
// updated over time as additional information about the change (e.g., PR title, author,
20-
// mergeability) becomes available.
18+
// The whole record is immutable: the (Queue, URI, RequestID) triple is its identity and the
19+
// Details (author, changed files, line counts) are captured once at claim time from the
20+
// change provider. There is no update path.
2121
type ChangeRecord struct {
2222
// URI identifies the change (RFC 3986). Same scheme/format as entity.Change.URIs.
2323
// Example: "github://uber/submitqueue/pull/123/c3a4d5e6f7890123456789abcdef0123456789ab".
@@ -29,29 +29,28 @@ type ChangeRecord struct {
2929
// RequestID participates in the change-store primary key so that concurrent claims
3030
// by different requests on the same URI coexist as distinct rows. Same-request
3131
// retries collide on the PK and are absorbed idempotently; different-request
32-
// collisions surface as additional rows that callers detect via FindOverlapping.
32+
// collisions surface as additional rows that callers detect via GetByURI.
3333
RequestID string `json:"request_id"`
3434

3535
// Queue is the queue the owning request belongs to. It is the leading column of
3636
// the change-store primary key, so queue-scoped duplicate checks become PK-prefix
3737
// scans and the table is shardable by queue.
3838
Queue string `json:"queue"`
3939

40-
// Metadata is a JSON-encoded blob of provider-specific information about the change
41-
// (e.g., PR title, author, mergeable state). Stored as `'{}'` when no metadata has
42-
// been populated yet; updated by downstream enrichment.
43-
Metadata string `json:"metadata,omitempty"`
40+
// Details holds the provider-supplied facts about the change (author, changed
41+
// files, line counts). It is captured at claim time (the validate controller, after
42+
// fetching from the change provider) and written once with the record — records are
43+
// immutable, so Details is never updated after Create.
44+
Details ChangeDetails `json:"details"`
4445

45-
// CreatedAt is the Unix milliseconds timestamp when this record was first created.
46+
// CreatedAt is the Unix milliseconds timestamp when this record was created.
4647
CreatedAt int64 `json:"created_at"`
4748

48-
// UpdatedAt is the Unix milliseconds timestamp when this record's Metadata was last updated.
49-
// Equal to CreatedAt when the record has never been updated.
49+
// UpdatedAt is the Unix milliseconds timestamp when this record was created. Records
50+
// are immutable, so it always equals CreatedAt; retained for schema symmetry.
5051
UpdatedAt int64 `json:"updated_at"`
5152

52-
// Version is the optimistic-locking counter for mutable fields (Metadata).
53-
// Starts at 1 on Create and is incremented by callers on every update.
54-
// Mirrors the request-store convention: callers compute newVersion = oldVersion + 1
55-
// and pass both to the update method; the store performs a pure conditional write.
53+
// Version is the record version. Records are immutable, so it is always 1; retained
54+
// for schema symmetry with the other stores.
5655
Version int32 `json:"version"`
5756
}

submitqueue/extension/changeprovider/change_provider.go

Lines changed: 5 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -22,44 +22,17 @@ import (
2222
"github.com/uber/submitqueue/submitqueue/entity"
2323
)
2424

25-
// User represents the author of a change.
26-
type User struct {
27-
// Name is the display name of the user.
28-
Name string
29-
// Email is the email address of the user.
30-
Email string
31-
}
32-
33-
// ChangedFile represents a single file modification in a change.
34-
type ChangedFile struct {
35-
// Path is the file path relative to the repository root.
36-
Path string
37-
// Patch is the diff patch content for this file.
38-
Patch string
39-
// LinesAdded is the number of lines added in this file.
40-
LinesAdded int
41-
// LinesDeleted is the number of lines deleted in this file.
42-
LinesDeleted int
43-
}
44-
45-
// ChangeInfo contains metadata and file changes for a code change.
46-
type ChangeInfo struct {
47-
// URI is the full change URI for correlation with the input request
48-
// (e.g., "github://uber/repo/pull/98/c3a4d5e6f7890123456789abcdef0123456789ab" or "phab://D123/xyz789").
49-
URI string
50-
// User is the author of the change.
51-
User User
52-
// ChangedFiles is the list of files modified in this change. Order is unspecified.
53-
ChangedFiles []ChangedFile
54-
}
55-
5625
// ChangeProvider fetches change metadata from external systems
5726
// Each implementation is configured for a specific provider (GitHub, GitLab, Phabricator).
27+
//
28+
// The change value types it produces — entity.ChangeInfo, entity.ChangeDetails,
29+
// entity.Author, entity.ChangedFile — live in the entity package so the same typed
30+
// facts can be persisted (entity.ChangeRecord) and scored without re-declaration.
5831
type ChangeProvider interface {
5932
// Get retrieves change information for the provided Change.
6033
// For a Change with multiple URIs (e.g., stacked PRs), returns one ChangeInfo per URI.
6134
// Returns a slice of ChangeInfo, one for each change in the stack.
62-
Get(ctx context.Context, change entity.Change) ([]ChangeInfo, error)
35+
Get(ctx context.Context, change entity.Change) ([]entity.ChangeInfo, error)
6336
}
6437

6538
// Config carries the per-queue identity handed to a Factory. The system knows

submitqueue/extension/changeprovider/github/convert.go

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,32 +1,33 @@
11
package github
22

33
import (
4+
"github.com/uber/submitqueue/submitqueue/entity"
45
entitygithub "github.com/uber/submitqueue/submitqueue/entity/github"
5-
"github.com/uber/submitqueue/submitqueue/extension/changeprovider"
66
)
77

8-
// convertToChangeInfo converts GitHub PR data to ChangeInfo.
9-
func convertToChangeInfo(parsed entitygithub.ChangeID, prData *pullRequestData) changeprovider.ChangeInfo {
10-
changedFiles := convertFiles(prData.Files.Nodes)
11-
12-
return changeprovider.ChangeInfo{
8+
// convertToChangeInfo converts GitHub PR data to an entity.ChangeInfo.
9+
func convertToChangeInfo(parsed entitygithub.ChangeID, prData *pullRequestData) entity.ChangeInfo {
10+
return entity.ChangeInfo{
1311
URI: parsed.String(),
14-
User: changeprovider.User{
15-
Name: prData.Author.Name,
16-
Email: prData.Author.Email,
12+
Details: entity.ChangeDetails{
13+
Author: entity.Author{
14+
Name: prData.Author.Name,
15+
Email: prData.Author.Email,
16+
},
17+
ChangedFiles: convertFiles(prData.Files.Nodes),
1718
},
18-
ChangedFiles: changedFiles,
1919
}
2020
}
2121

22-
// convertFiles converts GitHub file nodes to ChangedFile structs.
23-
func convertFiles(nodes []fileNode) []changeprovider.ChangedFile {
24-
changedFiles := make([]changeprovider.ChangedFile, 0, len(nodes))
22+
// convertFiles converts GitHub file nodes to entity.ChangedFile structs.
23+
// GitHub's API reports only additions and deletions per file, so LinesModified
24+
// is left zero here.
25+
func convertFiles(nodes []fileNode) []entity.ChangedFile {
26+
changedFiles := make([]entity.ChangedFile, 0, len(nodes))
2527

2628
for _, file := range nodes {
27-
changedFiles = append(changedFiles, changeprovider.ChangedFile{
29+
changedFiles = append(changedFiles, entity.ChangedFile{
2830
Path: file.Path,
29-
Patch: file.Patch,
3031
LinesAdded: file.Additions,
3132
LinesDeleted: file.Deletions,
3233
})

submitqueue/extension/changeprovider/github/graphql.go

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@ query($owner: String!, $repo: String!, $prNumber: Int!, $filesCursor: String) {
3434
additions
3535
deletions
3636
changeType
37-
patch
3837
}
3938
}
4039
}
@@ -98,7 +97,6 @@ type fileNode struct {
9897
Additions int `json:"additions"`
9998
Deletions int `json:"deletions"`
10099
ChangeType string `json:"changeType"`
101-
Patch string `json:"patch"`
102100
}
103101

104102
// buildGraphQLRequest builds a GraphQL request for fetching pull request data.

submitqueue/extension/changeprovider/github/graphql_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ func TestParseGraphQLResponse(t *testing.T) {
7777
"files": {
7878
"totalCount": 1,
7979
"pageInfo": {"endCursor": "cur1", "hasNextPage": false},
80-
"nodes": [{"path": "main.go", "additions": 10, "deletions": 2, "changeType": "MODIFIED", "patch": "diff content"}]
80+
"nodes": [{"path": "main.go", "additions": 10, "deletions": 2, "changeType": "MODIFIED"}]
8181
}
8282
}
8383
}
@@ -90,7 +90,7 @@ func TestParseGraphQLResponse(t *testing.T) {
9090
Files: filesData{
9191
TotalCount: 1,
9292
PageInfo: pageInfo{EndCursor: "cur1", HasNextPage: false},
93-
Nodes: []fileNode{{Path: "main.go", Additions: 10, Deletions: 2, ChangeType: "MODIFIED", Patch: "diff content"}},
93+
Nodes: []fileNode{{Path: "main.go", Additions: 10, Deletions: 2, ChangeType: "MODIFIED"}},
9494
},
9595
},
9696
},

submitqueue/extension/changeprovider/github/provider.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ func NewProvider(params Params) changeprovider.ChangeProvider {
4444

4545
// Get retrieves change information from GitHub for the provided Change.
4646
// Returns one ChangeInfo per URI (one per PR in stacked changes).
47-
func (p *provider) Get(ctx context.Context, change entity.Change) (_ []changeprovider.ChangeInfo, retErr error) {
47+
func (p *provider) Get(ctx context.Context, change entity.Change) (_ []entity.ChangeInfo, retErr error) {
4848
op := coremetrics.Begin(p.metricsScope, "get")
4949
defer func() { op.Complete(retErr) }()
5050

@@ -85,8 +85,8 @@ func (p *provider) Get(ctx context.Context, change entity.Change) (_ []changepro
8585
func (p *provider) fetchAllPRs(
8686
ctx context.Context,
8787
changeIDs []entitygithub.ChangeID,
88-
) ([]changeprovider.ChangeInfo, error) {
89-
changeInfos := make([]changeprovider.ChangeInfo, 0, len(changeIDs))
88+
) ([]entity.ChangeInfo, error) {
89+
changeInfos := make([]entity.ChangeInfo, 0, len(changeIDs))
9090

9191
for _, cid := range changeIDs {
9292
prData, err := p.fetchPullRequest(ctx, cid)
@@ -109,7 +109,7 @@ func (p *provider) fetchAllPRs(
109109
"org", cid.Org,
110110
"repo", cid.Repo,
111111
"pr", cid.PRNumber,
112-
"files_count", len(changeInfo.ChangedFiles),
112+
"files_count", len(changeInfo.Details.ChangedFiles),
113113
"head_sha", prData.HeadRefOid,
114114
)
115115
}

submitqueue/extension/changeprovider/github/provider_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ func TestProvider_Get(t *testing.T) {
115115
require.NoError(t, err)
116116
require.Len(t, infos, 1)
117117
assert.Equal(t, tt.uris[0], infos[0].URI)
118-
assert.Len(t, infos[0].ChangedFiles, 2)
118+
assert.Len(t, infos[0].Details.ChangedFiles, 2)
119119
})
120120
}
121121
}
@@ -154,7 +154,7 @@ func TestProvider_Get_Pagination(t *testing.T) {
154154
require.NoError(t, err)
155155
assert.Equal(t, 2, callCount)
156156
require.Len(t, infos, 1)
157-
assert.Len(t, infos[0].ChangedFiles, 2)
157+
assert.Len(t, infos[0].Details.ChangedFiles, 2)
158158
}
159159

160160
func TestProvider_Get_MultiplePRs(t *testing.T) {

submitqueue/extension/changeprovider/mock/change_provider_mock.go

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)