Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CPMStar: Fixed for loop index reference bug #3604

Merged
merged 4 commits into from
Apr 29, 2024
Merged

CPMStar: Fixed for loop index reference bug #3604

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5c1bd8b
CPMStar Adapter - Fixed for loop index reference bug
JoshuaMGoldstein Apr 1, 2024
207c3d5
updated openrtb/v19 to openrtb/v20
JoshuaMGoldstein Apr 2, 2024
2a29f00
Merge branch 'prebid:master' into master
JoshuaMGoldstein Apr 9, 2024
781d9f4
CPMStar Adapter: added multi-banner test
JoshuaMGoldstein Apr 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions adapters/cpmstar/cpmstar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@JoshuaMGoldstein JoshuaMGoldstein Apr 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood that this is current flow. But I would highly recommend to use openrtb2.Bid.MType field here to get mediaTypeForImp.

Copy link
Contributor Author

@JoshuaMGoldstein JoshuaMGoldstein Apr 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@JoshuaMGoldstein JoshuaMGoldstein Apr 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

foundMatchingBid = true
if imp.Banner != nil {
bidType = openrtb_ext.BidTypeBanner
Expand All @@ -142,12 +142,12 @@ func (a *Adapter) MakeBids(bidRequest *openrtb2.BidRequest, unused *adapters.Req

if foundMatchingBid {
rv.Bids = append(rv.Bids, &adapters.TypedBid{
Bid: &bid,
Bid: &seatbid.Bid[i],
BidType: bidType,
})
} else {
errors = append(errors, &errortypes.BadServerResponse{
Message: fmt.Sprintf("bid id='%s' could not find valid impid='%s'", bid.ID, bid.ImpID),
Message: fmt.Sprintf("bid id='%s' could not find valid impid='%s'", seatbid.Bid[i].ID, seatbid.Bid[i].ImpID),
})
}
}
Expand Down
Loading