Skip to content

Commit 7cbbbe6

Browse files
ubettigoleclaude
andauthored
refactor(gateway): make LandController operate on entities (#359)
## Summary Move proto<->entity translation out of the gateway Land controller and into a dedicated mapper package. The controller now takes entity.LandRequest and returns entity.LandResult, keeping business logic proto-free; the GatewayServer adapter maps the wire request in and the response out. - add entity.LandResult; consolidate LandRequest/LandResult into entity/land.go - add service/.../server/mapper with ProtoToLandRequest + strategy resolution - keep field validation in the controller so it runs on controller-to-controller calls Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> ## Test Plan ## Issues ## Stack 1. @ #359 1. #360 1. #363 1. #361 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 22f670e commit 7cbbbe6

11 files changed

Lines changed: 282 additions & 116 deletions

File tree

service/submitqueue/gateway/server/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ go_library(
1919
"//platform/extension/counter/mysql:go_default_library",
2020
"//platform/extension/messagequeue:go_default_library",
2121
"//platform/extension/messagequeue/mysql:go_default_library",
22+
"//service/submitqueue/gateway/server/mapper:go_default_library",
2223
"//submitqueue/core/topickey:go_default_library",
2324
"//submitqueue/extension/queueconfig/yaml:go_default_library",
2425
"//submitqueue/extension/storage/mysql:go_default_library",

service/submitqueue/gateway/server/main.go

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ import (
3636
mysqlcounter "github.com/uber/submitqueue/platform/extension/counter/mysql"
3737
extqueue "github.com/uber/submitqueue/platform/extension/messagequeue"
3838
queueMySQL "github.com/uber/submitqueue/platform/extension/messagequeue/mysql"
39+
"github.com/uber/submitqueue/service/submitqueue/gateway/server/mapper"
3940
"github.com/uber/submitqueue/submitqueue/core/topickey"
4041
yamlqueueconfig "github.com/uber/submitqueue/submitqueue/extension/queueconfig/yaml"
4142
mysqlstorage "github.com/uber/submitqueue/submitqueue/extension/storage/mysql"
@@ -62,9 +63,18 @@ func (s *GatewayServer) Ping(ctx context.Context, req *pb.PingRequest) (*pb.Ping
6263
return s.pingController.Ping(ctx, req)
6364
}
6465

65-
// Land delegates to the controller
66+
// Land maps the wire request to an entity, delegates to the controller, and maps
67+
// the result back to the wire response.
6668
func (s *GatewayServer) Land(ctx context.Context, req *pb.LandRequest) (*pb.LandResponse, error) {
67-
return s.landController.Land(ctx, req)
69+
landReq, err := mapper.ProtoToLandRequest(req)
70+
if err != nil {
71+
return nil, err
72+
}
73+
result, err := s.landController.Land(ctx, landReq)
74+
if err != nil {
75+
return nil, err
76+
}
77+
return &pb.LandResponse{Sqid: result.ID}, nil
6878
}
6979

7080
// Cancel delegates to the controller
Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
load("@rules_go//go:def.bzl", "go_library", "go_test")
2+
3+
go_library(
4+
name = "go_default_library",
5+
srcs = ["land.go"],
6+
importpath = "github.com/uber/submitqueue/service/submitqueue/gateway/server/mapper",
7+
visibility = ["//visibility:public"],
8+
deps = [
9+
"//api/base/mergestrategy/protopb:go_default_library",
10+
"//api/submitqueue/gateway/protopb:go_default_library",
11+
"//platform/base/change:go_default_library",
12+
"//platform/base/mergestrategy:go_default_library",
13+
"//submitqueue/entity:go_default_library",
14+
],
15+
)
16+
17+
go_test(
18+
name = "go_default_test",
19+
srcs = ["land_test.go"],
20+
embed = [":go_default_library"],
21+
deps = [
22+
"//api/base/change/protopb:go_default_library",
23+
"//api/base/mergestrategy/protopb:go_default_library",
24+
"//api/submitqueue/gateway/protopb:go_default_library",
25+
"//platform/base/change:go_default_library",
26+
"//platform/base/mergestrategy:go_default_library",
27+
"//submitqueue/entity:go_default_library",
28+
"@com_github_stretchr_testify//assert:go_default_library",
29+
"@com_github_stretchr_testify//require:go_default_library",
30+
],
31+
)
Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
// Copyright (c) 2025 Uber Technologies, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
// Package mapper translates gateway wire (proto) types to and from the domain
16+
// entities the controllers operate on. Each RPC gets its own file (land.go,
17+
// status.go, cancel.go, …); translation lives here so controllers stay
18+
// proto-free.
19+
package mapper
20+
21+
import (
22+
"errors"
23+
"fmt"
24+
25+
mergestrategypb "github.com/uber/submitqueue/api/base/mergestrategy/protopb"
26+
pb "github.com/uber/submitqueue/api/submitqueue/gateway/protopb"
27+
"github.com/uber/submitqueue/platform/base/change"
28+
"github.com/uber/submitqueue/platform/base/mergestrategy"
29+
"github.com/uber/submitqueue/submitqueue/entity"
30+
)
31+
32+
// errUnknownStrategy is returned when a proto Strategy enum has no known
33+
// mergestrategy.MergeStrategy mapping.
34+
var errUnknownStrategy = errors.New("unknown land strategy in proto message")
35+
36+
// ProtoToLandRequest maps the wire LandRequest to the entity.LandRequest the controller operates on.
37+
// The ID is left empty; the controller assigns it.
38+
func ProtoToLandRequest(req *pb.LandRequest) (entity.LandRequest, error) {
39+
strategy, err := resolveMergeStrategy(req.GetStrategy())
40+
if err != nil {
41+
return entity.LandRequest{}, fmt.Errorf("failed to map land strategy: %w", err)
42+
}
43+
return entity.LandRequest{
44+
Queue: req.GetQueue(),
45+
Change: change.Change{URIs: req.GetChange().GetUris()},
46+
LandStrategy: strategy,
47+
}, nil
48+
}
49+
50+
// resolveMergeStrategy maps a proto Strategy enum to the shared mergestrategy.MergeStrategy.
51+
func resolveMergeStrategy(s mergestrategypb.Strategy) (mergestrategy.MergeStrategy, error) {
52+
switch s {
53+
case mergestrategypb.Strategy_DEFAULT:
54+
// TODO: resolve default strategy based on queue configuration
55+
return mergestrategy.MergeStrategyRebase, nil
56+
case mergestrategypb.Strategy_REBASE:
57+
return mergestrategy.MergeStrategyRebase, nil
58+
case mergestrategypb.Strategy_SQUASH_REBASE:
59+
return mergestrategy.MergeStrategySquashRebase, nil
60+
case mergestrategypb.Strategy_MERGE:
61+
return mergestrategy.MergeStrategyMerge, nil
62+
default:
63+
return mergestrategy.MergeStrategyUnknown, fmt.Errorf("%w: %v", errUnknownStrategy, s)
64+
}
65+
}
Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
// Copyright (c) 2025 Uber Technologies, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package mapper
16+
17+
import (
18+
"testing"
19+
20+
"github.com/stretchr/testify/assert"
21+
"github.com/stretchr/testify/require"
22+
changepb "github.com/uber/submitqueue/api/base/change/protopb"
23+
mergestrategypb "github.com/uber/submitqueue/api/base/mergestrategy/protopb"
24+
pb "github.com/uber/submitqueue/api/submitqueue/gateway/protopb"
25+
"github.com/uber/submitqueue/platform/base/change"
26+
"github.com/uber/submitqueue/platform/base/mergestrategy"
27+
"github.com/uber/submitqueue/submitqueue/entity"
28+
)
29+
30+
func TestProtoToLandRequest(t *testing.T) {
31+
const uri = "github://github.example.com/uber/test-repo/pull/1/c3a4d5e6f7890123456789abcdef0123456789ab"
32+
33+
tests := []struct {
34+
name string
35+
req *pb.LandRequest
36+
expected entity.LandRequest
37+
expectedErr error
38+
}{
39+
{
40+
name: "maps all fields and leaves ID empty",
41+
req: &pb.LandRequest{
42+
Queue: "test-queue",
43+
Change: &changepb.Change{Uris: []string{uri}},
44+
Strategy: mergestrategypb.Strategy_SQUASH_REBASE,
45+
},
46+
// ID is not assigned by the mapper — the controller mints it.
47+
expected: entity.LandRequest{
48+
Queue: "test-queue",
49+
Change: change.Change{URIs: []string{uri}},
50+
LandStrategy: mergestrategy.MergeStrategySquashRebase,
51+
},
52+
},
53+
{
54+
name: "nil change yields empty URIs without erroring",
55+
req: &pb.LandRequest{Queue: "test-queue", Change: nil},
56+
// The mapper does not validate; it leaves URIs empty for the controller to reject.
57+
expected: entity.LandRequest{
58+
Queue: "test-queue",
59+
LandStrategy: mergestrategy.MergeStrategyRebase,
60+
},
61+
},
62+
{
63+
name: "unknown strategy errors",
64+
req: &pb.LandRequest{
65+
Queue: "test-queue",
66+
Change: &changepb.Change{Uris: []string{uri}},
67+
Strategy: mergestrategypb.Strategy(9999),
68+
},
69+
expectedErr: errUnknownStrategy,
70+
},
71+
}
72+
73+
for _, tt := range tests {
74+
t.Run(tt.name, func(t *testing.T) {
75+
got, err := ProtoToLandRequest(tt.req)
76+
if tt.expectedErr != nil {
77+
require.ErrorIs(t, err, tt.expectedErr)
78+
return
79+
}
80+
require.NoError(t, err)
81+
assert.Equal(t, tt.expected, got)
82+
})
83+
}
84+
}
85+
86+
func TestResolveMergeStrategy(t *testing.T) {
87+
tests := []struct {
88+
name string
89+
in mergestrategypb.Strategy
90+
want mergestrategy.MergeStrategy
91+
errMsg string
92+
}{
93+
{name: "default", in: mergestrategypb.Strategy_DEFAULT, want: mergestrategy.MergeStrategyRebase},
94+
{name: "rebase", in: mergestrategypb.Strategy_REBASE, want: mergestrategy.MergeStrategyRebase},
95+
{name: "squash_rebase", in: mergestrategypb.Strategy_SQUASH_REBASE, want: mergestrategy.MergeStrategySquashRebase},
96+
{name: "merge", in: mergestrategypb.Strategy_MERGE, want: mergestrategy.MergeStrategyMerge},
97+
{name: "unknown", in: mergestrategypb.Strategy(9999), errMsg: "unknown land strategy in proto message"},
98+
}
99+
100+
for _, tt := range tests {
101+
t.Run(tt.name, func(t *testing.T) {
102+
got, err := resolveMergeStrategy(tt.in)
103+
if tt.errMsg != "" {
104+
assert.ErrorContains(t, err, tt.errMsg)
105+
return
106+
}
107+
assert.NoError(t, err)
108+
assert.Equal(t, tt.want, got)
109+
})
110+
}
111+
}

submitqueue/entity/BUILD.bazel

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ go_library(
1111
"change_provider.go",
1212
"change_record.go",
1313
"conflict.go",
14-
"land_request.go",
14+
"land.go",
1515
"merge_result.go",
1616
"push_result.go",
1717
"queue.go",
@@ -36,7 +36,7 @@ go_test(
3636
"batch_test.go",
3737
"build_test.go",
3838
"cancel_request_test.go",
39-
"land_request_test.go",
39+
"land_test.go",
4040
"queue_test.go",
4141
"request_log_test.go",
4242
"request_test.go",
Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,3 +46,12 @@ func LandRequestFromBytes(data []byte) (LandRequest, error) {
4646
err := json.Unmarshal(data, &req)
4747
return req, err
4848
}
49+
50+
// LandResult is the outcome of accepting a land request. It carries the ID the
51+
// controller assigned to the request so the transport layer can echo it back to
52+
// the caller.
53+
type LandResult struct {
54+
// ID is the globally unique identifier assigned to the accepted land request.
55+
// Format: "<queue>/<counter_value>".
56+
ID string
57+
}

submitqueue/gateway/controller/BUILD.bazel

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,7 @@ go_library(
1111
importpath = "github.com/uber/submitqueue/submitqueue/gateway/controller",
1212
visibility = ["//visibility:public"],
1313
deps = [
14-
"//api/base/mergestrategy/protopb:go_default_library",
1514
"//api/submitqueue/gateway/protopb:go_default_library",
16-
"//platform/base/change:go_default_library",
17-
"//platform/base/mergestrategy:go_default_library",
1815
"//platform/base/messagequeue:go_default_library",
1916
"//platform/consumer:go_default_library",
2017
"//platform/errs:go_default_library",
@@ -40,9 +37,8 @@ go_test(
4037
],
4138
embed = [":go_default_library"],
4239
deps = [
43-
"//api/base/change/protopb:go_default_library",
44-
"//api/base/mergestrategy/protopb:go_default_library",
4540
"//api/submitqueue/gateway/protopb:go_default_library",
41+
"//platform/base/change:go_default_library",
4642
"//platform/base/mergestrategy:go_default_library",
4743
"//platform/base/messagequeue:go_default_library",
4844
"//platform/consumer:go_default_library",

0 commit comments

Comments
 (0)