Core: ORTB video params validation (work on dupe) #11970
Conversation
|
Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:
Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀 |
osazos
force-pushed
the
chore/dupe/ortb-video-params-validation-bis
branch
from
4bb7799
to
545e5dc
Compare
|
Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:
Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀 |
osazos
force-pushed
the
chore/dupe/ortb-video-params-validation-bis
branch
from
545e5dc
to
335f25f
Compare
|
Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:
Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀 |
|
Exciting, many more opportunities to import are out there I imagine, eg Prebid.js/modules/pubmaticBidAdapter.js Line 47 in f50570e |
|
And Prebid.js/modules/ttdBidAdapter.js Line 308 in 996b235 |
| * Not included: `companionad`, `durfloors`, `ext` | ||
| * reference: https://github.com/InteractiveAdvertisingBureau/openrtb2.x/blob/main/2.6.md | ||
| */ | ||
| export const ORTB_VIDEO_PARAMS = new Map([ |
There was a problem hiding this comment.
While this feels like a lot to add to core, so many prominent bid adapters do exactly this that it would likely be a net code reduction, very exciting
patmmccann
requested review from
pm-harshad-mane and
dgirardi
and removed request for
pm-harshad-mane
patmmccann
changed the title
src/video.js
Outdated
| */ | ||
| export const ORTB_VIDEO_PARAMS = new Map([ | ||
| [ 'mimes', { validate: (value) => Array.isArray(value) && value.length > 0 && value.every(v => typeof v === 'string') } ], | ||
| [ 'minduration', { validate: (value) => isInteger(value) } ], |
There was a problem hiding this comment.
This could be more compact ('minduration' -> isInteger, 'podid' -> isStr, etc). Are you envisioning it to contain more?
There was a problem hiding this comment.
The idea was to make a map that was not a strict "validation map", it is easier to plug-in something else later, but honestly I don't know how to extend it right now. Tell me if you prefer I use the "compact" way.
There was a problem hiding this comment.
I think compact is better because in terms of bundle size, N maps are still smaller than a map where each node has N things in it.
src/video.js
Outdated
| const valid = ORTB_VIDEO_PARAMS.get(key).validate(value); | ||
| if (!valid) { | ||
| delete videoParams[key]; | ||
| logWarn(`Invalid value for mediaTypes.video.${key} ORTB property. The property has been removed.`); |
There was a problem hiding this comment.
Is this enough info to track down the issue - should we log the offending adUnit?
There was a problem hiding this comment.
This way, I should update the function to validateOrtbVideoFields(adUnit); would it sound better?
In 1st attempt I would like to keep the function scoped on the videoParams object, but perhaps it makes no sense.
There was a problem hiding this comment.
I tend to agree that the scope should be as small as possible, but I'm not sure code aesthetics will convince someone with a complicated setup and a bunch of cryptic warning messages :)
You could also do something like validateOrtbVideoFields(videoParams, onInvalidParam) to keep the scope limited, but let something else that has the adUnit do the logging.
|
Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:
Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀 |
Type of change
Description of change
This is part of the work currently done to remove dupe code and any feedback would be appreciated.
The PR
validateVideoMediaType()call to ensure only valid params are passed along the auctionIt could be a starting point to address the following point:
There are a lot of adapters that use the pretty same logic which is to get the video params from an adUnit (at mediaTypes and/or bids{}.params level) ensure they are matching the Ortb spec and remove them if not during the buildRequests() function.
Note this PR is not dedicated to fix all of them, just to try to introduce a common way for now.