Add bidder level toggle for alternatebiddercodes config #2326

pm-nilesh-chate · 2022-07-26T08:58:36Z

This change introduces bidder level toggle in alternatebiddercodes config #2174 (similar to that in PBJS).
The bidder level toggle needs to be explicitly enabled with below config combinations (pfa).

Old pbs.yaml example:

 account_defaults:
   alternatebiddercodes:
     enabled: true
     bidders:
       pubmatic:
         allowedbiddercodes: [groupm]
       appnexus:
         allowedbiddercodes: [*]

New pbs.yaml example:

 account_defaults:
   alternatebiddercodes:
     enabled: true
     bidders:
       pubmatic:
         enabled: true
         allowedbiddercodes: [groupm]
       appnexus:
         enabled: true
         allowedbiddercodes: [*]

pm-nilesh-chate · 2022-07-26T10:31:16Z

Should we handle whitespaces, case-sensitivity, etc for the allowedbiddercodes values.

SyntaxNode · 2022-07-28T19:25:32Z

Should we handle whitespaces, case-sensitivity, etc for the allowedbiddercodes values.

I wouldn't worry about whitespace. However, I wouldn't mind chatting about case sensitivity. Right now, we are across the board case sensitive for bidder names/codes. Is this the right behavior? I'm less certain.

bsardo · 2022-07-28T16:55:28Z

config/accounts.go

 		return false, fmt.Errorf("alternateBidderCodes disabled for %q, rejecting bids for %q", bidder, alternateBidder)
 	}

