From 8abf961ca5a07894f3a8149169b987ccb1aa9fd0 Mon Sep 17 00:00:00 2001 From: Justin Won Date: Thu, 8 Oct 2026 19:36:43 -0700 Subject: [PATCH] refactor(handler): move error conversion to the handler The handler now owns the failure log and the wire error conversion. Each handler method uses a named retErr and one defer. If retErr is not nil, the defer logs " failed" with tangoerrors.Fields and converts the error with toWireError. The controller no longer logs failures and no longer converts errors. It returns the classified error. The metrics lifecycle and the repository resolver stay in the controller, so the metrics are the same as on main. handler.Params gets a Logger field. The server and the integration test pass it. Test plan: - make build, make test, make lint - handler wire error tests run the real controller through the handler Co-Authored-By: Claude Sonnet 5.5 --- controller/BUILD.bazel | 4 ---- controller/getchangedtargets.go | 4 ---- controller/gettargetgraph.go | 4 ---- example/main.go | 2 +- handler/BUILD.bazel | 15 ++++++++++++ {controller => handler}/errors.go | 2 +- handler/getchangedtargets.go | 9 +++++++- handler/getchangedtargets_test.go | 3 ++- handler/gettargetgraph.go | 9 +++++++- handler/gettargetgraph_test.go | 3 ++- handler/handler.go | 5 +++- handler/testhelper_test.go | 28 +++++++++++++++++++++++ {controller => handler}/wireerror_test.go | 15 ++++++------ integration/integration_test.go | 2 +- 14 files changed, 78 insertions(+), 27 deletions(-) rename {controller => handler}/errors.go (98%) create mode 100644 handler/testhelper_test.go rename {controller => handler}/wireerror_test.go (92%) diff --git a/controller/BUILD.bazel b/controller/BUILD.bazel index 4e1485f6..0f3c0fee 100644 --- a/controller/BUILD.bazel +++ b/controller/BUILD.bazel @@ -5,7 +5,6 @@ go_library( srcs = [ "controller.go", "distance_filter.go", - "errors.go", "getchangedtargetgraph.go", "getchangedtargets.go", "gettargetgraph.go", @@ -48,7 +47,6 @@ go_test( "metrics_test.go", "output_filter_test.go", "testhelper_test.go", - "wireerror_test.go", ], embed = [":controller"], deps = [ @@ -66,8 +64,6 @@ go_test( "@com_github_stretchr_testify//require", "@com_github_uber_go_tally//:tally", "@org_uber_go_mock//gomock", - "@org_uber_go_yarpc//encoding/protobuf", - "@org_uber_go_yarpc//yarpcerrors", "@org_uber_go_zap//:zap", "@org_uber_go_zap//zaptest", ], diff --git a/controller/getchangedtargets.go b/controller/getchangedtargets.go index 2dd1b5f0..941565eb 100644 --- a/controller/getchangedtargets.go +++ b/controller/getchangedtargets.go @@ -95,10 +95,6 @@ func (c *controller) GetChangedTargets(request *pb.GetChangedTargetsRequest, str ) defer func() { op.Complete(retErr) - if retErr != nil { - logger.Error("GetChangedTargets failed", tangoerrors.Fields(retErr)...) - retErr = toWireError(retErr) - } }() if repositoryErr != nil { return repositoryErr diff --git a/controller/gettargetgraph.go b/controller/gettargetgraph.go index 82e61790..621fd95f 100644 --- a/controller/gettargetgraph.go +++ b/controller/gettargetgraph.go @@ -47,10 +47,6 @@ func (c *controller) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb ) defer func() { op.Complete(retErr) - if retErr != nil { - logger.Error("GetTargetGraph failed", tangoerrors.Fields(retErr)...) - retErr = toWireError(retErr) - } }() start := time.Now() ctx, cancelLink := c.linkRequestCtx(stream.Context()) diff --git a/example/main.go b/example/main.go index d5352278..449a7b1a 100644 --- a/example/main.go +++ b/example/main.go @@ -109,7 +109,7 @@ func run() error { GraphConfig: cfg, }) - h := handler.New(handler.Params{Controller: ctrl}) + h := handler.New(handler.Params{Logger: logger, Controller: ctrl}) // YARPC transports and dispatcher grpcTransport := yarpcgrpc.NewTransport() diff --git a/handler/BUILD.bazel b/handler/BUILD.bazel index ab00eabc..e049736e 100644 --- a/handler/BUILD.bazel +++ b/handler/BUILD.bazel @@ -3,6 +3,7 @@ load("@rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "handler", srcs = [ + "errors.go", "getchangedtargetgraph.go", "getchangedtargets.go", "gettargetgraph.go", @@ -12,7 +13,10 @@ go_library( visibility = ["//visibility:public"], deps = [ "//controller", + "//core/errors", + "//mapper/proto", "//tangopb", + "@org_uber_go_zap//:zap", ], ) @@ -22,13 +26,24 @@ go_test( "getchangedtargetgraph_test.go", "getchangedtargets_test.go", "gettargetgraph_test.go", + "testhelper_test.go", + "wireerror_test.go", ], embed = [":handler"], deps = [ + "//config", + "//controller", "//controller/controllermock", + "//core/errors", + "//core/storage", + "//core/storage/storagemock", "//tangopb", + "//tangopb/tangopbmock", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", "@org_uber_go_mock//gomock", + "@org_uber_go_yarpc//encoding/protobuf", + "@org_uber_go_yarpc//yarpcerrors", + "@org_uber_go_zap//zaptest", ], ) diff --git a/controller/errors.go b/handler/errors.go similarity index 98% rename from controller/errors.go rename to handler/errors.go index 3a3d6745..73a310b8 100644 --- a/controller/errors.go +++ b/handler/errors.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package controller +package handler import ( "github.com/uber/tango/mapper/proto" diff --git a/handler/getchangedtargets.go b/handler/getchangedtargets.go index 3d7cea3a..a7a064ad 100644 --- a/handler/getchangedtargets.go +++ b/handler/getchangedtargets.go @@ -15,9 +15,16 @@ package handler import ( + tangoerrors "github.com/uber/tango/core/errors" pb "github.com/uber/tango/tangopb" ) -func (h *handler) GetChangedTargets(request *pb.GetChangedTargetsRequest, stream pb.TangoServiceGetChangedTargetsYARPCServer) error { +func (h *handler) GetChangedTargets(request *pb.GetChangedTargetsRequest, stream pb.TangoServiceGetChangedTargetsYARPCServer) (retErr error) { + defer func() { + if retErr != nil { + h.logger.Error("GetChangedTargets failed", tangoerrors.Fields(retErr)...) + retErr = toWireError(retErr) + } + }() return h.controller.GetChangedTargets(request, stream) } diff --git a/handler/getchangedtargets_test.go b/handler/getchangedtargets_test.go index 3b841c94..f12b3811 100644 --- a/handler/getchangedtargets_test.go +++ b/handler/getchangedtargets_test.go @@ -22,6 +22,7 @@ import ( mock_controller "github.com/uber/tango/controller/controllermock" pb "github.com/uber/tango/tangopb" "go.uber.org/mock/gomock" + "go.uber.org/zap/zaptest" ) type fakeGetChangedTargetsStream struct { @@ -30,7 +31,7 @@ type fakeGetChangedTargetsStream struct { func TestGetChangedTargetsForwardsToController(t *testing.T) { ctrl := mock_controller.NewMockController(gomock.NewController(t)) - h := New(Params{Controller: ctrl}) + h := New(Params{Logger: zaptest.NewLogger(t), Controller: ctrl}) req := &pb.GetChangedTargetsRequest{} stream := &fakeGetChangedTargetsStream{} diff --git a/handler/gettargetgraph.go b/handler/gettargetgraph.go index 6c1a19db..c0b1c4e4 100644 --- a/handler/gettargetgraph.go +++ b/handler/gettargetgraph.go @@ -15,9 +15,16 @@ package handler import ( + tangoerrors "github.com/uber/tango/core/errors" pb "github.com/uber/tango/tangopb" ) -func (h *handler) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb.TangoServiceGetTargetGraphYARPCServer) error { +func (h *handler) GetTargetGraph(request *pb.GetTargetGraphRequest, stream pb.TangoServiceGetTargetGraphYARPCServer) (retErr error) { + defer func() { + if retErr != nil { + h.logger.Error("GetTargetGraph failed", tangoerrors.Fields(retErr)...) + retErr = toWireError(retErr) + } + }() return h.controller.GetTargetGraph(request, stream) } diff --git a/handler/gettargetgraph_test.go b/handler/gettargetgraph_test.go index e502aed6..58da7e74 100644 --- a/handler/gettargetgraph_test.go +++ b/handler/gettargetgraph_test.go @@ -22,6 +22,7 @@ import ( mock_controller "github.com/uber/tango/controller/controllermock" pb "github.com/uber/tango/tangopb" "go.uber.org/mock/gomock" + "go.uber.org/zap/zaptest" ) type fakeGetTargetGraphStream struct { @@ -30,7 +31,7 @@ type fakeGetTargetGraphStream struct { func TestGetTargetGraphForwardsToController(t *testing.T) { ctrl := mock_controller.NewMockController(gomock.NewController(t)) - h := New(Params{Controller: ctrl}) + h := New(Params{Logger: zaptest.NewLogger(t), Controller: ctrl}) req := &pb.GetTargetGraphRequest{} stream := &fakeGetTargetGraphStream{} diff --git a/handler/handler.go b/handler/handler.go index aff260b7..2c0f5396 100644 --- a/handler/handler.go +++ b/handler/handler.go @@ -17,16 +17,19 @@ package handler import ( "github.com/uber/tango/controller" pb "github.com/uber/tango/tangopb" + "go.uber.org/zap" ) type Params struct { + Logger *zap.Logger Controller controller.Controller } type handler struct { + logger *zap.Logger controller controller.Controller } func New(p Params) pb.TangoYARPCServer { - return &handler{controller: p.Controller} + return &handler{logger: p.Logger, controller: p.Controller} } diff --git a/handler/testhelper_test.go b/handler/testhelper_test.go new file mode 100644 index 00000000..0722e615 --- /dev/null +++ b/handler/testhelper_test.go @@ -0,0 +1,28 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package handler + +import "github.com/uber/tango/config" + +// allowAnyRepositoryConfigProvider resolves every remote to a fixed +// repository, mirroring the stub of the same name in controller's tests. +type allowAnyRepositoryConfigProvider struct{} + +func (allowAnyRepositoryConfigProvider) GetRepositoryConfig(remote string) (config.RepositoryConfig, bool) { + return config.RepositoryConfig{ + Remote: remote, + RepositoryID: "test-repository", + }, true +} diff --git a/controller/wireerror_test.go b/handler/wireerror_test.go similarity index 92% rename from controller/wireerror_test.go rename to handler/wireerror_test.go index ae42f33c..7569df55 100644 --- a/controller/wireerror_test.go +++ b/handler/wireerror_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package controller +package handler import ( "context" @@ -21,6 +21,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/uber/tango/controller" tangoerrors "github.com/uber/tango/core/errors" "github.com/uber/tango/core/storage" storagemock "github.com/uber/tango/core/storage/storagemock" @@ -88,10 +89,10 @@ func TestGetTargetGraph_ValidationError_WiresTangoError(t *testing.T) { stream := tangomock.NewMockTangoServiceGetTargetGraphYARPCServer(ctrl) stream.EXPECT().Context().Return(context.Background()) - c := NewController(context.Background(), Params{ + c := New(Params{Logger: zaptest.NewLogger(t), Controller: controller.NewController(context.Background(), controller.Params{ RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), - }) + })}) // Missing BaseSha triggers a validation error classified as ERROR_USER. err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ @@ -117,10 +118,10 @@ func TestGetChangedTargets_ValidationError_WiresTangoError(t *testing.T) { stream := tangomock.NewMockTangoServiceGetChangedTargetsYARPCServer(ctrl) stream.EXPECT().Context().Return(context.Background()).AnyTimes() - c := NewController(context.Background(), Params{ + c := New(Params{Logger: zaptest.NewLogger(t), Controller: controller.NewController(context.Background(), controller.Params{ RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), - }) + })}) // Missing first revision triggers validation error classified as ERROR_USER. err := c.GetChangedTargets(&pb.GetChangedTargetsRequest{ @@ -146,11 +147,11 @@ func TestGetTargetGraph_InfraError_WiresTangoError(t *testing.T) { store := storagemock.NewMockStorage(ctrl) store.EXPECT().Get(gomock.Any(), gomock.Any()).Return(storage.DownloadResponse{}, errors.New("disk on fire")) - c := NewController(context.Background(), Params{ + c := New(Params{Logger: zaptest.NewLogger(t), Controller: controller.NewController(context.Background(), controller.Params{ RepoConfig: allowAnyRepositoryConfigProvider{}, Logger: zaptest.NewLogger(t), Storage: store, - }) + })}) err := c.GetTargetGraph(&pb.GetTargetGraphRequest{ BuildDescription: &pb.BuildDescription{ diff --git a/integration/integration_test.go b/integration/integration_test.go index d8daf620..05afcf99 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -156,7 +156,7 @@ func startServerWithLogger(t testing.TB, remote string, zl *zap.Logger) string { GraphConfig: cfg, }) - h := handler.New(handler.Params{Controller: ctrl}) + h := handler.New(handler.Params{Logger: zl, Controller: ctrl}) grpcTransport := yarpcgrpc.NewTransport() listener, err := net.Listen("tcp", "127.0.0.1:0")