-
Notifications
You must be signed in to change notification settings - Fork 760
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
Pass Through First Party Context Data #1479
Changes from 2 commits
43bd697
df0fbc9
dd0c680
715bfa1
079426e
a8c66ed
1bc35a4
15f4a9c
7e1a7e6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -141,6 +141,16 @@ func TestGoodRequests(t *testing.T) { | |
supplementary.assert(t) | ||
} | ||
|
||
func TestFirstPartyDataRequests(t *testing.T) { | ||
validRequests := &getResponseFromDirectory{ | ||
dir: "sample-requests/first-party-data", | ||
payloadGetter: getRequestPayload, | ||
messageGetter: nilReturner, | ||
expectedCode: http.StatusOK, | ||
} | ||
validRequests.assert(t) | ||
} | ||
|
||
// TestGoodNativeRequests makes sure we return 200s on well-formed Native requests. | ||
func TestGoodNativeRequests(t *testing.T) { | ||
tests := &getResponseFromDirectory{ | ||
|
@@ -1126,31 +1136,104 @@ func TestDisabledBidder(t *testing.T) { | |
} | ||
} | ||
|
||
func TestValidateImpExtDisabledBidder(t *testing.T) { | ||
imp := &openrtb.Imp{ | ||
Ext: json.RawMessage(`{"appnexus":{"placement_id":555},"unknownbidder":{"foo":"bar"}}`), | ||
func TestValidateImpExt(t *testing.T) { | ||
testCases := []struct { | ||
description string | ||
requestExt json.RawMessage | ||
expectedExt string | ||
expectedErrs []error | ||
}{ | ||
{ | ||
description: "Empty", | ||
requestExt: nil, | ||
expectedExt: "", | ||
expectedErrs: []error{errors.New("request.imp[0].ext is required")}, | ||
}, | ||
{ | ||
description: "Valid Bidder", | ||
requestExt: json.RawMessage(`{"appnexus":{"placement_id":555}}`), | ||
expectedExt: `{"appnexus":{"placement_id":555}}`, | ||
expectedErrs: []error{}, | ||
}, | ||
{ | ||
description: "Valid Bidder + Disabled Bidder", | ||
requestExt: json.RawMessage(`{"appnexus":{"placement_id":555},"unknownbidder":{"foo":"bar"}}`), | ||
expectedExt: `{"appnexus":{"placement_id":555}}`, | ||
expectedErrs: []error{&errortypes.BidderTemporarilyDisabled{Message: "The bidder 'unknownbidder' has been disabled."}}, | ||
}, | ||
{ | ||
description: "Valid Bidder + Disabled Bidder + First Party Data Context", | ||
requestExt: json.RawMessage(`{"appnexus":{"placement_id":555},"unknownbidder":{"foo":"bar"},"context":{"data":{"keywords":"prebid server example"}}}`), | ||
expectedExt: `{"appnexus":{"placement_id":555},"context":{"data":{"keywords":"prebid server example"}}}`, | ||
expectedErrs: []error{&errortypes.BidderTemporarilyDisabled{Message: "The bidder 'unknownbidder' has been disabled."}}, | ||
}, | ||
{ | ||
description: "Valid Bidder + First Party Data Context", | ||
requestExt: json.RawMessage(`{"appnexus":{"placement_id":555},"context":{"data":{"keywords":"prebid server example"}}}`), | ||
expectedExt: `{"appnexus":{"placement_id":555},"context":{"data":{"keywords":"prebid server example"}}}`, | ||
expectedErrs: []error{}, | ||
}, | ||
{ | ||
description: "Valid Prebid Ext Bidder", | ||
requestExt: json.RawMessage(`{"prebid":{"bidder":{"appnexus":{"placement_id":555}}}}`), | ||
expectedExt: `{"prebid":{"bidder":{"appnexus":{"placement_id":555}}}}`, | ||
expectedErrs: []error{}, | ||
// request.ext.prebid.bidder.{biddername} is only promoted/copied to request.ext.{biddername} if there is at least one disabled bidder. | ||
}, | ||
{ | ||
description: "Valid Prebid Ext Bidder + First Party Data Context", | ||
requestExt: json.RawMessage(`{"prebid":{"bidder":{"appnexus":{"placement_id":555}}} ,"context":{"data":{"keywords":"prebid server example"}}}`), | ||
expectedExt: `{"prebid":{"bidder":{"appnexus":{"placement_id":555}}},"context":{"data":{"keywords":"prebid server example"}}}`, | ||
expectedErrs: []error{}, | ||
// request.ext.prebid.bidder.{biddername} is only promoted/copied to request.ext.{biddername} if there is at least one disabled bidder. | ||
}, | ||
{ | ||
description: "Valid Prebid Ext Bidder + Disabled Bidder", | ||
requestExt: json.RawMessage(`{"prebid":{"bidder":{"appnexus":{"placement_id":555},"unknownbidder":{"foo":"bar"}}}}`), | ||
expectedExt: `{"prebid":{"bidder":{"appnexus":{"placement_id": 555},"unknownbidder":{"foo":"bar"}}},"appnexus":{"placement_id":555}}`, | ||
expectedErrs: []error{&errortypes.BidderTemporarilyDisabled{Message: "The bidder 'unknownbidder' has been disabled."}}, | ||
// request.ext.prebid.bidder.{biddername} disabled bidders are not removed. if there is a disabled bidder, the valid ones are promoted/copied to request.ext.{biddername}. | ||
}, | ||
{ | ||
description: "Valid Prebid Ext Bidder + Disabled Bidder + First Party Data Context", | ||
requestExt: json.RawMessage(`{"prebid":{"bidder":{"appnexus":{"placement_id":555},"unknownbidder":{"foo":"bar"}}},"context":{"data":{"keywords":"prebid server example"}}}`), | ||
expectedExt: `{"prebid":{"bidder":{"appnexus":{"placement_id": 555},"unknownbidder":{"foo":"bar"}}},"appnexus":{"placement_id":555},"context":{"data":{"keywords":"prebid server example"}}}`, | ||
expectedErrs: []error{&errortypes.BidderTemporarilyDisabled{Message: "The bidder 'unknownbidder' has been disabled."}}, | ||
// request.ext.prebid.bidder.{biddername} disabled bidders are not removed. if there is a disabled bidder, the valid ones are promoted/copied to request.ext.{biddername}. | ||
}, | ||
} | ||
deps := &endpointDeps{ | ||
&nobidExchange{}, | ||
newParamsValidator(t), | ||
&mockStoredReqFetcher{}, | ||
empty_fetcher.EmptyFetcher{}, | ||
empty_fetcher.EmptyFetcher{}, | ||
empty_fetcher.EmptyFetcher{}, | ||
&config.Configuration{MaxRequestSize: int64(8096)}, | ||
pbsmetrics.NewMetrics(metrics.NewRegistry(), openrtb_ext.BidderList(), config.DisabledMetrics{}), | ||
analyticsConf.NewPBSAnalytics(&config.Analytics{}), | ||
map[string]string{"unknownbidder": "The bidder 'unknownbidder' has been disabled."}, | ||
false, | ||
[]byte{}, | ||
openrtb_ext.BidderMap, | ||
nil, | ||
nil, | ||
hardcodedResponseIPValidator{response: true}, | ||
|
||
for _, test := range testCases { | ||
deps := &endpointDeps{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd suggest moving the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved. |
||
&nobidExchange{}, | ||
newParamsValidator(t), | ||
&mockStoredReqFetcher{}, | ||
empty_fetcher.EmptyFetcher{}, | ||
empty_fetcher.EmptyFetcher{}, | ||
empty_fetcher.EmptyFetcher{}, | ||
&config.Configuration{MaxRequestSize: int64(8096)}, | ||
pbsmetrics.NewMetrics(metrics.NewRegistry(), openrtb_ext.BidderList(), config.DisabledMetrics{}), | ||
analyticsConf.NewPBSAnalytics(&config.Analytics{}), | ||
map[string]string{"unknownbidder": "The bidder 'unknownbidder' has been disabled."}, | ||
false, | ||
[]byte{}, | ||
openrtb_ext.BidderMap, | ||
nil, | ||
nil, | ||
hardcodedResponseIPValidator{response: true}, | ||
} | ||
|
||
imp := &openrtb.Imp{Ext: test.requestExt} | ||
errs := deps.validateImpExt(imp, nil, 0) | ||
|
||
if len(test.expectedExt) > 0 { | ||
assert.JSONEq(t, test.expectedExt, string(imp.Ext)) | ||
} else { | ||
assert.Empty(t, imp.Ext) | ||
} | ||
|
||
assert.Equal(t, test.expectedErrs, errs) | ||
} | ||
errs := deps.validateImpExt(imp, nil, 0) | ||
assert.JSONEq(t, `{"appnexus":{"placement_id":555}}`, string(imp.Ext)) | ||
assert.Equal(t, []error{&errortypes.BidderTemporarilyDisabled{Message: "The bidder 'unknownbidder' has been disabled."}}, errs) | ||
} | ||
|
||
func validRequest(t *testing.T, filename string) string { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
{ | ||
"description": "xxx", | ||
|
||
"requestPayload": { | ||
"id": "some-request-id", | ||
"site": { | ||
"page": "test.somepage.com" | ||
}, | ||
"imp": [{ | ||
"id": "some-imp-id", | ||
"banner": { | ||
"format": [{ | ||
"w": 600, | ||
"h": 500 | ||
}, { | ||
"w": 300, | ||
"h": 600 | ||
}] | ||
}, | ||
"ext": { | ||
"appnexus": { | ||
"placementId": 12883451 | ||
}, | ||
"context": { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This throws a validation exception currently. |
||
"data": { | ||
"keywords": "prebid server example" | ||
} | ||
} | ||
} | ||
}] | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,173 @@ | ||
{ | ||
"incomingRequest": { | ||
"ortbRequest": { | ||
"id": "some-request-id", | ||
"site": { | ||
"page": "test.somepage.com" | ||
}, | ||
"imp": [{ | ||
"id": "some-imp-id", | ||
"banner": { | ||
"format": [{ | ||
"w": 600, | ||
"h": 500 | ||
}, { | ||
"w": 300, | ||
"h": 600 | ||
}] | ||
}, | ||
"ext": { | ||
"appnexus": { | ||
"placementId": 1 | ||
}, | ||
"rubicon": { | ||
"accountId": 1, | ||
"siteId": 2, | ||
"zoneId": 3 | ||
}, | ||
"context": { | ||
"data": { | ||
"keywords": "prebid server example" | ||
} | ||
} | ||
} | ||
}] | ||
} | ||
}, | ||
"outgoingRequests": { | ||
"appnexus": { | ||
"expectRequest": { | ||
"ortbRequest": { | ||
"id": "some-request-id", | ||
"site": { | ||
"page": "test.somepage.com" | ||
}, | ||
"imp": [{ | ||
"id": "some-imp-id", | ||
"banner": { | ||
"format": [{ | ||
"w": 600, | ||
"h": 500 | ||
}, { | ||
"w": 300, | ||
"h": 600 | ||
}] | ||
}, | ||
"ext": { | ||
"bidder": { | ||
"placementId": 1 | ||
}, | ||
"context": { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is considered a bidder today and causes all sorts of panics currently (when the validation now allows it). |
||
"data": { | ||
"keywords": "prebid server example" | ||
} | ||
} | ||
} | ||
}] | ||
}, | ||
"bidAdjustment": 1.0 | ||
}, | ||
"mockResponse": { | ||
"pbsSeatBid": { | ||
"pbsBids": [{ | ||
"ortbBid": { | ||
"id": "apn-bid", | ||
"impid": "some-imp-id", | ||
"price": 0.3, | ||
"w": 200, | ||
"h": 500, | ||
"crid": "creative-1" | ||
}, | ||
"bidType": "banner" | ||
}] | ||
} | ||
} | ||
}, | ||
"rubicon": { | ||
"expectRequest": { | ||
"ortbRequest": { | ||
"id": "some-request-id", | ||
"site": { | ||
"page": "test.somepage.com" | ||
}, | ||
"imp": [{ | ||
"id": "some-imp-id", | ||
"banner": { | ||
"format": [{ | ||
"w": 600, | ||
"h": 500 | ||
}, { | ||
"w": 300, | ||
"h": 600 | ||
}] | ||
}, | ||
"ext": { | ||
"bidder": { | ||
"accountId": 1, | ||
"siteId": 2, | ||
"zoneId": 3 | ||
}, | ||
"context": { | ||
"data": { | ||
"keywords": "prebid server example" | ||
} | ||
} | ||
} | ||
}] | ||
}, | ||
"bidAdjustment": 1.0 | ||
}, | ||
"mockResponse": { | ||
"pbsSeatBid": { | ||
"pbsBids": [{ | ||
"ortbBid": { | ||
"id": "rubi-bid", | ||
"impid": "some-imp-id", | ||
"price": 0.4, | ||
"w": 200, | ||
"h": 500, | ||
"crid": "creative-2" | ||
}, | ||
"bidType": "banner" | ||
}] | ||
} | ||
} | ||
} | ||
}, | ||
"response": { | ||
"bids": { | ||
"id": "some-request-id", | ||
"seatbid": [{ | ||
"seat": "appnexus", | ||
"bid": [{ | ||
"id": "apn-bid", | ||
"impid": "some-imp-id", | ||
"price": 0.3, | ||
"w": 200, | ||
"h": 500, | ||
"crid": "creative-1", | ||
"ext": { | ||
"prebid": { | ||
"type": "banner" | ||
} | ||
} | ||
}] | ||
}, { | ||
"seat": "rubicon", | ||
"bid": [{ | ||
"id": "rubi-bid", | ||
"impid": "some-imp-id", | ||
"price": 0.4, | ||
"w": 200, | ||
"h": 500, | ||
"crid": "creative-2", | ||
"ext": { | ||
"prebid": { | ||
"type": "banner" | ||
} | ||
} | ||
}] | ||
}] | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to talk about this behavior. It makes no sense to me and looks like a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this more and now consider it a bug. As a follow-up PR, I'd like to make the validate method only actually validate and move the disabled bidder logic to
exchange.go
, if it's not already there.