CPMStar: Fixed for loop index reference bug #3604
Conversation
Code coverage summaryNote:
cpmstarRefer here for heat map coverage report |
| @@ -125,11 +125,11 @@ func (a *Adapter) MakeBids(bidRequest *openrtb2.BidRequest, unused *adapters.Req | |||
| var errors []error | |||
|
|
|||
| for _, seatbid := range bidResponse.SeatBid { | |||
| for _, bid := range seatbid.Bid { | |||
| for i := range seatbid.Bid { | |||
There was a problem hiding this comment.
This should have been caught if you had a test case with multiple impressions returning multiple bids. Could you please create a json test case for the same?
There was a problem hiding this comment.
Ok I added a multi banner json test case under adapters/cpmstar/cpmstartest/exemplary/multiple-banners.json
| foundMatchingBid := false | ||
| bidType := openrtb_ext.BidTypeBanner | ||
| for _, imp := range bidRequest.Imp { | ||
| if imp.ID == bid.ImpID { | ||
| if imp.ID == seatbid.Bid[i].ImpID { |
There was a problem hiding this comment.
Understood that this is current flow. But I would highly recommend to use openrtb2.Bid.MType field here to get mediaTypeForImp.
There was a problem hiding this comment.
mtype is not a normal parameter we expect to receive from RTB 2.5 requests. According to the RTB 2.5 spec, we can only be sure to see banner, video objects, as we check for in the code.
Even if supported in RTB 2.6 or later, it does not seem like a good idea to make the code based on the presence of such parameters which are unlikely to be sent in earlier RTB version requests. So I prefer to leave the code as it is for back compatibility reasons
In any event the implementation is not wrong and seems to maximize compatibility..
There was a problem hiding this comment.
We're not proposing any alterations to the RTB request. Instead, we're requesting a modification to the seatBid response to enable the identification of adapters.TypedBid.BidType based on the Mtype value set in the bidder response.
I believe there won't be any backward compatibility concerns since we're not altering any existing aspects of the bidder response; you're solely populating the openrtb2.Bid.MType field in the response.
There was a problem hiding this comment.
I'm sorry but I am still not understanding.
Our server endpoint doesn't respond with MType since it doesnt support RTB 2.6 yet. So while I agree that mtype can (in theory) be available on the response, our ad-server endpoint which responds to the ultimate RTB request would be the one responsible for populating this field, if it wants to?
Again. looking at the banner and video objects seems to be more compatible. Perhaps in terms of endpoint bid responses, and not bid requests, but still its the same rationale, on the other side. Which in this case is in fact a real compatibility issue because our actual RTB 2.5 endpoint doesn't yet return seatbid[x].bid[y].mtype.
Furthermore, the bidType is already populated and returned in rv.Bids -- so it seems to be superflous.
I don't see any issue with how the adapter is programmed. Your request to modify the bid adapter code seems to extend beyond the actual bid adapter, rather, you are asking us to upgrade our RTB endpoints to support 2.6.
And I'm not sure why this is something Prebid Server should be concerned with -- since the whole point of PBS adapters is to be agnostic as to the RTB operation of the ultimate endpoints.
|
@JoshuaMGoldstein Requesting you to please add the open comments. |
Code coverage summaryNote:
cpmstarRefer here for heat map coverage report |
Code coverage summaryNote:
cpmstarRefer here for heat map coverage report |
|
It may make more sense to update Go instead here. This common bug has been remediated in the latest release - Go 1.22. |
We fixed a for loop index reference bug in CPMStar Adapter, per: https://go.dev/blog/loopvar-preview