From 48c8d749105d888a22129a1f24139c79e1c1f255 Mon Sep 17 00:00:00 2001 From: Adam Bettigole Date: Thu, 2 Jul 2026 16:46:56 -0700 Subject: [PATCH 1/2] feat(orchestrator): add pluggable validator extension for custom validation checks Add a validator extension that allows integrators to inject company-specific validation logic into the validate controller. The extension follows the existing Factory/Config pattern and includes a Compose utility for combining multiple validators. Co-Authored-By: Claude Opus 4.6 (1M context) --- Makefile | 2 +- .../orchestrator/server/BUILD.bazel | 1 + .../submitqueue/orchestrator/server/main.go | 18 +++ submitqueue/extension/validator/BUILD.bazel | 28 +++++ submitqueue/extension/validator/compose.go | 39 +++++++ .../extension/validator/compose_test.go | 106 ++++++++++++++++++ .../extension/validator/mock/BUILD.bazel | 13 +++ .../validator/mock/validator_mock.go | 96 ++++++++++++++++ submitqueue/extension/validator/validator.go | 52 +++++++++ .../controller/validate/BUILD.bazel | 3 + .../controller/validate/validate.go | 24 ++++ .../controller/validate/validate_test.go | 100 ++++++++++++++++- 12 files changed, 479 insertions(+), 3 deletions(-) create mode 100644 submitqueue/extension/validator/BUILD.bazel create mode 100644 submitqueue/extension/validator/compose.go create mode 100644 submitqueue/extension/validator/compose_test.go create mode 100644 submitqueue/extension/validator/mock/BUILD.bazel create mode 100644 submitqueue/extension/validator/mock/validator_mock.go create mode 100644 submitqueue/extension/validator/validator.go diff --git a/Makefile b/Makefile index 5aeff768..a0178520 100644 --- a/Makefile +++ b/Makefile @@ -364,7 +364,7 @@ local-stovepipe-stop: ## Stop the Stovepipe service mocks: ## Generate mock files using mockgen @echo "Generating mocks..." - @$(BAZEL) run @rules_go//go -- generate ./submitqueue/extension/storage/... ./submitqueue/extension/buildrunner/... ./submitqueue/extension/changeprovider/... ./platform/extension/counter/... ./platform/extension/messagequeue/... ./submitqueue/extension/queueconfig/... ./submitqueue/extension/mergechecker/... ./submitqueue/extension/pusher/... ./submitqueue/extension/scorer/... ./submitqueue/extension/conflict/... ./platform/consumer/... ./stovepipe/extension/storage/... ./stovepipe/extension/sourcecontrol/... + @$(BAZEL) run @rules_go//go -- generate ./submitqueue/extension/storage/... ./submitqueue/extension/buildrunner/... ./submitqueue/extension/changeprovider/... ./platform/extension/counter/... ./platform/extension/messagequeue/... ./submitqueue/extension/queueconfig/... ./submitqueue/extension/mergechecker/... ./submitqueue/extension/pusher/... ./submitqueue/extension/scorer/... ./submitqueue/extension/conflict/... ./submitqueue/extension/validator/... ./platform/consumer/... ./stovepipe/extension/storage/... ./stovepipe/extension/sourcecontrol/... @echo "Mocks generated successfully!" proto: ## Generate protobuf files from .proto definitions diff --git a/service/submitqueue/orchestrator/server/BUILD.bazel b/service/submitqueue/orchestrator/server/BUILD.bazel index 8cf638a2..6ab7fabb 100644 --- a/service/submitqueue/orchestrator/server/BUILD.bazel +++ b/service/submitqueue/orchestrator/server/BUILD.bazel @@ -43,6 +43,7 @@ go_library( "//submitqueue/extension/scorer/heuristic:go_default_library", "//submitqueue/extension/storage:go_default_library", "//submitqueue/extension/storage/mysql:go_default_library", + "//submitqueue/extension/validator:go_default_library", "//submitqueue/orchestrator/controller:go_default_library", "//submitqueue/orchestrator/controller/batch:go_default_library", "//submitqueue/orchestrator/controller/build:go_default_library", diff --git a/service/submitqueue/orchestrator/server/main.go b/service/submitqueue/orchestrator/server/main.go index 2ed845c3..ae58aeed 100644 --- a/service/submitqueue/orchestrator/server/main.go +++ b/service/submitqueue/orchestrator/server/main.go @@ -63,6 +63,7 @@ import ( "github.com/uber/submitqueue/submitqueue/extension/scorer/heuristic" "github.com/uber/submitqueue/submitqueue/extension/storage" mysqlstorage "github.com/uber/submitqueue/submitqueue/extension/storage/mysql" + "github.com/uber/submitqueue/submitqueue/extension/validator" "github.com/uber/submitqueue/submitqueue/orchestrator/controller" "github.com/uber/submitqueue/submitqueue/orchestrator/controller/batch" "github.com/uber/submitqueue/submitqueue/orchestrator/controller/build" @@ -537,6 +538,22 @@ func (f analyzerFactory) For(cfg conflict.Config) (conflict.Analyzer, error) { return f.reg.get(cfg.QueueName).analyzer, nil } +// noopValidatorFactory is an example validator.Factory that always returns a no-op validator (validation passes). +// Replace with a real implementation to enforce company-specific policies (e.g. org-policy, repo-level rules). +type noopValidatorFactory struct{} + +// For returns a no-op validator (validation passes). +// In a real implementation, it could choose which validator(s) to return, based on the specific queue or change type. +func (noopValidatorFactory) For(validator.Config) (validator.Validator, error) { + return noopValidator{}, nil +} + +type noopValidator struct{} + +func (noopValidator) Validate(context.Context, entity.Request, []entity.ChangeInfo) error { + return nil +} + func registerPrimaryControllers(c consumer.Consumer, logger *zap.SugaredLogger, scope tally.Scope, registry consumer.TopicRegistry, cpf changeprovider.Factory, brf buildrunner.Factory, scf scorer.Factory, cof conflict.Factory, cnt counter.Counter, store storage.Storage) (int, error) { var count int requestController := start.NewController( @@ -571,6 +588,7 @@ func registerPrimaryControllers(c consumer.Consumer, logger *zap.SugaredLogger, store, registry, cpf, + noopValidatorFactory{}, runwaymq.TopicKeyMergeConflictCheck, topickey.TopicKeyValidate, "orchestrator-validate", diff --git a/submitqueue/extension/validator/BUILD.bazel b/submitqueue/extension/validator/BUILD.bazel new file mode 100644 index 00000000..1222011d --- /dev/null +++ b/submitqueue/extension/validator/BUILD.bazel @@ -0,0 +1,28 @@ +load("@rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = [ + "compose.go", + "validator.go", + ], + importpath = "github.com/uber/submitqueue/submitqueue/extension/validator", + visibility = ["//visibility:public"], + deps = [ + "//platform/base/change:go_default_library", + "//submitqueue/entity:go_default_library", + ], +) + +go_test( + name = "go_default_test", + srcs = ["compose_test.go"], + deps = [ + ":go_default_library", + "//submitqueue/entity:go_default_library", + "//submitqueue/extension/validator/mock:go_default_library", + "@com_github_stretchr_testify//assert:go_default_library", + "@com_github_stretchr_testify//require:go_default_library", + "@org_uber_go_mock//gomock:go_default_library", + ], +) diff --git a/submitqueue/extension/validator/compose.go b/submitqueue/extension/validator/compose.go new file mode 100644 index 00000000..a5d5ce54 --- /dev/null +++ b/submitqueue/extension/validator/compose.go @@ -0,0 +1,39 @@ +// Copyright (c) 2025 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 validator + +import ( + "context" + "errors" + + "github.com/uber/submitqueue/submitqueue/entity" +) + +// Compose returns a Validator that runs all provided validators and joins their errors. +func Compose(validators []Validator) Validator { + return compositeValidator(validators) +} + +type compositeValidator []Validator + +func (c compositeValidator) Validate(ctx context.Context, request entity.Request, changes []entity.ChangeInfo) error { + var errs []error + for _, v := range c { + if err := v.Validate(ctx, request, changes); err != nil { + errs = append(errs, err) + } + } + return errors.Join(errs...) +} diff --git a/submitqueue/extension/validator/compose_test.go b/submitqueue/extension/validator/compose_test.go new file mode 100644 index 00000000..ee733366 --- /dev/null +++ b/submitqueue/extension/validator/compose_test.go @@ -0,0 +1,106 @@ +// Copyright (c) 2025 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 validator_test + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/uber/submitqueue/submitqueue/entity" + "github.com/uber/submitqueue/submitqueue/extension/validator" + validatormock "github.com/uber/submitqueue/submitqueue/extension/validator/mock" + "go.uber.org/mock/gomock" +) + +func TestCompose(t *testing.T) { + errA := errors.New("validation A failed") + errB := errors.New("validation B failed") + + tests := []struct { + name string + setup func(v1, v2 *validatormock.MockValidator) []validator.Validator + wantErrs []error + }{ + { + name: "no validators returns no error", + setup: func(v1, v2 *validatormock.MockValidator) []validator.Validator { return nil }, + }, + { + name: "single passing validator", + setup: func(v1, v2 *validatormock.MockValidator) []validator.Validator { + v1.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + return []validator.Validator{v1} + }, + }, + { + name: "single failing validator", + setup: func(v1, v2 *validatormock.MockValidator) []validator.Validator { + v1.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(errA) + return []validator.Validator{v1} + }, + wantErrs: []error{errA}, + }, + { + name: "all pass", + setup: func(v1, v2 *validatormock.MockValidator) []validator.Validator { + v1.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + v2.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + return []validator.Validator{v1, v2} + }, + }, + { + name: "some fail", + setup: func(v1, v2 *validatormock.MockValidator) []validator.Validator { + v1.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + v2.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(errA) + return []validator.Validator{v1, v2} + }, + wantErrs: []error{errA}, + }, + { + name: "all fail", + setup: func(v1, v2 *validatormock.MockValidator) []validator.Validator { + v1.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(errA) + v2.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(errB) + return []validator.Validator{v1, v2} + }, + wantErrs: []error{errA, errB}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + v1 := validatormock.NewMockValidator(ctrl) + v2 := validatormock.NewMockValidator(ctrl) + validators := tt.setup(v1, v2) + + v := validator.Compose(validators) + + err := v.Validate(context.Background(), entity.Request{}, nil) + if len(tt.wantErrs) == 0 { + assert.NoError(t, err) + return + } + require.Error(t, err) + for _, want := range tt.wantErrs { + assert.ErrorIs(t, err, want) + } + }) + } +} diff --git a/submitqueue/extension/validator/mock/BUILD.bazel b/submitqueue/extension/validator/mock/BUILD.bazel new file mode 100644 index 00000000..e07ef68e --- /dev/null +++ b/submitqueue/extension/validator/mock/BUILD.bazel @@ -0,0 +1,13 @@ +load("@rules_go//go:def.bzl", "go_library") + +go_library( + name = "go_default_library", + srcs = ["validator_mock.go"], + importpath = "github.com/uber/submitqueue/submitqueue/extension/validator/mock", + visibility = ["//visibility:public"], + deps = [ + "//submitqueue/entity:go_default_library", + "//submitqueue/extension/validator:go_default_library", + "@org_uber_go_mock//gomock:go_default_library", + ], +) diff --git a/submitqueue/extension/validator/mock/validator_mock.go b/submitqueue/extension/validator/mock/validator_mock.go new file mode 100644 index 00000000..1eda0f03 --- /dev/null +++ b/submitqueue/extension/validator/mock/validator_mock.go @@ -0,0 +1,96 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: validator.go +// +// Generated by this command: +// +// mockgen -source=validator.go -destination=mock/validator_mock.go -package=mock +// + +// Package mock is a generated GoMock package. +package mock + +import ( + context "context" + reflect "reflect" + + entity "github.com/uber/submitqueue/submitqueue/entity" + validator "github.com/uber/submitqueue/submitqueue/extension/validator" + gomock "go.uber.org/mock/gomock" +) + +// MockValidator is a mock of Validator interface. +type MockValidator struct { + ctrl *gomock.Controller + recorder *MockValidatorMockRecorder + isgomock struct{} +} + +// MockValidatorMockRecorder is the mock recorder for MockValidator. +type MockValidatorMockRecorder struct { + mock *MockValidator +} + +// NewMockValidator creates a new mock instance. +func NewMockValidator(ctrl *gomock.Controller) *MockValidator { + mock := &MockValidator{ctrl: ctrl} + mock.recorder = &MockValidatorMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockValidator) EXPECT() *MockValidatorMockRecorder { + return m.recorder +} + +// Validate mocks base method. +func (m *MockValidator) Validate(ctx context.Context, request entity.Request, changes []entity.ChangeInfo) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Validate", ctx, request, changes) + ret0, _ := ret[0].(error) + return ret0 +} + +// Validate indicates an expected call of Validate. +func (mr *MockValidatorMockRecorder) Validate(ctx, request, changes any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Validate", reflect.TypeOf((*MockValidator)(nil).Validate), ctx, request, changes) +} + +// MockFactory is a mock of Factory interface. +type MockFactory struct { + ctrl *gomock.Controller + recorder *MockFactoryMockRecorder + isgomock struct{} +} + +// MockFactoryMockRecorder is the mock recorder for MockFactory. +type MockFactoryMockRecorder struct { + mock *MockFactory +} + +// NewMockFactory creates a new mock instance. +func NewMockFactory(ctrl *gomock.Controller) *MockFactory { + mock := &MockFactory{ctrl: ctrl} + mock.recorder = &MockFactoryMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockFactory) EXPECT() *MockFactoryMockRecorder { + return m.recorder +} + +// For mocks base method. +func (m *MockFactory) For(cfg validator.Config) (validator.Validator, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "For", cfg) + ret0, _ := ret[0].(validator.Validator) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// For indicates an expected call of For. +func (mr *MockFactoryMockRecorder) For(cfg any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "For", reflect.TypeOf((*MockFactory)(nil).For), cfg) +} diff --git a/submitqueue/extension/validator/validator.go b/submitqueue/extension/validator/validator.go new file mode 100644 index 00000000..8d03f633 --- /dev/null +++ b/submitqueue/extension/validator/validator.go @@ -0,0 +1,52 @@ +// Copyright (c) 2025 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 validator + +//go:generate mockgen -source=validator.go -destination=mock/validator_mock.go -package=mock + +import ( + "context" + + "github.com/uber/submitqueue/platform/base/change" + "github.com/uber/submitqueue/submitqueue/entity" +) + +// Validator runs custom validation checks against a request and its changes. +// Implementations are provided by integrators for company-specific checks +// (e.g. org-policy enforcement, repo-specific rules). The built-in validate +// controller invokes this after fetching change metadata and before claiming +// URIs in the change store. +type Validator interface { + // Validate runs custom validation on the request and its fetched change metadata. + // Returns nil if validation passes, error if the request should be rejected. + Validate(ctx context.Context, request entity.Request, changes []entity.ChangeInfo) error +} + +// Config carries the routing identity handed to a Factory. +type Config struct { + // QueueName identifies the queue this request belongs to. + QueueName string + // Change is the code change being validated, carrying the full set of URIs. + // The URI scheme identifies the change provider (e.g. "github", "phabricator"). + Change change.Change +} + +// Factory builds the Validator for a given config. Implementations are provided +// by integrators and inject whatever they need at construction. +type Factory interface { + // For returns the Validator for the given config. Returning nil indicates + // no custom validation is needed for this config. + For(cfg Config) (Validator, error) +} diff --git a/submitqueue/orchestrator/controller/validate/BUILD.bazel b/submitqueue/orchestrator/controller/validate/BUILD.bazel index 7c012139..0b0beff7 100644 --- a/submitqueue/orchestrator/controller/validate/BUILD.bazel +++ b/submitqueue/orchestrator/controller/validate/BUILD.bazel @@ -17,6 +17,7 @@ go_library( "//submitqueue/entity:go_default_library", "//submitqueue/extension/changeprovider:go_default_library", "//submitqueue/extension/storage:go_default_library", + "//submitqueue/extension/validator:go_default_library", "@com_github_uber_go_tally//:go_default_library", "@org_uber_go_zap//:go_default_library", ], @@ -40,6 +41,8 @@ go_test( "//submitqueue/extension/changeprovider/mock:go_default_library", "//submitqueue/extension/storage:go_default_library", "//submitqueue/extension/storage/mock:go_default_library", + "//submitqueue/extension/validator:go_default_library", + "//submitqueue/extension/validator/mock:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", "@com_github_uber_go_tally//:go_default_library", diff --git a/submitqueue/orchestrator/controller/validate/validate.go b/submitqueue/orchestrator/controller/validate/validate.go index 222cdac3..b04cc677 100644 --- a/submitqueue/orchestrator/controller/validate/validate.go +++ b/submitqueue/orchestrator/controller/validate/validate.go @@ -32,6 +32,7 @@ import ( "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/changeprovider" "github.com/uber/submitqueue/submitqueue/extension/storage" + "github.com/uber/submitqueue/submitqueue/extension/validator" "go.uber.org/zap" ) @@ -46,6 +47,7 @@ type Controller struct { store storage.Storage registry consumer.TopicRegistry changeProviders changeprovider.Factory + validators validator.Factory runwayTopicKey consumer.TopicKey topicKey consumer.TopicKey consumerGroup string @@ -57,12 +59,14 @@ var _ consumer.Controller = (*Controller)(nil) // NewController creates a new validate controller for the orchestrator. // runwayTopicKey is the runway-owned topic the merge-conflict check request is // published to (TopicKeyMergeConflictCheck). +// validators is an optional factory for custom validation checks; pass nil to skip. func NewController( logger *zap.SugaredLogger, scope tally.Scope, store storage.Storage, registry consumer.TopicRegistry, changeProviders changeprovider.Factory, + validators validator.Factory, runwayTopicKey consumer.TopicKey, topicKey consumer.TopicKey, consumerGroup string, @@ -73,6 +77,7 @@ func NewController( store: store, registry: registry, changeProviders: changeProviders, + validators: validators, runwayTopicKey: runwayTopicKey, topicKey: topicKey, consumerGroup: consumerGroup, @@ -159,6 +164,25 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r "total_files", totalFiles(changeInfos), ) + // Run custom validation checks if a validator factory was provided. + if c.validators != nil { + cfg := validator.Config{ + QueueName: request.Queue, + Change: request.Change, + } + v, err := c.validators.For(cfg) + if err != nil { + coremetrics.NamedCounter(c.metricsScope, "process", "validator_errors", 1) + return fmt.Errorf("failed to build validator for request %s: %w", request.ID, err) + } + if v != nil { + if err := v.Validate(ctx, request, changeInfos); err != nil { + coremetrics.NamedCounter(c.metricsScope, "process", "custom_validation_failures", 1) + return fmt.Errorf("custom validation failed for request %s: %w", request.ID, err) + } + } + } + // Claim each URI in the change store with its provider details. The claim is // created here — after duplicate detection and the merge/provider checks — so a // rejected request never leaves a claim, and the record is written once with its diff --git a/submitqueue/orchestrator/controller/validate/validate_test.go b/submitqueue/orchestrator/controller/validate/validate_test.go index 8f0c6fbc..a3fe5613 100644 --- a/submitqueue/orchestrator/controller/validate/validate_test.go +++ b/submitqueue/orchestrator/controller/validate/validate_test.go @@ -35,6 +35,8 @@ import ( changeprovidermock "github.com/uber/submitqueue/submitqueue/extension/changeprovider/mock" "github.com/uber/submitqueue/submitqueue/extension/storage" storagemock "github.com/uber/submitqueue/submitqueue/extension/storage/mock" + "github.com/uber/submitqueue/submitqueue/extension/validator" + validatormock "github.com/uber/submitqueue/submitqueue/extension/validator/mock" "go.uber.org/mock/gomock" "go.uber.org/zap/zaptest" ) @@ -119,7 +121,7 @@ func newTestController( cpFactory := changeprovidermock.NewMockFactory(ctrl) cpFactory.EXPECT().For(gomock.Any()).Return(cp, nil).AnyTimes() - return NewController(logger, scope, store, registry, cpFactory, runwaymq.TopicKeyMergeConflictCheck, topickey.TopicKeyValidate, "orchestrator-validate") + return NewController(logger, scope, store, registry, cpFactory, nil, runwaymq.TopicKeyMergeConflictCheck, topickey.TopicKeyValidate, "orchestrator-validate") } func TestNewController(t *testing.T) { @@ -201,7 +203,7 @@ func TestController_Process_PublishesCheckToRunway(t *testing.T) { cpFactory := changeprovidermock.NewMockFactory(ctrl) cpFactory.EXPECT().For(gomock.Any()).Return(&mockChangeProvider{}, nil).AnyTimes() - controller := NewController(logger, tally.NoopScope, store, registry, cpFactory, runwaymq.TopicKeyMergeConflictCheck, topickey.TopicKeyValidate, "orchestrator-validate") + controller := NewController(logger, tally.NoopScope, store, registry, cpFactory, nil, runwaymq.TopicKeyMergeConflictCheck, topickey.TopicKeyValidate, "orchestrator-validate") msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) delivery := queuemock.NewMockDelivery(ctrl) @@ -563,3 +565,97 @@ func TestController_Process_TerminalShortCircuit(t *testing.T) { }) } } + +func TestController_Process_CustomValidatorPasses(t *testing.T) { + ctrl := gomock.NewController(t) + + request := entity.Request{ + ID: "test-queue/123", + Queue: "test-queue", + Change: change.Change{URIs: []string{"github://uber/service/pull/456/abcdef0123456789abcdef0123456789abcdef01"}}, + LandStrategy: mergestrategy.MergeStrategyRebase, + State: entity.RequestStateStarted, + Version: 1, + } + store, _ := newMockStorage(ctrl, request) + store.EXPECT().GetChangeStore().Return(newMockChangeStore(ctrl)).AnyTimes() + + logger := zaptest.NewLogger(t).Sugar() + + mockPub := queuemock.NewMockPublisher(ctrl) + mockPub.EXPECT().Publish(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + mockQ := queuemock.NewMockQueue(ctrl) + mockQ.EXPECT().Publisher().Return(mockPub).AnyTimes() + registry, err := consumer.NewTopicRegistry( + []consumer.TopicConfig{{Key: runwaymq.TopicKeyMergeConflictCheck, Name: "merge-conflict-check", Queue: mockQ}}, + ) + require.NoError(t, err) + + cpFactory := changeprovidermock.NewMockFactory(ctrl) + cpFactory.EXPECT().For(gomock.Any()).Return(&mockChangeProvider{}, nil).AnyTimes() + + mockValidator := validatormock.NewMockValidator(ctrl) + // mockValidator returns nil - validation succeeded + mockValidator.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + mockValidatorFactory := validatormock.NewMockFactory(ctrl) + mockValidatorFactory.EXPECT().For(validator.Config{ + QueueName: request.Queue, + Change: request.Change, + }).Return(mockValidator, nil) + + controller := NewController(logger, tally.NoopScope, store, registry, cpFactory, mockValidatorFactory, runwaymq.TopicKeyMergeConflictCheck, topickey.TopicKeyValidate, "orchestrator-validate") + + msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) + delivery := queuemock.NewMockDelivery(ctrl) + delivery.EXPECT().Message().Return(msg).AnyTimes() + delivery.EXPECT().Attempt().Return(1).AnyTimes() + + require.NoError(t, controller.Process(context.Background(), delivery)) +} + +func TestController_Process_CustomValidatorFails(t *testing.T) { + ctrl := gomock.NewController(t) + + request := entity.Request{ + ID: "test-queue/123", + Queue: "test-queue", + Change: change.Change{URIs: []string{"github://uber/service/pull/456/abcdef0123456789abcdef0123456789abcdef01"}}, + LandStrategy: mergestrategy.MergeStrategyRebase, + State: entity.RequestStateStarted, + Version: 1, + } + store, _ := newMockStorage(ctrl, request) + store.EXPECT().GetChangeStore().Return(newMockChangeStore(ctrl)).AnyTimes() + + logger := zaptest.NewLogger(t).Sugar() + + mockPub := queuemock.NewMockPublisher(ctrl) + mockQ := queuemock.NewMockQueue(ctrl) + mockQ.EXPECT().Publisher().Return(mockPub).AnyTimes() + registry, err := consumer.NewTopicRegistry( + []consumer.TopicConfig{{Key: runwaymq.TopicKeyMergeConflictCheck, Name: "merge-conflict-check", Queue: mockQ}}, + ) + require.NoError(t, err) + + cpFactory := changeprovidermock.NewMockFactory(ctrl) + cpFactory.EXPECT().For(gomock.Any()).Return(&mockChangeProvider{}, nil).AnyTimes() + + mockValidator := validatormock.NewMockValidator(ctrl) + // mockValidator returns an error - validation failed + mockValidator.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("some validation error")) + mockValidatorFactory := validatormock.NewMockFactory(ctrl) + mockValidatorFactory.EXPECT().For(validator.Config{ + QueueName: request.Queue, + Change: request.Change, + }).Return(mockValidator, nil) + + controller := NewController(logger, tally.NoopScope, store, registry, cpFactory, mockValidatorFactory, runwaymq.TopicKeyMergeConflictCheck, topickey.TopicKeyValidate, "orchestrator-validate") + + msg := entityqueue.NewMessage(request.ID, requestIDPayload(t, request.ID), request.Queue, nil) + delivery := queuemock.NewMockDelivery(ctrl) + delivery.EXPECT().Message().Return(msg).AnyTimes() + delivery.EXPECT().Attempt().Return(1).AnyTimes() + + err = controller.Process(context.Background(), delivery) + require.ErrorContains(t, err, "some validation error") +} From 08596b2fc8827aa14059908e26fe3a93c52695d5 Mon Sep 17 00:00:00 2001 From: Adam Bettigole Date: Wed, 8 Jul 2026 12:45:46 -0400 Subject: [PATCH 2/2] addressed comments --- .../orchestrator/server/BUILD.bazel | 2 +- .../submitqueue/orchestrator/server/main.go | 20 +------- submitqueue/extension/validator/BUILD.bazel | 25 ++-------- .../extension/validator/composite/BUILD.bazel | 26 ++++++++++ .../{compose.go => composite/validator.go} | 21 +++++---- .../validator_test.go} | 25 +++++----- .../extension/validator/fake/BUILD.bazel | 23 +++++++++ submitqueue/extension/validator/fake/fake.go | 47 +++++++++++++++++++ .../extension/validator/fake/fake_test.go | 44 +++++++++++++++++ .../validator/mock/validator_mock.go | 8 ++-- submitqueue/extension/validator/validator.go | 10 ++-- .../controller/validate/validate.go | 3 +- .../controller/validate/validate_test.go | 6 +-- 13 files changed, 182 insertions(+), 78 deletions(-) create mode 100644 submitqueue/extension/validator/composite/BUILD.bazel rename submitqueue/extension/validator/{compose.go => composite/validator.go} (54%) rename submitqueue/extension/validator/{compose_test.go => composite/validator_test.go} (76%) create mode 100644 submitqueue/extension/validator/fake/BUILD.bazel create mode 100644 submitqueue/extension/validator/fake/fake.go create mode 100644 submitqueue/extension/validator/fake/fake_test.go diff --git a/service/submitqueue/orchestrator/server/BUILD.bazel b/service/submitqueue/orchestrator/server/BUILD.bazel index 6ab7fabb..3bd852c8 100644 --- a/service/submitqueue/orchestrator/server/BUILD.bazel +++ b/service/submitqueue/orchestrator/server/BUILD.bazel @@ -43,7 +43,7 @@ go_library( "//submitqueue/extension/scorer/heuristic:go_default_library", "//submitqueue/extension/storage:go_default_library", "//submitqueue/extension/storage/mysql:go_default_library", - "//submitqueue/extension/validator:go_default_library", + "//submitqueue/extension/validator/fake:go_default_library", "//submitqueue/orchestrator/controller:go_default_library", "//submitqueue/orchestrator/controller/batch:go_default_library", "//submitqueue/orchestrator/controller/build:go_default_library", diff --git a/service/submitqueue/orchestrator/server/main.go b/service/submitqueue/orchestrator/server/main.go index ae58aeed..2038b2e7 100644 --- a/service/submitqueue/orchestrator/server/main.go +++ b/service/submitqueue/orchestrator/server/main.go @@ -63,7 +63,7 @@ import ( "github.com/uber/submitqueue/submitqueue/extension/scorer/heuristic" "github.com/uber/submitqueue/submitqueue/extension/storage" mysqlstorage "github.com/uber/submitqueue/submitqueue/extension/storage/mysql" - "github.com/uber/submitqueue/submitqueue/extension/validator" + validatorfake "github.com/uber/submitqueue/submitqueue/extension/validator/fake" "github.com/uber/submitqueue/submitqueue/orchestrator/controller" "github.com/uber/submitqueue/submitqueue/orchestrator/controller/batch" "github.com/uber/submitqueue/submitqueue/orchestrator/controller/build" @@ -538,22 +538,6 @@ func (f analyzerFactory) For(cfg conflict.Config) (conflict.Analyzer, error) { return f.reg.get(cfg.QueueName).analyzer, nil } -// noopValidatorFactory is an example validator.Factory that always returns a no-op validator (validation passes). -// Replace with a real implementation to enforce company-specific policies (e.g. org-policy, repo-level rules). -type noopValidatorFactory struct{} - -// For returns a no-op validator (validation passes). -// In a real implementation, it could choose which validator(s) to return, based on the specific queue or change type. -func (noopValidatorFactory) For(validator.Config) (validator.Validator, error) { - return noopValidator{}, nil -} - -type noopValidator struct{} - -func (noopValidator) Validate(context.Context, entity.Request, []entity.ChangeInfo) error { - return nil -} - func registerPrimaryControllers(c consumer.Consumer, logger *zap.SugaredLogger, scope tally.Scope, registry consumer.TopicRegistry, cpf changeprovider.Factory, brf buildrunner.Factory, scf scorer.Factory, cof conflict.Factory, cnt counter.Counter, store storage.Storage) (int, error) { var count int requestController := start.NewController( @@ -588,7 +572,7 @@ func registerPrimaryControllers(c consumer.Consumer, logger *zap.SugaredLogger, store, registry, cpf, - noopValidatorFactory{}, + validatorfake.NewFactory(), runwaymq.TopicKeyMergeConflictCheck, topickey.TopicKeyValidate, "orchestrator-validate", diff --git a/submitqueue/extension/validator/BUILD.bazel b/submitqueue/extension/validator/BUILD.bazel index 1222011d..39c569c5 100644 --- a/submitqueue/extension/validator/BUILD.bazel +++ b/submitqueue/extension/validator/BUILD.bazel @@ -1,28 +1,9 @@ -load("@rules_go//go:def.bzl", "go_library", "go_test") +load("@rules_go//go:def.bzl", "go_library") go_library( name = "go_default_library", - srcs = [ - "compose.go", - "validator.go", - ], + srcs = ["validator.go"], importpath = "github.com/uber/submitqueue/submitqueue/extension/validator", visibility = ["//visibility:public"], - deps = [ - "//platform/base/change:go_default_library", - "//submitqueue/entity:go_default_library", - ], -) - -go_test( - name = "go_default_test", - srcs = ["compose_test.go"], - deps = [ - ":go_default_library", - "//submitqueue/entity:go_default_library", - "//submitqueue/extension/validator/mock:go_default_library", - "@com_github_stretchr_testify//assert:go_default_library", - "@com_github_stretchr_testify//require:go_default_library", - "@org_uber_go_mock//gomock:go_default_library", - ], + deps = ["//submitqueue/entity:go_default_library"], ) diff --git a/submitqueue/extension/validator/composite/BUILD.bazel b/submitqueue/extension/validator/composite/BUILD.bazel new file mode 100644 index 00000000..434491e6 --- /dev/null +++ b/submitqueue/extension/validator/composite/BUILD.bazel @@ -0,0 +1,26 @@ +load("@rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = ["validator.go"], + importpath = "github.com/uber/submitqueue/submitqueue/extension/validator/composite", + visibility = ["//visibility:public"], + deps = [ + "//submitqueue/entity:go_default_library", + "//submitqueue/extension/validator:go_default_library", + ], +) + +go_test( + name = "go_default_test", + srcs = ["validator_test.go"], + deps = [ + ":go_default_library", + "//submitqueue/entity:go_default_library", + "//submitqueue/extension/validator:go_default_library", + "//submitqueue/extension/validator/mock:go_default_library", + "@com_github_stretchr_testify//assert:go_default_library", + "@com_github_stretchr_testify//require:go_default_library", + "@org_uber_go_mock//gomock:go_default_library", + ], +) diff --git a/submitqueue/extension/validator/compose.go b/submitqueue/extension/validator/composite/validator.go similarity index 54% rename from submitqueue/extension/validator/compose.go rename to submitqueue/extension/validator/composite/validator.go index a5d5ce54..5bdca52f 100644 --- a/submitqueue/extension/validator/compose.go +++ b/submitqueue/extension/validator/composite/validator.go @@ -12,26 +12,31 @@ // See the License for the specific language governing permissions and // limitations under the License. -package validator +package composite import ( "context" "errors" "github.com/uber/submitqueue/submitqueue/entity" + "github.com/uber/submitqueue/submitqueue/extension/validator" ) -// Compose returns a Validator that runs all provided validators and joins their errors. -func Compose(validators []Validator) Validator { - return compositeValidator(validators) +// compositeValidator runs all validators and joins their errors. +type compositeValidator struct { + // validators is the set of validators to run. + validators []validator.Validator } -type compositeValidator []Validator +// New creates a Validator that evaluates all child validators and joins their errors. +func New(validators []validator.Validator) validator.Validator { + return &compositeValidator{validators: validators} +} -func (c compositeValidator) Validate(ctx context.Context, request entity.Request, changes []entity.ChangeInfo) error { +func (c *compositeValidator) Validate(ctx context.Context, request entity.Request) error { var errs []error - for _, v := range c { - if err := v.Validate(ctx, request, changes); err != nil { + for _, v := range c.validators { + if err := v.Validate(ctx, request); err != nil { errs = append(errs, err) } } diff --git a/submitqueue/extension/validator/compose_test.go b/submitqueue/extension/validator/composite/validator_test.go similarity index 76% rename from submitqueue/extension/validator/compose_test.go rename to submitqueue/extension/validator/composite/validator_test.go index ee733366..9785e31a 100644 --- a/submitqueue/extension/validator/compose_test.go +++ b/submitqueue/extension/validator/composite/validator_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package validator_test +package composite_test import ( "context" @@ -23,11 +23,12 @@ import ( "github.com/stretchr/testify/require" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/validator" + "github.com/uber/submitqueue/submitqueue/extension/validator/composite" validatormock "github.com/uber/submitqueue/submitqueue/extension/validator/mock" "go.uber.org/mock/gomock" ) -func TestCompose(t *testing.T) { +func TestNew(t *testing.T) { errA := errors.New("validation A failed") errB := errors.New("validation B failed") @@ -43,14 +44,14 @@ func TestCompose(t *testing.T) { { name: "single passing validator", setup: func(v1, v2 *validatormock.MockValidator) []validator.Validator { - v1.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + v1.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) return []validator.Validator{v1} }, }, { name: "single failing validator", setup: func(v1, v2 *validatormock.MockValidator) []validator.Validator { - v1.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(errA) + v1.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(errA) return []validator.Validator{v1} }, wantErrs: []error{errA}, @@ -58,16 +59,16 @@ func TestCompose(t *testing.T) { { name: "all pass", setup: func(v1, v2 *validatormock.MockValidator) []validator.Validator { - v1.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) - v2.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + v1.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) + v2.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) return []validator.Validator{v1, v2} }, }, { name: "some fail", setup: func(v1, v2 *validatormock.MockValidator) []validator.Validator { - v1.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) - v2.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(errA) + v1.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) + v2.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(errA) return []validator.Validator{v1, v2} }, wantErrs: []error{errA}, @@ -75,8 +76,8 @@ func TestCompose(t *testing.T) { { name: "all fail", setup: func(v1, v2 *validatormock.MockValidator) []validator.Validator { - v1.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(errA) - v2.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(errB) + v1.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(errA) + v2.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(errB) return []validator.Validator{v1, v2} }, wantErrs: []error{errA, errB}, @@ -90,9 +91,9 @@ func TestCompose(t *testing.T) { v2 := validatormock.NewMockValidator(ctrl) validators := tt.setup(v1, v2) - v := validator.Compose(validators) + v := composite.New(validators) - err := v.Validate(context.Background(), entity.Request{}, nil) + err := v.Validate(context.Background(), entity.Request{}) if len(tt.wantErrs) == 0 { assert.NoError(t, err) return diff --git a/submitqueue/extension/validator/fake/BUILD.bazel b/submitqueue/extension/validator/fake/BUILD.bazel new file mode 100644 index 00000000..1c863b4a --- /dev/null +++ b/submitqueue/extension/validator/fake/BUILD.bazel @@ -0,0 +1,23 @@ +load("@rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = ["fake.go"], + importpath = "github.com/uber/submitqueue/submitqueue/extension/validator/fake", + visibility = ["//visibility:public"], + deps = [ + "//submitqueue/entity:go_default_library", + "//submitqueue/extension/validator:go_default_library", + ], +) + +go_test( + name = "go_default_test", + srcs = ["fake_test.go"], + embed = [":go_default_library"], + deps = [ + "//submitqueue/entity:go_default_library", + "//submitqueue/extension/validator:go_default_library", + "@com_github_stretchr_testify//assert:go_default_library", + ], +) diff --git a/submitqueue/extension/validator/fake/fake.go b/submitqueue/extension/validator/fake/fake.go new file mode 100644 index 00000000..342bda6e --- /dev/null +++ b/submitqueue/extension/validator/fake/fake.go @@ -0,0 +1,47 @@ +// Copyright (c) 2025 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 fake provides a validator.Validator that always passes and a +// validator.Factory that returns it. Intended for examples, tests, and default +// wiring when no custom validation is needed. +package fake + +import ( + "context" + + "github.com/uber/submitqueue/submitqueue/entity" + "github.com/uber/submitqueue/submitqueue/extension/validator" +) + +type fakeValidator struct{} + +// New returns a validator.Validator that always passes (returns nil). +func New() validator.Validator { + return fakeValidator{} +} + +func (fakeValidator) Validate(context.Context, entity.Request) error { + return nil +} + +type fakeFactory struct{} + +// NewFactory returns a validator.Factory that always returns a passing validator. +func NewFactory() validator.Factory { + return fakeFactory{} +} + +func (fakeFactory) For(validator.Config) (validator.Validator, error) { + return fakeValidator{}, nil +} diff --git a/submitqueue/extension/validator/fake/fake_test.go b/submitqueue/extension/validator/fake/fake_test.go new file mode 100644 index 00000000..5f914250 --- /dev/null +++ b/submitqueue/extension/validator/fake/fake_test.go @@ -0,0 +1,44 @@ +// Copyright (c) 2025 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 fake + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/uber/submitqueue/submitqueue/entity" + "github.com/uber/submitqueue/submitqueue/extension/validator" +) + +func TestNew_ImplementsInterface(t *testing.T) { + var _ validator.Validator = New() +} + +func TestNew_AlwaysPasses(t *testing.T) { + v := New() + assert.NoError(t, v.Validate(context.Background(), entity.Request{})) +} + +func TestNewFactory_ImplementsInterface(t *testing.T) { + var _ validator.Factory = NewFactory() +} + +func TestNewFactory_ReturnsPassingValidator(t *testing.T) { + f := NewFactory() + v, err := f.For(validator.Config{QueueName: "test-queue"}) + assert.NoError(t, err) + assert.NoError(t, v.Validate(context.Background(), entity.Request{})) +} diff --git a/submitqueue/extension/validator/mock/validator_mock.go b/submitqueue/extension/validator/mock/validator_mock.go index 1eda0f03..89b047ad 100644 --- a/submitqueue/extension/validator/mock/validator_mock.go +++ b/submitqueue/extension/validator/mock/validator_mock.go @@ -43,17 +43,17 @@ func (m *MockValidator) EXPECT() *MockValidatorMockRecorder { } // Validate mocks base method. -func (m *MockValidator) Validate(ctx context.Context, request entity.Request, changes []entity.ChangeInfo) error { +func (m *MockValidator) Validate(ctx context.Context, request entity.Request) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Validate", ctx, request, changes) + ret := m.ctrl.Call(m, "Validate", ctx, request) ret0, _ := ret[0].(error) return ret0 } // Validate indicates an expected call of Validate. -func (mr *MockValidatorMockRecorder) Validate(ctx, request, changes any) *gomock.Call { +func (mr *MockValidatorMockRecorder) Validate(ctx, request any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Validate", reflect.TypeOf((*MockValidator)(nil).Validate), ctx, request, changes) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Validate", reflect.TypeOf((*MockValidator)(nil).Validate), ctx, request) } // MockFactory is a mock of Factory interface. diff --git a/submitqueue/extension/validator/validator.go b/submitqueue/extension/validator/validator.go index 8d03f633..c3929c31 100644 --- a/submitqueue/extension/validator/validator.go +++ b/submitqueue/extension/validator/validator.go @@ -19,28 +19,24 @@ package validator import ( "context" - "github.com/uber/submitqueue/platform/base/change" "github.com/uber/submitqueue/submitqueue/entity" ) -// Validator runs custom validation checks against a request and its changes. +// Validator runs custom validation checks against a request. // Implementations are provided by integrators for company-specific checks // (e.g. org-policy enforcement, repo-specific rules). The built-in validate // controller invokes this after fetching change metadata and before claiming // URIs in the change store. type Validator interface { - // Validate runs custom validation on the request and its fetched change metadata. + // Validate runs custom validation on the request. // Returns nil if validation passes, error if the request should be rejected. - Validate(ctx context.Context, request entity.Request, changes []entity.ChangeInfo) error + Validate(ctx context.Context, request entity.Request) error } // Config carries the routing identity handed to a Factory. type Config struct { // QueueName identifies the queue this request belongs to. QueueName string - // Change is the code change being validated, carrying the full set of URIs. - // The URI scheme identifies the change provider (e.g. "github", "phabricator"). - Change change.Change } // Factory builds the Validator for a given config. Implementations are provided diff --git a/submitqueue/orchestrator/controller/validate/validate.go b/submitqueue/orchestrator/controller/validate/validate.go index b04cc677..390969d2 100644 --- a/submitqueue/orchestrator/controller/validate/validate.go +++ b/submitqueue/orchestrator/controller/validate/validate.go @@ -168,7 +168,6 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r if c.validators != nil { cfg := validator.Config{ QueueName: request.Queue, - Change: request.Change, } v, err := c.validators.For(cfg) if err != nil { @@ -176,7 +175,7 @@ func (c *Controller) Process(ctx context.Context, delivery consumer.Delivery) (r return fmt.Errorf("failed to build validator for request %s: %w", request.ID, err) } if v != nil { - if err := v.Validate(ctx, request, changeInfos); err != nil { + if err := v.Validate(ctx, request); err != nil { coremetrics.NamedCounter(c.metricsScope, "process", "custom_validation_failures", 1) return fmt.Errorf("custom validation failed for request %s: %w", request.ID, err) } diff --git a/submitqueue/orchestrator/controller/validate/validate_test.go b/submitqueue/orchestrator/controller/validate/validate_test.go index a3fe5613..d2c57efa 100644 --- a/submitqueue/orchestrator/controller/validate/validate_test.go +++ b/submitqueue/orchestrator/controller/validate/validate_test.go @@ -596,11 +596,10 @@ func TestController_Process_CustomValidatorPasses(t *testing.T) { mockValidator := validatormock.NewMockValidator(ctrl) // mockValidator returns nil - validation succeeded - mockValidator.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + mockValidator.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(nil) mockValidatorFactory := validatormock.NewMockFactory(ctrl) mockValidatorFactory.EXPECT().For(validator.Config{ QueueName: request.Queue, - Change: request.Change, }).Return(mockValidator, nil) controller := NewController(logger, tally.NoopScope, store, registry, cpFactory, mockValidatorFactory, runwaymq.TopicKeyMergeConflictCheck, topickey.TopicKeyValidate, "orchestrator-validate") @@ -642,11 +641,10 @@ func TestController_Process_CustomValidatorFails(t *testing.T) { mockValidator := validatormock.NewMockValidator(ctrl) // mockValidator returns an error - validation failed - mockValidator.EXPECT().Validate(gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("some validation error")) + mockValidator.EXPECT().Validate(gomock.Any(), gomock.Any()).Return(fmt.Errorf("some validation error")) mockValidatorFactory := validatormock.NewMockFactory(ctrl) mockValidatorFactory.EXPECT().For(validator.Config{ QueueName: request.Queue, - Change: request.Change, }).Return(mockValidator, nil) controller := NewController(logger, tally.NoopScope, store, registry, cpFactory, mockValidatorFactory, runwaymq.TopicKeyMergeConflictCheck, topickey.TopicKeyValidate, "orchestrator-validate")