From f4a59290347fb42afdee4d47be9f308e40704f91 Mon Sep 17 00:00:00 2001 From: Albert Wu Date: Wed, 27 May 2026 15:16:27 -0700 Subject: [PATCH] fix(github): enforce /pull/ segment and full SHA in change-ID parser ParseChangeID silently misparsed the canonical /pull/-form URIs used by the proto example and the bulk of orchestrator/gateway/integration fixtures (e.g. github://uber/repo/pull/123/), producing Org="uber/repo" Repo="pull" instead of erroring. Align the parser with the documented format and reject the no-/pull/ form explicitly. Also require the head commit SHA to be 40 lowercase hex characters (GitHub's canonical headRefOid form). The downstream staleness check in mergechecker/changeprovider already compares with strict string equality against the full SHA, so abbreviated or non-hex SHAs would fail later with a confusing "head SHA changed" message. Fail fast at the parser instead. Fixtures throughout entity/, gateway/, orchestrator/, extension/, test/e2e, and test/integration updated to use the canonical form with realistic 40-char hex SHAs. Co-Authored-By: Claude Opus 4.7 (1M context) --- entity/change_record.go | 2 +- entity/github/change_id.go | 54 +++++++++-- entity/github/change_id_test.go | 92 +++++++++++++------ entity/land_request_test.go | 21 +++-- entity/request.go | 5 +- entity/request_test.go | 21 +++-- extension/changeprovider/change_provider.go | 2 +- .../changeprovider/github/provider_test.go | 45 +++++---- extension/mergechecker/github/checker_test.go | 35 ++++--- extension/mergechecker/multi_test.go | 8 +- extension/pusher/git/git_pusher_test.go | 2 +- gateway/controller/land_test.go | 14 +-- gateway/proto/gateway.proto | 9 +- gateway/protopb/gateway.pb.go | 9 +- orchestrator/controller/batch/batch_test.go | 4 +- orchestrator/controller/merge/merge_test.go | 14 +-- orchestrator/controller/score/score_test.go | 6 +- orchestrator/controller/start/start_test.go | 20 ++-- .../controller/validate/validate_test.go | 18 ++-- test/e2e/suite_test.go | 2 +- .../changestore/mysql/changestore_test.go | 18 ++-- test/integration/extension/storage/suite.go | 10 +- test/integration/gateway/suite_test.go | 2 +- 23 files changed, 266 insertions(+), 147 deletions(-) diff --git a/entity/change_record.go b/entity/change_record.go index 34997a78..8ba0ff7d 100644 --- a/entity/change_record.go +++ b/entity/change_record.go @@ -20,7 +20,7 @@ package entity // mergeability) becomes available. type ChangeRecord struct { // URI identifies the change (RFC 3986). Same scheme/format as entity.Change.URIs. - // Example: "github://uber/submitqueue/pull/123/abc123def". + // Example: "github://uber/submitqueue/pull/123/c3a4d5e6f7890123456789abcdef0123456789ab". URI string `json:"uri"` // RequestID is the owning land request that claimed this URI. diff --git a/entity/github/change_id.go b/entity/github/change_id.go index 85d14d22..c5556ec5 100644 --- a/entity/github/change_id.go +++ b/entity/github/change_id.go @@ -21,12 +21,25 @@ import ( ) // changeIDFormat is the expected format for change IDs, included in error messages. -const changeIDFormat = "{scheme}://{owner}/{repo}/{pr_number}/{head_commit_sha}" +const changeIDFormat = "{scheme}://{owner}/{repo}/pull/{pr_number}/{head_commit_sha}" + +// pullSegment is the literal segment separating the repo path from the pull +// request number. Mirrors the path layout of an actual GitHub PR URL +// (e.g. https://github.com/uber/repo/pull/123) so URIs can be constructed by +// trivial substitution rather than reshaping. +const pullSegment = "pull" + +// shaLength is the length of a GitHub commit SHA. GitHub's GraphQL API returns +// full 40-char lowercase hex SHA-1 hashes via headRefOid, and the staleness +// check compares the URI's SHA against that value with strict string equality, +// so anything shorter or otherwise non-canonical will always fail later. +// Validate up-front to fail fast at the gateway with a clearer error. +const shaLength = 40 // ChangeID represents a parsed GitHub-family change identifier. // Covers GitHub.com, GitHub Enterprise (GHE), and GitHub Enterprise Server (GHES) // since they share the same pull request model. -// Format: {scheme}://{owner}/{repo}/{pr_number}/{head_commit_sha} +// Format: {scheme}://{owner}/{repo}/pull/{pr_number}/{head_commit_sha} type ChangeID struct { // Scheme captures the source variant (e.g., "github", "ghe", "ghes"). Scheme string @@ -41,9 +54,10 @@ type ChangeID struct { } // ParseChangeID parses a raw change ID string into a ChangeID. -// Expected format: {scheme}://{owner}/{repo}/{pr_number}/{head_commit_sha} +// Expected format: {scheme}://{owner}/{repo}/pull/{pr_number}/{head_commit_sha} // The parser works from the end: SHA (last), PR number (second-to-last), -// and everything before is the repo path (split into owner and repo). +// the literal "pull" segment (third-to-last), and everything before is the +// repo path (split into owner and repo). func ParseChangeID(raw string) (ChangeID, error) { // Split on "://" to get scheme and path schemeSplit := strings.SplitN(raw, "://", 2) @@ -60,18 +74,26 @@ func ParseChangeID(raw string) (ChangeID, error) { // Split the path into segments and parse from the end. segments := strings.Split(path, "/") - // Need at least 4 segments: {owner}/{repo}/{pr_number}/{sha} - if len(segments) < 4 { - return ChangeID{}, fmt.Errorf("invalid change ID %q: need at least owner/repo/pr/sha, got %d segments (expected format: %s)", raw, len(segments), changeIDFormat) + // Need at least 5 segments: {owner}/{repo}/pull/{pr_number}/{sha} + if len(segments) < 5 { + return ChangeID{}, fmt.Errorf("invalid change ID %q: need at least owner/repo/pull/pr/sha, got %d segments (expected format: %s)", raw, len(segments), changeIDFormat) } sha := segments[len(segments)-1] prStr := segments[len(segments)-2] - repoSegments := segments[:len(segments)-2] + pullLiteral := segments[len(segments)-3] + repoSegments := segments[:len(segments)-3] + + if pullLiteral != pullSegment { + return ChangeID{}, fmt.Errorf("invalid change ID %q: expected literal %q segment before PR number, got %q (expected format: %s)", raw, pullSegment, pullLiteral, changeIDFormat) + } if sha == "" { return ChangeID{}, fmt.Errorf("invalid change ID %q: empty head commit SHA (expected format: %s)", raw, changeIDFormat) } + if !isFullHexSHA(sha) { + return ChangeID{}, fmt.Errorf("invalid change ID %q: head commit SHA %q must be %d lowercase hex characters (expected format: %s)", raw, sha, shaLength, changeIDFormat) + } prNumber, err := strconv.Atoi(prStr) if err != nil { @@ -104,10 +126,24 @@ func ParseChangeID(raw string) (ChangeID, error) { // String returns the string representation of the change ID. func (c ChangeID) String() string { - return fmt.Sprintf("%s://%s/%s/%d/%s", c.Scheme, c.Org, c.Repo, c.PRNumber, c.HeadCommitSHA) + return fmt.Sprintf("%s://%s/%s/%s/%d/%s", c.Scheme, c.Org, c.Repo, pullSegment, c.PRNumber, c.HeadCommitSHA) } // OwnerRepo returns the "{org}/{repo}" string. func (c ChangeID) OwnerRepo() string { return fmt.Sprintf("%s/%s", c.Org, c.Repo) } + +// isFullHexSHA reports whether s is exactly shaLength lowercase hex characters. +func isFullHexSHA(s string) bool { + if len(s) != shaLength { + return false + } + for i := 0; i < len(s); i++ { + c := s[i] + if !(c >= '0' && c <= '9') && !(c >= 'a' && c <= 'f') { + return false + } + } + return true +} diff --git a/entity/github/change_id_test.go b/entity/github/change_id_test.go index 20ce386e..7f8c90ea 100644 --- a/entity/github/change_id_test.go +++ b/entity/github/change_id_test.go @@ -21,6 +21,15 @@ import ( "github.com/stretchr/testify/require" ) +// Sample 40-char lowercase hex SHAs used across the test cases. +const ( + sha1Full = "1111111111111111111111111111111111111111" + sha2Full = "2222222222222222222222222222222222222222" + shaAFull = "abcdef0123456789abcdef0123456789abcdef01" + shaBFull = "0123456789abcdef0123456789abcdef01234567" + shaCFull = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef" +) + func TestParseChangeID(t *testing.T) { tests := []struct { name string @@ -30,86 +39,116 @@ func TestParseChangeID(t *testing.T) { }{ { name: "valid github scheme", - raw: "github://uber/submitqueue/123/abc123def", + raw: "github://uber/submitqueue/pull/123/" + shaAFull, want: ChangeID{ Scheme: "github", Org: "uber", Repo: "submitqueue", PRNumber: 123, - HeadCommitSHA: "abc123def", + HeadCommitSHA: shaAFull, }, }, { name: "valid ghe scheme", - raw: "ghe://uber/monorepo/456/deadbeef", + raw: "ghe://uber/monorepo/pull/456/" + shaCFull, want: ChangeID{ Scheme: "ghe", Org: "uber", Repo: "monorepo", PRNumber: 456, - HeadCommitSHA: "deadbeef", + HeadCommitSHA: shaCFull, }, }, { name: "valid ghes scheme", - raw: "ghes://org/repo/1/sha1", + raw: "ghes://org/repo/pull/1/" + sha1Full, want: ChangeID{ Scheme: "ghes", Org: "org", Repo: "repo", PRNumber: 1, - HeadCommitSHA: "sha1", + HeadCommitSHA: sha1Full, }, }, { name: "nested org path", - raw: "github://uber/frontend/webapp/42/abc123", + raw: "github://uber/frontend/webapp/pull/42/" + shaAFull, want: ChangeID{ Scheme: "github", Org: "uber/frontend", Repo: "webapp", PRNumber: 42, - HeadCommitSHA: "abc123", + HeadCommitSHA: shaAFull, }, }, + { + name: "missing pull segment", + raw: "github://uber/submitqueue/123/" + shaAFull, + wantErr: true, + }, + { + name: "wrong literal segment (issues instead of pull)", + raw: "github://uber/submitqueue/issues/123/" + shaAFull, + wantErr: true, + }, { name: "missing separator", - raw: "github/uber/submitqueue/123/abc123", + raw: "github/uber/submitqueue/pull/123/" + shaAFull, wantErr: true, }, { name: "empty scheme", - raw: "://uber/submitqueue/123/abc123", + raw: "://uber/submitqueue/pull/123/" + shaAFull, wantErr: true, }, { name: "too few segments", - raw: "github://uber/123/abc123", + raw: "github://uber/pull/123/" + shaAFull, wantErr: true, }, { name: "only one segment", - raw: "github://abc123", + raw: "github://" + shaAFull, wantErr: true, }, { name: "empty owner", - raw: "github:///submitqueue/123/abc123", + raw: "github:///submitqueue/pull/123/" + shaAFull, wantErr: true, }, { name: "empty repo", - raw: "github://uber//123/abc123", + raw: "github://uber//pull/123/" + shaAFull, wantErr: true, }, { name: "non-numeric PR number", - raw: "github://uber/submitqueue/abc/abc123", + raw: "github://uber/submitqueue/pull/abc/" + shaAFull, wantErr: true, }, { name: "empty SHA", - raw: "github://uber/submitqueue/123/", + raw: "github://uber/submitqueue/pull/123/", + wantErr: true, + }, + { + name: "abbreviated SHA", + raw: "github://uber/submitqueue/pull/123/abc123def", + wantErr: true, + }, + { + name: "uppercase SHA", + raw: "github://uber/submitqueue/pull/123/ABCDEF0123456789ABCDEF0123456789ABCDEF01", + wantErr: true, + }, + { + name: "non-hex SHA", + raw: "github://uber/submitqueue/pull/123/zzzzzz0123456789abcdef0123456789abcdef01", + wantErr: true, + }, + { + name: "SHA too long", + raw: "github://uber/submitqueue/pull/123/" + shaAFull + "ab", wantErr: true, }, { @@ -145,9 +184,9 @@ func TestChangeID_String(t *testing.T) { Org: "uber", Repo: "submitqueue", PRNumber: 123, - HeadCommitSHA: "abc123", + HeadCommitSHA: shaAFull, }, - want: "github://uber/submitqueue/123/abc123", + want: "github://uber/submitqueue/pull/123/" + shaAFull, }, { name: "ghe", @@ -156,9 +195,9 @@ func TestChangeID_String(t *testing.T) { Org: "corp", Repo: "app", PRNumber: 99, - HeadCommitSHA: "deadbeef", + HeadCommitSHA: shaCFull, }, - want: "ghe://corp/app/99/deadbeef", + want: "ghe://corp/app/pull/99/" + shaCFull, }, { name: "ghes", @@ -167,9 +206,9 @@ func TestChangeID_String(t *testing.T) { Org: "org", Repo: "repo", PRNumber: 1, - HeadCommitSHA: "sha1", + HeadCommitSHA: sha1Full, }, - want: "ghes://org/repo/1/sha1", + want: "ghes://org/repo/pull/1/" + sha1Full, }, } @@ -186,16 +225,17 @@ func TestChangeID_OwnerRepo(t *testing.T) { Org: "uber", Repo: "submitqueue", PRNumber: 1, - HeadCommitSHA: "abc", + HeadCommitSHA: shaAFull, } assert.Equal(t, "uber/submitqueue", id.OwnerRepo()) } func TestParseChangeID_RoundTrip(t *testing.T) { originals := []string{ - "github://uber/submitqueue/123/abc123def456", - "ghe://corp/monorepo/99/deadbeef01234567", - "ghes://org/repo/1/a1b2c3", + "github://uber/submitqueue/pull/123/" + shaAFull, + "ghe://corp/monorepo/pull/99/" + shaCFull, + "ghes://org/repo/pull/1/" + sha2Full, + "github://uber/frontend/webapp/pull/42/" + shaBFull, } for _, raw := range originals { diff --git a/entity/land_request_test.go b/entity/land_request_test.go index 5c57e034..80769ab4 100644 --- a/entity/land_request_test.go +++ b/entity/land_request_test.go @@ -23,9 +23,12 @@ import ( func TestLandRequest_ToBytes(t *testing.T) { req := LandRequest{ - ID: "test-queue/123", - Queue: "test-queue", - Change: Change{URIs: []string{"github://uber/submitqueue/pull/456/abc123def", "github://uber/submitqueue/pull/789/def456abc"}}, + ID: "test-queue/123", + Queue: "test-queue", + Change: Change{URIs: []string{ + "github://uber/submitqueue/pull/456/abcdef0123456789abcdef0123456789abcdef01", + "github://uber/submitqueue/pull/789/0123456789abcdef0123456789abcdef01234567", + }}, LandStrategy: RequestLandStrategyRebase, } @@ -36,7 +39,7 @@ func TestLandRequest_ToBytes(t *testing.T) { // Verify JSON contains expected fields jsonStr := string(data) assert.Contains(t, jsonStr, "test-queue/123") - assert.Contains(t, jsonStr, "github://uber/submitqueue/pull/456/abc123def") + assert.Contains(t, jsonStr, "github://uber/submitqueue/pull/456/abcdef0123456789abcdef0123456789abcdef01") assert.Contains(t, jsonStr, "rebase") } @@ -90,9 +93,13 @@ func TestLandRequest_SerializationRoundTrip(t *testing.T) { { name: "github stacked diff", req: LandRequest{ - ID: "queue1/100", - Queue: "queue1", - Change: Change{URIs: []string{"github://uber/repo-a/pull/101/aaa111", "github://uber/repo-a/pull/102/bbb222", "github://uber/repo-a/pull/103/ccc333"}}, + ID: "queue1/100", + Queue: "queue1", + Change: Change{URIs: []string{ + "github://uber/repo-a/pull/101/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "github://uber/repo-a/pull/102/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + "github://uber/repo-a/pull/103/cccccccccccccccccccccccccccccccccccccccc", + }}, LandStrategy: RequestLandStrategySquashRebase, }, }, diff --git a/entity/request.go b/entity/request.go index 3736bec9..8b9e274d 100644 --- a/entity/request.go +++ b/entity/request.go @@ -60,8 +60,9 @@ type Change struct { // The scheme identifies the change provider, and the path contains provider-specific resource identifiers. // // GitHub is supported by default (though other providers can be added): - // Template: "github:////pull//" - // Example: "github://uber/submitqueue/pull/123/abc123def" + // Template: ":////pull//" + // Example: "github://uber/submitqueue/pull/123/c3a4d5e6f7890123456789abcdef0123456789ab" + // Schemes: "github", "ghe", "ghes". Head commit SHA must be full 40-char lowercase hex. // URIs []string `json:"uris"` } diff --git a/entity/request_test.go b/entity/request_test.go index f19b5887..32d3ba65 100644 --- a/entity/request_test.go +++ b/entity/request_test.go @@ -23,9 +23,12 @@ import ( func TestRequest_ToBytes(t *testing.T) { req := Request{ - ID: "test-queue/123", - Queue: "test-queue", - Change: Change{URIs: []string{"github://uber/submitqueue/pull/456/abc123def", "github://uber/submitqueue/pull/789/def456abc"}}, + ID: "test-queue/123", + Queue: "test-queue", + Change: Change{URIs: []string{ + "github://uber/submitqueue/pull/456/abcdef0123456789abcdef0123456789abcdef01", + "github://uber/submitqueue/pull/789/0123456789abcdef0123456789abcdef01234567", + }}, LandStrategy: RequestLandStrategyRebase, State: RequestStateStarted, Version: 1, @@ -38,7 +41,7 @@ func TestRequest_ToBytes(t *testing.T) { // Verify JSON contains expected fields jsonStr := string(data) assert.Contains(t, jsonStr, "test-queue/123") - assert.Contains(t, jsonStr, "github://uber/submitqueue/pull/456/abc123def") + assert.Contains(t, jsonStr, "github://uber/submitqueue/pull/456/abcdef0123456789abcdef0123456789abcdef01") assert.Contains(t, jsonStr, "rebase") assert.Contains(t, jsonStr, "started") } @@ -99,9 +102,13 @@ func TestRequest_SerializationRoundTrip(t *testing.T) { { name: "github stacked diff", req: Request{ - ID: "queue1/100", - Queue: "queue1", - Change: Change{URIs: []string{"github://uber/repo-a/pull/101/aaa111", "github://uber/repo-a/pull/102/bbb222", "github://uber/repo-a/pull/103/ccc333"}}, + ID: "queue1/100", + Queue: "queue1", + Change: Change{URIs: []string{ + "github://uber/repo-a/pull/101/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "github://uber/repo-a/pull/102/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + "github://uber/repo-a/pull/103/cccccccccccccccccccccccccccccccccccccccc", + }}, LandStrategy: RequestLandStrategySquashRebase, State: RequestStateLanded, Version: 5, diff --git a/extension/changeprovider/change_provider.go b/extension/changeprovider/change_provider.go index 379e03b3..078df0f5 100644 --- a/extension/changeprovider/change_provider.go +++ b/extension/changeprovider/change_provider.go @@ -43,7 +43,7 @@ type ChangedFile struct { // ChangeInfo contains metadata and file changes for a code change. type ChangeInfo struct { // URI is the full change URI for correlation with the input request - // (e.g., "github://uber/repo/98/abc123sha" or "phab://D123/xyz789"). + // (e.g., "github://uber/repo/pull/98/c3a4d5e6f7890123456789abcdef0123456789ab" or "phab://D123/xyz789"). URI string // User is the author of the change. User User diff --git a/extension/changeprovider/github/provider_test.go b/extension/changeprovider/github/provider_test.go index cb7e380c..cb255c7b 100644 --- a/extension/changeprovider/github/provider_test.go +++ b/extension/changeprovider/github/provider_test.go @@ -17,6 +17,15 @@ import ( "github.com/uber/submitqueue/extension/changeprovider" ) +// Sample 40-char lowercase hex SHAs used across the test cases. +const ( + shaA = "abcdef0123456789abcdef0123456789abcdef01" + shaB = "0123456789abcdef0123456789abcdef01234567" + shaXYZ = "1234567890abcdef1234567890abcdef12345678" + shaOld = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef" + shaNew = "feedfacefeedfacefeedfacefeedfacefeedface" +) + func newTestProvider(t *testing.T, serverURL string) changeprovider.ChangeProvider { t.Helper() client, err := httpclient.NewClient(serverURL) @@ -48,7 +57,7 @@ func TestProvider_Get(t *testing.T) { handler: func(w http.ResponseWriter, r *http.Request) { servePR(t, w, pullRequestData{ Number: 123, - HeadRefOid: "abc123", + HeadRefOid: shaA, Author: authorData{Name: "Test User", Email: "test@example.com"}, Files: filesData{ Nodes: []fileNode{ @@ -58,7 +67,7 @@ func TestProvider_Get(t *testing.T) { }, }) }, - uris: []string{"github://uber/submitqueue/123/abc123"}, + uris: []string{"github://uber/submitqueue/pull/123/" + shaA}, }, { name: "invalid URI returns error", @@ -68,8 +77,8 @@ func TestProvider_Get(t *testing.T) { { name: "inconsistent change set returns error", uris: []string{ - "github://uber/submitqueue/123/abc123", - "github://uber/different-repo/456/def456", + "github://uber/submitqueue/pull/123/" + shaA, + "github://uber/different-repo/pull/456/" + shaB, }, wantErr: true, }, @@ -78,11 +87,11 @@ func TestProvider_Get(t *testing.T) { handler: func(w http.ResponseWriter, r *http.Request) { servePR(t, w, pullRequestData{ Number: 123, - HeadRefOid: "newsha", + HeadRefOid: shaNew, Files: filesData{Nodes: []fileNode{{Path: "main.go"}}}, }) }, - uris: []string{"github://uber/submitqueue/123/oldsha"}, + uris: []string{"github://uber/submitqueue/pull/123/" + shaOld}, wantErr: true, }, } @@ -115,7 +124,7 @@ func TestProvider_Get_Pagination(t *testing.T) { pages := []pullRequestData{ { Number: 456, - HeadRefOid: "xyz789", + HeadRefOid: shaXYZ, Files: filesData{ PageInfo: pageInfo{EndCursor: "cursor1", HasNextPage: true}, Nodes: []fileNode{{Path: "file1.go"}}, @@ -123,7 +132,7 @@ func TestProvider_Get_Pagination(t *testing.T) { }, { Number: 456, - HeadRefOid: "xyz789", + HeadRefOid: shaXYZ, Files: filesData{ PageInfo: pageInfo{HasNextPage: false}, Nodes: []fileNode{{Path: "file2.go"}}, @@ -139,7 +148,7 @@ func TestProvider_Get_Pagination(t *testing.T) { p := newTestProvider(t, server.URL) infos, err := p.Get(context.Background(), entity.Change{ - URIs: []string{"github://uber/submitqueue/456/xyz789"}, + URIs: []string{"github://uber/submitqueue/pull/456/" + shaXYZ}, }) require.NoError(t, err) @@ -150,8 +159,8 @@ func TestProvider_Get_Pagination(t *testing.T) { func TestProvider_Get_MultiplePRs(t *testing.T) { prData := []pullRequestData{ - {Number: 123, HeadRefOid: "abc123", Files: filesData{Nodes: []fileNode{{Path: "file1.go"}}}}, - {Number: 456, HeadRefOid: "def456", Files: filesData{Nodes: []fileNode{{Path: "file2.go"}}}}, + {Number: 123, HeadRefOid: shaA, Files: filesData{Nodes: []fileNode{{Path: "file1.go"}}}}, + {Number: 456, HeadRefOid: shaB, Files: filesData{Nodes: []fileNode{{Path: "file2.go"}}}}, } callCount := 0 server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -163,16 +172,16 @@ func TestProvider_Get_MultiplePRs(t *testing.T) { p := newTestProvider(t, server.URL) infos, err := p.Get(context.Background(), entity.Change{ URIs: []string{ - "github://uber/submitqueue/123/abc123", - "github://uber/submitqueue/456/def456", + "github://uber/submitqueue/pull/123/" + shaA, + "github://uber/submitqueue/pull/456/" + shaB, }, }) require.NoError(t, err) assert.Equal(t, 2, callCount) require.Len(t, infos, 2) - assert.Equal(t, "github://uber/submitqueue/123/abc123", infos[0].URI) - assert.Equal(t, "github://uber/submitqueue/456/def456", infos[1].URI) + assert.Equal(t, "github://uber/submitqueue/pull/123/"+shaA, infos[0].URI) + assert.Equal(t, "github://uber/submitqueue/pull/456/"+shaB, infos[1].URI) } func TestProvider_Get_FetchError_StopsOnFirstFailure(t *testing.T) { @@ -185,7 +194,7 @@ func TestProvider_Get_FetchError_StopsOnFirstFailure(t *testing.T) { } servePR(t, w, pullRequestData{ Number: 123, - HeadRefOid: "abc123", + HeadRefOid: shaA, Files: filesData{Nodes: []fileNode{{Path: "file1.go"}}}, }) callCount++ @@ -195,8 +204,8 @@ func TestProvider_Get_FetchError_StopsOnFirstFailure(t *testing.T) { p := newTestProvider(t, server.URL) _, err := p.Get(context.Background(), entity.Change{ URIs: []string{ - "github://uber/submitqueue/123/abc123", - "github://uber/submitqueue/456/def456", + "github://uber/submitqueue/pull/123/" + shaA, + "github://uber/submitqueue/pull/456/" + shaB, }, }) diff --git a/extension/mergechecker/github/checker_test.go b/extension/mergechecker/github/checker_test.go index e39f36c2..9ab224a7 100644 --- a/extension/mergechecker/github/checker_test.go +++ b/extension/mergechecker/github/checker_test.go @@ -42,6 +42,15 @@ func newTestMergeChecker(t *testing.T, serverURL string) mergechecker.MergeCheck }) } +// Sample 40-char lowercase hex SHAs used across the test cases. +const ( + sha1Full = "1111111111111111111111111111111111111111" + sha2Full = "2222222222222222222222222222222222222222" + shaAFull = "abcdef0123456789abcdef0123456789abcdef01" + shaOldFull = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef" + shaNewFull = "feedfacefeedfacefeedfacefeedfacefeedface" +) + func graphQLHandler(t *testing.T, prInfos []PRInfo) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { t.Helper() @@ -83,50 +92,50 @@ func TestMergeChecker_Check(t *testing.T) { { name: "single PR mergeable", handler: graphQLHandler(t, []PRInfo{ - {Number: 1, Mergeable: PRMergeableStateMergeable, BaseRefName: "main", HeadRefName: "feature-1", HeadRefOid: "abc123", State: PRStateOpen}, + {Number: 1, Mergeable: PRMergeableStateMergeable, BaseRefName: "main", HeadRefName: "feature-1", HeadRefOid: shaAFull, State: PRStateOpen}, }), queue: "test-queue", - change: entity.Change{URIs: []string{"github://uber/repo/1/abc123"}}, + change: entity.Change{URIs: []string{"github://uber/repo/pull/1/" + shaAFull}}, wantMergeable: true, }, { name: "single PR conflicting", handler: graphQLHandler(t, []PRInfo{ - {Number: 1, Mergeable: PRMergeableStateConflicting, BaseRefName: "main", HeadRefName: "feature-1", HeadRefOid: "abc123", State: PRStateOpen}, + {Number: 1, Mergeable: PRMergeableStateConflicting, BaseRefName: "main", HeadRefName: "feature-1", HeadRefOid: shaAFull, State: PRStateOpen}, }), queue: "test-queue", - change: entity.Change{URIs: []string{"github://uber/repo/1/abc123"}}, + change: entity.Change{URIs: []string{"github://uber/repo/pull/1/" + shaAFull}}, wantMergeable: false, wantReason: "PR #1 has merge conflicts", }, { name: "stack of two PRs mergeable", handler: graphQLHandler(t, []PRInfo{ - {Number: 1, Mergeable: PRMergeableStateMergeable, BaseRefName: "main", HeadRefName: "feature-1", HeadRefOid: "sha1", State: PRStateOpen}, - {Number: 2, Mergeable: PRMergeableStateMergeable, BaseRefName: "feature-1", HeadRefName: "feature-2", HeadRefOid: "sha2", State: PRStateOpen}, + {Number: 1, Mergeable: PRMergeableStateMergeable, BaseRefName: "main", HeadRefName: "feature-1", HeadRefOid: sha1Full, State: PRStateOpen}, + {Number: 2, Mergeable: PRMergeableStateMergeable, BaseRefName: "feature-1", HeadRefName: "feature-2", HeadRefOid: sha2Full, State: PRStateOpen}, }), queue: "test-queue", - change: entity.Change{URIs: []string{"github://uber/repo/1/sha1", "github://uber/repo/2/sha2"}}, + change: entity.Change{URIs: []string{"github://uber/repo/pull/1/" + sha1Full, "github://uber/repo/pull/2/" + sha2Full}}, wantMergeable: true, }, { name: "unknown mergeability returns error", handler: graphQLHandler(t, []PRInfo{ - {Number: 1, Mergeable: PRMergeableStateUnknown, BaseRefName: "main", HeadRefName: "feature-1", HeadRefOid: "abc123", State: PRStateOpen}, + {Number: 1, Mergeable: PRMergeableStateUnknown, BaseRefName: "main", HeadRefName: "feature-1", HeadRefOid: shaAFull, State: PRStateOpen}, }), queue: "test-queue", - change: entity.Change{URIs: []string{"github://uber/repo/1/abc123"}}, + change: entity.Change{URIs: []string{"github://uber/repo/pull/1/" + shaAFull}}, wantErr: true, }, { name: "stale SHA not mergeable", handler: graphQLHandler(t, []PRInfo{ - {Number: 1, Mergeable: PRMergeableStateMergeable, BaseRefName: "main", HeadRefName: "feature-1", HeadRefOid: "new_sha", State: PRStateOpen}, + {Number: 1, Mergeable: PRMergeableStateMergeable, BaseRefName: "main", HeadRefName: "feature-1", HeadRefOid: shaNewFull, State: PRStateOpen}, }), queue: "test-queue", - change: entity.Change{URIs: []string{"github://uber/repo/1/old_sha"}}, + change: entity.Change{URIs: []string{"github://uber/repo/pull/1/" + shaOldFull}}, wantMergeable: false, - wantReason: "PR #1 head SHA changed: expected old_sha, got new_sha", + wantReason: fmt.Sprintf("PR #1 head SHA changed: expected %s, got %s", shaOldFull, shaNewFull), }, { name: "invalid change ID", @@ -144,7 +153,7 @@ func TestMergeChecker_Check(t *testing.T) { _, _ = w.Write([]byte("internal server error")) }), queue: "test-queue", - change: entity.Change{URIs: []string{"github://uber/repo/1/abc123"}}, + change: entity.Change{URIs: []string{"github://uber/repo/pull/1/" + shaAFull}}, wantErr: true, }, } diff --git a/extension/mergechecker/multi_test.go b/extension/mergechecker/multi_test.go index 22af958e..422fe3aa 100644 --- a/extension/mergechecker/multi_test.go +++ b/extension/mergechecker/multi_test.go @@ -44,12 +44,12 @@ func TestMultiChecker_RoutesToCorrectChecker(t *testing.T) { }) // Route to github checker - result, err := mc.Check(context.Background(), "test-queue", entity.Change{URIs: []string{"github://uber/repo/1/abc123"}}) + result, err := mc.Check(context.Background(), "test-queue", entity.Change{URIs: []string{"github://uber/repo/pull/1/abcdef0123456789abcdef0123456789abcdef01"}}) require.NoError(t, err) assert.True(t, result.Mergeable) // Route to ghe checker - result, err = mc.Check(context.Background(), "test-queue", entity.Change{URIs: []string{"ghe://uber/repo/1/abc123"}}) + result, err = mc.Check(context.Background(), "test-queue", entity.Change{URIs: []string{"ghe://uber/repo/pull/1/abcdef0123456789abcdef0123456789abcdef01"}}) require.NoError(t, err) assert.False(t, result.Mergeable) } @@ -59,7 +59,7 @@ func TestMultiChecker_UnknownScheme(t *testing.T) { "github": &stubChecker{result: Result{Mergeable: true}}, }) - _, err := mc.Check(context.Background(), "test-queue", entity.Change{URIs: []string{"unknown://uber/repo/1/abc123"}}) + _, err := mc.Check(context.Background(), "test-queue", entity.Change{URIs: []string{"unknown://uber/repo/1/abcdef0123456789abcdef0123456789abcdef01"}}) require.Error(t, err) } @@ -68,7 +68,7 @@ func TestMultiChecker_PropagatesError(t *testing.T) { "github": &stubChecker{err: fmt.Errorf("api failure")}, }) - _, err := mc.Check(context.Background(), "test-queue", entity.Change{URIs: []string{"github://uber/repo/1/abc123"}}) + _, err := mc.Check(context.Background(), "test-queue", entity.Change{URIs: []string{"github://uber/repo/pull/1/abcdef0123456789abcdef0123456789abcdef01"}}) require.Error(t, err) } diff --git a/extension/pusher/git/git_pusher_test.go b/extension/pusher/git/git_pusher_test.go index e79221c0..c6b853ef 100644 --- a/extension/pusher/git/git_pusher_test.go +++ b/extension/pusher/git/git_pusher_test.go @@ -144,7 +144,7 @@ func (f gitFixture) landOnMain(t *testing.T, sha string) { // uri builds a github-format URI ending in `sha` so the Pusher's parser // resolves it to that SHA. func uri(sha string) string { - return fmt.Sprintf("github://uber/submitqueue/1/%s", sha) + return fmt.Sprintf("github://uber/submitqueue/pull/1/%s", sha) } func (f gitFixture) newPusher(t *testing.T) pusher.Pusher { diff --git a/gateway/controller/land_test.go b/gateway/controller/land_test.go index 58b6f353..1d629f5e 100644 --- a/gateway/controller/land_test.go +++ b/gateway/controller/land_test.go @@ -83,7 +83,7 @@ func TestLand_ReturnsSqid(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Uris: []string{"github://uber/test-repo/pull/123/abc123def"}}, + Change: &pb.Change{Uris: []string{"github://uber/test-repo/pull/123/c3a4d5e6f7890123456789abcdef0123456789ab"}}, } resp, err := controller.Land(ctx, req) @@ -101,7 +101,7 @@ func TestLand_ReturnsErrorOnCounterFailure(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Uris: []string{"github://uber/test-repo/pull/123/abc123def"}}, + Change: &pb.Change{Uris: []string{"github://uber/test-repo/pull/123/c3a4d5e6f7890123456789abcdef0123456789ab"}}, } _, err := controller.Land(ctx, req) @@ -125,7 +125,7 @@ func TestLand_CounterDomainIncludesQueue(t *testing.T) { req := &pb.LandRequest{ Queue: "my-queue", - Change: &pb.Change{Uris: []string{"github://uber/test-repo/pull/123/abc123def"}}, + Change: &pb.Change{Uris: []string{"github://uber/test-repo/pull/123/c3a4d5e6f7890123456789abcdef0123456789ab"}}, } _, err := controller.Land(ctx, req) @@ -142,7 +142,7 @@ func TestLand_ReturnsErrorOnEmptyQueue(t *testing.T) { req := &pb.LandRequest{ Queue: "", - Change: &pb.Change{Uris: []string{"github://uber/test-repo/pull/123/abc123def"}}, + Change: &pb.Change{Uris: []string{"github://uber/test-repo/pull/123/c3a4d5e6f7890123456789abcdef0123456789ab"}}, } _, err := controller.Land(ctx, req) @@ -207,7 +207,7 @@ func TestLand_PublishesToQueue(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Uris: []string{"github://uber/backend/pull/456/fed987cba"}}, + Change: &pb.Change{Uris: []string{"github://uber/backend/pull/456/fedcba9876543210fedcba9876543210fedcba98"}}, Strategy: pb.Strategy_REBASE, } resp, err := controller.Land(ctx, req) @@ -225,7 +225,7 @@ func TestLand_PublishesToQueue(t *testing.T) { require.NoError(t, err) assert.Equal(t, "test-queue/123", deserializedReq.ID) assert.Equal(t, "test-queue", deserializedReq.Queue) - assert.Equal(t, []string{"github://uber/backend/pull/456/fed987cba"}, deserializedReq.Change.URIs) + assert.Equal(t, []string{"github://uber/backend/pull/456/fedcba9876543210fedcba9876543210fedcba98"}, deserializedReq.Change.URIs) assert.Equal(t, entity.RequestLandStrategyRebase, deserializedReq.LandStrategy) } @@ -243,7 +243,7 @@ func TestLand_ContinuesWhenPublishFails(t *testing.T) { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Uris: []string{"github://uber/service/pull/1/abc123def"}}, + Change: &pb.Change{Uris: []string{"github://uber/service/pull/1/c3a4d5e6f7890123456789abcdef0123456789ab"}}, } _, err := controller.Land(ctx, req) diff --git a/gateway/proto/gateway.proto b/gateway/proto/gateway.proto index 18f9c06d..26d1a257 100644 --- a/gateway/proto/gateway.proto +++ b/gateway/proto/gateway.proto @@ -62,8 +62,13 @@ message Change { // The scheme identifies the change provider, and the path contains provider-specific resource identifiers. // // GitHub is supported by default (though other providers can be added): - // Template: "github:////pull//" - // Example: "github://uber/submitqueue/pull/123/abc123def" + // Template: ":////pull//" + // Example: "github://uber/submitqueue/pull/123/c3a4d5e6f7890123456789abcdef0123456789ab" + // Schemes: "github" (github.com), "ghe" (GitHub Enterprise), "ghes" (GitHub Enterprise Server). + // The path mirrors the canonical PR URL so URIs can be constructed by trivial substitution. + // The head commit SHA must be the full 40-character lowercase hex SHA of the PR's head commit, + // as returned by GitHub's API (graphql `headRefOid`). Abbreviated SHAs are rejected because + // the staleness check downstream compares with strict string equality against the full SHA. // // SubmitQueue guarantees changes are landed in order with no other changes in between. // SubmitQueue does not guarantee each change is individually valid, but produces a validity marker on such changes. diff --git a/gateway/protopb/gateway.pb.go b/gateway/protopb/gateway.pb.go index ee85ac9e..30bc6348 100644 --- a/gateway/protopb/gateway.pb.go +++ b/gateway/protopb/gateway.pb.go @@ -207,8 +207,13 @@ type Change struct { // // GitHub is supported by default (though other providers can be added): // - // Template: "github:////pull//" - // Example: "github://uber/submitqueue/pull/123/abc123def" + // Template: ":////pull//" + // Example: "github://uber/submitqueue/pull/123/c3a4d5e6f7890123456789abcdef0123456789ab" + // Schemes: "github" (github.com), "ghe" (GitHub Enterprise), "ghes" (GitHub Enterprise Server). + // The path mirrors the canonical PR URL so URIs can be constructed by trivial substitution. + // The head commit SHA must be the full 40-character lowercase hex SHA of the PR's head commit, + // as returned by GitHub's API (graphql `headRefOid`). Abbreviated SHAs are rejected because + // the staleness check downstream compares with strict string equality against the full SHA. // // SubmitQueue guarantees changes are landed in order with no other changes in between. // SubmitQueue does not guarantee each change is individually valid, but produces a validity marker on such changes. diff --git a/orchestrator/controller/batch/batch_test.go b/orchestrator/controller/batch/batch_test.go index 3bb49d42..986349bb 100644 --- a/orchestrator/controller/batch/batch_test.go +++ b/orchestrator/controller/batch/batch_test.go @@ -60,7 +60,7 @@ func testRequest() entity.Request { return entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{URIs: []string{"github://uber/service/pull/456/abc123def"}}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/456/abcdef0123456789abcdef0123456789abcdef01"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateStarted, Version: 1, @@ -197,7 +197,7 @@ func TestController_Process_WithDependencies(t *testing.T) { request := entity.Request{ ID: "test-queue/456", Queue: "test-queue", - Change: entity.Change{URIs: []string{"github://uber/service/pull/789/abc123def"}}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/789/789abc1234567890abcdef1234567890abcdef12"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateStarted, Version: 1, diff --git a/orchestrator/controller/merge/merge_test.go b/orchestrator/controller/merge/merge_test.go index 51e45b83..b08480ef 100644 --- a/orchestrator/controller/merge/merge_test.go +++ b/orchestrator/controller/merge/merge_test.go @@ -100,7 +100,7 @@ func TestController_Process_SuccessfulMerge(t *testing.T) { State: entity.BatchStateMerging, Version: 4, } - change := entity.Change{URIs: []string{"github://o/r/1/sha"}} + change := entity.Change{URIs: []string{"github://o/r/pull/1/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}} request := entity.Request{ ID: reqID, Queue: "test-queue", @@ -153,9 +153,9 @@ func TestController_Process_PassesAllChangesInBatchOrder(t *testing.T) { const batchID = "test-queue/batch/multi" requestIDs := []string{"test-queue/1", "test-queue/2", "test-queue/3"} changes := []entity.Change{ - {URIs: []string{"github://o/r/1/sha1"}}, - {URIs: []string{"github://o/r/2/sha2"}}, - {URIs: []string{"github://o/r/3/sha3"}}, + {URIs: []string{"github://o/r/pull/1/1111111111111111111111111111111111111111"}}, + {URIs: []string{"github://o/r/pull/2/2222222222222222222222222222222222222222"}}, + {URIs: []string{"github://o/r/pull/3/3333333333333333333333333333333333333333"}}, } batch := entity.Batch{ @@ -231,7 +231,7 @@ func TestController_Process_PushConflictMarksBatchFailed(t *testing.T) { requestStore := storagemock.NewMockRequestStore(ctrl) requestStore.EXPECT().Get(gomock.Any(), reqID).Return(entity.Request{ - ID: reqID, Change: entity.Change{URIs: []string{"github://o/r/2/sha"}}, + ID: reqID, Change: entity.Change{URIs: []string{"github://o/r/pull/2/2222222222222222222222222222222222222222"}}, }, nil) store := storagemock.NewMockStorage(ctrl) @@ -277,7 +277,7 @@ func TestController_Process_PushInfraFailureReturnsError(t *testing.T) { requestStore := storagemock.NewMockRequestStore(ctrl) requestStore.EXPECT().Get(gomock.Any(), reqID).Return(entity.Request{ - ID: reqID, Change: entity.Change{URIs: []string{"github://o/r/3/sha"}}, + ID: reqID, Change: entity.Change{URIs: []string{"github://o/r/pull/3/3333333333333333333333333333333333333333"}}, }, nil) store := storagemock.NewMockStorage(ctrl) @@ -423,7 +423,7 @@ func TestController_Process_PublishFailureSurfaces(t *testing.T) { requestStore := storagemock.NewMockRequestStore(ctrl) requestStore.EXPECT().Get(gomock.Any(), reqID).Return(entity.Request{ - ID: reqID, Change: entity.Change{URIs: []string{"github://o/r/7/sha"}}, + ID: reqID, Change: entity.Change{URIs: []string{"github://o/r/pull/7/7777777777777777777777777777777777777777"}}, }, nil) store := storagemock.NewMockStorage(ctrl) diff --git a/orchestrator/controller/score/score_test.go b/orchestrator/controller/score/score_test.go index 1ed89a00..3af9c4ad 100644 --- a/orchestrator/controller/score/score_test.go +++ b/orchestrator/controller/score/score_test.go @@ -57,7 +57,7 @@ func testRequest() entity.Request { ID: "test-queue/1", Queue: "test-queue", Change: entity.Change{ - URIs: []string{"github://uber/repo/pull/1/abc123"}, + URIs: []string{"github://uber/repo/pull/1/abcdef0123456789abcdef0123456789abcdef01"}, }, State: entity.RequestStateStarted, Version: 1, @@ -154,14 +154,14 @@ func TestController_Process_MultipleRequests_MinScore(t *testing.T) { request1 := entity.Request{ ID: "test-queue/1", Queue: "test-queue", - Change: entity.Change{URIs: []string{"github://uber/repo/pull/1/abc"}}, + Change: entity.Change{URIs: []string{"github://uber/repo/pull/1/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}}, State: entity.RequestStateStarted, Version: 1, } request2 := entity.Request{ ID: "test-queue/2", Queue: "test-queue", - Change: entity.Change{URIs: []string{"github://uber/repo/pull/2/def"}}, + Change: entity.Change{URIs: []string{"github://uber/repo/pull/2/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"}}, State: entity.RequestStateStarted, Version: 1, } diff --git a/orchestrator/controller/start/start_test.go b/orchestrator/controller/start/start_test.go index 8793b912..3905a673 100644 --- a/orchestrator/controller/start/start_test.go +++ b/orchestrator/controller/start/start_test.go @@ -112,7 +112,7 @@ func TestController_Process_Success(t *testing.T) { delivery := makeDelivery(t, ctrl, entity.LandRequest{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{URIs: []string{"github://uber/service/pull/456/abc123def"}}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/456/abcdef0123456789abcdef0123456789abcdef01"}}, LandStrategy: entity.RequestLandStrategyRebase, }) @@ -153,7 +153,7 @@ func TestController_Process_ConstructsRequestWithStateAndVersion(t *testing.T) { landRequest := entity.LandRequest{ ID: "test-queue/42", Queue: "test-queue", - Change: entity.Change{URIs: []string{"github://uber/service/pull/1/abc123def"}}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/1/abcdef0123456789abcdef0123456789abcdef01"}}, LandStrategy: entity.RequestLandStrategySquashRebase, } delivery := makeDelivery(t, ctrl, landRequest) @@ -185,7 +185,7 @@ func TestController_Process_AllStrategies(t *testing.T) { delivery := makeDelivery(t, ctrl, entity.LandRequest{ ID: fmt.Sprintf("queue/%s", tt.strategy), Queue: "test-queue", - Change: entity.Change{URIs: []string{"github://uber/service/pull/1/aaa111bbb"}}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/1/aaa111bbbaaa111bbbaaa111bbbaaa111bbbaaa1"}}, LandStrategy: tt.strategy, }) @@ -209,9 +209,9 @@ func TestController_Process_MultipleChanges(t *testing.T) { controller := newTestController(t, ctrl, newMockStorage(ctrl), cs, nil) uris := []string{ - "github://uber/monorepo/pull/1/aaa111", - "github://uber/monorepo/pull/2/bbb222", - "github://uber/monorepo/pull/3/ccc333", + "github://uber/monorepo/pull/1/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "github://uber/monorepo/pull/2/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + "github://uber/monorepo/pull/3/cccccccccccccccccccccccccccccccccccccccc", } delivery := makeDelivery(t, ctrl, entity.LandRequest{ ID: "queue/999", @@ -240,7 +240,7 @@ func TestController_Process_PublishFailure(t *testing.T) { delivery := makeDelivery(t, ctrl, entity.LandRequest{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{URIs: []string{"github://uber/service/pull/1/xyz789abc"}}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/1/789abc1234567890abcdef1234567890abcdef12"}}, LandStrategy: entity.RequestLandStrategyRebase, }) @@ -260,7 +260,7 @@ func TestController_Process_StorageFailure(t *testing.T) { delivery := makeDelivery(t, ctrl, entity.LandRequest{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{URIs: []string{"github://uber/service/pull/1/xyz789abc"}}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/1/789abc1234567890abcdef1234567890abcdef12"}}, LandStrategy: entity.RequestLandStrategyRebase, }) @@ -282,7 +282,7 @@ func TestController_Process_AlreadyExistsSucceeds(t *testing.T) { delivery := makeDelivery(t, ctrl, entity.LandRequest{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{URIs: []string{"github://uber/service/pull/1/xyz789abc"}}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/1/789abc1234567890abcdef1234567890abcdef12"}}, LandStrategy: entity.RequestLandStrategyRebase, }) @@ -300,7 +300,7 @@ func TestController_Process_ChangeStoreFailure(t *testing.T) { delivery := makeDelivery(t, ctrl, entity.LandRequest{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{URIs: []string{"github://uber/service/pull/1/xyz789abc"}}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/1/789abc1234567890abcdef1234567890abcdef12"}}, LandStrategy: entity.RequestLandStrategyRebase, }) diff --git a/orchestrator/controller/validate/validate_test.go b/orchestrator/controller/validate/validate_test.go index ad592e2e..e8672b2d 100644 --- a/orchestrator/controller/validate/validate_test.go +++ b/orchestrator/controller/validate/validate_test.go @@ -50,7 +50,7 @@ type mockChangeProvider struct{} func (m *mockChangeProvider) Get(ctx context.Context, change entity.Change) ([]changeprovider.ChangeInfo, error) { return []changeprovider.ChangeInfo{ { - URI: "github://org/repo/123/abc123", + URI: "github://org/repo/pull/123/abcdef0123456789abcdef0123456789abcdef01", User: changeprovider.User{ Name: "Test User", Email: "test@example.com", @@ -127,7 +127,7 @@ func TestNewController(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{URIs: []string{"github://uber/service/pull/456/abc123def"}}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/456/abcdef0123456789abcdef0123456789abcdef01"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateStarted, Version: 1, @@ -148,7 +148,7 @@ func TestController_Process_Success(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{URIs: []string{"github://uber/service/pull/456/abc123def"}}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/456/abcdef0123456789abcdef0123456789abcdef01"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateStarted, Version: 1, @@ -192,7 +192,7 @@ func TestController_Process_PublishFailure(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{URIs: []string{"github://uber/service/pull/1/xyz789abc"}}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/1/789abc1234567890abcdef1234567890abcdef12"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateStarted, Version: 1, @@ -228,7 +228,7 @@ func TestController_Process_NotMergeable(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{URIs: []string{"github://uber/repo/1/abc123"}}, + Change: entity.Change{URIs: []string{"github://uber/repo/pull/1/abcdef0123456789abcdef0123456789abcdef01"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateStarted, Version: 1, @@ -255,7 +255,7 @@ func TestController_Process_MergeCheckError(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{URIs: []string{"github://uber/repo/1/abc123"}}, + Change: entity.Change{URIs: []string{"github://uber/repo/pull/1/abcdef0123456789abcdef0123456789abcdef01"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateStarted, Version: 1, @@ -278,8 +278,8 @@ func TestController_Process_DuplicateDetection(t *testing.T) { queueName = "test-queue" newRequestID = queueName + "/123" dupRequestID = queueName + "/100" - uriA = "github://uber/service/pull/1/abc" - uriB = "github://uber/service/pull/2/def" + uriA = "github://uber/service/pull/1/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + uriB = "github://uber/service/pull/2/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" anotherReqID = queueName + "/050" orphanReqID = queueName + "/999" terminalReqID = queueName + "/200" @@ -452,7 +452,7 @@ func TestController_Process_ChangeStoreQueryFailure(t *testing.T) { request := entity.Request{ ID: "test-queue/123", Queue: "test-queue", - Change: entity.Change{URIs: []string{"github://uber/service/pull/1/abc"}}, + Change: entity.Change{URIs: []string{"github://uber/service/pull/1/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}}, LandStrategy: entity.RequestLandStrategyRebase, State: entity.RequestStateStarted, Version: 1, diff --git a/test/e2e/suite_test.go b/test/e2e/suite_test.go index ee2e19c9..2d2ddcd0 100644 --- a/test/e2e/suite_test.go +++ b/test/e2e/suite_test.go @@ -156,7 +156,7 @@ func (s *E2EIntegrationSuite) TestPingOrchestrator() { func (s *E2EIntegrationSuite) TestLandRequest_SinglePR() { req := &gatewaypb.LandRequest{ Queue: "e2e-test-queue", - Change: &gatewaypb.Change{Uris: []string{"github://uber/e2e-service/pull/123/abc123def"}}, + Change: &gatewaypb.Change{Uris: []string{"github://uber/e2e-service/pull/123/abcdef0123456789abcdef0123456789abcdef01"}}, Strategy: gatewaypb.Strategy_REBASE, } diff --git a/test/integration/extension/changestore/mysql/changestore_test.go b/test/integration/extension/changestore/mysql/changestore_test.go index 25c67199..bba3e74d 100644 --- a/test/integration/extension/changestore/mysql/changestore_test.go +++ b/test/integration/extension/changestore/mysql/changestore_test.go @@ -91,17 +91,17 @@ func (s *MySQLChangeStoreIntegrationSuite) SetupTest() { func (s *MySQLChangeStoreIntegrationSuite) TestCreateAndGet_NoMatch() { t := s.T() require.NoError(t, s.store.Create(s.ctx, entity.ChangeRecord{ - URI: "github://uber/x/pull/1/aaa", RequestID: "q/1", Queue: "q", CreatedAt: 1, UpdatedAt: 1, Version: 1, + URI: "github://uber/x/pull/1/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", RequestID: "q/1", Queue: "q", CreatedAt: 1, UpdatedAt: 1, Version: 1, })) - got, err := s.store.GetByURI(s.ctx, "q", "github://uber/x/pull/2/bbb") + got, err := s.store.GetByURI(s.ctx, "q", "github://uber/x/pull/2/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb") require.NoError(t, err) assert.Empty(t, got) } func (s *MySQLChangeStoreIntegrationSuite) TestCreateAndGet_Match() { t := s.T() - uri := "github://uber/x/pull/1/aaa" + uri := "github://uber/x/pull/1/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" require.NoError(t, s.store.Create(s.ctx, entity.ChangeRecord{ URI: uri, RequestID: "q/1", Queue: "q", CreatedAt: 1, UpdatedAt: 1, Version: 1, })) @@ -118,7 +118,7 @@ func (s *MySQLChangeStoreIntegrationSuite) TestCreateAndGet_Match() { func (s *MySQLChangeStoreIntegrationSuite) TestGetByURI_DoesNotExcludeSelf() { // The store does not filter by request_id; callers filter self if they wish. t := s.T() - uri := "github://uber/x/pull/1/aaa" + uri := "github://uber/x/pull/1/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" require.NoError(t, s.store.Create(s.ctx, entity.ChangeRecord{ URI: uri, RequestID: "q/1", Queue: "q", CreatedAt: 1, UpdatedAt: 1, Version: 1, })) @@ -131,7 +131,7 @@ func (s *MySQLChangeStoreIntegrationSuite) TestGetByURI_DoesNotExcludeSelf() { func (s *MySQLChangeStoreIntegrationSuite) TestGetByURI_QueueScoped() { t := s.T() - uri := "github://uber/x/pull/1/aaa" + uri := "github://uber/x/pull/1/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" require.NoError(t, s.store.Create(s.ctx, entity.ChangeRecord{ URI: uri, RequestID: "qA/1", Queue: "qA", CreatedAt: 1, UpdatedAt: 1, Version: 1, })) @@ -143,7 +143,7 @@ func (s *MySQLChangeStoreIntegrationSuite) TestGetByURI_QueueScoped() { func (s *MySQLChangeStoreIntegrationSuite) TestCreate_Idempotent() { t := s.T() - rec := entity.ChangeRecord{URI: "github://uber/x/pull/1/aaa", RequestID: "q/1", Queue: "q", CreatedAt: 1, UpdatedAt: 1, Version: 1} + rec := entity.ChangeRecord{URI: "github://uber/x/pull/1/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", RequestID: "q/1", Queue: "q", CreatedAt: 1, UpdatedAt: 1, Version: 1} require.NoError(t, s.store.Create(s.ctx, rec)) require.NoError(t, s.store.Create(s.ctx, rec), "second insert with same PK must succeed (INSERT IGNORE)") @@ -155,7 +155,7 @@ func (s *MySQLChangeStoreIntegrationSuite) TestCreate_Idempotent() { func (s *MySQLChangeStoreIntegrationSuite) TestCreate_DifferentRequestSameURI() { t := s.T() - uri := "github://uber/x/pull/1/aaa" + uri := "github://uber/x/pull/1/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" require.NoError(t, s.store.Create(s.ctx, entity.ChangeRecord{ URI: uri, RequestID: "q/1", Queue: "q", CreatedAt: 1, UpdatedAt: 1, Version: 1, })) @@ -175,7 +175,7 @@ func (s *MySQLChangeStoreIntegrationSuite) TestCreate_DifferentRequestSameURI() func (s *MySQLChangeStoreIntegrationSuite) TestCreate_PreservesMetadata() { t := s.T() const meta = `{"title":"add new feature"}` - uri := "github://uber/x/pull/1/aaa" + uri := "github://uber/x/pull/1/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" require.NoError(t, s.store.Create(s.ctx, entity.ChangeRecord{ URI: uri, RequestID: "q/1", Queue: "q", Metadata: meta, CreatedAt: 1, UpdatedAt: 1, Version: 1, })) @@ -190,7 +190,7 @@ func (s *MySQLChangeStoreIntegrationSuite) TestCreate_EmptyMetadataStoredAsObjec // metadata is NOT NULL in the schema. The impl substitutes '{}' for an empty // Metadata field so callers don't need to know about the column constraint. t := s.T() - uri := "github://uber/x/pull/1/aaa" + uri := "github://uber/x/pull/1/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" require.NoError(t, s.store.Create(s.ctx, entity.ChangeRecord{ URI: uri, RequestID: "q/1", Queue: "q", CreatedAt: 1, UpdatedAt: 1, Version: 1, })) diff --git a/test/integration/extension/storage/suite.go b/test/integration/extension/storage/suite.go index fe1e65d5..377b2340 100644 --- a/test/integration/extension/storage/suite.go +++ b/test/integration/extension/storage/suite.go @@ -60,7 +60,7 @@ func (s *StorageContractSuite) TestStorage_CreateAndGet() { Queue: "test-queue", State: entity.RequestStateStarted, Change: entity.Change{ - URIs: []string{"github://uber/storage-test/pull/123/abc123def"}, + URIs: []string{"github://uber/storage-test/pull/123/abcdef0123456789abcdef0123456789abcdef01"}, }, LandStrategy: entity.RequestLandStrategyMerge, Version: 1, @@ -92,10 +92,10 @@ func (s *StorageContractSuite) TestStorage_CreateAndGet_StackedPRs() { // Stacked PRs as separate URIs stackedURIs := []string{ - "github://uber/monorepo/pull/101/aaa111bbb", - "github://uber/monorepo/pull/102/ccc222ddd", - "github://uber/monorepo/pull/103/eee333fff", - "github://uber/monorepo/pull/104/ggg444hhh", + "github://uber/monorepo/pull/101/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "github://uber/monorepo/pull/102/cccccccccccccccccccccccccccccccccccccccc", + "github://uber/monorepo/pull/103/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee", + "github://uber/monorepo/pull/104/0000000000000000000000000000000000000004", } request := entity.Request{ diff --git a/test/integration/gateway/suite_test.go b/test/integration/gateway/suite_test.go index e50c48e6..e9577576 100644 --- a/test/integration/gateway/suite_test.go +++ b/test/integration/gateway/suite_test.go @@ -125,7 +125,7 @@ func (s *GatewayIntegrationSuite) TestLandAPI() { req := &pb.LandRequest{ Queue: "test-queue", - Change: &pb.Change{Uris: []string{"github://uber/integration-test/pull/123/abc123def"}}, + Change: &pb.Change{Uris: []string{"github://uber/integration-test/pull/123/abcdef0123456789abcdef0123456789abcdef01"}}, Strategy: pb.Strategy_REBASE, }