From 0311a4b44f6c136c9d764186a7b5fe68eb152c2a Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Mon, 5 Oct 2026 16:21:12 -0700 Subject: [PATCH] refactor(submitqueue): retire legacy entity JSON serializers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary ### Why? The internal queue migration landed in #703, but unused JSON serializers remained on the full Request, Batch, and Build entities. These are the final serialization helpers covered by #356; production publishers, consumers, and DLQ reconciliation already use the proto contracts. ### What? - Remove the remaining ToBytes/FromBytes helpers and their serializer-only tests, preserving lifecycle behavior tests and regenerating Bazel dependencies. - Verify unknown-field tolerance for all 11 current internal proto payloads without losing known fields. - Document the original hard-cutover rollout, including dead-letter handling and legacy enum/timestamp incompatibilities. This cleanup does not change the current wire format or message IDs. ## Test Plan - ✅ `./tool/bazel test //submitqueue/... //service/submitqueue/... --test_output=errors` — all 55 test targets pass, including contract, gateway, orchestrator, and DLQ tests. - ✅ `make lint`, `make check-tidy`, and `make check-gazelle`. - ✅ `./tool/bazel test //test/e2e/submitqueue:go_default_test --test_output=summary --sandbox_writable_path="$HOME/.docker" --test_filter=TestE2EIntegration` — fake-provider end-to-end suite, passing on retry after one invocation received RPC Unimplemented responses. The writable path permits macOS Docker buildx to update its activity file. - ✅ `./tool/bazel test //test/e2e/submitqueue:go_default_test --test_output=summary --sandbox_writable_path="$HOME/.docker" --test_filter='TestGitMergeE2E/TestLand_(SingleChange_ReachesTheTargetBranch|Stack_LandsInOrderInOneRefUpdate)'` — real-Git single-change and stacked-change cases pass with the cleanup. - ⚠️ The combined E2E run encountered real-Git failures: Git rejected the Docker-mounted `/srv/git/sandbox.git` as having dubious ownership. The full `TestGitMergeE2E` suite also reproduces this on unchanged `main` at `276bfcc2`; its individual single-change/stack cases pass in isolation on that baseline. This pre-existing macOS/Docker ownership issue is outside the queue-contract cleanup. `aifx verify` passed its ureview/artifact checks but could not complete its Go lint/coverage adapters; coverage reports a missing `.arcconfig` in this OSS repository. Repository-native checks above are the validation source. ## Issue Closes #356 # Conflicts: # submitqueue/entity/batch_test.go # submitqueue/entity/request_test.go # Please enter the commit message for your changes. Lines starting # with '#' will be kept; you may remove them yourself if you want to. # An empty message aborts the commit. # # interactive rebase in progress; onto 8ddc4c49b # Last command done (1 command done): # pick 31c46f142 # refactor(submitqueue): retire legacy entity JSON serializers # No commands remaining. # You are currently rebasing branch 'preetam/codex/356-retire-entity-serializers' on '8ddc4c49b'. # # Changes to be committed: # modified: submitqueue/core/messagequeue/README.md # modified: submitqueue/core/messagequeue/messagequeue_test.go # modified: submitqueue/entity/BUILD.bazel # modified: submitqueue/entity/batch.go # modified: submitqueue/entity/batch_test.go # modified: submitqueue/entity/build.go # modified: submitqueue/entity/build_test.go # modified: submitqueue/entity/request.go # modified: submitqueue/entity/request_test.go # --- submitqueue/core/messagequeue/README.md | 6 + .../core/messagequeue/messagequeue_test.go | 41 ++++++ submitqueue/entity/BUILD.bazel | 7 +- submitqueue/entity/batch.go | 14 -- submitqueue/entity/batch_test.go | 92 ------------ submitqueue/entity/build.go | 14 -- submitqueue/entity/build_test.go | 106 -------------- submitqueue/entity/request.go | 14 -- submitqueue/entity/request_test.go | 136 ------------------ 9 files changed, 48 insertions(+), 382 deletions(-) diff --git a/submitqueue/core/messagequeue/README.md b/submitqueue/core/messagequeue/README.md index f27e1be0d..23019770e 100644 --- a/submitqueue/core/messagequeue/README.md +++ b/submitqueue/core/messagequeue/README.md @@ -23,3 +23,9 @@ Each topic key has its own message, even when the first version is only an id an - **log** (`TopicKeyLog`, `Log`) — orchestrator publishes a full request-log entry; the gateway materializes it. `type`, `status`, and `event` are open strings matching the domain vocabularies. In-boundary stages (validate through conclude, except start/cancel/log) put only an id on the queue because producer and consumer share storage. + +## Wire compatibility + +Consumers discard unknown fields, so additive proto changes do not require a coordinated producer/consumer rollout. Enum fields use protobuf names (for example, `SQUASH_REBASE`) and int64 fields such as `timestamp_ms` are JSON strings. + +The original entity-JSON to protojson migration uses a hard-cutover rollout, not a separate legacy codec. For deployments crossing that cutover, drain or discard queued `start` and `log` messages, including their dead-letter copies, and switch their producers and consumers together. Legacy start messages use lowercase land-strategy values instead of protobuf enum names; log producers emit quoted timestamps instead of numbers, which legacy entity-JSON consumers cannot read. The id-only payloads retain the `id` and `queue` fields; cancellation retains `id`, `queue`, and `reason`. Removing unused entity serialization helpers does not change the current wire format. diff --git a/submitqueue/core/messagequeue/messagequeue_test.go b/submitqueue/core/messagequeue/messagequeue_test.go index 79624f227..73d7d3ff5 100644 --- a/submitqueue/core/messagequeue/messagequeue_test.go +++ b/submitqueue/core/messagequeue/messagequeue_test.go @@ -15,6 +15,7 @@ package messagequeue import ( + "strings" "testing" "github.com/stretchr/testify/assert" @@ -109,6 +110,46 @@ func TestLogEventRoundTrip(t *testing.T) { assert.Equal(t, int32(0), got.RequestVersion) } +func TestUnmarshalDiscardsUnknownFields(t *testing.T) { + messages := []proto.Message{ + StartFromLandRequest(entity.LandRequest{ + ID: "q/1", + Queue: "q", + Change: change.Change{URIs: []string{"change-1"}}, + LandStrategy: mergestrategy.MergeStrategySquashRebase, + }), + &Cancel{Id: "q/1", Queue: "q", Reason: "user"}, + &Validate{Id: "q/1", Queue: "q"}, + &Batch{Id: "q/1", Queue: "q"}, + &DependencyAnalysis{Id: "q/batch/1", Queue: "q"}, + &Speculate{Id: "q/batch/1", Queue: "q"}, + &Build{Id: "q/batch/1", Queue: "q"}, + &BuildSignal{Id: "build-1", Queue: "q"}, + &Merge{Id: "q/batch/1", Queue: "q"}, + &Conclude{Id: "q/batch/1", Queue: "q"}, + LogFromEntity(entity.RequestLog{ + RequestID: "q/1", + Queue: "q", + TimestampMs: 1700000000000, + Type: entity.RequestLogTypeStatus, + Status: entity.RequestStatusStarted, + RequestVersion: 1, + Metadata: map[string]string{"build_id": "build-1"}, + }), + } + for _, message := range messages { + t.Run(string(message.ProtoReflect().Descriptor().Name()), func(t *testing.T) { + data, err := Marshal(message) + require.NoError(t, err) + payload := strings.TrimSuffix(string(data), "}") + `,"future_field":{"enabled":true}}` + got := message.ProtoReflect().Type().New().Interface() + + require.NoError(t, Unmarshal([]byte(payload), got)) + assert.True(t, proto.Equal(message, got)) + }) + } +} + func TestLandStrategyMapping(t *testing.T) { tests := []struct { name string diff --git a/submitqueue/entity/BUILD.bazel b/submitqueue/entity/BUILD.bazel index 6d1177f33..b0890f6f2 100644 --- a/submitqueue/entity/BUILD.bazel +++ b/submitqueue/entity/BUILD.bazel @@ -43,10 +43,5 @@ go_test( "speculation_test.go", ], embed = [":go_default_library"], - deps = [ - "//platform/base/change:go_default_library", - "//platform/base/mergestrategy:go_default_library", - "@com_github_stretchr_testify//assert:go_default_library", - "@com_github_stretchr_testify//require:go_default_library", - ], + deps = ["@com_github_stretchr_testify//assert:go_default_library"], ) diff --git a/submitqueue/entity/batch.go b/submitqueue/entity/batch.go index 3f9242be7..819d4420d 100644 --- a/submitqueue/entity/batch.go +++ b/submitqueue/entity/batch.go @@ -14,8 +14,6 @@ package entity -import "encoding/json" - // BatchState defines the possible states of a batch. type BatchState string @@ -175,18 +173,6 @@ type Batch struct { Version int32 } -// ToBytes serializes the Batch to JSON bytes for queue message payload. -func (b Batch) ToBytes() ([]byte, error) { - return json.Marshal(b) -} - -// BatchFromBytes deserializes a Batch from JSON bytes. -func BatchFromBytes(data []byte) (Batch, error) { - var batch Batch - err := json.Unmarshal(data, &batch) - return batch, err -} - // BatchID is a lightweight entity for publishing and consuming just the batch identifier via the queue. type BatchID struct { // ID is the queue-scoped identifier for the batch. diff --git a/submitqueue/entity/batch_test.go b/submitqueue/entity/batch_test.go index f774a1750..05d6bf668 100644 --- a/submitqueue/entity/batch_test.go +++ b/submitqueue/entity/batch_test.go @@ -18,7 +18,6 @@ import ( "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) func TestBatchState_IsTerminal(t *testing.T) { @@ -73,94 +72,3 @@ func TestAllBatchStates_SupersetOfStateSubsets(t *testing.T) { func TestDependencyBatchStates_ExcludesCreating(t *testing.T) { assert.NotContains(t, DependencyBatchStates(), BatchStateCreating) } - -func TestBatch_SerializationRoundTrip(t *testing.T) { - tests := []struct { - name string - batch Batch - }{ - { - name: "batch with single request", - batch: Batch{ - ID: "1", - Queue: "queueA", - Contains: []string{"1"}, - State: BatchStateCreated, - Version: 1, - }, - }, - { - name: "batch with multiple requests", - batch: Batch{ - ID: "42", - Queue: "queueB", - Contains: []string{"10", "11", "12"}, - State: BatchStateSpeculating, - Version: 3, - }, - }, - { - name: "batch with dependencies", - batch: Batch{ - ID: "3", - Queue: "queueA", - Contains: []string{"5"}, - Dependencies: []string{ - "1", - "2", - }, - State: BatchStateCreated, - Version: 1, - }, - }, - { - name: "batch in terminal state", - batch: Batch{ - ID: "99", - Queue: "queueC", - Contains: []string{"50"}, - State: BatchStateSucceeded, - Version: 5, - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - data, err := tt.batch.ToBytes() - require.NoError(t, err) - - deserialized, err := BatchFromBytes(data) - require.NoError(t, err) - - assert.Equal(t, tt.batch, deserialized) - }) - } -} - -func TestBatchFromBytes_InvalidJSON(t *testing.T) { - _, err := BatchFromBytes([]byte(`{"invalid": json"}`)) - assert.Error(t, err) -} - -func TestBatchFromBytes_EmptyJSON(t *testing.T) { - batch, err := BatchFromBytes([]byte(`{}`)) - require.NoError(t, err) - - assert.Empty(t, batch.ID) - assert.Empty(t, batch.Queue) - assert.Nil(t, batch.Contains) - assert.Nil(t, batch.Dependencies) - assert.Equal(t, BatchStateUnknown, batch.State) - assert.Equal(t, int32(0), batch.Version) -} - -func TestBatchFromBytes_EmptyBytes(t *testing.T) { - _, err := BatchFromBytes([]byte{}) - assert.Error(t, err) -} - -func TestBatchFromBytes_NilBytes(t *testing.T) { - _, err := BatchFromBytes(nil) - assert.Error(t, err) -} diff --git a/submitqueue/entity/build.go b/submitqueue/entity/build.go index 0cb95577a..34ae9dfd0 100644 --- a/submitqueue/entity/build.go +++ b/submitqueue/entity/build.go @@ -14,8 +14,6 @@ package entity -import "encoding/json" - // BuildStatus defines the possible states of a build. The set is // intentionally narrow: every supported build provider must be able to map // its native lifecycle into one of these values without leaking @@ -78,18 +76,6 @@ type Build struct { Status BuildStatus } -// ToBytes serializes the Build to JSON bytes for queue message payload. -func (b Build) ToBytes() ([]byte, error) { - return json.Marshal(b) -} - -// BuildFromBytes deserializes a Build from JSON bytes. -func BuildFromBytes(data []byte) (Build, error) { - var build Build - err := json.Unmarshal(data, &build) - return build, err -} - // BuildID is a lightweight entity for publishing and consuming just the build identifier via the queue. type BuildID struct { // ID is the globally unique identifier for the build. diff --git a/submitqueue/entity/build_test.go b/submitqueue/entity/build_test.go index 67db1cd43..79afb2cfd 100644 --- a/submitqueue/entity/build_test.go +++ b/submitqueue/entity/build_test.go @@ -18,7 +18,6 @@ import ( "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) func TestBuildStatus_IsTerminal(t *testing.T) { @@ -65,108 +64,3 @@ func TestBuildStatus_IsTerminal(t *testing.T) { }) } } - -func TestBuild_ToBytes(t *testing.T) { - build := Build{ - ID: "build-1", - BatchID: "batch-1", - Status: BuildStatusAccepted, - } - - data, err := build.ToBytes() - require.NoError(t, err) - assert.NotEmpty(t, data) - - // Verify JSON contains expected fields - jsonStr := string(data) - assert.Contains(t, jsonStr, "build-1") - assert.Contains(t, jsonStr, "batch-1") - assert.Contains(t, jsonStr, "accepted") -} - -func TestBuildFromBytes(t *testing.T) { - original := Build{ - ID: "build-42", - BatchID: "batch-7", - Status: BuildStatusAccepted, - } - - // Serialize - data, err := original.ToBytes() - require.NoError(t, err) - - // Deserialize - deserialized, err := BuildFromBytes(data) - require.NoError(t, err) - - // Verify all fields match - assert.Equal(t, original.ID, deserialized.ID) - assert.Equal(t, original.BatchID, deserialized.BatchID) - assert.Equal(t, original.Status, deserialized.Status) -} - -func TestBuildFromBytes_InvalidJSON(t *testing.T) { - invalidJSON := []byte(`{"invalid": json"}`) - - _, err := BuildFromBytes(invalidJSON) - assert.Error(t, err) -} - -func TestBuildFromBytes_EmptyData(t *testing.T) { - emptyJSON := []byte(`{}`) - - build, err := BuildFromBytes(emptyJSON) - require.NoError(t, err) - - // Empty JSON should deserialize with zero values - assert.Empty(t, build.ID) - assert.Empty(t, build.BatchID) - assert.Equal(t, BuildStatusUnknown, build.Status) -} - -func TestBuild_SerializationRoundTrip(t *testing.T) { - tests := []struct { - name string - build Build - }{ - { - name: "accepted build", - build: Build{ - ID: "build-100", - BatchID: "batch-50", - Status: BuildStatusAccepted, - }, - }, - { - name: "succeeded build", - build: Build{ - ID: "build-200", - BatchID: "batch-60", - Status: BuildStatusSucceeded, - }, - }, - { - name: "failed build", - build: Build{ - ID: "build-300", - BatchID: "batch-70", - Status: BuildStatusFailed, - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Serialize - data, err := tt.build.ToBytes() - require.NoError(t, err) - - // Deserialize - deserialized, err := BuildFromBytes(data) - require.NoError(t, err) - - // Verify complete equality - assert.Equal(t, tt.build, deserialized) - }) - } -} diff --git a/submitqueue/entity/request.go b/submitqueue/entity/request.go index 6bc4b72ab..0399518ed 100644 --- a/submitqueue/entity/request.go +++ b/submitqueue/entity/request.go @@ -15,8 +15,6 @@ package entity import ( - "encoding/json" - "github.com/uber/submitqueue/platform/base/change" "github.com/uber/submitqueue/platform/base/mergestrategy" ) @@ -96,18 +94,6 @@ type Request struct { Version int32 `json:"version"` } -// ToBytes serializes the Request to JSON bytes for queue message payload. -func (r Request) ToBytes() ([]byte, error) { - return json.Marshal(r) -} - -// RequestFromBytes deserializes a Request from JSON bytes. -func RequestFromBytes(data []byte) (Request, error) { - var req Request - err := json.Unmarshal(data, &req) - return req, err -} - // RequestID is a lightweight entity for publishing and consuming just the request identifier via the queue. type RequestID struct { // ID is the queue-scoped identifier for the land request. diff --git a/submitqueue/entity/request_test.go b/submitqueue/entity/request_test.go index 8c4c33441..0b85cb843 100644 --- a/submitqueue/entity/request_test.go +++ b/submitqueue/entity/request_test.go @@ -18,84 +18,8 @@ import ( "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "github.com/uber/submitqueue/platform/base/change" - "github.com/uber/submitqueue/platform/base/mergestrategy" ) -func TestRequest_ToBytes(t *testing.T) { - req := Request{ - ID: "123", - Queue: "test-queue", - Change: change.Change{URIs: []string{ - "github://github.example.com/uber/submitqueue/pull/456/abcdef0123456789abcdef0123456789abcdef01", - "github://github.example.com/uber/submitqueue/pull/789/0123456789abcdef0123456789abcdef01234567", - }}, - LandStrategy: mergestrategy.MergeStrategyRebase, - State: RequestStateStarted, - Version: 1, - } - - data, err := req.ToBytes() - require.NoError(t, err) - assert.NotEmpty(t, data) - - // Verify JSON contains expected fields - jsonStr := string(data) - assert.Contains(t, jsonStr, "123") - assert.Contains(t, jsonStr, "github://github.example.com/uber/submitqueue/pull/456/abcdef0123456789abcdef0123456789abcdef01") - assert.Contains(t, jsonStr, "rebase") - assert.Contains(t, jsonStr, "started") -} - -func TestRequestFromBytes(t *testing.T) { - original := Request{ - ID: "999", - Queue: "my-queue", - Change: change.Change{URIs: []string{"code.uber.internal.com/D111"}}, - LandStrategy: mergestrategy.MergeStrategyMerge, - State: RequestStateProcessing, - Version: 3, - } - - // Serialize - data, err := original.ToBytes() - require.NoError(t, err) - - // Deserialize - deserialized, err := RequestFromBytes(data) - require.NoError(t, err) - - // Verify all fields match - assert.Equal(t, original.ID, deserialized.ID) - assert.Equal(t, original.Queue, deserialized.Queue) - assert.Equal(t, original.Change.URIs, deserialized.Change.URIs) - assert.Equal(t, original.LandStrategy, deserialized.LandStrategy) - assert.Equal(t, original.State, deserialized.State) - assert.Equal(t, original.Version, deserialized.Version) -} - -func TestRequestFromBytes_InvalidJSON(t *testing.T) { - invalidJSON := []byte(`{"invalid": json"}`) - - _, err := RequestFromBytes(invalidJSON) - assert.Error(t, err) -} - -func TestRequestFromBytes_EmptyData(t *testing.T) { - emptyJSON := []byte(`{}`) - - req, err := RequestFromBytes(emptyJSON) - require.NoError(t, err) - - // Empty JSON should deserialize with zero values - assert.Empty(t, req.ID) - assert.Empty(t, req.Queue) - assert.Equal(t, RequestStateUnknown, req.State) - assert.Equal(t, mergestrategy.MergeStrategyUnknown, req.LandStrategy) - assert.Equal(t, int32(0), req.Version) -} - func TestIsRequestStateTerminal(t *testing.T) { tests := []struct { state RequestState @@ -137,63 +61,3 @@ func TestIsRequestStateHalted(t *testing.T) { }) } } - -func TestRequest_SerializationRoundTrip(t *testing.T) { - tests := []struct { - name string - req Request - }{ - { - name: "github stacked diff", - req: Request{ - ID: "100", - Queue: "queue1", - Change: change.Change{URIs: []string{ - "github://github.example.com/uber/repo-a/pull/101/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", - "github://github.example.com/uber/repo-a/pull/102/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", - "github://github.example.com/uber/repo-a/pull/103/cccccccccccccccccccccccccccccccccccccccc", - }}, - LandStrategy: mergestrategy.MergeStrategySquashRebase, - State: RequestStateLanded, - Version: 5, - }, - }, - { - name: "phabricator revision", - req: Request{ - ID: "200", - Queue: "queue2", - Change: change.Change{URIs: []string{"code.uber.internal.com/D12345"}}, - LandStrategy: mergestrategy.MergeStrategyRebase, - State: RequestStateStarted, - Version: 1, - }, - }, - { - name: "github enterprise request", - req: Request{ - ID: "300", - Queue: "queue3", - Change: change.Change{URIs: []string{"github.uber.com/internal/service/999/deadbeef12"}}, - LandStrategy: mergestrategy.MergeStrategyMerge, - State: RequestStateError, - Version: 10, - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Serialize - data, err := tt.req.ToBytes() - require.NoError(t, err) - - // Deserialize - deserialized, err := RequestFromBytes(data) - require.NoError(t, err) - - // Verify complete equality - assert.Equal(t, tt.req, deserialized) - }) - } -}