-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Prebid Server Adapter: add native asset id parameter #8317
Conversation
try { | ||
nativeAssets = nativeAssetCache[impressionId] = Object.keys(nativeParams).reduce((assets, type) => { | ||
let params = nativeParams[type]; | ||
|
||
function newAsset(obj) { | ||
return Object.assign({ | ||
required: params.required ? 1 : 0 | ||
required: params.required ? 1 : 0, | ||
id: (isNumber(params.id)) ? params.id : idCounter++ |
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.
@jsnellbaker - this doesn't cover the case where some assets have IDs and others don't. So there could wind up being duplicate IDs. I guess it might be ok to just document this behavior clearly.
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 agree it should be clearly documented, but I thought of a potential alternative that maybe could better handle the id
s. I'll push the commit shortly, let me know if it's worthwhile.
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.
This assumes that the user has specified increasing and non-duplicate ids, but that was the case before and this approach is a nice improvement over the original. Thanks.
Type of change
Description of change
This PR adds in the missing
id
field(s) for thenative.request.assets
object array of a PBS request. The ORTB native spec has this field denoted as required (see PDF link below).While leaving out the field did not appear to be an issue for some of the PBS endpoints, it was found to be an issue for the appnexus PSP endpoint.
Reference - OpenRTB Native doc:
https://www.iab.com/wp-content/uploads/2018/03/OpenRTB-Native-Ads-Specification-Final-1.2.pdf
-- section 4.2 (pg 13)