-	if adapterCfg.AllowedBidderCodes[0] == "*" {
+	if len(adapterCfg.AllowedBidderCodes) == 0 || adapterCfg.AllowedBidderCodes[0] == "*" {


I think this should be if adapterCfg.AllowedBidderCodes == nil || as I would expect an explicitly defined empty array to mean that all bidder codes are not allowed while the absence of the array would mean allow all. I raised the question here: #2174 (comment)

I think this would defeat the purpose of bidder level alternatebiddercodes.bidders.BIDDER.enabled flag. User can simply set it false to disable and true to enable. Just that empty array would be one of the corner case under bidder level flag enabled. Wonder, why would user set bidder level flag enabled and then define empty array to reject all the bidder codes instead of simply setting the bidder level flag false.

I see your point but I think this behavior is counter intuitive. If an array is defined then only the items in it should be allowed. If for some reason the host decides to define the array but they leave it empty it is probably a mistake, in which case I think the restrict all behavior makes sense since we would be erring on the side of caution rather than allowing all alternate bidder codes. I confirmed this behavior with Bret on the corresponding issue.

Updated the code. I hope this behaviour is similar in PBJS aswell.

I left a comment here prebid/Prebid.js#8760 (comment) to ensure that the behavior is the same for both.

Catching on this thread, I disagree with @bsardo on this point. I prefer the original implementation where an empty list is treated the same as a nil list. Otherwise, I believe we're leaning too much on the config system for how to represent the two concepts. I could easily forsee one account fetcher returning nil and another returning empty for the same concept.

@pm-nilesh-chate please hold while we figure out what to do here 🙂

Sure, I'll wait until this discussion is concluded. Whatever the outcome is, I would prefer to have similar behaviour in both the PBS and PBJS (fyi, PBJS change has been merged prebid/Prebid.js#8760 (comment)).

We'll leave it as is, especially since PBJS already made the same change.

config/accounts.go

config/accounts_test.go

config/accounts.go

bsardo · 2022-08-02T13:56:12Z

config/accounts.go

 		return false, fmt.Errorf("alternateBidderCodes disabled for %q, rejecting bids for %q", bidder, alternateBidder)
 	}

-	if adapterCfg.AllowedBidderCodes[0] == "*" {
+	if len(adapterCfg.AllowedBidderCodes) == 0 || adapterCfg.AllowedBidderCodes[0] == "*" {


I left a comment here prebid/Prebid.js#8760 (comment) to ensure that the behavior is the same for both.

bsardo

LGTM

SyntaxNode · 2022-08-04T15:51:45Z

config/accounts_test.go

+			fields: fields{
+				Enabled: true,
+				Bidders: map[string]AdapterAlternateBidderCodes{
+					"pubmatic": {Enabled: true},


Nitpick: I recommend using the following declaration to make it clear to the reader this is the case for a nil AllowedBidderCodes:

"pubmatic": { Enabled: true, AllowedBidderCodes: nil },

SyntaxNode · 2022-08-04T15:56:42Z

config/accounts.go

 		return false, fmt.Errorf("alternateBidderCodes disabled for %q, rejecting bids for %q", bidder, alternateBidder)
 	}

-	if adapterCfg.AllowedBidderCodes[0] == "*" {
+	if len(adapterCfg.AllowedBidderCodes) == 0 || adapterCfg.AllowedBidderCodes[0] == "*" {


Catching on this thread, I disagree with @bsardo on this point. I prefer the original implementation where an empty list is treated the same as a nil list. Otherwise, I believe we're leaning too much on the config system for how to represent the two concepts. I could easily forsee one account fetcher returning nil and another returning empty for the same concept.

pm-nilesh-chate added 2 commits July 26, 2022 08:56

Add bidder level toggle for alternatebiddercodes config

744c94a

use original error msg

bdf8303

pm-nilesh-chate marked this pull request as ready for review July 26, 2022 10:36

bsardo self-requested a review July 26, 2022 14:20

bsardo self-assigned this Jul 26, 2022

SyntaxNode self-requested a review July 26, 2022 15:17

SyntaxNode self-assigned this Jul 26, 2022

SyntaxNode previously approved these changes Jul 28, 2022

View reviewed changes

bsardo reviewed Jul 29, 2022

View reviewed changes

pm-azhar-mulla mentioned this pull request Jul 30, 2022

Prebid Core: Handled edge cases for allowedAlternateBidderCodes functionality prebid/Prebid.js#8760

Merged

1 task

do not allow empty biddercode list

e18d654

pm-nilesh-chate dismissed SyntaxNode’s stale review via e18d654 August 1, 2022 15:39

bsardo reviewed Aug 2, 2022

View reviewed changes

update correct test description

79189fd

bsardo approved these changes Aug 2, 2022

View reviewed changes

SyntaxNode reviewed Aug 4, 2022

View reviewed changes

SyntaxNode approved these changes Aug 8, 2022

View reviewed changes

SyntaxNode merged commit cdbd8be into prebid:master Aug 8, 2022

pm-aadit-patil pushed a commit to PubMatic-OpenWrap/prebid-server that referenced this pull request Sep 22, 2022

Add bidder level toggle for alternatebiddercodes config (prebid#2326)

7de0e51

pm-aadit-patil pushed a commit to PubMatic-OpenWrap/prebid-server that referenced this pull request Sep 22, 2022

Add bidder level toggle for alternatebiddercodes config (prebid#2326)

88f1cd9

pm-aadit-patil pushed a commit to PubMatic-OpenWrap/prebid-server that referenced this pull request Sep 22, 2022

Add bidder level toggle for alternatebiddercodes config (prebid#2326)

0298068

jorgeluisrocha pushed a commit to jwplayer/prebid-server that referenced this pull request Sep 28, 2022

Add bidder level toggle for alternatebiddercodes config (prebid#2326)

47bf6c9

pm-nilesh-chate deleted the feature-2174-update-alternatebiddercodes-config branch October 8, 2022 04:04

shunj-nb pushed a commit to ParticleMedia/prebid-server that referenced this pull request Nov 8, 2022

Add bidder level toggle for alternatebiddercodes config (prebid#2326)

bfe3823

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bidder level toggle for alternatebiddercodes config #2326

Add bidder level toggle for alternatebiddercodes config #2326

pm-nilesh-chate commented Jul 26, 2022 •

edited

Loading

pm-nilesh-chate commented Jul 26, 2022

SyntaxNode commented Jul 28, 2022

bsardo Jul 28, 2022

pm-nilesh-chate Jul 31, 2022

bsardo Aug 1, 2022

pm-nilesh-chate Aug 1, 2022 •

edited

Loading

bsardo Aug 2, 2022

SyntaxNode Aug 4, 2022

bsardo Aug 4, 2022

pm-nilesh-chate Aug 5, 2022

SyntaxNode Aug 8, 2022

bsardo Aug 2, 2022

bsardo left a comment

SyntaxNode Aug 4, 2022

SyntaxNode Aug 4, 2022

Add bidder level toggle for alternatebiddercodes config #2326

Add bidder level toggle for alternatebiddercodes config #2326

Conversation

pm-nilesh-chate commented Jul 26, 2022 • edited Loading

pm-nilesh-chate commented Jul 26, 2022

SyntaxNode commented Jul 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pm-nilesh-chate Aug 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsardo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pm-nilesh-chate commented Jul 26, 2022 •

edited

Loading

pm-nilesh-chate Aug 1, 2022 •

edited

Loading