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

Jump to bottom

PulsePoint: Marking cp/ct params to be either String or Int #3677

Merged
merged 7 commits into from
Jul 1, 2024
Merged

PulsePoint: Marking cp/ct params to be either String or Int #3677

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3730923
Marking cp/ct params to be either String or Int
anand-venkatraman May 9, 2024
929b96b
fixing test
anand-venkatraman May 9, 2024
e732c3f
adding tests for valid string parameters for ct/cp
anand-venkatraman Jun 3, 2024
4539af1
Adding json framework tests and some fixes
anand-venkatraman Jun 12, 2024
394b0da
addressing review comment to use StringInt
anand-venkatraman Jun 28, 2024
0a2a52c
running gofmt
anand-venkatraman Jun 28, 2024
f4c20f4
minor refactor
anand-venkatraman Jun 28, 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
7 changes: 3 additions & 4 deletions adapters/pulsepoint/params_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ func TestInvalidParams(t *testing.T) {
var validParams = []string{
`{"cp":1000, "ct": 2000}`,
`{"cp":1001, "ct": 2001}`,
`{"cp":"1000", "ct": "2000"}`,
`{"cp":"1000", "ct": 2000}`,
`{"cp":1000, "ct": "2000"}`,
`{"cp":1001, "ct": 2001, "cf": "1x1"}`,
}

Expand All @@ -52,8 +55,4 @@ var invalidParams = []string{
`{"ct":"1000"}`,
`{"cp":1000}`,
`{"ct":1000}`,
`{"cp":1000, "ct":"1000"}`,
`{"cp":1000, "ct": "abcd"}`,
`{"cp":"abcd", "ct": 1000}`,
`{"cp":"1000.2", "ct": "1000.1"}`,
}
35 changes: 32 additions & 3 deletions adapters/pulsepoint/pulsepoint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package pulsepoint

import (
"encoding/json"
"errors"
"fmt"
"net/http"
"strconv"
Expand Down Expand Up @@ -49,11 +50,25 @@ func (a *PulsePointAdapter) MakeRequests(request *openrtb2.BidRequest, reqInfo *
continue
}
// parse pubid and keep it for reference
if pubID == "" && pulsepointExt.PubID > 0 {
pubID = strconv.Itoa(pulsepointExt.PubID)
if pubID == "" {
pubID, err = parseParam("pubID", pulsepointExt.PubID)
if err != nil {
errs = append(errs, &errortypes.BadInput{
Message: err.Error(),
})
continue
}
}
// tag id to be sent
imp.TagID = strconv.Itoa(pulsepointExt.TagID)
var tagID string
tagID, err = parseParam("tagID", pulsepointExt.TagID)
if err != nil {
errs = append(errs, &errortypes.BadInput{
Message: err.Error(),
})
continue
}
imp.TagID = tagID
imps = append(imps, imp)
}

Expand Down Expand Up @@ -163,3 +178,17 @@ func getBidType(imp openrtb2.Imp) openrtb_ext.BidType {
}
return ""
}

func parseParam(paramName string, paramValue interface{}) (string, error) {
if paramValue != nil {
extractedValue := fmt.Sprint(paramValue)
_, err := strconv.Atoi(extractedValue)
if err != nil {
return "", errors.New("expected int value for param - " + paramName + ", got value - " + extractedValue)
} else {
return extractedValue, nil
}
} else {
return "", errors.New("param not found - " + paramName)
}
}
102 changes: 102 additions & 0 deletions adapters/pulsepoint/pulsepointtest/exemplary/banner-string-bidder-params.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
{
"mockBidRequest": {
"id": "request-id",
"site": {
"page": "http://publisher.com/index.html",
"publisher": {
"id": "123456789",
"name": "publisher.com"
}
},
"imp": [{
"id": "banner-1",
"banner": {
"w": 320,
"h": 50
},
"ext": {
"bidder": {
"cp": "1234",
"ct": "1001"
}
}
}]
},
"httpCalls": [{
"expectedRequest": {
"headers": {
"Content-Type": [
"application/json;charset=utf-8"
],
"Accept": [
"application/json"
]
},
"uri": "http://bidder.pulsepoint.com",
"body": {
"id": "request-id",
"site": {
"page": "http://publisher.com/index.html",
"publisher": {
"id": "1234",
"name": "publisher.com"
}
},
"imp": [{
"id": "banner-1",
"tagid": "1001",
"banner": {
"w": 320,
"h": 50
},
"ext": {
"bidder": {
"cp": "1234",
"ct": "1001"
}
}
}]
},
"impIDs":["banner-1"]
},
"mockResponse": {
"status": 200,
"body": {
"id": "response-id",
"seatbid": [{
"bid": [{
"id": "banner-1-bid",
"impid": "banner-1",
"price": 3.5,
"adm": "<div>Creative</div>",
"adomain": [
"advertiser.com"
],
"crid": "20",
"w": 300,
"h": 250
}],
"seat": "pulsepoint-seat"
}],
"cur": "USD"
}
}
}],
"expectedBidResponses": [{
"bids": [{
"bid": {
"id": "banner-1-bid",
"impid": "banner-1",
"price": 3.5,
"adm": "<div>Creative</div>",
"adomain": [
"advertiser.com"
],
"crid": "20",
"w": 300,
"h": 250
},
"type": "banner"
}]
}]
}
31 changes: 31 additions & 0 deletions adapters/pulsepoint/pulsepointtest/supplemental/bad-prebid-params-missing-param.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
{
"mockBidRequest": {
"id": "request-id",
"site": {
"page": "http://publisher.com/index.html",
"publisher": {
"id": "123456789",
"name": "publisher.com"
}
},
"imp": [{
"id": "banner-1",
"banner": {
"w": 320,
"h": 50
},
"ext": {
"bidder": {
"cp": "1234"
}
}
}]
},
"httpCalls": [],
"expectedMakeRequestsErrors": [{
"value": "param not found - tagID",
"comparison": "literal"
}],
"expectedBidResponses": [],
"expectedMakeBidsErrors": []
}
4 changes: 2 additions & 2 deletions adapters/pulsepoint/pulsepointtest/supplemental/bad-prebid-params.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@
"ext": {
"bidder": {
"cp": "1234",
"ct": "1001"
"ct": "ab1001"
}
}
}]
},
"httpCalls": [],
"expectedMakeRequestsErrors": [{
"value": "json: cannot unmarshal string into Go struct field ExtImpPulsePoint.cp of type int",
"value": "expected int value for param - tagID, got value - ab1001",
"comparison": "literal"
}],
"expectedBidResponses": [],
Expand Down
4 changes: 2 additions & 2 deletions openrtb_ext/imp_pulsepoint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,6 @@ package openrtb_ext
// PubId/TagId are mandatory params

