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.

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

Pubmatic: Forward displaymanager and displaymanagerver from app ext #4000

Merged
merged 4 commits into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
31 changes: 29 additions & 2 deletions adapters/pubmatic/pubmatic.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,11 @@ func (a *PubmaticAdapter) MakeRequests(request *openrtb2.BidRequest, reqInfo *ad
extractWrapperExtFromImp := true
extractPubIDFromImp := true

displayManager, displayManagerVer := "", ""
if request.App != nil && request.App.Ext != nil {
displayManager, displayManagerVer = getDisplayManagerAndVer(request.App)
}

newReqExt, err := extractPubmaticExtFromRequest(request)
if err != nil {
return nil, []error{err}
Expand All @@ -99,7 +104,7 @@ func (a *PubmaticAdapter) MakeRequests(request *openrtb2.BidRequest, reqInfo *ad
}

for i := 0; i < len(request.Imp); i++ {
wrapperExtFromImp, pubIDFromImp, err := parseImpressionObject(&request.Imp[i], extractWrapperExtFromImp, extractPubIDFromImp)
wrapperExtFromImp, pubIDFromImp, err := parseImpressionObject(&request.Imp[i], extractWrapperExtFromImp, extractPubIDFromImp, displayManager, displayManagerVer)

// If the parsing is failed, remove imp and add the error.
if err != nil {
Expand Down Expand Up @@ -246,7 +251,7 @@ func assignBannerWidthAndHeight(banner *openrtb2.Banner, w, h int64) *openrtb2.B
}

// parseImpressionObject parse the imp to get it ready to send to pubmatic
func parseImpressionObject(imp *openrtb2.Imp, extractWrapperExtFromImp, extractPubIDFromImp bool) (*pubmaticWrapperExt, string, error) {
func parseImpressionObject(imp *openrtb2.Imp, extractWrapperExtFromImp, extractPubIDFromImp bool, displayManager, displayManagerVer string) (*pubmaticWrapperExt, string, error) {
var wrapExt *pubmaticWrapperExt
var pubID string

Expand All @@ -259,6 +264,12 @@ func parseImpressionObject(imp *openrtb2.Imp, extractWrapperExtFromImp, extractP
imp.Audio = nil
}

// Populate imp.displaymanager and imp.displaymanagerver if the SDK failed to do it.
if imp.DisplayManager == "" && imp.DisplayManagerVer == "" && displayManager != "" && displayManagerVer != "" {
imp.DisplayManager = displayManager
imp.DisplayManagerVer = displayManagerVer
}

var bidderExt ExtImpBidderPubmatic
if err := json.Unmarshal(imp.Ext, &bidderExt); err != nil {
return wrapExt, pubID, err
Expand Down Expand Up @@ -655,3 +666,19 @@ func Builder(bidderName openrtb_ext.BidderName, config config.Adapter, server co
}
return bidder, nil
}

// getDisplayManagerAndVer returns the display manager and version from the request.app.ext or request.app.prebid.ext source and version
func getDisplayManagerAndVer(app *openrtb2.App) (string, string) {
if source, err := jsonparser.GetString(app.Ext, openrtb_ext.PrebidExtKey, "source"); err == nil && source != "" {
if version, err := jsonparser.GetString(app.Ext, openrtb_ext.PrebidExtKey, "version"); err == nil && version != "" {
return source, version
}
}

if source, err := jsonparser.GetString(app.Ext, "source"); err == nil && source != "" {
if version, err := jsonparser.GetString(app.Ext, "version"); err == nil && version != "" {
return source, version
}
}
Comment on lines +678 to +682
Copy link
Collaborator

Choose a reason for hiding this comment

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

As mentioned earlier here, you're missing JSON test coverage.
Please add another display manager JSON test where the source and version are present at app.ext but not at app.ext.prebid to cover these lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return "", ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

As mentioned earlier here, you're missing JSON test coverage. Please either add a new JSON test or modify an existing JSON test so app.ext is present but without source or version being set so this default case is covered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}
224 changes: 212 additions & 12 deletions adapters/pubmatic/pubmatic_test.go
Copy link
Contributor

Choose a reason for hiding this comment

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

Coded unit tests are fine, but we look at the json test coverage since that includes additional assertions for memory safety. Please add json tests to cover this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the test cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

@SyntaxNode Can you please review the latest changes?

Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,22 @@ func TestParseImpressionObject(t *testing.T) {
imp *openrtb2.Imp
extractWrapperExtFromImp bool
extractPubIDFromImp bool
displayManager string
displayManagerVer string
}
type want struct {
bidfloor float64
impExt json.RawMessage
displayManager string
displayManagerVer string
}
tests := []struct {
name string
args args
expectedWrapperExt *pubmaticWrapperExt
expectedPublisherId string
want want
wantErr bool
expectedBidfloor float64
}{
{
name: "imp.bidfloor empty and kadfloor set",
Expand All @@ -97,7 +105,9 @@ func TestParseImpressionObject(t *testing.T) {
Ext: json.RawMessage(`{"bidder":{"kadfloor":"0.12"}}`),
},
},
expectedBidfloor: 0.12,
want: want{
bidfloor: 0.12,
},
},
{
name: "imp.bidfloor set and kadfloor empty",
Expand All @@ -108,7 +118,9 @@ func TestParseImpressionObject(t *testing.T) {
Ext: json.RawMessage(`{"bidder":{}}`),
},
},
expectedBidfloor: 0.12,
want: want{
bidfloor: 0.12,
},
},
{
name: "imp.bidfloor set and kadfloor invalid",
Expand All @@ -119,8 +131,9 @@ func TestParseImpressionObject(t *testing.T) {
Ext: json.RawMessage(`{"bidder":{"kadfloor":"aaa"}}`),
},
},
expectedBidfloor: 0.12,
},
want: want{
bidfloor: 0.12,
}},
{
name: "imp.bidfloor set and kadfloor set, higher imp.bidfloor",
args: args{
Expand All @@ -130,8 +143,9 @@ func TestParseImpressionObject(t *testing.T) {
Ext: json.RawMessage(`{"bidder":{"kadfloor":"0.11"}}`),
},
},
expectedBidfloor: 0.12,
},
want: want{
bidfloor: 0.12,
}},
{
name: "imp.bidfloor set and kadfloor set, higher kadfloor",
args: args{
Expand All @@ -141,8 +155,9 @@ func TestParseImpressionObject(t *testing.T) {
Ext: json.RawMessage(`{"bidder":{"kadfloor":"0.13"}}`),
},
},
expectedBidfloor: 0.13,
},
want: want{
bidfloor: 0.13,
}},
{
name: "kadfloor string set with whitespace",
args: args{
Expand All @@ -152,16 +167,72 @@ func TestParseImpressionObject(t *testing.T) {
Ext: json.RawMessage(`{"bidder":{"kadfloor":" \t 0.13 "}}`),
},
},
expectedBidfloor: 0.13,
want: want{
bidfloor: 0.13,
}},
{
name: "Populate imp.displaymanager and imp.displaymanagerver if both are empty in imp",
args: args{
imp: &openrtb2.Imp{
Video: &openrtb2.Video{},
Ext: json.RawMessage(`{"bidder":{"kadfloor":"0.12"}}`),
},
displayManager: "prebid-mobile",
displayManagerVer: "1.0.0",
},
want: want{
bidfloor: 0.12,
impExt: json.RawMessage(nil),
displayManager: "prebid-mobile",
displayManagerVer: "1.0.0",
},
},
{
name: "do not populate imp.displaymanager and imp.displaymanagerver in imp if only displaymanager or displaymanagerver is present in args",
args: args{
imp: &openrtb2.Imp{
Video: &openrtb2.Video{},
Ext: json.RawMessage(`{"bidder":{"kadfloor":"0.12"}}`),
DisplayManagerVer: "1.0.0",
},
displayManager: "prebid-mobile",
displayManagerVer: "1.0.0",
},
want: want{
bidfloor: 0.12,
impExt: json.RawMessage(nil),
displayManagerVer: "1.0.0",
},
},
{
name: "do not populate imp.displaymanager and imp.displaymanagerver if already present in imp",
args: args{
imp: &openrtb2.Imp{
Video: &openrtb2.Video{},
Ext: json.RawMessage(`{"bidder":{"kadfloor":"0.12"}}`),
DisplayManager: "prebid-mobile",
DisplayManagerVer: "1.0.0",
},
displayManager: "prebid-android",
displayManagerVer: "2.0.0",
},
want: want{
bidfloor: 0.12,
impExt: json.RawMessage(nil),
displayManager: "prebid-mobile",
displayManagerVer: "1.0.0",
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
receivedWrapperExt, receivedPublisherId, err := parseImpressionObject(tt.args.imp, tt.args.extractWrapperExtFromImp, tt.args.extractPubIDFromImp)
receivedWrapperExt, receivedPublisherId, err := parseImpressionObject(tt.args.imp, tt.args.extractWrapperExtFromImp, tt.args.extractPubIDFromImp, tt.args.displayManager, tt.args.displayManagerVer)
assert.Equal(t, tt.wantErr, err != nil)
assert.Equal(t, tt.expectedWrapperExt, receivedWrapperExt)
assert.Equal(t, tt.expectedPublisherId, receivedPublisherId)
assert.Equal(t, tt.expectedBidfloor, tt.args.imp.BidFloor)
assert.Equal(t, tt.want.bidfloor, tt.args.imp.BidFloor)
assert.Equal(t, tt.want.displayManager, tt.args.imp.DisplayManager)
assert.Equal(t, tt.want.displayManagerVer, tt.args.imp.DisplayManagerVer)
})
}
}
Expand Down Expand Up @@ -716,3 +787,132 @@ func TestGetMapFromJSON(t *testing.T) {
})
}
}

