Pubmatic: Set bid.meta.mediaType=video when bid.ext.ibv=true #4189
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Code coverage summaryNote:
pubmaticRefer here for heat map coverage report |
adapters/pubmatic/pubmatictest/supplemental/bid_ext_ibv_true.json
Outdated
Show resolved
Hide resolved
Code coverage summaryNote:
pubmaticRefer here for heat map coverage report |
Code coverage summaryNote:
pubmaticRefer here for heat map coverage report |
Code coverage summaryNote:
pubmaticRefer here for heat map coverage report |
Code coverage summaryNote:
pubmaticRefer here for heat map coverage report |
pm-isha-bharti
previously approved these changes
Feb 18, 2025
|
@guscarreon / @scr-oath Can someone please review this PR? |
scr-oath
previously approved these changes
Mar 11, 2025
guscarreon
reviewed
Mar 11, 2025
| @@ -637,24 +649,27 @@ func getStringArray(array []interface{}) []string { | |||
| return aString | |||
| } | |||
|
|
|||
| // getBidType returns the bid type specified in the response bid.ext | |||
There was a problem hiding this comment.
Can we update the comment? getMediaTypeForBid doesn't return the bid type from bid.ext anymore.
There was a problem hiding this comment.
Yes sure
| bidType = openrtb_ext.BidTypeNative | ||
| mType := openrtb_ext.BidTypeBanner | ||
| if bid != nil { | ||
| switch bid.MType { |
There was a problem hiding this comment.
Are we sure from now on the media type will always be located in bid.MType and not in bid.Ext? Do you think we should look in bid.MType first and then bid.Ext second for backwards compatibility?
There was a problem hiding this comment.
Yes, we are sure on media type will always be located in bid.MType. I don't think we need to have any backward compatibility logic.
|
|
||
| typedBid.BidMeta = &openrtb_ext.ExtBidPrebidMeta{MediaType: string(mType)} | ||
| if bidExt.InBannerVideo { | ||
| typedBid.BidMeta.MediaType = string(openrtb_ext.BidTypeVideo) |
There was a problem hiding this comment.
Is the scenario where bidExt.InBannerVideo is true, but bidExt.VideoCreativeInfo or bidExt.VideoCreativeInfo.Duration is nil, valid? If not, should we validate and append an error to the errs array in such cases?
There was a problem hiding this comment.
Yes, It is valid case bidExt.VideoCreativeInfo and bidExt.VideoCreativeInfo.Duration is nil in case of bidExt.InBannerVideo is true.
adapters/pubmatic/pubmatic_test.go
Outdated
| } | ||
| if tt.expectedError != nil && actualError == nil { | ||
| t.Errorf("Expected error: %v, but got nil", tt.expectedError) | ||
| } else if tt.expectedError != nil && actualError != nil && actualError.Error() != tt.expectedError.Error() { |
There was a problem hiding this comment.
The if-else statement seems to not compare an tt.expectedError equal to nil. If tt.expectedError equals nil, shouldn't we assert that actualError is also nil?
There was a problem hiding this comment.
Added check if expectedError is nil.
Modified code accordingly.
| @@ -461,26 +462,37 @@ func (a *PubmaticAdapter) MakeBids(internalRequest *openrtb2.BidRequest, externa | |||
| bid.Cat = bid.Cat[0:1] | |||
| } | |||
|
|
|||
| mType, err := getMediaTypeForBid(&bid) | |||
| if err != nil { | |||
There was a problem hiding this comment.
Can we write a JSON test case to cover the error scenario?
There was a problem hiding this comment.
Yes Added test case
|
Hi @pm-priyanka-bagade, please see recent comments. |
pm-priyanka-bagade
dismissed stale reviews from scr-oath and pm-isha-bharti
via
dd73a53
Code coverage summaryNote:
pubmaticRefer here for heat map coverage report |
|
@guscarreon Can you please review this PR again? |
bsardo
reviewed
Apr 22, 2025
adapters/pubmatic/pubmatic_test.go
Outdated
Comment on lines
417
to
570
| // args: args{ | ||
| // response: &adapters.ResponseData{ | ||
| // StatusCode: http.StatusOK, | ||
| // Body: []byte(`{"id": "test-request-id", "seatbid":[{"seat": "958", "bid":[{"id": "7706636740145184841", "impid": "test-imp-id", "price": 0.500000, "adid": "29681110", "adm": "some-test-ad", "adomain":["pubmatic.com"], "crid": "29681110", "h": 250, "w": 300, "dealid": "testdeal","mtype": 1, "ext":{"dspid": 6, "deal_channel": 1, "prebiddealpriority": -1}}]}], "bidid": "5778926625248726496", "cur": "USD"}`), | ||
| // }, | ||
| // }, | ||
| // wantErr: nil, | ||
| // wantResp: &adapters.BidderResponse{ | ||
| // Bids: []*adapters.TypedBid{ | ||
| // { | ||
| // Bid: &openrtb2.Bid{ | ||
| // ID: "7706636740145184841", | ||
| // ImpID: "test-imp-id", | ||
| // Price: 0.500000, | ||
| // AdID: "29681110", | ||
| // AdM: "some-test-ad", | ||
| // ADomain: []string{"pubmatic.com"}, | ||
| // CrID: "29681110", | ||
| // H: 250, | ||
| // W: 300, | ||
| // DealID: "testdeal", | ||
| // MType: 1, | ||
| // Ext: json.RawMessage(`{"dspid": 6, "deal_channel": 1, "prebiddealpriority": -1}`), | ||
| // }, | ||
| // BidType: openrtb_ext.BidTypeBanner, | ||
| // BidVideo: &openrtb_ext.ExtBidPrebidVideo{}, | ||
| // BidMeta: &openrtb_ext.ExtBidPrebidMeta{ | ||
| // AdapterCode: "", | ||
| // AdvertiserDomains: nil, | ||
| // AdvertiserID: 0, | ||
| // AdvertiserName: "", | ||
| // AgencyID: 0, | ||
| // AgencyName: "", | ||
| // DChain: nil, | ||
| // DemandSource: "", | ||
| // MediaType: "banner", | ||
| // NetworkID: 0, | ||
| // NetworkName: "", | ||
| // RendererName: "", | ||
| // RendererVersion: "", | ||
| // RendererData: nil, | ||
| // RendererUrl: "", | ||
| // SecondaryCategoryIDs: nil, | ||
| // }, | ||
| // }, | ||
| // }, | ||
| // Currency: "USD", | ||
| // }, | ||
| // }, | ||
| // { | ||
| // name: "bid_ext_InBannerVideo_is_true", | ||
| // args: args{ | ||
| // response: &adapters.ResponseData{ | ||
| // StatusCode: http.StatusOK, | ||
| // Body: []byte(`{"id": "test-request-id", "seatbid":[{"seat": "958", "bid":[{"id": "7706636740145184841", "impid": "test-imp-id", "price": 0.500000, "adid": "29681110", "adm": "some-test-ad", "adomain":["pubmatic.com"], "crid": "29681110", "h": 250, "w": 300, "dealid": "testdeal", "mtype": 1, "ext":{"dspid": 6, "deal_channel": 1, "prebiddealpriority": -1, "ibv": true}}]}], "bidid": "5778926625248726496", "cur": "USD"}`), | ||
| // }, | ||
| // }, | ||
| // wantErr: nil, | ||
| // wantResp: &adapters.BidderResponse{ | ||
| // Bids: []*adapters.TypedBid{ | ||
| // { | ||
| // Bid: &openrtb2.Bid{ | ||
| // ID: "7706636740145184841", | ||
| // ImpID: "test-imp-id", | ||
| // Price: 0.500000, | ||
| // AdID: "29681110", | ||
| // AdM: "some-test-ad", | ||
| // ADomain: []string{"pubmatic.com"}, | ||
| // CrID: "29681110", | ||
| // H: 250, | ||
| // W: 300, | ||
| // DealID: "testdeal", | ||
| // MType: 1, | ||
| // Ext: json.RawMessage(`{"dspid": 6, "deal_channel": 1, "prebiddealpriority": -1, "ibv": true}`), | ||
| // }, | ||
| // BidType: openrtb_ext.BidTypeBanner, | ||
| // BidVideo: &openrtb_ext.ExtBidPrebidVideo{}, | ||
| // BidMeta: &openrtb_ext.ExtBidPrebidMeta{ | ||
| // AdapterCode: "", | ||
| // AdvertiserDomains: nil, | ||
| // AdvertiserID: 0, | ||
| // AdvertiserName: "", | ||
| // AgencyID: 0, | ||
| // AgencyName: "", | ||
| // DChain: nil, | ||
| // DemandSource: "", | ||
| // MediaType: "video", | ||
| // NetworkID: 0, | ||
| // NetworkName: "", | ||
| // RendererName: "", | ||
| // RendererVersion: "", | ||
| // RendererData: nil, | ||
| // RendererUrl: "", | ||
| // SecondaryCategoryIDs: nil, | ||
| // }, | ||
| // }, | ||
| // }, | ||
| // Currency: "USD", | ||
| // }, | ||
| // }, |
There was a problem hiding this comment.
@pm-priyanka-bagade please address the commented out test cases.
There was a problem hiding this comment.
So sorry i forgot to uncomment test cases
Code coverage summaryNote:
pubmaticRefer here for heat map coverage report |
|
Please check @guscarreon |
guscarreon
reviewed
Apr 29, 2025
adapters/pubmatic/pubmatic_test.go
Outdated
| t.Errorf("Expected Bid Type value was: %v, actual value is: %v", tt.expectedMType, actualBidTypeValue) | ||
| } | ||
| if tt.expectedError == nil { | ||
| if actualError != nil { |
There was a problem hiding this comment.
I'm ok with this approach but, wouldn't an assert.Equal() call produce the same results?
81 if actualBidTypeValue != tt.expectedMType {
82 t.Errorf("Expected Bid Type value was: %v, actual value is: %v", tt.expectedMType, actualBidTypeValue)
83 }
+ assert.Equal(t, tt.expectedError, actualError)
84 - if tt.expectedError == nil {
85 - if actualError != nil {
86 - t.Errorf("Expected no error, but got: %v", actualError)
87 - }
88 - } else {
89 - if actualError == nil {
90 - t.Errorf("Expected error: %v, but got nil", tt.expectedError)
91 - } else if actualError.Error() != tt.expectedError.Error() {
92 - t.Errorf("Expected error: %v, but got: %v", tt.expectedError, actualError)
93 - }
94 - }
95 })
adapters/pubmatic/pubmatic_test.go
There was a problem hiding this comment.
Yes you are right
Made changes
adapters/pubmatic/pubmatic.go
Outdated
| if err != nil { | ||
| errs = append(errs, err) | ||
| } else if bidExt != nil { | ||
| typedBid.Seat = openrtb_ext.BidderName(bidExt.Marketplace) | ||
| typedBid.BidType = getBidType(bidExt) | ||
| typedBid.BidType = mType |
There was a problem hiding this comment.
The changes implemented in this pull request make that setting typedBid.BidType does not depend on bidExt not being nil because we're now taking mType from bid.MType and not from bid.Ext.BidType. Do we still need to assign typedBid.BidType = mType inside the if bidExt != nil statement?
470
471 typedBid := &adapters.TypedBid{
472 Bid: &bid,
473 BidType: openrtb_ext.BidTypeBanner,
474 BidVideo: &openrtb_ext.ExtBidPrebidVideo{},
+ BidType: mType,
475 }
476
477 var bidExt *pubmaticBidExt
478 err = jsonutil.Unmarshal(bid.Ext, &bidExt)
479 if err != nil {
480 errs = append(errs, err)
481 } else if bidExt != nil {
482 typedBid.Seat = openrtb_ext.BidderName(bidExt.Marketplace)
483 - typedBid.BidType = mType
484 if bidExt.PrebidDealPriority > 0 {
485 typedBid.DealPriority = bidExt.PrebidDealPriority
486 }
adapters/pubmatic/pubmatic.go
There was a problem hiding this comment.
yes you are right,
anyway we are setting default value in getMediaTypeForBid as "banner" and we don't need to assign in if block.
Code coverage summaryNote:
pubmaticRefer here for heat map coverage report |
|
@guscarreon / @bsardo |
Code coverage summaryNote:
pubmaticRefer here for heat map coverage report |
|
@guscarreon Could you please review it again? Thanks! |
scr-oath
reviewed
Jun 9, 2025
adapters/pubmatic/pubmatic_test.go
Outdated
Comment on lines
441
to
458
| BidMeta: &openrtb_ext.ExtBidPrebidMeta{ | ||
| AdapterCode: "", | ||
| AdvertiserDomains: nil, | ||
| AdvertiserID: 0, | ||
| AdvertiserName: "", | ||
| AgencyID: 0, | ||
| AgencyName: "", | ||
| DChain: nil, | ||
| DemandSource: "", | ||
| MediaType: "banner", | ||
| NetworkID: 0, | ||
| NetworkName: "", | ||
| RendererName: "", | ||
| RendererVersion: "", | ||
| RendererData: nil, | ||
| RendererUrl: "", | ||
| SecondaryCategoryIDs: nil, | ||
| }, |
There was a problem hiding this comment.
nitpick: reduce complexity by omitting "zero values"
Suggested change
| BidMeta: &openrtb_ext.ExtBidPrebidMeta{ | |
| AdapterCode: "", | |
| AdvertiserDomains: nil, | |
| AdvertiserID: 0, | |
| AdvertiserName: "", | |
| AgencyID: 0, | |
| AgencyName: "", | |
| DChain: nil, | |
| DemandSource: "", | |
| MediaType: "banner", | |
| NetworkID: 0, | |
| NetworkName: "", | |
| RendererName: "", | |
| RendererVersion: "", | |
| RendererData: nil, | |
| RendererUrl: "", | |
| SecondaryCategoryIDs: nil, | |
| }, | |
| BidMeta: &openrtb_ext.ExtBidPrebidMeta{ | |
| MediaType: "banner", | |
| }, |
Code coverage summaryNote:
pubmaticRefer here for heat map coverage report |
scr-oath
reviewed
Jun 17, 2025
adapters/pubmatic/pubmatic.go
Outdated
| // default value is banner | ||
| bidType = openrtb_ext.BidTypeBanner | ||
| return "", &errortypes.BadServerResponse{ | ||
| Message: fmt.Sprintf("failed to parse bid mtype (%d) for impression id %s", bid.MType, bid.ImpID), |
There was a problem hiding this comment.
nitpick: superfluous space.
Suggested change
| Message: fmt.Sprintf("failed to parse bid mtype (%d) for impression id %s", bid.MType, bid.ImpID), | |
| Message: fmt.Sprintf("failed to parse bid mtype (%d) for impression id %s", bid.MType, bid.ImpID), |
There was a problem hiding this comment.
Removed extra space
adapters/pubmatic/pubmatic_test.go
Outdated
Comment on lines
85
to
89
| if tt.expectedError != nil { | ||
| assert.EqualError(t, actualError, tt.expectedError.Error(), "unexpected error message") | ||
| } else { | ||
| assert.NoError(t, actualError, "expected no error but got one") | ||
| } |
There was a problem hiding this comment.
suggestion: Use require for things that cannot proceed if unmet. Also, in the expect error case, use the guard pattern (merely return rather than needing else clause).
Suggested change
| if tt.expectedError != nil { | |
| assert.EqualError(t, actualError, tt.expectedError.Error(), "unexpected error message") | |
| } else { | |
| assert.NoError(t, actualError, "expected no error but got one") | |
| } | |
| if tt.expectedError != nil { | |
| require.EqualError(t, actualError, tt.expectedError.Error(), "unexpected error message") | |
| return | |
| } | |
| require.NoError(t, actualError, "expected no error but got one") |
There was a problem hiding this comment.
Thank you
Updated code with require.
guscarreon
reviewed
Jun 23, 2025
| @@ -34,6 +34,7 @@ type pubmaticBidExt struct { | |||
| VideoCreativeInfo *pubmaticBidExtVideo `json:"video,omitempty"` | |||
| Marketplace string `json:"marketplace,omitempty"` | |||
| PrebidDealPriority int `json:"prebiddealpriority,omitempty"` | |||
| InBannerVideo bool `json:"ibv,omitempty"` | |||
There was a problem hiding this comment.
Now that getBidType(bidExt *pubmaticBidExt) has become getMediaTypeForBid(bid *openrtb2.Bid), the BidType *int json:"BidType,omitempty" field seems to not be used anywhere in your adapter's code anymore. Should we remove?
There was a problem hiding this comment.
Yes, removed BidType from adapter code
| if bidExt.PrebidDealPriority > 0 { | ||
| typedBid.DealPriority = bidExt.PrebidDealPriority | ||
| } | ||
|
|
||
| if bidExt.VideoCreativeInfo != nil && bidExt.VideoCreativeInfo.Duration != nil { | ||
| typedBid.BidVideo.Duration = *bidExt.VideoCreativeInfo.Duration | ||
| } | ||
|
|
||
| typedBid.BidMeta = &openrtb_ext.ExtBidPrebidMeta{MediaType: string(mType)} | ||
| if bidExt.InBannerVideo { |
There was a problem hiding this comment.
Suppose a native type bid comes and, for some unknown reason, its bidExt.InBannerVideo is set to true. Would it be misleading to set typedBid.BidMeta.MediaType to "video" given that this is not a banner? In other words, would it be desirable to add an additional check here to make sure this is really a banner that contains a video?
There was a problem hiding this comment.
This scenario will not occur on Pubmatic-openwrap side. It will be handled by Pubmatic SSP side.
So, we don't need any extra check for this scenario.
Code coverage summaryNote:
pubmaticRefer here for heat map coverage report |
scr-oath
approved these changes
Jun 25, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.