Implemented spec changes to satisfy unit tests

prebid#1015 (comment) - all VIDEO bids, cached and returned, have modified VAST if account and bidder enabled - not just winning cached bids. - all NON-VIDEO returned bids have `ext.prebid.events` if account or request enabled - all cached NON-VIDEO bids have `wurl` patched in if account or request enabled.
openx · Dec 16, 2020 · 5f3018a · 5f3018a
1 parent 6797b3a
commit 5f3018a
Show file tree

Hide file tree

Showing 5 changed files with 83 additions and 79 deletions.
diff --git a/exchange/auction.go b/exchange/auction.go
@@ -195,7 +195,7 @@ func (a *auction) doCache(ctx context.Context, cache prebid_cache_client.Client,
 			}
 			if bids {
 				if jsonBytes, err := json.Marshal(topBidPerBidder.bid); err == nil {
-					jsonBytes = evTracking.modifyBidJSON(topBidPerBidder.bid, bidderName, jsonBytes)
+					jsonBytes = evTracking.modifyBidJSON(topBidPerBidder, bidderName, jsonBytes)
 					if useCustomCacheKey {
 						// not allowed if bids is true; log error and cache normally
 						errs = append(errs, errors.New("cannot use custom cache key for non-vast bids"))

diff --git a/exchange/bidder.go b/exchange/bidder.go
@@ -58,13 +58,15 @@ type adaptedBidder interface {
 // pbsOrtbBid.bidType will become "response.seatbid[i].bid.ext.prebid.type" in the final OpenRTB response.
 // pbsOrtbBid.bidTargets does not need to be filled out by the Bidder. It will be set later by the exchange.
 // pbsOrtbBid.bidVideo is optional but should be filled out by the Bidder if bidType is video.
+// pbsOrtbBid.bidEvents is set by exchange when event tracking is enabled
 // pbsOrtbBid.dealPriority is optionally provided by adapters and used internally by the exchange to support deal targeted campaigns.
 // pbsOrtbBid.dealTierSatisfied is set to true by exchange.updateHbPbCatDur if deal tier satisfied otherwise it will be set to false
 type pbsOrtbBid struct {
 	bid               *openrtb.Bid
 	bidType           openrtb_ext.BidType
 	bidTargets        map[string]string
 	bidVideo          *openrtb_ext.ExtBidPrebidVideo
+	bidEvents         *openrtb_ext.ExtBidPrebidEvents
 	dealPriority      int
 	dealTierSatisfied bool
 }

diff --git a/exchange/events.go b/exchange/events.go
@@ -5,7 +5,6 @@ import (
 	"time"
 
 	jsonpatch "github.com/evanphx/json-patch"
-	"github.com/mxmCherry/openrtb"
 	"github.com/prebid/prebid-server/adapters"
 	"github.com/prebid/prebid-server/analytics"
 	"github.com/prebid/prebid-server/config"
@@ -38,46 +37,50 @@ func getEventTracking(requestExtPrebid *openrtb_ext.ExtRequestPrebid, ts time.Ti
 	}
 }
 
-// modifyVASTForWinningBids
-func (ev *eventTracking) modifyVASTForWinningBids(auc *auction) {
-	for impID, topBidsPerImp := range auc.winningBidsByBidder {
-		overallWinner := auc.winningBids[impID]
-		for bidderName, topBidPerBidder := range topBidsPerImp {
-			isOverallWinner := overallWinner.bid.ID == topBidPerBidder.bid.ID
-			if !isOverallWinner {
-				continue
-			}
-			bid := topBidPerBidder.bid
-			if topBidPerBidder.bidType == openrtb_ext.BidTypeVideo && (len(bid.AdM) > 0 || len(bid.NURL) > 0) {
-				vastXML := makeVAST(bid)
-				bid.AdM = ev.modifyVAST(bid, bidderName, vastXML)
+// modifyBidsForEvents adds bidEvents and modifies VAST AdM if necessary.
+func (ev *eventTracking) modifyBidsForEvents(seatBids map[openrtb_ext.BidderName]*pbsOrtbSeatBid) map[openrtb_ext.BidderName]*pbsOrtbSeatBid {
+	for bidderName, seatBid := range seatBids {
+		modifyingVastXMLAllowed := ev.isModifyingVASTXMLAllowed(bidderName.String())
+		for _, pbsBid := range seatBid.bids {
+			if modifyingVastXMLAllowed {
+				ev.modifyBidVAST(pbsBid, bidderName)
 			}
+			pbsBid.bidEvents = ev.makeBidExtEvents(pbsBid, bidderName)
 		}
 	}
+	return seatBids
 }
 
 // isModifyingVASTXMLAllowed returns true if this bidder config allows modifying VAST XML for event tracking
 func (ev *eventTracking) isModifyingVASTXMLAllowed(bidderName string) bool {
 	return ev.bidderInfos[bidderName].ModifyingVastXmlAllowed && ev.enabledForAccount
 }
 
-// modifyVAST injects event Impression url if needed, otherwise returns original VAST string
-func (ev *eventTracking) modifyVAST(bid *openrtb.Bid, bidderName openrtb_ext.BidderName, vastXML string) string {
-	if ev.isModifyingVASTXMLAllowed(bidderName.String()) {
-		if newVastXML, ok := events.ModifyVastXmlString(ev.externalURL, vastXML, bid.ID, bidderName.String(), ev.accountID, ev.auctionTimestampMs); ok {
-			return newVastXML
-		}
+// modifyBidVAST injects event Impression url if needed, otherwise returns original VAST string
+func (ev *eventTracking) modifyBidVAST(pbsBid *pbsOrtbBid, bidderName openrtb_ext.BidderName) {
+	bid := pbsBid.bid
+	if pbsBid.bidType != openrtb_ext.BidTypeVideo || len(bid.AdM) == 0 && len(bid.NURL) == 0 {
+		return
+	}
+	vastXML := makeVAST(bid)
+	if newVastXML, ok := events.ModifyVastXmlString(ev.externalURL, vastXML, bid.ID, bidderName.String(), ev.accountID, ev.auctionTimestampMs); ok {
+		bid.AdM = newVastXML
 	}
-	return vastXML
 }
 
 // modifyBidJSON injects "wurl" (win) event url if needed, otherwise returns original json
-func (ev *eventTracking) modifyBidJSON(bid *openrtb.Bid, bidderName openrtb_ext.BidderName, jsonBytes []byte) []byte {
-	if !ev.enabledForAccount && !ev.enabledForRequest {
+func (ev *eventTracking) modifyBidJSON(pbsBid *pbsOrtbBid, bidderName openrtb_ext.BidderName, jsonBytes []byte) []byte {
+	if !ev.enabledForAccount && !ev.enabledForRequest || pbsBid.bidType == openrtb_ext.BidTypeVideo {
 		return jsonBytes
 	}
+	var winEventURL string
+	if pbsBid.bidEvents != nil { // All bids should have already been updated with win/imp event URLs
+		winEventURL = pbsBid.bidEvents.Win
+	} else {
+		winEventURL = ev.makeEventURL(analytics.Win, pbsBid, bidderName)
+	}
 	// wurl attribute is not in the schema, so we have to patch
-	if patch, err := json.Marshal(map[string]string{"wurl": ev.makeEventURL(analytics.Win, bid, bidderName)}); err == nil {
+	if patch, err := json.Marshal(map[string]string{"wurl": winEventURL}); err == nil {
 		if modifiedJSON, err := jsonpatch.MergePatch(jsonBytes, patch); err == nil {
 			jsonBytes = modifiedJSON
 		}
@@ -86,22 +89,22 @@ func (ev *eventTracking) modifyBidJSON(bid *openrtb.Bid, bidderName openrtb_ext.
 }
 
 // makeBidExtEvents make the data for bid.ext.prebid.events if needed, otherwise returns nil
-func (ev *eventTracking) makeBidExtEvents(bid *openrtb.Bid, bidderName openrtb_ext.BidderName) *openrtb_ext.ExtBidPrebidEvents {
-	if !ev.enabledForAccount && !ev.enabledForRequest {
+func (ev *eventTracking) makeBidExtEvents(pbsBid *pbsOrtbBid, bidderName openrtb_ext.BidderName) *openrtb_ext.ExtBidPrebidEvents {
+	if !ev.enabledForAccount && !ev.enabledForRequest || pbsBid.bidType == openrtb_ext.BidTypeVideo {
 		return nil
 	}
 	return &openrtb_ext.ExtBidPrebidEvents{
-		Win: ev.makeEventURL(analytics.Win, bid, bidderName),
-		Imp: ev.makeEventURL(analytics.Imp, bid, bidderName),
+		Win: ev.makeEventURL(analytics.Win, pbsBid, bidderName),
+		Imp: ev.makeEventURL(analytics.Imp, pbsBid, bidderName),
 	}
 }
 
 // makeEventURL returns an analytics event url for the requested type (win or imp)
-func (ev *eventTracking) makeEventURL(evType analytics.EventType, bid *openrtb.Bid, bidderName openrtb_ext.BidderName) string {
+func (ev *eventTracking) makeEventURL(evType analytics.EventType, pbsBid *pbsOrtbBid, bidderName openrtb_ext.BidderName) string {
 	return events.EventRequestToUrl(ev.externalURL,
 		&analytics.EventRequest{
 			Type:      evType,
-			BidID:     bid.ID,
+			BidID:     pbsBid.bid.ID,
 			Bidder:    string(bidderName),
 			AccountID: ev.accountID,
 			Timestamp: ev.auctionTimestampMs,

diff --git a/exchange/exchange.go b/exchange/exchange.go
@@ -130,7 +130,6 @@ func (e *exchange) HoldAuction(ctx context.Context, r AuctionRequest, debugLog *
 	}
 
 	cacheInstructions := getExtCacheInstructions(requestExt)
-	evTracking := getEventTracking(&requestExt.Prebid, r.StartTime, &r.Account, e.bidderInfo, e.externalURL)
 	targData := getExtTargetData(requestExt, &cacheInstructions)
 	if targData != nil {
 		_, targData.cacheHost, targData.cachePath = e.cache.GetExtCacheData()
@@ -183,6 +182,9 @@ func (e *exchange) HoldAuction(ctx context.Context, r AuctionRequest, debugLog *
 			}
 		}
 
+		evTracking := getEventTracking(&requestExt.Prebid, r.StartTime, &r.Account, e.bidderInfo, e.externalURL)
+		adapterBids = evTracking.modifyBidsForEvents(adapterBids)
+
 		if targData != nil {
 			// A non-nil auction is only needed if targeting is active. (It is used below this block to extract cache keys)
 			auc = newAuction(adapterBids, len(r.BidRequest.Imp), targData.preferDeals)
@@ -192,7 +194,6 @@ func (e *exchange) HoldAuction(ctx context.Context, r AuctionRequest, debugLog *
 				dealErrs := applyDealSupport(r.BidRequest, auc, bidCategory)
 				errs = append(errs, dealErrs...)
 			}
-			evTracking.modifyVASTForWinningBids(auc)
 			cacheErrs := auc.doCache(ctx, e.cache, targData, evTracking, r.BidRequest, 60, &r.Account.CacheTTL, bidCategory, debugLog)
 			if len(cacheErrs) > 0 {
 				errs = append(errs, cacheErrs...)
@@ -227,7 +228,7 @@ func (e *exchange) HoldAuction(ctx context.Context, r AuctionRequest, debugLog *
 	}
 
 	// Build the response
-	return e.buildBidResponse(ctx, liveAdapters, adapterBids, r.BidRequest, adapterExtra, auc, evTracking, bidResponseExt, cacheInstructions.returnCreative, errs)
+	return e.buildBidResponse(ctx, liveAdapters, adapterBids, r.BidRequest, adapterExtra, auc, bidResponseExt, cacheInstructions.returnCreative, errs)
 }
 
 func (e *exchange) parseUsersyncIfAmbiguous(bidRequest *openrtb.BidRequest) bool {
@@ -493,7 +494,7 @@ func errsToBidderErrors(errs []error) []openrtb_ext.ExtBidderError {
 }
 
 // This piece takes all the bids supplied by the adapters and crafts an openRTB response to send back to the requester
-func (e *exchange) buildBidResponse(ctx context.Context, liveAdapters []openrtb_ext.BidderName, adapterBids map[openrtb_ext.BidderName]*pbsOrtbSeatBid, bidRequest *openrtb.BidRequest, adapterExtra map[openrtb_ext.BidderName]*seatResponseExtra, auc *auction, evData *eventTracking, bidResponseExt *openrtb_ext.ExtBidResponse, returnCreative bool, errList []error) (*openrtb.BidResponse, error) {
+func (e *exchange) buildBidResponse(ctx context.Context, liveAdapters []openrtb_ext.BidderName, adapterBids map[openrtb_ext.BidderName]*pbsOrtbSeatBid, bidRequest *openrtb.BidRequest, adapterExtra map[openrtb_ext.BidderName]*seatResponseExtra, auc *auction, bidResponseExt *openrtb_ext.ExtBidResponse, returnCreative bool, errList []error) (*openrtb.BidResponse, error) {
 	bidResponse := new(openrtb.BidResponse)
 	var err error
 
@@ -509,7 +510,7 @@ func (e *exchange) buildBidResponse(ctx context.Context, liveAdapters []openrtb_
 	for _, a := range liveAdapters {
 		//while processing every single bib, do we need to handle categories here?
 		if adapterBids[a] != nil && len(adapterBids[a].bids) > 0 {
-			sb := e.makeSeatBid(adapterBids[a], a, adapterExtra, auc, evData, returnCreative)
+			sb := e.makeSeatBid(adapterBids[a], a, adapterExtra, auc, returnCreative)
 			seatBids = append(seatBids, *sb)
 			bidResponse.Cur = adapterBids[a].currency
 		}
@@ -776,7 +777,7 @@ func (e *exchange) makeExtBidResponse(adapterBids map[openrtb_ext.BidderName]*pb
 
 // Return an openrtb seatBid for a bidder
 // BuildBidResponse is responsible for ensuring nil bid seatbids are not included
-func (e *exchange) makeSeatBid(adapterBid *pbsOrtbSeatBid, adapter openrtb_ext.BidderName, adapterExtra map[openrtb_ext.BidderName]*seatResponseExtra, auc *auction, evData *eventTracking, returnCreative bool) *openrtb.SeatBid {
+func (e *exchange) makeSeatBid(adapterBid *pbsOrtbSeatBid, adapter openrtb_ext.BidderName, adapterExtra map[openrtb_ext.BidderName]*seatResponseExtra, auc *auction, returnCreative bool) *openrtb.SeatBid {
 	seatBid := new(openrtb.SeatBid)
 	seatBid.Seat = adapter.String()
 	// Prebid cannot support roadblocking
@@ -799,7 +800,7 @@ func (e *exchange) makeSeatBid(adapterBid *pbsOrtbSeatBid, adapter openrtb_ext.B
 	}
 
 	var errList []error
-	seatBid.Bid, errList = e.makeBid(adapterBid.bids, auc, evData, returnCreative, adapter)
+	seatBid.Bid, errList = e.makeBid(adapterBid.bids, auc, returnCreative)
 	if len(errList) > 0 {
 		adapterExtra[adapter].Errors = append(adapterExtra[adapter].Errors, errsToBidderErrors(errList)...)
 	}
@@ -808,7 +809,7 @@ func (e *exchange) makeSeatBid(adapterBid *pbsOrtbSeatBid, adapter openrtb_ext.B
 }
 
 // Create the Bid array inside of SeatBid
-func (e *exchange) makeBid(Bids []*pbsOrtbBid, auc *auction, evData *eventTracking, returnCreative bool, adapter openrtb_ext.BidderName) ([]openrtb.Bid, []error) {
+func (e *exchange) makeBid(Bids []*pbsOrtbBid, auc *auction, returnCreative bool) ([]openrtb.Bid, []error) {
 	bids := make([]openrtb.Bid, 0, len(Bids))
 	errList := make([]error, 0, 1)
 	for _, thisBid := range Bids {
@@ -818,6 +819,7 @@ func (e *exchange) makeBid(Bids []*pbsOrtbBid, auc *auction, evData *eventTracki
 				Targeting:         thisBid.bidTargets,
 				Type:              thisBid.bidType,
 				Video:             thisBid.bidVideo,
+				Events:            thisBid.bidEvents,
 				DealPriority:      thisBid.dealPriority,
 				DealTierSatisfied: thisBid.dealTierSatisfied,
 			},
@@ -827,9 +829,6 @@ func (e *exchange) makeBid(Bids []*pbsOrtbBid, auc *auction, evData *eventTracki
 				Bids: &cacheInfo,
 			}
 		}
-		if evData != nil {
-			bidExt.Prebid.Events = evData.makeBidExtEvents(thisBid.bid, adapter)
-		}
 		ext, err := json.Marshal(bidExt)
 		if err != nil {
 			errList = append(errList, err)