func TestGetDisplayManagerAndVer(t *testing.T) {
type args struct {
app *openrtb2.App
}
type want struct {
displayManager string
displayManagerVer string
}
tests := []struct {
name string
args args
want want
}{
{
name: "request app object is not nil but app.ext has no source and version",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please update the name strings throughout this test substituting dashes or underscores for spaces. t.Run does this automatically so doing this in the tests makes it easy to search for failed tests in the code base.
e.g. name: "request-app-object-is-not-nil-but-app.ext-has-no-source-and-version"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

args: args{

app: &openrtb2.App{
Name: "AutoScout24",
Ext: json.RawMessage(`{}`),
},
},
want: want{
displayManager: "",
displayManagerVer: "",
},
},
{
name: "request app object is not nil and app.ext has source and version",
args: args{

app: &openrtb2.App{
Name: "AutoScout24",
Ext: json.RawMessage(`{"source":"prebid-mobile","version":"1.0.0"}`),
},
},
want: want{
displayManager: "prebid-mobile",
displayManagerVer: "1.0.0",
},
},
{
name: "request app object is not nil and app.ext.prebid has source and version",
args: args{
app: &openrtb2.App{
Name: "AutoScout24",
Ext: json.RawMessage(`{"prebid":{"source":"prebid-mobile","version":"1.0.0"}}`),
},
},
want: want{
displayManager: "prebid-mobile",
displayManagerVer: "1.0.0",
},
},
{
name: "request app object is not nil and app.ext has only version",
args: args{
app: &openrtb2.App{
Name: "AutoScout24",
Ext: json.RawMessage(`{"version":"1.0.0"}`),
},
},
want: want{
displayManager: "",
displayManagerVer: "",
},
},
{
name: "request app object is not nil and app.ext has only source",
args: args{
app: &openrtb2.App{
Name: "AutoScout24",
Ext: json.RawMessage(`{"source":"prebid-mobile"}`),
},
},
want: want{
displayManager: "",
displayManagerVer: "",
},
},
{
name: "request app object is not nil and app.ext have empty source but version is present",
args: args{
app: &openrtb2.App{
Name: "AutoScout24",
Ext: json.RawMessage(`{"source":"", "version":"1.0.0"}`),
},
},
want: want{
displayManager: "",
displayManagerVer: "",
},
},
{
name: "request app object is not nil and app.ext have empty version but source is present",
args: args{
app: &openrtb2.App{
Name: "AutoScout24",
Ext: json.RawMessage(`{"source":"prebid-mobile", "version":""}`),
},
},
want: want{
displayManager: "",
displayManagerVer: "",
},
},
{
name: "request app object is not nil and both app.ext and app.ext.prebid have source and version",
args: args{
app: &openrtb2.App{
Name: "AutoScout24",
Ext: json.RawMessage(`{"source":"prebid-mobile-android","version":"2.0.0","prebid":{"source":"prebid-mobile","version":"1.0.0"}}`),
},
},
want: want{
displayManager: "prebid-mobile",
displayManagerVer: "1.0.0",
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
displayManager, displayManagerVer := getDisplayManagerAndVer(tt.args.app)
assert.Equal(t, tt.want.displayManager, displayManager)
assert.Equal(t, tt.want.displayManagerVer, displayManagerVer)
})
}
}
Loading
Loading