Skip to content

Commit

Permalink
Enables proper processing for deprecated (disabled) bidders. (#716)
Browse files Browse the repository at this point in the history 
* Enables proper processing for deprecated (disabled) bidders.

* Fixes PR identified issues
  • Loading branch information
@hhhjort @dbemiller
hhhjort authored and dbemiller committed Oct 22, 2018
1 parent 1d13fb3 commit d9455e3
Show file tree
Hide file tree
Showing 8 changed files with 205 additions and 67 deletions.
13 changes: 7 additions & 6 deletions endpoints/openrtb2/amp_auction.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,12 @@ type AmpResponse struct {

// NewAmpEndpoint modifies the OpenRTB endpoint to handle AMP requests. This will basically modify the parsing
// of the request, and the return value, using the OpenRTB machinery to handle everything inbetween.
func NewAmpEndpoint(ex exchange.Exchange, validator openrtb_ext.BidderParamValidator, requestsById stored_requests.Fetcher, cfg *config.Configuration, met pbsmetrics.MetricsEngine, pbsAnalytics analytics.PBSAnalyticsModule) (httprouter.Handle, error) {
func NewAmpEndpoint(ex exchange.Exchange, validator openrtb_ext.BidderParamValidator, requestsById stored_requests.Fetcher, cfg *config.Configuration, met pbsmetrics.MetricsEngine, pbsAnalytics analytics.PBSAnalyticsModule, disabledBidders map[string]string) (httprouter.Handle, error) {
if ex == nil || validator == nil || requestsById == nil || cfg == nil || met == nil {
return nil, errors.New("NewAmpEndpoint requires non-nil arguments.")
}

return httprouter.Handle((&endpointDeps{ex, validator, requestsById, cfg, met, pbsAnalytics}).AmpAuction), nil
return httprouter.Handle((&endpointDeps{ex, validator, requestsById, cfg, met, pbsAnalytics, disabledBidders}).AmpAuction), nil
}

func (deps *endpointDeps) AmpAuction(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
Expand Down Expand Up @@ -95,7 +95,7 @@ func (deps *endpointDeps) AmpAuction(w http.ResponseWriter, r *http.Request, _ h

req, errL := deps.parseAmpRequest(r)

if len(errL) > 0 {
if fatalError(errL) {
w.WriteHeader(http.StatusBadRequest)
for _, err := range errL {
w.Write([]byte(fmt.Sprintf("Invalid request format: %s\n", err.Error())))
Expand Down Expand Up @@ -227,10 +227,11 @@ func (deps *endpointDeps) parseAmpRequest(httpRequest *http.Request) (req *openr

// At this point, we should have a valid request that definitely has Targeting and Cache turned on

if err := deps.validateRequest(req); err != nil {
errs = []error{err}
return
errL := deps.validateRequest(req)
if len(errL) > 0 {
errs = append(errs, errL...)
}

return
}

Expand Down
12 changes: 6 additions & 6 deletions endpoints/openrtb2/amp_auction_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ func TestGoodAmpRequests(t *testing.T) {
// NewMetrics() will create a new go_metrics MetricsEngine, bypassing the need for a crafted configuration set to support it.
// As a side effect this gives us some coverage of the go_metrics piece of the metrics engine.
theMetrics := pbsmetrics.NewMetrics(metrics.NewRegistry(), openrtb_ext.BidderList())
endpoint, _ := NewAmpEndpoint(&mockAmpExchange{}, newParamsValidator(t), &mockAmpStoredReqFetcher{goodRequests}, &config.Configuration{MaxRequestSize: maxSize}, theMetrics, analyticsConf.NewPBSAnalytics(&config.Analytics{}))
endpoint, _ := NewAmpEndpoint(&mockAmpExchange{}, newParamsValidator(t), &mockAmpStoredReqFetcher{goodRequests}, &config.Configuration{MaxRequestSize: maxSize}, theMetrics, analyticsConf.NewPBSAnalytics(&config.Analytics{}), map[string]string{})

for requestID := range goodRequests {
request := httptest.NewRequest("GET", fmt.Sprintf("/openrtb2/auction/amp?tag_id=%s", requestID), nil)
Expand Down Expand Up @@ -82,7 +82,7 @@ func TestAMPPageInfo(t *testing.T) {
}
theMetrics := pbsmetrics.NewMetrics(metrics.NewRegistry(), openrtb_ext.BidderList())
exchange := &mockAmpExchange{}
endpoint, _ := NewAmpEndpoint(exchange, newParamsValidator(t), &mockAmpStoredReqFetcher{stored}, &config.Configuration{MaxRequestSize: maxSize}, theMetrics, analyticsConf.NewPBSAnalytics(&config.Analytics{}))
endpoint, _ := NewAmpEndpoint(exchange, newParamsValidator(t), &mockAmpStoredReqFetcher{stored}, &config.Configuration{MaxRequestSize: maxSize}, theMetrics, analyticsConf.NewPBSAnalytics(&config.Analytics{}), map[string]string{})
request := httptest.NewRequest("GET", fmt.Sprintf("/openrtb2/auction/amp?tag_id=1&curl=%s", url.QueryEscape(page)), nil)
recorder := httptest.NewRecorder()
endpoint(recorder, request, nil)
Expand Down Expand Up @@ -131,7 +131,7 @@ func TestAmpBadRequests(t *testing.T) {
// NewMetrics() will create a new go_metrics MetricsEngine, bypassing the need for a crafted configuration set to support it.
// As a side effect this gives us some coverage of the go_metrics piece of the metrics engine.
theMetrics := pbsmetrics.NewMetrics(metrics.NewRegistry(), openrtb_ext.BidderList())
endpoint, _ := NewEndpoint(&mockAmpExchange{}, newParamsValidator(t), &mockAmpStoredReqFetcher{badRequests}, &config.Configuration{MaxRequestSize: maxSize}, theMetrics, analyticsConf.NewPBSAnalytics(&config.Analytics{}))
endpoint, _ := NewEndpoint(&mockAmpExchange{}, newParamsValidator(t), &mockAmpStoredReqFetcher{badRequests}, &config.Configuration{MaxRequestSize: maxSize}, theMetrics, analyticsConf.NewPBSAnalytics(&config.Analytics{}), map[string]string{})
for requestID := range badRequests {
request := httptest.NewRequest("GET", fmt.Sprintf("/openrtb2/auction/amp?tag_id=%s", requestID), nil)
recorder := httptest.NewRecorder()
Expand All @@ -151,7 +151,7 @@ func TestAmpDebug(t *testing.T) {
}

theMetrics := pbsmetrics.NewMetrics(metrics.NewRegistry(), openrtb_ext.BidderList())
endpoint, _ := NewAmpEndpoint(&mockAmpExchange{}, newParamsValidator(t), &mockAmpStoredReqFetcher{requests}, &config.Configuration{MaxRequestSize: maxSize}, theMetrics, analyticsConf.NewPBSAnalytics(&config.Analytics{}))
endpoint, _ := NewAmpEndpoint(&mockAmpExchange{}, newParamsValidator(t), &mockAmpStoredReqFetcher{requests}, &config.Configuration{MaxRequestSize: maxSize}, theMetrics, analyticsConf.NewPBSAnalytics(&config.Analytics{}), map[string]string{})

for requestID := range requests {
request := httptest.NewRequest("GET", fmt.Sprintf("/openrtb2/auction/amp?tag_id=%s&debug=1", requestID), nil)
Expand Down Expand Up @@ -212,7 +212,7 @@ func TestQueryParamOverrides(t *testing.T) {
"1": json.RawMessage(validRequest(t, "site.json")),
}
theMetrics := pbsmetrics.NewMetrics(metrics.NewRegistry(), openrtb_ext.BidderList())
endpoint, _ := NewAmpEndpoint(&mockAmpExchange{}, newParamsValidator(t), &mockAmpStoredReqFetcher{requests}, &config.Configuration{MaxRequestSize: maxSize}, theMetrics, analyticsConf.NewPBSAnalytics(&config.Analytics{}))
endpoint, _ := NewAmpEndpoint(&mockAmpExchange{}, newParamsValidator(t), &mockAmpStoredReqFetcher{requests}, &config.Configuration{MaxRequestSize: maxSize}, theMetrics, analyticsConf.NewPBSAnalytics(&config.Analytics{}), map[string]string{})

requestID := "1"
curl := "http://example.com"
Expand Down Expand Up @@ -329,7 +329,7 @@ func (s formatOverrideSpec) execute(t *testing.T) {
"1": json.RawMessage(validRequest(t, "site.json")),
}
theMetrics := pbsmetrics.NewMetrics(metrics.NewRegistry(), openrtb_ext.BidderList())
endpoint, _ := NewAmpEndpoint(&mockAmpExchange{}, newParamsValidator(t), &mockAmpStoredReqFetcher{requests}, &config.Configuration{MaxRequestSize: maxSize}, theMetrics, analyticsConf.NewPBSAnalytics(&config.Analytics{}))
endpoint, _ := NewAmpEndpoint(&mockAmpExchange{}, newParamsValidator(t), &mockAmpStoredReqFetcher{requests}, &config.Configuration{MaxRequestSize: maxSize}, theMetrics, analyticsConf.NewPBSAnalytics(&config.Analytics{}), map[string]string{})

url := fmt.Sprintf("/openrtb2/auction/amp?tag_id=1&debug=1&w=%d&h=%d&ow=%d&oh=%d&ms=%s", s.width, s.height, s.overrideWidth, s.overrideHeight, s.multisize)
request := httptest.NewRequest("GET", url, nil)
Expand Down
127 changes: 85 additions & 42 deletions endpoints/openrtb2/auction.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
nativeRequests "github.com/mxmCherry/openrtb/native/request"
"github.com/prebid/prebid-server/analytics"
"github.com/prebid/prebid-server/config"
"github.com/prebid/prebid-server/errortypes"
"github.com/prebid/prebid-server/exchange"
"github.com/prebid/prebid-server/openrtb_ext"
"github.com/prebid/prebid-server/pbsmetrics"
Expand All @@ -33,12 +34,12 @@ import (

const storedRequestTimeoutMillis = 50

func NewEndpoint(ex exchange.Exchange, validator openrtb_ext.BidderParamValidator, requestsById stored_requests.Fetcher, cfg *config.Configuration, met pbsmetrics.MetricsEngine, pbsAnalytics analytics.PBSAnalyticsModule) (httprouter.Handle, error) {
func NewEndpoint(ex exchange.Exchange, validator openrtb_ext.BidderParamValidator, requestsById stored_requests.Fetcher, cfg *config.Configuration, met pbsmetrics.MetricsEngine, pbsAnalytics analytics.PBSAnalyticsModule, disabledBidders map[string]string) (httprouter.Handle, error) {
if ex == nil || validator == nil || requestsById == nil || cfg == nil || met == nil {
return nil, errors.New("NewEndpoint requires non-nil arguments.")
}

return httprouter.Handle((&endpointDeps{ex, validator, requestsById, cfg, met, pbsAnalytics}).Auction), nil
return httprouter.Handle((&endpointDeps{ex, validator, requestsById, cfg, met, pbsAnalytics, disabledBidders}).Auction), nil
}

type endpointDeps struct {
Expand All @@ -48,6 +49,7 @@ type endpointDeps struct {
cfg *config.Configuration
metricsEngine pbsmetrics.MetricsEngine
analytics analytics.PBSAnalyticsModule
disabledBidders map[string]string
}

func (deps *endpointDeps) Auction(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
Expand Down Expand Up @@ -87,7 +89,7 @@ func (deps *endpointDeps) Auction(w http.ResponseWriter, r *http.Request, _ http

req, errL := deps.parseRequest(r)

if writeError(errL, w) {
if fatalError(errL) && writeError(errL, w) {
labels.RequestStatus = pbsmetrics.RequestStatusBadInput
return
}
Expand Down Expand Up @@ -201,9 +203,9 @@ func (deps *endpointDeps) parseRequest(httpRequest *http.Request) (req *openrtb.
// Populate any "missing" OpenRTB fields with info from other sources, (e.g. HTTP request headers).
deps.setFieldsImplicitly(httpRequest, req)

if err := deps.validateRequest(req); err != nil {
errs = []error{err}
return
errL := deps.validateRequest(req)
if len(errL) > 0 {
errs = append(errs, errL...)
}

return
Expand All @@ -224,57 +226,66 @@ func parseTimeout(requestJson []byte, defaultTimeout time.Duration) time.Duratio
return defaultTimeout
}

func (deps *endpointDeps) validateRequest(req *openrtb.BidRequest) error {
func (deps *endpointDeps) validateRequest(req *openrtb.BidRequest) []error {
errL := []error{}
if req.ID == "" {
return errors.New("request missing required field: \"id\"")
return []error{errors.New("request missing required field: \"id\"")}
}

if req.TMax < 0 {
return fmt.Errorf("request.tmax must be nonnegative. Got %d", req.TMax)
return []error{fmt.Errorf("request.tmax must be nonnegative. Got %d", req.TMax)}
}

if len(req.Imp) < 1 {
return errors.New("request.imp must contain at least one element.")
return []error{errors.New("request.imp must contain at least one element.")}
}

var aliases map[string]string
if bidExt, err := deps.parseBidExt(req.Ext); err != nil {
return err
return []error{err}
} else if bidExt != nil {
aliases = bidExt.Prebid.Aliases

if err := deps.validateAliases(aliases); err != nil {
return err
return []error{err}
}

if err := validateBidAdjustmentFactors(bidExt.Prebid.BidAdjustmentFactors, aliases); err != nil {
return err
return []error{err}
}
}

for index, imp := range req.Imp {
if err := deps.validateImp(&imp, aliases, index); err != nil {
return err
errs := deps.validateImp(&imp, aliases, index)
if len(errs) > 0 {
errL = append(errL, errs...)
}
if fatalError(errs) {
return errL
}
}

if (req.Site == nil && req.App == nil) || (req.Site != nil && req.App != nil) {
return errors.New("request.site or request.app must be defined, but not both.")
errL = append(errL, errors.New("request.site or request.app must be defined, but not both."))
return errL
}

if err := deps.validateSite(req.Site); err != nil {
return err
errL = append(errL, err)
return errL
}

if err := validateUser(req.User, aliases); err != nil {
return err
errL = append(errL, err)
return errL
}

if err := validateRegs(req.Regs); err != nil {
return err
errL = append(errL, err)
return errL
}

return nil
return errL
}

func validateBidAdjustmentFactors(adjustmentFactors map[string]float64, aliases map[string]string) error {
Expand All @@ -291,45 +302,46 @@ func validateBidAdjustmentFactors(adjustmentFactors map[string]float64, aliases
return nil
}

func (deps *endpointDeps) validateImp(imp *openrtb.Imp, aliases map[string]string, index int) error {
func (deps *endpointDeps) validateImp(imp *openrtb.Imp, aliases map[string]string, index int) []error {
if imp.ID == "" {
return fmt.Errorf("request.imp[%d] missing required field: \"id\"", index)
return []error{fmt.Errorf("request.imp[%d] missing required field: \"id\"", index)}
}

if len(imp.Metric) != 0 {
return fmt.Errorf("request.imp[%d].metric is not yet supported by prebid-server. Support may be added in the future", index)
return []error{fmt.Errorf("request.imp[%d].metric is not yet supported by prebid-server. Support may be added in the future", index)}
}

if imp.Banner == nil && imp.Video == nil && imp.Audio == nil && imp.Native == nil {
return fmt.Errorf("request.imp[%d] must contain at least one of \"banner\", \"video\", \"audio\", or \"native\"", index)
return []error{fmt.Errorf("request.imp[%d] must contain at least one of \"banner\", \"video\", \"audio\", or \"native\"", index)}
}

if err := validateBanner(imp.Banner, index); err != nil {
return err
return []error{err}
}

if imp.Video != nil {
if len(imp.Video.MIMEs) < 1 {
return fmt.Errorf("request.imp[%d].video.mimes must contain at least one supported MIME type", index)
return []error{fmt.Errorf("request.imp[%d].video.mimes must contain at least one supported MIME type", index)}
}
}

if imp.Audio != nil {
if len(imp.Audio.MIMEs) < 1 {
return fmt.Errorf("request.imp[%d].audio.mimes must contain at least one supported MIME type", index)
return []error{fmt.Errorf("request.imp[%d].audio.mimes must contain at least one supported MIME type", index)}
}
}

if err := fillAndValidateNative(imp.Native, index); err != nil {
return err
return []error{err}
}

if err := validatePmp(imp.PMP, index); err != nil {
return err
return []error{err}
}

if err := deps.validateImpExt(imp.Ext, aliases, index); err != nil {
return err
errL := deps.validateImpExt(imp, aliases, index)
if len(errL) != 0 {
return errL
}

return nil
Expand Down Expand Up @@ -630,19 +642,17 @@ func validatePmp(pmp *openrtb.PMP, impIndex int) error {
return nil
}

func (deps *endpointDeps) validateImpExt(ext json.RawMessage, aliases map[string]string, impIndex int) error {
if len(ext) == 0 {
return fmt.Errorf("request.imp[%d].ext is required", impIndex)
func (deps *endpointDeps) validateImpExt(imp *openrtb.Imp, aliases map[string]string, impIndex int) []error {
errL := []error{}
if len(imp.Ext) == 0 {
return []error{fmt.Errorf("request.imp[%d].ext is required", impIndex)}
}
var bidderExts map[string]json.RawMessage
if err := json.Unmarshal(ext, &bidderExts); err != nil {
return err
}

if len(bidderExts) < 1 {
return fmt.Errorf("request.imp[%d].ext must contain at least one bidder", impIndex)
if err := json.Unmarshal(imp.Ext, &bidderExts); err != nil {
return []error{err}
}

disabledBidders := []string{}
for bidder, ext := range bidderExts {
if bidder != "prebid" {
coreBidder := bidder
Expand All @@ -651,14 +661,37 @@ func (deps *endpointDeps) validateImpExt(ext json.RawMessage, aliases map[string
}
if bidderName, isValid := openrtb_ext.BidderMap[coreBidder]; isValid {
if err := deps.paramsValidator.Validate(bidderName, ext); err != nil {
return fmt.Errorf("request.imp[%d].ext.%s failed validation.\n%v", impIndex, coreBidder, err)
return []error{fmt.Errorf("request.imp[%d].ext.%s failed validation.\n%v", impIndex, coreBidder, err)}
}
} else {
return fmt.Errorf("request.imp[%d].ext contains unknown bidder: %s. Did you forget an alias in request.ext.prebid.aliases?", impIndex, bidder)
if msg, isDisabled := deps.disabledBidders[bidder]; isDisabled {
errL = append(errL, &errortypes.BidderTemporarilyDisabled{Message: msg})
disabledBidders = append(disabledBidders, bidder)
} else {
return []error{fmt.Errorf("request.imp[%d].ext contains unknown bidder: %s. Did you forget an alias in request.ext.prebid.aliases?", impIndex, bidder)}
}
}
}
}

// defer deleting disabled bidders so we don't disrupt the loop
if len(disabledBidders) > 0 {
for _, bidder := range disabledBidders {
delete(bidderExts, bidder)
}
extJSON, err := json.Marshal(bidderExts)
if err != nil {
return []error{err}
}
imp.Ext = extJSON
}

// TODO #713 Fix this here
if len(bidderExts) < 1 {
errL = append(errL, fmt.Errorf("request.imp[%d].ext must contain at least one bidder", impIndex))
return errL
}

return nil
}

Expand Down Expand Up @@ -982,3 +1015,13 @@ func writeError(errs []error, w http.ResponseWriter) bool {
}
return false
}

// Checks to see if an error in an error list is a fatal error
func fatalError(errL []error) bool {
for _, err := range errL {
if errortypes.DecodeError(err) != errortypes.BidderTemporarilyDisabledCode {
return true
}
}
return false
}
2 changes: 1 addition & 1 deletion endpoints/openrtb2/auction_benchmark_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ func BenchmarkOpenrtbEndpoint(b *testing.B) {
if err != nil {
return
}
endpoint, _ := NewEndpoint(exchange.NewExchange(server.Client(), nil, &config.Configuration{}, theMetrics, infos, gdpr.AlwaysAllow{}), paramValidator, empty_fetcher.EmptyFetcher{}, &config.Configuration{MaxRequestSize: maxSize}, theMetrics, analyticsConf.NewPBSAnalytics(&config.Analytics{}))
endpoint, _ := NewEndpoint(exchange.NewExchange(server.Client(), nil, &config.Configuration{}, theMetrics, infos, gdpr.AlwaysAllow{}), paramValidator, empty_fetcher.EmptyFetcher{}, &config.Configuration{MaxRequestSize: maxSize}, theMetrics, analyticsConf.NewPBSAnalytics(&config.Analytics{}), map[string]string{})

b.ResetTimer()
for n := 0; n < b.N; n++ {
Expand Down
Loading

0 comments on commit d9455e3

Please sign in to comment.