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.

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

PBjs Core : filter bidders from config when not server-side #9632

Merged

Conversation

ghost
Copy link

@ghost ghost commented Mar 7, 2023

Type of change

  • Bugfix

  • Feature

  • New bidder adapter

  • Code style update (formatting, local variables)

  • Refactoring (no functional changes, no api changes)

  • Build related changes

  • CI related changes

  • Does this change affect user-facing APIs or examples documented on http://prebid.org?

  • Other

Description of change

Filter bidders from config when not they're not server-side. This was causing the site codes object to be duplicated in the rubicon bidder params as well as the global object.

@ChrisHuie ChrisHuie requested a review from dgirardi March 8, 2023 14:38
@ChrisHuie ChrisHuie changed the title filter bidders from config when not server-side PBjs Core : filter bidders from config when not server-side Mar 8, 2023
@dgirardi
Copy link
Collaborator

dgirardi commented Mar 8, 2023

This trips a unit test; it's also not obvious to me that it's a bugfix rather than a breaking change. If I allowUnknownBidderCodes, should I expect them to get my client-side configuration? Or is that what you mean by:

This was causing the site codes object to be duplicated in the rubicon bidder params as well as the global object.

@bretg - should ext.prebid.bidderconfig continue to contain all bidder-specific configuration, or just the subset for s2sConfig.bidders?

If the latter, we may need to think about how aliases work. if I set aliasX -> bidderX, and set up aliasX for s2s; will server read ext.prebid.bidderconfig.bidderX (given it knows about the alias through ext.prebid.aliases?)

@bretg
Copy link
Collaborator

bretg commented Mar 8, 2023

@r-schweitzer - what does "site codes object" mean here? Would you have an example handy?

@bretg
Copy link
Collaborator

bretg commented Mar 8, 2023

if I set aliasX -> bidderX, and set up aliasX for s2s; will server read ext.prebid.bidderconfig.bidderX (given it knows about the alias through ext.prebid.aliases?)

No - config for aliasX is separate from config for bidderX.

should ext.prebid.bidderconfig continue to contain all bidder-specific configuration, or just the subset for s2sConfig.bidders

I think 'the subset for s2sConfig.bidders' should be fine, but this area is sort of a hairball (at least in my mind) and am barely 80% confident this isn't going to break something for someone.

I might not be following how rubicon is ending up with "site codes" in both global and bidder-specific params -- is this because the rubicon adapter is using the ortb2 library for video? I can see that duplicating in this scenario, but seems to me the fix should be in the rubicon adapter proper. Would you agree @musikele / @robertrmartinez ?

@ghost
Copy link
Author

ghost commented Mar 10, 2023

Sorry, to be more clear, what I'm referring to is the fact that client side bidder's configs are included in the bidder configs we send through Prebid server which are also sent client side, which isn't necessary.

As for allowUnknownBidderCodes it should still send the bidder config for all server.

image

I'm gonna have to look into the unit test and see whats going on. Currently having some issues running unit tests in prebid

@ghost
Copy link
Author

ghost commented Mar 10, 2023

Alright so after looking at the unit test, this actually confirms our code is working as it should because rubicon is client side in this test, we wouldn't want to pass the bidder config info for them in the server request, only appnexus.

@ncolletti
Copy link
Contributor

@r-schweitzer and I discussed making an update to this PR to support allowUnknownBidderCodes

Copy link
Contributor

@ncolletti ncolletti left a comment

Choose a reason for hiding this comment

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

lgtm

@patmmccann patmmccann merged commit 45867d4 into master May 6, 2023
@patmmccann patmmccann deleted the bugfix-filter-bidder-configs-if-not-server-side-bidder branch May 6, 2023 11:23
jorgeluisrocha pushed a commit to jwplayer/Prebid.js that referenced this pull request May 23, 2023
)

* filter bidders from config when not server-side

* fixed prebid server spec

* added filter for allowUnknownBidderCodes

* added unit test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants