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

Prebid Core: Restore use of server-side adapter without client-side adapter #7662

Merged
merged 3 commits into from
Nov 15, 2021

Conversation

jorgeluisrocha
Copy link
Contributor

@jorgeluisrocha jorgeluisrocha commented Nov 3, 2021

Type of change

  • Bugfix

Description of change

Fixes a bug that requires client-side adapter codes to be imported for server-side bidding. This obviates the need to import client-side adapter specifications.

Other information

Issue: #6361
Issue: #6388

@jorgeluisrocha jorgeluisrocha changed the title requestBids bug fix for s2sBidders and getConfig Allow use of server-side adapter without client-side adapter Nov 3, 2021
@patmmccann patmmccann requested a review from a user November 4, 2021 18:00
@patmmccann patmmccann added bugfix needs 2nd review Core module updates require two approvals from the core team labels Nov 4, 2021
@patmmccann patmmccann changed the title Allow use of server-side adapter without client-side adapter Prebid Core: Restore use of server-side adapter without client-side adapter Nov 4, 2021
@jorgeluisrocha
Copy link
Contributor Author

the unit tests passed locally, but for some reason they are not passing on CircleCI. could someone restart the build? might just be flaky.

src/prebid.js Outdated Show resolved Hide resolved
@ChrisHuie
Copy link
Collaborator

@jorgeluisrocha we just merged a recent commit that fixes testing issues. If you pull in recent commits to master should fix the testing issue

@muuki88
Copy link
Collaborator

muuki88 commented Nov 10, 2021

Thanks for your pull request @jorgeluisrocha , but I'm really curious on how this fixes the issue you describe? This looks like a minimal refactoring of how the s2sConfig is fetched from the prebid configuration. Can you elaborate on how this removes the need for client side adapters?

@jorgeluisrocha
Copy link
Contributor Author

Thanks for your pull request @jorgeluisrocha , but I'm really curious on how this fixes the issue you describe? This looks like a minimal refactoring of how the s2sConfig is fetched from the prebid configuration. Can you elaborate on how this removes the need for client side adapters?

The method config.getConfig expects for there to be at most one argument in the parameter. The existing version of requestBids passes two arguments to the method - 's2sConfig' and a lambda function. What ends up occurring is if there is more than one argument, then rather than actually grabbing something from the config using _getConfig(), getConfig will simply add the arguments to subscribe to configuration updates.

Now, you may say "wouldn't adding to the subscriber be fine?". Well, there are three reasons why it is failing:

  1. Given this lambda function is meant to grab the results of getConfig, then at no point will this be possible given it was added to the subcriber. When would it ever actually get 's2sConfig' from the configuration? The sheer fact it was added to the subscriber means it will not happen.
  2. Even if the lambda function was correct and is able to grab something from getConfig, then at no point is the callback function ever taken off the subscriber, i.e., the promise is never fulfilled. If this does not happen, then s2sConfigs will remain empty.
  3. Even if the lambda function was correct and the callback function was taken off the subscriber correctly, then given how requestBids is written, all of this is irrelevant. The reason it would be irrelevant is because the machine will have run through the rest of requestBids anyway because there is nothing to indicate needing to wait for the results of the callback function. Even if s2sConfigs gets populated correctly, the moment when the contents of this array is used to filter through allBidders in a request to determine whether or not certain bidder requests should be resolved client-side will have passed by.

In short, the source of the issue is because there is an additional parameter when calling the getConfig method. This ends that.

@FilipStamenkovic FilipStamenkovic removed the needs 2nd review Core module updates require two approvals from the core team label Nov 15, 2021
@FilipStamenkovic FilipStamenkovic merged commit b927d8d into prebid:master Nov 15, 2021
FilipStamenkovic added a commit that referenced this pull request Nov 15, 2021
ChrisHuie pushed a commit that referenced this pull request Nov 15, 2021
cpabst pushed a commit to sovrn/Prebid.js that referenced this pull request Jan 10, 2022
…dapter (prebid#7662)

* requestBids bug fix for s2sBidders and getConfig

* merged duplicate variables in requestBids
cpabst pushed a commit to sovrn/Prebid.js that referenced this pull request Jan 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants