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

OpenX: accept incoming string fields to support Prebid.js 9 #3668

Merged
merged 6 commits into from
Jul 24, 2024
Merged

OpenX: accept incoming string fields to support Prebid.js 9 #3668

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
47306c4
OpenX: accept customFloor as string or float
laurb9 May 7, 2024
67c1018
OpenX: Accept unit as integer or string, emit string
laurb9 May 7, 2024
370ff75
OpenX: enable gzip compression
laurb9 May 9, 2024
028d8db
Add more test cases per feedback
laurb9 May 9, 2024
21d557e
Merge branch 'prebid:master' into plat-1150
laurb9 May 14, 2024
2a84206
Merge branch 'master' into plat-1150
laurb9 Jun 17, 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
9 changes: 6 additions & 3 deletions adapters/openx/openx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,9 +133,12 @@ func preprocess(imp *openrtb2.Imp, reqExt *openxReqExt) error {
reqExt.DelDomain = openxExt.DelDomain
reqExt.Platform = openxExt.Platform

imp.TagID = openxExt.Unit
if imp.BidFloor == 0 && openxExt.CustomFloor > 0 {
imp.BidFloor = openxExt.CustomFloor
imp.TagID = openxExt.Unit.String()
if imp.BidFloor == 0 {
customFloor, err := openxExt.CustomFloor.Float64()
if err == nil && customFloor > 0 {
imp.BidFloor = customFloor
}
}

// outgoing imp.ext should be same as incoming imp.ext minus prebid and bidder
Expand Down
6 changes: 3 additions & 3 deletions adapters/openx/openxtest/exemplary/optional-params.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
"unit": "539439964",
"delDomain": "se-demo-d.openx.net",
"platform": "PLATFORM",
"customFloor": 0.1,
"customFloor": "0.4",
"customParams": {"foo": "bar"}
}
}
Expand All @@ -25,7 +25,7 @@
},
"ext": {
"bidder": {
"unit": "539439964",
"unit": 539439964,
"delDomain": "se-demo-d.openx.net",
"platform": "PLATFORM",
"customFloor": 0.1
Expand All @@ -48,7 +48,7 @@
"format": [{"w": 728, "h": 90}]
},
"tagid": "539439964",
"bidfloor": 0.1,
"bidfloor": 0.4,
"ext": {
"customParams": {"foo": "bar"}
}
Expand Down
20 changes: 10 additions & 10 deletions adapters/openx/openxtest/supplemental/multi-imp.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"banner": {},
"ext": {
"bidder": {
"unit": "banner-unit-1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this test case previously invalid? Doesn't look like it adhered to the unit json schema pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't recall if these were valid at some point in the past, but they are not now.

"unit": "111",
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious, why "111" instead of "1"? Is there a test involved with the length of the unit string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No functional reason, just think it's easier to find and replace if needed.

"delDomain": "se-demo-d.openx.net"
}
}
Expand All @@ -17,7 +17,7 @@
"video": {"mimes": ["video/mp4"]},
"ext": {
"bidder": {
"unit": "video-unit-1",
"unit": "333",
"delDomain": "se-demo-d.openx.net"
}
}
Expand All @@ -27,7 +27,7 @@
"banner": {},
"ext": {
"bidder": {
"unit": "banner-unit-2",
"unit": "222",
"delDomain": "se-demo-d.openx.net"
}
}
Expand All @@ -37,7 +37,7 @@
"video": {"mimes": ["video/mp4"]},
"ext": {
"bidder": {
"unit": "video-unit-2",
"unit": "444",
"delDomain": "se-demo-d.openx.net"
}
}
Expand All @@ -48,7 +48,7 @@
"video": {"mimes": ["video/mp4"]},
"ext": {
"bidder": {
"unit": "multi-type-unit",
"unit": "555",
"delDomain": "se-demo-d.openx.net"
}
}
Expand All @@ -66,18 +66,18 @@
{
"id": "banner-imp-1",
"banner": {},
"tagid": "banner-unit-1"
"tagid": "111"
},
{
"id": "banner-imp-2",
"banner": {},
"tagid": "banner-unit-2"
"tagid": "222"
},
{
"id": "multi-type-imp",
"banner": {},
"video": {"mimes": ["video/mp4"]},
"tagid": "multi-type-unit"
"tagid": "555"
}
],
"ext": {
Expand Down Expand Up @@ -125,7 +125,7 @@
{
"id": "video-imp-1",
"video": {"mimes": ["video/mp4"]},
"tagid": "video-unit-1"
"tagid": "333"
}
],
"ext": {
Expand Down Expand Up @@ -163,7 +163,7 @@
{
"id": "video-imp-2",
"video": {"mimes": ["video/mp4"]},
"tagid": "video-unit-2"
"tagid": "444"
}
],
"ext": {
Expand Down
8 changes: 7 additions & 1 deletion adapters/openx/params_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,17 @@ func TestInvalidParams(t *testing.T) {
}

var validParams = []string{
`{"unit": 123, "delDomain": "foo.ba"}`,
`{"unit": "123", "delDomain": "foo.ba"}`,
`{"unit": "123", "delDomain": "foo.bar"}`,
`{"unit": "123", "delDomain": "foo.bar", "customFloor": 0.1}`,
`{"unit": "123", "delDomain": "foo.bar", "customFloor": "0.1"}`,
`{"unit": "123", "delDomain": "foo.bar", "customParams": {"foo": "bar"}}`,
`{"unit": "123", "delDomain": "foo.bar", "customParams": {"foo": ["bar", "baz"]}}`,
}

var invalidParams = []string{
`{"unit": "", "delDomain": "foo.bar"}`,
`{"unit": "123"}`,
`{"delDomain": "foo.bar"}`,
`{"unit": "", "delDomain": "foo.bar"}`,
Expand All @@ -56,7 +59,10 @@ var invalidParams = []string{
`{"unit": "123", "delDomain": "foo.b"}`,
`{"unit": "123", "delDomain": "foo.barr"}`,
`{"unit": "123", "delDomain": ".bar"}`,
`{"unit": "123", "delDomain": "foo.bar", "customFloor": "0.1"}`,
`{"unit": "123", "delDomain": "foo.bar", "customFloor": ""}`,
`{"unit": "123", "delDomain": "foo.bar", "customFloor": "1."}`,
`{"unit": "123", "delDomain": "foo.bar", "customFloor": "1.0x"}`,
`{"unit": "123", "delDomain": "foo.bar", "customFloor": "-0.1"}`,
`{"unit": "123", "delDomain": "foo.bar", "customFloor": -0.1}`,
`{"unit": "123", "delDomain": "foo.bar", "customParams": "foo: bar"}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend adding customFloor tests invalid test cases as "1." and "1.0x" to add more test coverage of the regex.

}
2 changes: 1 addition & 1 deletion endpoints/openrtb2/sample-requests/disabled/good/partial.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
"openx": {
"unit": "539439964",
"delDomain": "se-demo-d.openx.net",
"customFloor": 0.1,
"customFloor": "0.5",
"customParams": {"foo": "bar"}
}
}
Comment on lines 43 to 49
Copy link
Contributor

Choose a reason for hiding this comment

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

@laurb9 why was this file changed? Adapter change should be limited to Adapter package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file contains an OpenX adapter request, and we thought it would be best if it were updated to the most recent request format.

Copy link
Contributor

Choose a reason for hiding this comment

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

could move this to openx specific exemplary test folder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can undo this particular line change if it blocks the review, but moving it to the openx test folder does not add any new use cases there. Removing samples from openrtb2/sample-requests is outside the scope of this PR, aside from that openrtb2/sample-requests contains requests for other adapters as well, so the file looks consistent with the intended purpose of that directory, not as an adapter-specific request, but an usage example for certain pbs configurations.

Expand Down
6 changes: 4 additions & 2 deletions openrtb_ext/imp_openx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
package openrtb_ext

import "encoding/json"

// ExtImpOpenx defines the contract for bidrequest.imp[i].ext.prebid.bidder.openx
type ExtImpOpenx struct {
Unit string `json:"unit"`
Unit json.Number `json:"unit"`
Platform string `json:"platform"`
DelDomain string `json:"delDomain"`
CustomFloor float64 `json:"customFloor"`
CustomFloor json.Number `json:"customFloor"`
Comment on lines +7 to +10
Copy link
Contributor

Choose a reason for hiding this comment

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

@laurb9
could use StringInt data type for Unit, CustomFloor

prebid-server/util/jsonutil/stringInt.go

Lines 7 to 29 in a1f48b5

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

@laurb9 laurb9 Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@laurb9 could use StringInt data type for Unit, CustomFloor

CustomFloor is a string|float emitted as float and Unit is a string|int emitted as a string.
I believe neither can make use of StringInt which is a int|string serialized to int.

CustomParams map[string]interface{} `json:"customParams"`
}
1 change: 1 addition & 0 deletions static/bidder-info/openx.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ endpoint: "http://rtb.openx.net/prebid"
maintainer:
email: "[email protected]"
gvlVendorID: 69
endpointCompression: gzip
capabilities:
app:
mediaTypes:
Expand Down
8 changes: 5 additions & 3 deletions static/bidder-params/openx.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@
"type": "object",
"properties": {
"unit": {
"type": "string",
"type": ["number", "string"],
"description": "The ad unit id.",
"minimum": 0,
"pattern": "^[0-9]+$"
},
"delDomain": {
Expand All @@ -22,9 +23,10 @@
"format": "uuid"
},
"customFloor": {
"type": "number",
"type": ["number", "string"],
"description": "The minimum CPM price in USD.",
"minimum": 0
Copy link
Contributor

@onkarvhanumante onkarvhanumante Jun 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

should add/update json tests

  • to have request where unit is string
  • to have request where unit is number
  • to have request where customFloor is string
  • to have request where customFloor is number

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These parameter variations are in optional-params.json:
unit is string: https://github.com/prebid/prebid-server/pull/3668/files#diff-4214c2dc95f1c24891a70e424d9576ab40f10447745b95f68a877ca7145f1bd5R12
unit is number: https://github.com/prebid/prebid-server/pull/3668/files#diff-4214c2dc95f1c24891a70e424d9576ab40f10447745b95f68a877ca7145f1bd5R28
customFloor is string: https://github.com/prebid/prebid-server/pull/3668/files#diff-4214c2dc95f1c24891a70e424d9576ab40f10447745b95f68a877ca7145f1bd5R15
customFloor is number: https://github.com/prebid/prebid-server/pull/3668/files#diff-4214c2dc95f1c24891a70e424d9576ab40f10447745b95f68a877ca7145f1bd5R31

"minimum": 0,
"pattern": "^[0-9]+(\\.[0-9]+)?$"
},
"customParams": {
"type": "object",
Expand Down
Loading