PubMatic + OpenWrap: Support request level Bidder Params #1042

[sachin-pubmatic]

Changes for #1042

[hhhjort]

Seems to e some confusion. This seems more in line with the initially proposed spec in #1042 , however in the discussion that followed, it was proposed that PBS core take the parameters in ext.prebid.bidderparameters and merge them into imp.ext.bidder, so each imp would have a unified set of parameters that could easily be validated as conforming to the bidder parameter schema. It would also allow for some flexibility in that any bidder parameter could be moved into the request level if it had the same value in all the imps, or even have a default request level value, with one or more imps overriding it at the imp level.

What you have here seems to be an independent set of request level parameters that gets no schema validation, so always optional and unvalidated. Also all adapters must be aware of the existence of the request level parameters to use them. Should probably return to the issue and hash out what we want to support here and how, as it seems like this PR hasn't taken note of the later discussions in the issue.

[hhhjort]

Wanted to be clear, what you are doing here is only a small part of #1042, a concern that was independent of the code level issues that you have addressed. Is this the extent of your interest in #1042, or is this just the first of a series of PRs?

[hhhjort]

To the summarize the feature set as I understand it:

• map request.ext.prebid.bidderparams.<bidder> into bidderparams for each bidder, hiding the the adapter configs from each other.
• merge the request level parameters into all the imp level parameters, giving priority to the imp level version of the parameter
• Validate the merged set of imp level bidder parameters against the bidder's schema to ensure that a valid set of parameters have been applied.

You have done the first item. The other two would help drive a more powerful and flexible usage of the feature. With the merger, publishers would be free to move any bidder parameters to the request level if they are consistent across imps. Bidder adapters would not need to be updated to support request level parameters.

[bretg]

@sachin-pubmatic - please ask your team to give you the time to complete the original feature as specified in #1042 . Thanks.

[sachin-pubmatic]

Thanks @hhhjort for highlighting and summarising the pending changes.

I have incorporated changes for second item i.e merging the request level parameters into all the imp level parameters.
Third item i.e validate the merged set of imp level bidder parameters, gets automatically taken care off with the existing flow which happens after merging parameters.

Please review and let me know if I am missing anything.

[SyntaxNode]

Thanks for picking up this older spec.

[SyntaxNode]

@sachin-pubmatic Friendly reminder for the review comments.

[sachin-pubmatic]

Updated the PR with suggested code changes. Please verify.

[sachin-pubmatic]

Updated the PR with suggested code changes. Please verify.

Note: We are planning to have one round of QA after approval, so request you not to merge PR to master immediately after approval.

[guscarreon]

LGTM

[sachin-pubmatic]

Hi, Friendly reminder for the review and approval of the PR.
Once PR is approved for merge, we will do one round of QA and then we can merge.

[hhhjort]

LGTM

[sachin-pubmatic]

Thanks @hhhjort, @mansinahar & @guscarreon.
I will comment here after one round of QA, so that we can proceed with the merge.

[guscarreon]

A couple more comments before approving. Thank you for addressing our feedback.

[guscarreon]

LGTM. @sachin-pubmatic thank you for addressing our feedback

[sachin-pubmatic]

We have completed QA verification. We are good to merge the PR.