From d66cd10ce0f383912159896dbf13ca79485b3e84 Mon Sep 17 00:00:00 2001 From: mnoah1 Date: Wed, 7 Oct 2026 18:48:23 +0000 Subject: [PATCH] refactor(id): require canonical decimal request IDs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Intent: - Retire the temporary ID comparison workaround from #778 now that the decimal-ID rollout is complete. Changes: - Compare resource IDs numerically using only canonical positive decimal IDs; reject legacy prefixed IDs. - Update the comparison contracts and replace rollout compatibility tests with rejection coverage, including ingestion and bookmark behavior. --- Generated by the 🪄 [pr-create](https://sg.uberinternal.com/code.uber.internal/uber-code/devexp-agent-marketplace/-/blob/claude-code/plugins/dev/uber-dev/skills/pr-create/SKILL.md) skill in devexp-agent-marketplace --- platform/base/id/id.go | 28 ++-------------- platform/base/id/id_test.go | 38 +++++++++------------- stovepipe/controller/ingest_test.go | 14 +++----- stovepipe/controller/record/record_test.go | 21 +++++++++--- stovepipe/entity/request_id.go | 3 +- stovepipe/entity/request_id_test.go | 2 ++ 6 files changed, 42 insertions(+), 64 deletions(-) diff --git a/platform/base/id/id.go b/platform/base/id/id.go index d9e2eee5..c9ae4ed6 100644 --- a/platform/base/id/id.go +++ b/platform/base/id/id.go @@ -18,7 +18,6 @@ package id import ( "fmt" "strconv" - "strings" ) // FromCounter returns the canonical decimal resource ID for value. @@ -45,39 +44,18 @@ func Validate(id string) error { return nil } -// Compare orders legacy IDs by their numeric suffix, followed by decimal IDs numerically. +// Compare compares two canonical positive decimal resource IDs numerically. // Callers must ensure both IDs belong to the same queue and resource kind. -// This ordering requires a one-way writer switch from legacy to decimal IDs. func Compare(a, b string) (int, error) { - aCounter, bCounter := a, b - aSeparator := strings.LastIndexByte(a, '/') - bSeparator := strings.LastIndexByte(b, '/') - aLegacy, bLegacy := aSeparator >= 0, bSeparator >= 0 - if aLegacy { - if aSeparator == 0 { - return 0, fmt.Errorf("invalid first resource ID %q: legacy prefix must not be empty", a) - } - aCounter = a[aSeparator+1:] - } - if bLegacy { - if bSeparator == 0 { - return 0, fmt.Errorf("invalid second resource ID %q: legacy prefix must not be empty", b) - } - bCounter = b[bSeparator+1:] - } - aValue, err := parseResourceID(aCounter) + aValue, err := parseResourceID(a) if err != nil { return 0, fmt.Errorf("invalid first resource ID %q: %w", a, err) } - bValue, err := parseResourceID(bCounter) + bValue, err := parseResourceID(b) if err != nil { return 0, fmt.Errorf("invalid second resource ID %q: %w", b, err) } switch { - case aLegacy && !bLegacy: - return -1, nil - case !aLegacy && bLegacy: - return 1, nil case aValue < bValue: return -1, nil case aValue > bValue: diff --git a/platform/base/id/id_test.go b/platform/base/id/id_test.go index 5652a716..32506c60 100644 --- a/platform/base/id/id_test.go +++ b/platform/base/id/id_test.go @@ -90,28 +90,22 @@ func TestCompare(t *testing.T) { {name: "newer", a: "10", b: "9", want: 1}, {name: "invalid first", a: "bad", b: "10", wantErr: true}, {name: "invalid second", a: "9", b: "batch.10", wantErr: true}, - {name: "legacy counters sort numerically", a: "queue/9", b: "queue/10", want: -1}, - {name: "newer legacy counter", a: "queue/10", b: "queue/9", want: 1}, - {name: "decimal is newer despite smaller counter", a: "1", b: "queue/42", want: 1}, - {name: "legacy is older despite larger counter", a: "queue/42", b: "1", want: -1}, - {name: "same counter across formats is not equal", a: "queue/42", b: "42", want: -1}, - {name: "same legacy ID", a: "queue/42", b: "queue/42"}, - {name: "queue containing slashes", a: "1", b: "request/monorepo/main/42", want: 1}, - {name: "legacy queue containing slashes", a: "request/monorepo/main/9", b: "request/monorepo/main/10", want: -1}, - {name: "maximum legacy counter is older than first decimal", a: "request/monorepo/main/9223372036854775807", b: "1", want: -1}, - {name: "large legacy counters retain precision", a: "queue/9223372036854775806", b: "queue/9223372036854775807", want: -1}, - {name: "invalid legacy suffix", a: "queue/bad", b: "queue/42", wantErr: true}, - {name: "zero legacy suffix", a: "1", b: "queue/0", wantErr: true}, - {name: "noncanonical legacy suffix", a: "queue/01", b: "queue/42", wantErr: true}, - {name: "empty first legacy prefix", a: "/9", b: "10", wantErr: true}, - {name: "empty second legacy prefix", a: "9", b: "/10", wantErr: true}, - {name: "empty legacy suffix", a: "queue/", b: "10", wantErr: true}, - {name: "overflow legacy suffix", a: "queue/9223372036854775808", b: "10", wantErr: true}, - {name: "zero decimal against legacy", a: "0", b: "queue/42", wantErr: true}, - {name: "noncanonical decimal against legacy", a: "queue/42", b: "01", wantErr: true}, - {name: "negative decimal against legacy", a: "-1", b: "queue/42", wantErr: true}, - {name: "empty decimal against legacy", a: "queue/42", wantErr: true}, - {name: "overflow decimal against legacy", a: "9223372036854775808", b: "queue/42", wantErr: true}, + {name: "large counters retain precision", a: "9223372036854775806", b: "9223372036854775807", want: -1}, + {name: "legacy first ID", a: "queue/42", b: "1", wantErr: true}, + {name: "legacy second ID", a: "1", b: "queue/42", wantErr: true}, + {name: "both legacy IDs", a: "queue/9", b: "queue/10", wantErr: true}, + {name: "same legacy ID", a: "queue/42", b: "queue/42", wantErr: true}, + {name: "legacy queue containing slashes", a: "request/monorepo/main/9", b: "10", wantErr: true}, + {name: "empty first ID", b: "1", wantErr: true}, + {name: "zero first ID", a: "0", b: "1", wantErr: true}, + {name: "negative first ID", a: "-1", b: "1", wantErr: true}, + {name: "noncanonical first ID", a: "01", b: "1", wantErr: true}, + {name: "overflow first ID", a: "9223372036854775808", b: "1", wantErr: true}, + {name: "empty second ID", a: "1", wantErr: true}, + {name: "zero second ID", a: "1", b: "0", wantErr: true}, + {name: "negative second ID", a: "1", b: "-1", wantErr: true}, + {name: "noncanonical second ID", a: "1", b: "01", wantErr: true}, + {name: "overflow second ID", a: "1", b: "9223372036854775808", wantErr: true}, } for _, tt := range tests { diff --git a/stovepipe/controller/ingest_test.go b/stovepipe/controller/ingest_test.go index 5ab426eb..12650fec 100644 --- a/stovepipe/controller/ingest_test.go +++ b/stovepipe/controller/ingest_test.go @@ -185,7 +185,7 @@ func TestIngestController_Ingest(t *testing.T) { wantID: "7", }, { - name: "new ID advances legacy latest pointer and publishes", + name: "legacy latest pointer prevents advancement and publish", queue: testQueue, setup: func(m ingestMocks) { expectResolve(m) @@ -198,14 +198,11 @@ func TestIngestController_Ingest(t *testing.T) { m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{ Name: testQueue, LatestRequestID: "request/" + testQueue + "/42", Version: 1, }, nil) - updated := entity.Queue{Name: testQueue, LatestRequestID: "1", Version: 1} - updateCall := m.queueStore.EXPECT().Update(gomock.Any(), updated, int32(1), int32(2)).Return(nil) - m.publisher.EXPECT().Publish(gomock.Any(), "process", gomock.Any()).Return(nil).After(updateCall) }, - wantID: "1", + wantErr: true, }, { - name: "retry repairs accepted decimal request behind legacy pointer", + name: "retry rejects legacy latest pointer", queue: testQueue, setup: func(m ingestMocks) { expectResolve(m) @@ -215,11 +212,8 @@ func TestIngestController_Ingest(t *testing.T) { m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{ Name: testQueue, LatestRequestID: "request/" + testQueue + "/42", Version: 1, }, nil) - updated := entity.Queue{Name: testQueue, LatestRequestID: "1", Version: 1} - updateCall := m.queueStore.EXPECT().Update(gomock.Any(), updated, int32(1), int32(2)).Return(nil) - m.publisher.EXPECT().Publish(gomock.Any(), "process", gomock.Any()).Return(nil).After(updateCall) }, - wantID: "1", + wantErr: true, }, { name: "dedup with existing accepted request republishes without minting", diff --git a/stovepipe/controller/record/record_test.go b/stovepipe/controller/record/record_test.go index b12b1e37..1589eec6 100644 --- a/stovepipe/controller/record/record_test.go +++ b/stovepipe/controller/record/record_test.go @@ -291,11 +291,6 @@ func TestProcess_AdvancesBookmarkOnSuccess(t *testing.T) { stored: queueRow("git://remote/monorepo/main/old", "3", 4), wantURI: testURI, }, - { - name: "stored legacy bookmark is older", - stored: queueRow("git://remote/monorepo/main/old", "request/"+testQueue+"/42", 4), - wantURI: testURI, - }, } for _, tt := range tests { @@ -339,6 +334,22 @@ func TestProcess_AdvancesBookmarkOnSuccess(t *testing.T) { } } +func TestProcess_RejectsLegacyBookmark(t *testing.T) { + ctrl := gomock.NewController(t) + c, m := newController(t, ctrl) + + m.reqStore.EXPECT().Get(gomock.Any(), testID). + Return(requestWithState(entity.RequestStateSucceeded), nil) + var fact entity.ValidationFact + m.expectFactCreated(&fact) + m.queueStore.EXPECT().Get(gomock.Any(), testQueue). + Return(queueRow("git://remote/monorepo/main/old", "request/"+testQueue+"/42", 4), nil) + + err := c.Process(queueContext(), delivery(t, ctrl, recordPayload(t, testID))) + require.Error(t, err) + assert.Empty(t, m.hooks.events) +} + func TestProcess_RecordsNamedProjectStatusResults(t *testing.T) { ctrl := gomock.NewController(t) c, m := newController(t, ctrl) diff --git a/stovepipe/entity/request_id.go b/stovepipe/entity/request_id.go index 023cf0b1..cd30ac89 100644 --- a/stovepipe/entity/request_id.go +++ b/stovepipe/entity/request_id.go @@ -19,8 +19,7 @@ import "github.com/uber/submitqueue/platform/base/id" // CompareRequestID compares ingest order of two request IDs in the same queue. // Callers must ensure the IDs belong to the same queue. // Returns -1 if a is older than b, 0 if equal, 1 if a is newer than b. -// Decimal IDs are newer than all legacy request// IDs. -// Within each format, ordering is by numeric counter. +// IDs must be canonical positive decimal strings whose scope is carried separately. func CompareRequestID(a, b string) (int, error) { return id.Compare(a, b) } diff --git a/stovepipe/entity/request_id_test.go b/stovepipe/entity/request_id_test.go index 716ae063..faa7d17c 100644 --- a/stovepipe/entity/request_id_test.go +++ b/stovepipe/entity/request_id_test.go @@ -54,6 +54,8 @@ func TestCompareRequestID(t *testing.T) { want: -1, }, {name: "prefixed ID", a: "request.1", b: "2", wantErr: true}, + {name: "legacy first ID", a: "request/monorepo/main/1", b: "2", wantErr: true}, + {name: "legacy second ID", a: "1", b: "request/monorepo/main/2", wantErr: true}, } for _, tt := range tests {