Skip to content

Commit d9e69fd

Browse files
mnoah1mnoah1
authored
refactor(id): require canonical decimal request IDs (#804)
The decimal-ID rollout is complete, so the temporary comparison workaround from #778 can be retired. Resource-ID comparisons now accept only canonical positive decimal IDs, compare them numerically, and reject legacy prefixed IDs. Updates the comparison contracts and regression tests to cover legacy-ID rejection, including ingestion without publication and bookmark updates. ## Validation - Nine affected Bazel test targets passed. - `make lint`, `make check-tidy`, `make check-gazelle`, and `git diff --check` passed. - `aifx verify` passed review and artifact checks; generic Go lint failed without reporting issues, and coverage could not run because it expects a monorepo `.arcconfig` file. <sub>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</sub>
1 parent 561518b commit d9e69fd

6 files changed

Lines changed: 42 additions & 64 deletions

File tree

‎platform/base/id/id.go‎

Lines changed: 3 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ package id
1818
import (
1919
"fmt"
2020
"strconv"
21-
"strings"
2221
)
2322

2423
// FromCounter returns the canonical decimal resource ID for value.
@@ -45,39 +44,18 @@ func Validate(id string) error {
4544
return nil
4645
}
4746

48-
// Compare orders legacy IDs by their numeric suffix, followed by decimal IDs numerically.
47+
// Compare compares two canonical positive decimal resource IDs numerically.
4948
// Callers must ensure both IDs belong to the same queue and resource kind.
50-
// This ordering requires a one-way writer switch from legacy to decimal IDs.
5149
func Compare(a, b string) (int, error) {
52-
aCounter, bCounter := a, b
53-
aSeparator := strings.LastIndexByte(a, '/')
54-
bSeparator := strings.LastIndexByte(b, '/')
55-
aLegacy, bLegacy := aSeparator >= 0, bSeparator >= 0
56-
if aLegacy {
57-
if aSeparator == 0 {
58-
return 0, fmt.Errorf("invalid first resource ID %q: legacy prefix must not be empty", a)
59-
}
60-
aCounter = a[aSeparator+1:]
61-
}
62-
if bLegacy {
63-
if bSeparator == 0 {
64-
return 0, fmt.Errorf("invalid second resource ID %q: legacy prefix must not be empty", b)
65-
}
66-
bCounter = b[bSeparator+1:]
67-
}
68-
aValue, err := parseResourceID(aCounter)
50+
aValue, err := parseResourceID(a)
6951
if err != nil {
7052
return 0, fmt.Errorf("invalid first resource ID %q: %w", a, err)
7153
}
72-
bValue, err := parseResourceID(bCounter)
54+
bValue, err := parseResourceID(b)
7355
if err != nil {
7456
return 0, fmt.Errorf("invalid second resource ID %q: %w", b, err)
7557
}
7658
switch {
77-
case aLegacy && !bLegacy:
78-
return -1, nil
79-
case !aLegacy && bLegacy:
80-
return 1, nil
8159
case aValue < bValue:
8260
return -1, nil
8361
case aValue > bValue:

‎platform/base/id/id_test.go‎

Lines changed: 16 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -90,28 +90,22 @@ func TestCompare(t *testing.T) {
9090
{name: "newer", a: "10", b: "9", want: 1},
9191
{name: "invalid first", a: "bad", b: "10", wantErr: true},
9292
{name: "invalid second", a: "9", b: "batch.10", wantErr: true},
93-
{name: "legacy counters sort numerically", a: "queue/9", b: "queue/10", want: -1},
94-
{name: "newer legacy counter", a: "queue/10", b: "queue/9", want: 1},
95-
{name: "decimal is newer despite smaller counter", a: "1", b: "queue/42", want: 1},
96-
{name: "legacy is older despite larger counter", a: "queue/42", b: "1", want: -1},
97-
{name: "same counter across formats is not equal", a: "queue/42", b: "42", want: -1},
98-
{name: "same legacy ID", a: "queue/42", b: "queue/42"},
99-
{name: "queue containing slashes", a: "1", b: "request/monorepo/main/42", want: 1},
100-
{name: "legacy queue containing slashes", a: "request/monorepo/main/9", b: "request/monorepo/main/10", want: -1},
101-
{name: "maximum legacy counter is older than first decimal", a: "request/monorepo/main/9223372036854775807", b: "1", want: -1},
102-
{name: "large legacy counters retain precision", a: "queue/9223372036854775806", b: "queue/9223372036854775807", want: -1},
103-
{name: "invalid legacy suffix", a: "queue/bad", b: "queue/42", wantErr: true},
104-
{name: "zero legacy suffix", a: "1", b: "queue/0", wantErr: true},
105-
{name: "noncanonical legacy suffix", a: "queue/01", b: "queue/42", wantErr: true},
106-
{name: "empty first legacy prefix", a: "/9", b: "10", wantErr: true},
107-
{name: "empty second legacy prefix", a: "9", b: "/10", wantErr: true},
108-
{name: "empty legacy suffix", a: "queue/", b: "10", wantErr: true},
109-
{name: "overflow legacy suffix", a: "queue/9223372036854775808", b: "10", wantErr: true},
110-
{name: "zero decimal against legacy", a: "0", b: "queue/42", wantErr: true},
111-
{name: "noncanonical decimal against legacy", a: "queue/42", b: "01", wantErr: true},
112-
{name: "negative decimal against legacy", a: "-1", b: "queue/42", wantErr: true},
113-
{name: "empty decimal against legacy", a: "queue/42", wantErr: true},
114-
{name: "overflow decimal against legacy", a: "9223372036854775808", b: "queue/42", wantErr: true},
93+
{name: "large counters retain precision", a: "9223372036854775806", b: "9223372036854775807", want: -1},
94+
{name: "legacy first ID", a: "queue/42", b: "1", wantErr: true},
95+
{name: "legacy second ID", a: "1", b: "queue/42", wantErr: true},
96+
{name: "both legacy IDs", a: "queue/9", b: "queue/10", wantErr: true},
97+
{name: "same legacy ID", a: "queue/42", b: "queue/42", wantErr: true},
98+
{name: "legacy queue containing slashes", a: "request/monorepo/main/9", b: "10", wantErr: true},
99+
{name: "empty first ID", b: "1", wantErr: true},
100+
{name: "zero first ID", a: "0", b: "1", wantErr: true},
101+
{name: "negative first ID", a: "-1", b: "1", wantErr: true},
102+
{name: "noncanonical first ID", a: "01", b: "1", wantErr: true},
103+
{name: "overflow first ID", a: "9223372036854775808", b: "1", wantErr: true},
104+
{name: "empty second ID", a: "1", wantErr: true},
105+
{name: "zero second ID", a: "1", b: "0", wantErr: true},
106+
{name: "negative second ID", a: "1", b: "-1", wantErr: true},
107+
{name: "noncanonical second ID", a: "1", b: "01", wantErr: true},
108+
{name: "overflow second ID", a: "1", b: "9223372036854775808", wantErr: true},
115109
}
116110

117111
for _, tt := range tests {

‎stovepipe/controller/ingest_test.go‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ func TestIngestController_Ingest(t *testing.T) {
185185
wantID: "7",
186186
},
187187
{
188-
name: "new ID advances legacy latest pointer and publishes",
188+
name: "legacy latest pointer prevents advancement and publish",
189189
queue: testQueue,
190190
setup: func(m ingestMocks) {
191191
expectResolve(m)
@@ -198,14 +198,11 @@ func TestIngestController_Ingest(t *testing.T) {
198198
m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{
199199
Name: testQueue, LatestRequestID: "request/" + testQueue + "/42", Version: 1,
200200
}, nil)
201-
updated := entity.Queue{Name: testQueue, LatestRequestID: "1", Version: 1}
202-
updateCall := m.queueStore.EXPECT().Update(gomock.Any(), updated, int32(1), int32(2)).Return(nil)
203-
m.publisher.EXPECT().Publish(gomock.Any(), "process", gomock.Any()).Return(nil).After(updateCall)
204201
},
205-
wantID: "1",
202+
wantErr: true,
206203
},
207204
{
208-
name: "retry repairs accepted decimal request behind legacy pointer",
205+
name: "retry rejects legacy latest pointer",
209206
queue: testQueue,
210207
setup: func(m ingestMocks) {
211208
expectResolve(m)
@@ -215,11 +212,8 @@ func TestIngestController_Ingest(t *testing.T) {
215212
m.queueStore.EXPECT().Get(gomock.Any(), testQueue).Return(entity.Queue{
216213
Name: testQueue, LatestRequestID: "request/" + testQueue + "/42", Version: 1,
217214
}, nil)
218-
updated := entity.Queue{Name: testQueue, LatestRequestID: "1", Version: 1}
219-
updateCall := m.queueStore.EXPECT().Update(gomock.Any(), updated, int32(1), int32(2)).Return(nil)
220-
m.publisher.EXPECT().Publish(gomock.Any(), "process", gomock.Any()).Return(nil).After(updateCall)
221215
},
222-
wantID: "1",
216+
wantErr: true,
223217
},
224218
{
225219
name: "dedup with existing accepted request republishes without minting",

‎stovepipe/controller/record/record_test.go‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -291,11 +291,6 @@ func TestProcess_AdvancesBookmarkOnSuccess(t *testing.T) {
291291
stored: queueRow("git://remote/monorepo/main/old", "3", 4),
292292
wantURI: testURI,
293293
},
294-
{
295-
name: "stored legacy bookmark is older",
296-
stored: queueRow("git://remote/monorepo/main/old", "request/"+testQueue+"/42", 4),
297-
wantURI: testURI,
298-
},
299294
}
300295

301296
for _, tt := range tests {
@@ -339,6 +334,22 @@ func TestProcess_AdvancesBookmarkOnSuccess(t *testing.T) {
339334
}
340335
}
341336

337+
func TestProcess_RejectsLegacyBookmark(t *testing.T) {
338+
ctrl := gomock.NewController(t)
339+
c, m := newController(t, ctrl)
340+
341+
m.reqStore.EXPECT().Get(gomock.Any(), testID).
342+
Return(requestWithState(entity.RequestStateSucceeded), nil)
343+
var fact entity.ValidationFact
344+
m.expectFactCreated(&fact)
345+
m.queueStore.EXPECT().Get(gomock.Any(), testQueue).
346+
Return(queueRow("git://remote/monorepo/main/old", "request/"+testQueue+"/42", 4), nil)
347+
348+
err := c.Process(queueContext(), delivery(t, ctrl, recordPayload(t, testID)))
349+
require.Error(t, err)
350+
assert.Empty(t, m.hooks.events)
351+
}
352+
342353
func TestProcess_RecordsNamedProjectStatusResults(t *testing.T) {
343354
ctrl := gomock.NewController(t)
344355
c, m := newController(t, ctrl)

‎stovepipe/entity/request_id.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,7 @@ import "github.com/uber/submitqueue/platform/base/id"
1919
// CompareRequestID compares ingest order of two request IDs in the same queue.
2020
// Callers must ensure the IDs belong to the same queue.
2121
// Returns -1 if a is older than b, 0 if equal, 1 if a is newer than b.
22-
// Decimal IDs are newer than all legacy request/<queue>/<counter> IDs.
23-
// Within each format, ordering is by numeric counter.
22+
// IDs must be canonical positive decimal strings whose scope is carried separately.
2423
func CompareRequestID(a, b string) (int, error) {
2524
return id.Compare(a, b)
2625
}

‎stovepipe/entity/request_id_test.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,8 @@ func TestCompareRequestID(t *testing.T) {
5454
want: -1,
5555
},
5656
{name: "prefixed ID", a: "request.1", b: "2", wantErr: true},
57+
{name: "legacy first ID", a: "request/monorepo/main/1", b: "2", wantErr: true},
58+
{name: "legacy second ID", a: "1", b: "request/monorepo/main/2", wantErr: true},
5759
}
5860

5961
for _, tt := range tests {

0 commit comments

Comments
 (0)