Skip to content

Commit

Permalink
PubMatic Adslot validation (#886)
Browse files Browse the repository at this point in the history 
* testing a few changes to validate adslot, more to follow

* Changed AdSlot to optional parameter, modified validation and test cases for the same

* removed imp id from adslot validation error msgs

* assign banner size

* remove TrimSpace where it is not needed as recommended in the review
  • Loading branch information
@PubMatic-OpenWrap @mansinahar
PubMatic-OpenWrap authored and mansinahar committed May 20, 2019
1 parent f5c94ac commit a0043f5
Show file tree
Hide file tree
Showing 5 changed files with 116 additions and 40 deletions.
4 changes: 3 additions & 1 deletion adapters/pubmatic/params_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ func TestInvalidParams(t *testing.T) {
}

var validParams = []string{
`{"publisherId":"7890"}`,
`{"adSlot":"","publisherId":"7890"}`,
`{"adSlot":"AdTag_Div1","publisherId":"7890"}`,
`{"adSlot":"AdTag_Div1@728x90","publisherId":"7890"}`,
`{"adSlot":"AdTag_Div1@728x90","publisherId":"7890","keywords":[{"key": "pmZoneID", "value":["zone1"]},{"key": "dctr", "value":[ "v1","v2"]}]}`,
`{"adSlot":"AdTag_Div1@728x90","publisherId":"7890","keywords":[{"key": "pmZoneID", "value":["zone1", "zone2"]}], "wrapper":{"profile":5123}}`,
Expand All @@ -53,7 +56,6 @@ var invalidParams = []string{
`4.2`,
`[]`,
`{}`,
`{"publisherId":"7890"}`,
`{"adSlot":"AdTag_Div1@728x90:0"}`,
`{"adSlot":"AdTag_Div1@728x90:0","publisherId":1}`,
`{"adSlot":123,"publisherId":"7890"}`,
Expand Down
103 changes: 73 additions & 30 deletions adapters/pubmatic/pubmatic.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,73 @@ func (a *PubmaticAdapter) MakeRequests(request *openrtb.BidRequest) ([]*adapters
}}, errs
}

// validateAdslot validate the optional adslot string
// valid formats are 'adslot@WxH', 'adslot' and no adslot
func validateAdSlot(adslot string, imp *openrtb.Imp) error {
adSlotStr := strings.TrimSpace(adslot)

if len(adSlotStr) == 0 {
return nil
}

if !strings.Contains(adSlotStr, "@") {
imp.TagID = adSlotStr
return nil
}

adSlot := strings.Split(adSlotStr, "@")
if len(adSlot) == 2 && adSlot[0] != "" && adSlot[1] != "" {
imp.TagID = strings.TrimSpace(adSlot[0])

adSize := strings.Split(strings.ToLower(adSlot[1]), "x")
if len(adSize) != 2 {
return errors.New(fmt.Sprintf("Invalid size provided in adSlot %v", adSlotStr))
}

width, err := strconv.Atoi(strings.TrimSpace(adSize[0]))
if err != nil {
return errors.New(fmt.Sprintf("Invalid width provided in adSlot %v", adSlotStr))
}

heightStr := strings.Split(adSize[1], ":")
height, err := strconv.Atoi(strings.TrimSpace(heightStr[0]))
if err != nil {
return errors.New(fmt.Sprintf("Invalid height provided in adSlot %v", adSlotStr))
}

//In case of video, size could be derived from the player size
if imp.Banner != nil {
imp.Banner.H = openrtb.Uint64Ptr(uint64(height))
imp.Banner.W = openrtb.Uint64Ptr(uint64(width))
}
} else {
return errors.New(fmt.Sprintf("Invalid adSlot %v", adSlotStr))
}

return nil
}

func assignBannerSize(banner *openrtb.Banner) error {
if banner == nil {
return nil
}

if banner.W != nil && banner.H != nil {
return nil
}

if len(banner.Format) == 0 {
return errors.New(fmt.Sprintf("No sizes provided for Banner %v", banner.Format))
}

banner.W = new(uint64)
*banner.W = banner.Format[0].W
banner.H = new(uint64)
*banner.H = banner.Format[0].H

return nil
}

// parseImpressionObject parse the imp to get it ready to send to pubmatic
func parseImpressionObject(imp *openrtb.Imp, wrapExt *string, pubID *string) error {
// PubMatic supports banner and video impressions.
Expand Down Expand Up @@ -403,38 +470,14 @@ func parseImpressionObject(imp *openrtb.Imp, wrapExt *string, pubID *string) err
*wrapExt = string(pubmaticExt.WrapExt)
}

adSlotStr := strings.TrimSpace(pubmaticExt.AdSlot)

adSlot := strings.Split(adSlotStr, "@")
if len(adSlot) == 2 && adSlot[0] != "" && adSlot[1] != "" {
imp.TagID = strings.TrimSpace(adSlot[0])

adSize := strings.Split(strings.ToLower(strings.TrimSpace(adSlot[1])), "x")
if len(adSize) == 2 {
width, err := strconv.Atoi(strings.TrimSpace(adSize[0]))
if err != nil {
return errors.New("Invalid width provided in adSlot")
}
if err := validateAdSlot(strings.TrimSpace(pubmaticExt.AdSlot), imp); err != nil {
return err
}

heightStr := strings.Split(strings.TrimSpace(adSize[1]), ":")
height, err := strconv.Atoi(strings.TrimSpace(heightStr[0]))
if err != nil {
return errors.New("Invalid height provided in adSlot")
}
if imp.Banner != nil {
imp.Banner.H = openrtb.Uint64Ptr(uint64(height))
imp.Banner.W = openrtb.Uint64Ptr(uint64(width))
} /* In case of video, params.adSlot would always be in the format adunit@0x0,
so we are not replacing video.W and video.H with size passed in params.adSlot
else {
imp.Video.H = uint64(height)
imp.Video.W = uint64(width)
}*/
} else {
return errors.New("Invalid size provided in adSlot")
if imp.Banner != nil {
if err := assignBannerSize(imp.Banner); err != nil {
return err
}
} else {
return errors.New("Invalid adSlot provided")
}

if pubmaticExt.Keywords != nil && len(pubmaticExt.Keywords) != 0 {
Expand Down
42 changes: 36 additions & 6 deletions adapters/pubmatic/pubmatictest/supplemental/invalidparam.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
},
"ext": {
"bidder": {
"adSlot": "AdTag_Div1",
"adSlot": "AdTag_Div1@",
"publisherId": "999"
}
}
Expand Down Expand Up @@ -58,6 +58,34 @@
"publisherId": "999"
}
}
},{
"id": "test-imp-id-5",
"banner": {
"format": [{
"w": 300,
"h": 250
}]
},
"ext": {
"bidder": {
"adSlot": "AdTag_Div1@300x",
"publisherId": "999"
}
}
},{
"id": "test-imp-id-6",
"banner": {
"format": [{
"w": 300,
"h": 250
}]
},
"ext": {
"bidder": {
"adSlot": "AdTag_Div1@x250",
"publisherId": "999"
}
}
}],
"site": {
"id": "siteID",
Expand All @@ -68,9 +96,11 @@
},

