From 6359c8c4244e08bca3d28b7c6967792d1a9cd46c Mon Sep 17 00:00:00 2001 From: "ashish.shinde" Date: Wed, 24 Jul 2024 08:51:26 +0530 Subject: [PATCH] address review comments --- adapters/ortbbidder/resolver/errortypes.go | 59 ++++++------- .../ortbbidder/resolver/errortypes_test.go | 29 +++---- adapters/ortbbidder/response_builder.go | 30 +++++-- adapters/ortbbidder/response_builder_test.go | 82 ++++++++++++++++++- 4 files changed, 138 insertions(+), 62 deletions(-) diff --git a/adapters/ortbbidder/resolver/errortypes.go b/adapters/ortbbidder/resolver/errortypes.go index 5c047d02269..752399ee99a 100644 --- a/adapters/ortbbidder/resolver/errortypes.go +++ b/adapters/ortbbidder/resolver/errortypes.go @@ -2,78 +2,67 @@ package resolver import "fmt" -// Severity represents the severity level of a error. -type Severity int +// severity represents the severity level of a error. +type severity int const ( - SeverityFatal Severity = iota - SeverityWarning - SeverityDebug + severityFatal severity = iota // use this to discard the entire bid response + severityWarning // use this to include errors in responseExt.warnings + severityIgnore // use this to exclude errors from responseExt.warnings ) -// Coder is used to indicate the severity of an error. -type Coder interface { +// coder is used to indicate the severity of an error. +type coder interface { // Severity returns the severity level of the error. - Severity() Severity + Severity() severity } -// DefaultValueError is used to flag that a default value was found. -type DefaultValueError struct { +// defaultValueError is used to flag that a default value was found. +type defaultValueError struct { Message string } // Error returns the error message. -func (err *DefaultValueError) Error() string { +func (err *defaultValueError) Error() string { return err.Message } // Severity returns the severity level of the error. -func (err *DefaultValueError) Severity() Severity { - return SeverityDebug +func (err *defaultValueError) Severity() severity { + return severityIgnore } -// ValidationFailedError is used to flag that the value validation failed. -type ValidationFailedError struct { +// validationFailedError is used to flag that the value validation failed. +type validationFailedError struct { Message string // Message contains the error message. } // Error returns the error message for ValidationFailedError. -func (err *ValidationFailedError) Error() string { +func (err *validationFailedError) Error() string { return err.Message } // Severity returns the severity level for ValidationFailedError. -func (err *ValidationFailedError) Severity() Severity { - return SeverityWarning +func (err *validationFailedError) Severity() severity { + return severityWarning } -// isWarning returns true if an error is labeled with a Severity of SeverityWarning. -func isWarning(err error) bool { - s, ok := err.(Coder) - return ok && s.Severity() == SeverityWarning -} - -// ContainsWarning checks if the error list contains a warning. -func ContainsWarning(errors []error) bool { - for _, err := range errors { - if isWarning(err) { - return true - } - } - - return false +// IsWarning returns true if an error is labeled with a Severity of SeverityWarning. +func IsWarning(err error) bool { + s, ok := err.(coder) + return ok && s.Severity() == severityWarning } // NewDefaultValueError creates a new DefaultValueError with a formatted message. func NewDefaultValueError(message string, args ...any) error { - return &DefaultValueError{ + return &defaultValueError{ Message: fmt.Sprintf(message, args...), } } // NewValidationFailedError creates a new ValidationFailedError with a formatted message. func NewValidationFailedError(message string, args ...any) error { - return &ValidationFailedError{ + return &validationFailedError{ Message: fmt.Sprintf(message, args...), } } diff --git a/adapters/ortbbidder/resolver/errortypes_test.go b/adapters/ortbbidder/resolver/errortypes_test.go index 19780c2bd5f..e9f1dfef481 100644 --- a/adapters/ortbbidder/resolver/errortypes_test.go +++ b/adapters/ortbbidder/resolver/errortypes_test.go @@ -8,23 +8,23 @@ import ( func TestValidationFailedError(t *testing.T) { t.Run("validationFailedError", func(t *testing.T) { - err := ValidationFailedError{Message: "any validation message"} + err := validationFailedError{Message: "any validation message"} assert.Equal(t, "any validation message", err.Error()) - assert.Equal(t, SeverityWarning, err.Severity()) + assert.Equal(t, severityWarning, err.Severity()) }) } func TestDefaultValueError(t *testing.T) { t.Run("defaultValueError", func(t *testing.T) { - err := DefaultValueError{Message: "any validation message"} + err := defaultValueError{Message: "any validation message"} assert.Equal(t, "any validation message", err.Error()) - assert.Equal(t, SeverityDebug, err.Severity()) + assert.Equal(t, severityIgnore, err.Severity()) }) } -func TestContainsWarning(t *testing.T) { +func TestIsWarning(t *testing.T) { type args struct { - errors []error + err error } tests := []struct { name string @@ -32,29 +32,24 @@ func TestContainsWarning(t *testing.T) { want bool }{ { - name: "error list contains warning", + name: "input err is of severity warning", args: args{ - errors: []error{ - NewDefaultValueError("default value error"), - NewValidationFailedError("validation failed"), - }, + err: NewValidationFailedError("error"), }, want: true, }, { - name: "error list not contains warning", + name: "input err is of severity ignore", args: args{ - errors: []error{ - NewDefaultValueError("default value error"), - }, + err: NewDefaultValueError("error"), }, want: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := ContainsWarning(tt.args.errors); got != tt.want { - t.Errorf("ContainsWarning() = %v, want %v", got, tt.want) + if got := IsWarning(tt.args.err); got != tt.want { + t.Errorf("IsWarning() = %v, want %v", got, tt.want) } }) } diff --git a/adapters/ortbbidder/response_builder.go b/adapters/ortbbidder/response_builder.go index 36337932205..6b90f3edbda 100644 --- a/adapters/ortbbidder/response_builder.go +++ b/adapters/ortbbidder/response_builder.go @@ -3,6 +3,7 @@ package ortbbidder import ( "encoding/json" + "github.com/buger/jsonparser" "github.com/prebid/openrtb/v20/openrtb2" "github.com/prebid/prebid-server/v2/adapters" "github.com/prebid/prebid-server/v2/adapters/ortbbidder/bidderparams" @@ -16,12 +17,18 @@ type responseBuilder struct { adapterRespone map[string]any // Response in the prebid format. responseParams map[string]bidderparams.BidderParamMapper // Bidder response parameters. request *openrtb2.BidRequest // Bid request. + isDebugEnabled bool // flag to determine if requestExt.prebid.debug is enabled. } func newResponseBuilder(responseParams map[string]bidderparams.BidderParamMapper, request *openrtb2.BidRequest) *responseBuilder { + var isDebugEnabled bool + if request != nil { + isDebugEnabled, _ = jsonparser.GetBoolean(request.Ext, "prebid", "debug") + } return &responseBuilder{ responseParams: responseParams, request: request, + isDebugEnabled: isDebugEnabled, } } @@ -43,9 +50,7 @@ func (rb *responseBuilder) setPrebidBidderResponse(bidderResponseBytes json.RawM for _, param := range resolver.ResponseParams { bidderParam := rb.responseParams[param.String()] resolverErrors := paramResolver.Resolve(rb.bidderResponse, adapterResponse, bidderParam.Location, param) - if resolver.ContainsWarning(resolverErrors) { - errs = append(errs, util.NewWarning("failed to set the [%s]", param.String())) - } + errs = collectWarningMessages(errs, resolverErrors, param.String(), rb.isDebugEnabled) } // Extract the seat bids from the bidder response. seatBids, ok := rb.bidderResponse[seatBidKey].([]any) @@ -77,9 +82,7 @@ func (rb *responseBuilder) setPrebidBidderResponse(bidderResponseBytes json.RawM paramMapper := rb.responseParams[param.String()] location := util.ReplaceLocationMacro(paramMapper.Location, []int{seatIndex, bidIndex}) resolverErrors := paramResolver.Resolve(bid, typedBid, location, param) - if resolver.ContainsWarning(resolverErrors) { - errs = append(errs, util.NewWarning("failed to set the [%s]", param.String())) - } + errs = collectWarningMessages(errs, resolverErrors, param.String(), rb.isDebugEnabled) } // Add the type bid to the list of typed bids. typedBids = append(typedBids, typedBid) @@ -107,3 +110,18 @@ func (rb *responseBuilder) buildAdapterResponse() (resp *adapters.BidderResponse } return } + +// collectWarningMessages appends warning messages from resolverErrors to the errs slice. +// If debugging is disabled, it appends a generic warning message and returns immediately. +func collectWarningMessages(errs, resolverErrors []error, parameter string, isDebugEnabled bool) []error { + for _, err := range resolverErrors { + if resolver.IsWarning(err) { + if !isDebugEnabled { + errs = append(errs, util.NewWarning("Potential issue encountered while setting the response parameter [%s]", parameter)) + return errs + } + errs = append(errs, util.NewWarning(err.Error())) + } + } + return errs +} diff --git a/adapters/ortbbidder/response_builder_test.go b/adapters/ortbbidder/response_builder_test.go index dbb3eb21a8c..b1e57b27e6c 100644 --- a/adapters/ortbbidder/response_builder_test.go +++ b/adapters/ortbbidder/response_builder_test.go @@ -1,6 +1,7 @@ package ortbbidder import ( + "errors" "reflect" "testing" @@ -186,7 +187,7 @@ func TestSetPrebidBidderResponse(t *testing.T) { name: "Valid bidder respone, no bidder params", bidderResponseBytes: []byte(`{"id":"bid-resp-id","cur":"USD","seatbid":[{"seat":"test_bidder","bid":[{"id":"123"}]}]}`), responseParams: map[string]bidderparams.BidderParamMapper{}, - expectedError: []error{util.NewWarning("failed to set the [bidType]")}, + expectedError: []error{util.NewWarning("Potential issue encountered while setting the response parameter [bidType]")}, expectedResponse: map[string]any{ "Currency": "USD", "Bids": []any{ @@ -255,8 +256,8 @@ func TestSetPrebidBidderResponse(t *testing.T) { }, }, expectedError: []error{ - util.NewWarning("failed to set the [fledgeAuctionConfig]"), - util.NewWarning("failed to set the [bidType]"), + util.NewWarning("Potential issue encountered while setting the response parameter [fledgeAuctionConfig]"), + util.NewWarning("Potential issue encountered while setting the response parameter [bidType]"), }, expectedResponse: map[string]any{ "Currency": "USD", @@ -285,7 +286,7 @@ func TestSetPrebidBidderResponse(t *testing.T) { "bidMetaAdvertiserId": {Location: "seatbid.#.bid.#.ext.advertiserId"}, "bidMetaNetworkId": {Location: "seatbid.#.bid.#.ext.networkId"}, }, - expectedError: []error{util.NewWarning("failed to set the [bidMetaAdvertiserId]")}, + expectedError: []error{util.NewWarning("Potential issue encountered while setting the response parameter [bidMetaAdvertiserId]")}, expectedResponse: map[string]any{ "Currency": "USD", "Bids": []any{ @@ -370,3 +371,76 @@ func TestBidderResponseFields(t *testing.T) { t.Error(err) } } + +func TestCollectWarningMessages(t *testing.T) { + type args struct { + errs []error + resolverErrors []error + parameter string + isDebugEnabled bool + } + tests := []struct { + name string + args args + want []error + }{ + { + name: "No resolver errors", + args: args{ + errs: []error{}, + resolverErrors: []error{}, + parameter: "param1", + isDebugEnabled: false, + }, + want: []error{}, + }, + { + name: "Resolver errors with warnings and debugging enabled", + args: args{ + errs: []error{}, + resolverErrors: []error{ + resolver.NewValidationFailedError("Warning 1"), + resolver.NewValidationFailedError("Warning 2"), + }, + parameter: "param2", + isDebugEnabled: true, + }, + want: []error{ + util.NewWarning("Warning 1"), + util.NewWarning("Warning 2"), + }, + }, + { + name: "Resolver errors with warnings and debugging disabled", + args: args{ + errs: []error{}, + resolverErrors: []error{ + resolver.NewValidationFailedError("Warning 1"), + }, + parameter: "param3", + isDebugEnabled: false, + }, + want: []error{ + util.NewWarning("Potential issue encountered while setting the response parameter [param3]"), + }, + }, + { + name: "Resolver errors without warnings", + args: args{ + errs: []error{}, + resolverErrors: []error{ + errors.New("Non-warning error"), + }, + parameter: "param4", + isDebugEnabled: false, + }, + want: []error{}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := collectWarningMessages(tt.args.errs, tt.args.resolverErrors, tt.args.parameter, tt.args.isDebugEnabled) + assert.Equal(t, tt.want, got) + }) + } +}