Cache validation fix

[VeronikaSolovei9]

if no bids returned - don't throw cache error, just return empty result

[VeronikaSolovei9]

@hhhjort @mansinahar @josephveach @guscarreon Guys, can you please review this PR? Thank you.

[hhhjort]

LGTM

[mansinahar]

Would be great if we could move that error check I suggested but other than that LGTM! Thanks @Kalypsonika!

[mansinahar]

Shouldn't this err be checked before the previous line: auc := newAuction(adapterBids, len(bidRequest.Imp))? I understand that this PR isn't the one introducing this change and it was like that before but this seems like a small change that would be good to have because we're calling newAuction unnecessarily in case of an err from applyCategoryMapping.

[VeronikaSolovei9]

Totally makes sense. Done.

[mansinahar]

Sorry, I missed this earlier but also, it would be great if we could add/modify some tests to verify this is working as expected.

[guscarreon]

@Kalypsonika this is very, very minor but, just for the sake of saving memory in the heap and trying to declare as less local variables as possible, can we get rid of lines 406 and 419 and change the return statement in line 421?

	return &openrtb_ext.BidResponseVideo{AdPods:adPods}, nil

What do you think?

[VeronikaSolovei9]

Done!

[guscarreon]

Maybe we can save a bit of processing time if we remove the for statement in line 143 and make the getAllBids() method to return the anyBidsReturned value. Since the adapterBids map gets already traversed inside getAllBids(), we could save some processing time by not going through the rather large adapterBids map again and make our solution a bit less verbose.
In other words, go from this:

141 adapterBids, adapterExtra := e.getAllBids(auctionCtx, cleanRequests, aliases, bidAdjustmentFactors, blabels, conversions)
142 anyBidsReturned := false
143 for _, seatBid := range adapterBids {
144 	if seatBid != nil && len(seatBid.bids) > 0 {
145 		anyBidsReturned = true
146 		break
147 	}
148 }
149 if anyBidsReturned {

To something like this:

141 adapterBids, adapterExtra, anyBidsReturned := e.getAllBids(auctionCtx, cleanRequests, aliases, bidAdjustmentFactors, blabels, conversions)
142 if anyBidsReturned {

Inside of getAllBids() we can probably populate the boolean value in the last if statement:
Go from:

183 func (e *exchange) getAllBids(ctx context.Context, cleanRequests map[openrtb_ext.BidderName]*openrtb.BidRequest, aliases map[string]string, bidAdjustments map[string]float64, blabels map[openrtb_ext.BidderName]*pbsmetrics.AdapterLabels, conversions currencies.Conversions) (map[openrtb_ext.BidderName]*pbsOrtbSeatBid, map[openrtb_ext.BidderName]*seatResponseExtra) {
184     // Set up pointers to the bid results
185     adapterBids := make(map[openrtb_ext.BidderName]*pbsOrtbSeatBid, len(cleanRequests))
186     adapterExtra := make(map[openrtb_ext.BidderName]*seatResponseExtra, len(cleanRequests))
187     chBids := make(chan *bidResponseWrapper, len(cleanRequests))
188
189     for bidderName, req := range cleanRequests {
190         // Here we actually call the adapters and collect the bids.
191 +-- 45 lines: coreBidder := resolveBidder(string(bidderName), aliases)--------------------------------------------
236     }
237     // Wait for the bidders to do their thing
238     for i := 0; i < len(cleanRequests); i++ {
239         brw := <-chBids
240         adapterBids[brw.bidder] = brw.adapterBids
241         adapterExtra[brw.bidder] = brw.adapterExtra
242     }
243
244     return adapterBids, adapterExtra
245 }

To something like:

176 func (e *exchange) getAllBids(ctx context.Context, cleanRequests map[openrtb_ext.BidderName]*openrtb.BidRequest, aliases map[string]string, bidAdjustments map[string]float64, blabels map[openrtb_ext.BidderName]*pbsmetrics.AdapterLabels, conversions currencies.Conversions) (map[openrtb_ext.BidderName]*pbsOrtbSeatBid, map[openrtb_ext.BidderName]*seatResponseExtra, bool) {
177     // Set up pointers to the bid results
178     adapterBids := make(map[openrtb_ext.BidderName]*pbsOrtbSeatBid, len(cleanRequests))
179     adapterExtra := make(map[openrtb_ext.BidderName]*seatResponseExtra, len(cleanRequests))
180     bidsFound := false  // NEW
181     chBids := make(chan *bidResponseWrapper, len(cleanRequests))
182
183     for bidderName, req := range cleanRequests {
184         // Here we actually call the adapters and collect the bids.
185 +-- 45 lines: coreBidder := resolveBidder(string(bidderName), aliases)---------------------------------
230     }
231     // Wait for the bidders to do their thing
232     for range cleanRequests {
233         //for i := 0; i < len(cleanRequests); i++ {
234         brw := <-chBids
235         adapterBids[brw.bidder] = brw.adapterBids
236         if adapterBids[brw.bidder] != nil && len(adapterBids[brw.bidder].bids) > 0 { // NEW
237             bidsFound = true        // NEW
238         }                           // NEW
239         adapterExtra[brw.bidder] = brw.adapterExtra
240     }
241
242     return adapterBids, adapterExtra, bidsFound
243 }

What do you think? Please let me know if you agree with any of this.

[VeronikaSolovei9]

I think it's a good idea! Refactored.

[VeronikaSolovei9]

@mansinahar regarding unit tests. "Exchange" changes don't change a behavior of HoldAuction logic, this is just a minor optimization, and case with no bids returned is already covered by existing unit tests.
For Video endpoint I added simple unit test to check there is no error and empty response returned if there are no bids. All other cases are already covered.
Thank you!

[mansinahar]

Any reason to initialize podErrors with a capacity of 2 here?

Also, a minor suggestion (not a hard request): the next two lines can be potentially combined into: openRtbBidResp.SeatBid = make([]openrtb.SeatBid, 0) since seatBids is not used anywhere else.

[VeronikaSolovei9]

That was copy and paste, sorry. Fixed.

[mansinahar]

@Kalypsonika I see. Makes sense. Thanks!

[guscarreon]

LGTM, good job