type ExtImpPulsePoint struct {
PubID int `json:"cp"`
TagID int `json:"ct"`
PubID interface{} `json:"cp"`
TagID interface{} `json:"ct"`
Copy link
Contributor

Choose a reason for hiding this comment

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

PubID and TagID are expected to be int or string.
However interface{} will allow int, string and any other values

Should use StringInt type

prebid-server/util/jsonutil/stringInt.go

Lines 1 to 29 in a1f48b5

package jsonutil
import (
"github.com/buger/jsonparser"
)
type StringInt int
func (st *StringInt) UnmarshalJSON(b []byte) error {
if len(b) == 0 {
return nil
}
if b[0] == '"' {
b = b[1 : len(b)-1]
}
if len(b) == 0 {
return nil
}
i, err := jsonparser.ParseInt(b)
if err != nil {
return err
}
*st = StringInt(i)
return nil
}

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 params to use StringInt instead of interface{}

}
10 changes: 8 additions & 2 deletions static/bidder-params/pulsepoint.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,17 @@
"type": "object",
"properties": {
"cp": {
"type": "integer",
"type": [
"integer",
"string"
],
"description": "An ID which identifies the publisher selling the impression"
},
"ct": {
"type": "integer",
"type": [
"integer",
"string"
],
Comment on lines +8 to +18
Copy link
Contributor

Choose a reason for hiding this comment

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

@anand-venkatraman could you add JSON test where cp and ct are string

Copy link
Contributor

Choose a reason for hiding this comment

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

@anand-venkatraman requesting to address above comment ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry for the delay, i have added a few tests cases to validParams under params_test.go file.

Copy link
Contributor

Choose a reason for hiding this comment

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

@anand-venkatraman thanks for adding params test. Requesting to add Json framework tests as well for scenario where cp and ct are string in imp.ext

Copy link
Contributor Author

@anand-venkatraman anand-venkatraman Jun 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

addressed in commit 4539af1

onkarvhanumante marked this conversation as resolved.
Show resolved Hide resolved
"description": "An ID which identifies the ad slot being sold"
}
},
Expand Down
Loading