"expectedMakeRequestsErrors": [
"Invalid adSlot provided",
"Invalid size provided in adSlot",
"Invalid width provided in adSlot",
"Invalid height provided in adSlot"
"Invalid adSlot AdTag_Div1@",
"Invalid size provided in adSlot AdTag_Div1@300",
"Invalid width provided in adSlot AdTag_Div1@valx250",
"Invalid height provided in adSlot AdTag_Div1@300xval",
"Invalid height provided in adSlot AdTag_Div1@300x",
"Invalid width provided in adSlot AdTag_Div1@x250"
]
}
}
5 changes: 3 additions & 2 deletions openrtb_ext/imp_pubmatic.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ package openrtb_ext
import "encoding/json"

// ExtImpPubmatic defines the contract for bidrequest.imp[i].ext.pubmatic
// PublisherId and adSlot are mandatory parameters, others are optional parameters
// PublisherId is mandatory parameters, others are optional parameters
// AdSlot is identifier for specific ad placement or ad tag
// Keywords is bid specific parameter,
// WrapExt needs to be sent once per bid request

Expand All @@ -14,7 +15,7 @@ type ExtImpPubmatic struct {
Keywords []*ExtImpPubmaticKeyVal `json:"keywords,omitempty"`
}

// ExtImpAppnexusKeyVal defines the contract for bidrequest.imp[i].ext.appnexus.keywords[i]
// ExtImpPubmaticKeyVal defines the contract for bidrequest.imp[i].ext.pubmatic.keywords[i]
type ExtImpPubmaticKeyVal struct {
Key string `json:"key,omitempty"`
Values []string `json:"value,omitempty"`
Expand Down
2 changes: 1 addition & 1 deletion static/bidder-params/pubmatic.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,5 +49,5 @@
}
}
},
"required": ["publisherId", "adSlot"]
"required": ["publisherId"]
}

0 comments on commit a0043f5

Please sign in to comment.