Adapter Name Case Insensitive: Stored Bid Responses #3197

VeronikaSolovei9 · 2023-10-10T00:14:28Z

To support case insensitive bidder names in stored bid responses we need to remember that it is possible to specify req.imp[].ext like this:

"ext": {
                "APPnexus": {
                    "placementId": 12883451
                },
                "aPPnexuS": {
                    "placementId": 12883451
                },
}

For request with one imp and ext like this(without stored bid responses), "appnexus" code will be executed twice and 2 http calls will be added to debug info:

  "ext": {
        "debug": {
            "httpcalls": {
                "APPnexus": [ ... ],
                "aPPnexuS": [ ... ]
            }
        }
}

Stored bid responses should support this case. Because bidder name in stored bid responses should be case insensitive, then we can specify "appnexus" only one time in "storedbidresponse" and same bid response config will be applied for both "APPnexus" and "aPPnexuS":

"prebid": {
                 
                    "storedbidresponse": [
                        {
                            "bidder": "appnexus",
                            "id": "some_id"
                        }
                    ]
}

In order to handle this case properly, we need to keep track of bidder names specified in imp.ext.
Http calls also have bidder names in the same casing specified in imp.ext, same as it works with imps without stored responses

onkarvhanumante · 2023-10-10T07:10:11Z

endpoints/openrtb2/auction.go

 			}
-			if _, present := bidderExts[bidder]; !present {
+			if _, present := bidderExts[bidderName]; !present {


Mostly for my education. Why are we not using normalised bidder name to lookup on bidderExts.
Here bidderExts is prepared from core bidder names. So should use normalised bidder name

I added a description with explanations how it works. We need to use bidder name as it is specified in imp, not normalized bidder name.

onkarvhanumante · 2023-10-10T07:12:25Z

stored_responses/stored_responses.go

-					return nil, nil, nil, nil, fmt.Errorf("request.imp[impId: %s].ext.prebid.bidder contains unknown bidder: %s. Did you forget an alias in request.ext.prebid.aliases?", impId, bidderResp.Bidder)
-				}
-				// bidder is unique per one bid stored response
-				// if more than one bidder specified the last defined bidder id will take precedence
 				bidderStoredRespId[bidderResp.Bidder] = bidderResp.ID


Mostly for my education. Why was the code block from line 100 to 110 removed?

This was a fail-fast logic, that is not needed in extractStoredResponsesIds, we just want to extract ids.
This logic was sort of duplicated in validateStoredBidRespAndImpExtBidders. Now it's only one place with validation in validation function.

guscarreon · 2023-10-11T00:22:56Z

endpoints/openrtb2/auction.go

-			if ok {
-				bidder = normalizedCoreBidder.String()
+			_, bidderNameOk := deps.normalizeBidderName(bidderName)
+			if !bidderNameOk {


Nitpick: bidderNameOk is only used in the if statement. Can we merge the two lines?

2501 for bidderName := range bidResponses { 2502 - _, bidderNameOk := deps.normalizeBidderName(bidderName) 2503 - if !bidderNameOk { + if _, bidderNameOk := deps.normalizeBidderName(bidderName); !bidderNameOk { 2504 return fmt.Errorf(`unrecognized bidder "%v"`, bidderName) endpoints/openrtb2/auction.go

guscarreon

LGTM

bsardo · 2023-10-17T14:34:37Z

stored_responses/stored_responses.go

+			// bidders can be specified in imp.ext and in imp.ext.prebid.bidders
+			allBidderNames := make([]string, 0)
+			for bidderName := range impExtPrebid.Bidder {
+				allBidderNames = append(allBidderNames, bidderName)
+			}
+			for extData := range impExt.GetExt() {
+				// no bidders will not be processed
+				allBidderNames = append(allBidderNames, extData)
+			}


It looks like we are determining all of the bidders here by looking at both imp.ext and imp.ext.prebid.bidders. I didn't think this was necessary because we have code that promotes all bidders in imp.ext to imp.ext.prebid.bidders but it appears that code is executed during the request validation step which happens just after ProcessStoredResponses is called. Ideally we would promote bidders to imp.ext.prebid.bidders as early as possible so that logic downstream like this need only look in one location but it does not appear to be a trivial refactor so perhaps it is something we can do in the future.

Nevertheless, I think that this second for statement may need to be sure that reserved keywords are not considered bidders. If you look at the bidder promotion code in validateImpExt we are using the function isPossibleBidder to achieve this:

// promote imp[].ext.BIDDER to newer imp[].ext.prebid.bidder.BIDDER location, with the later taking precedence for k, v := range ext { if isPossibleBidder(k) { if _, exists := prebid.Bidder[k]; !exists { prebid.Bidder[k] = v prebidModified = true } delete(ext, k) extModified = true } }

Yes, this is one of those parts I got stuck with.
I tried to use isPossibleBidder here and ran into the circular dependency. We will need to extract it to utils maybe to use in all needed places.
Non-bidders fields will be skipped in further processing: if strings.EqualFold(bidderName, bidderResp.Bidder) { No-bidder will be only added if same thing is specified in imp.ext as a bidder.
This function only parses stored responses ids and matches them with bidders in imp.ext.
Later validation function will check if bidders are valid.

bsardo · 2023-10-17T14:41:34Z

stored_responses/stored_responses.go

-				//check if bidder is valid/exists
-				if _, isValid := bidderMap[bidderName]; !isValid {
-					return nil, nil, nil, nil, fmt.Errorf("request.imp[impId: %s].ext.prebid.bidder contains unknown bidder: %s. Did you forget an alias in request.ext.prebid.aliases?", impId, bidderResp.Bidder)
-				}


I see that this logic was removed. Isn't this logic checking that the stored response bidder name is the name of a bidder defined in the imp? Do we need this? Does this error detection exist elsewhere?

Yes, this function is only about parsing request. This check is now in validateStoredBidRespAndImpExtBidders:

if _, bidderNameOk := deps.normalizeBidderName(bidderName); !bidderNameOk { return fmt.Errorf(`unrecognized bidder "%v"`, bidderName) }

bsardo · 2023-10-17T14:45:48Z

stored_responses/stored_responses.go

-				//storedAuctionResponseIds are not unique, but fetch will return single data for repeated ids
-				allStoredResponseIDs = append(allStoredResponseIDs, bidderResp.ID)
+				for _, bidderName := range allBidderNames {
+					if strings.EqualFold(bidderName, bidderResp.Bidder) {


Should we add a break at the end of this if block so that the first stored response is chosen if multiple stored responses are specified for the same bidder?

No, because we need to build a map of bidderName to stored response. "appnexuS" stored response should be applied to any "Appnexus" or "APPnexus" bidder in imp. This is where case insensitivity comes into play and where we keep "original" bidder names.

Gotcha, can we make line 116 something like: if _, found := bidderStoredRespId[bidderName]; !found && strings.EqualFold(bidderName, bidderResp.Bidder) { to ensure that the first stored response is used instead of the last if there are multiple for the same bidder?

Yes! Added. And a unit test.

bsardo · 2023-10-17T14:55:21Z

endpoints/openrtb2/auction.go

-				bidder = normalizedCoreBidder.String()
+			if _, bidderNameOk := deps.normalizeBidderName(bidderName); !bidderNameOk {
+				return fmt.Errorf(`unrecognized bidder "%v"`, bidderName)
 			}
-			if _, present := bidderExts[bidder]; !present {
+			if _, present := bidderExts[bidderName]; !present {


I'm confused about what's supposed to happen here. Aren't we supposed to make a case insensitive comparison between the bidder name in the imp ext and the bidder name in the stored response? It doesn't look like that is happening.

At this time we have a map of all bidders in imp and all bidders with stored responses.
Stored responses already parsed and represented in a map[bidderName] to RawMessage(stored response) where bidder name is the same as it is specified in imp.
This check suppose to check that every bidder in imp ("AppNexus" or "APPnexus") has a stored response. This check is redundant here. I can delete it.

👍 I think you can leave it here as it's more intuitive to keep this logic in a validation function.

…of the last if there are multiple stored responses for the same bidder

bsardo · 2023-10-18T16:25:01Z

...quests/valid-whole/supplementary/imp-with-stored-bid-resp-case-not-matching-bidder-name.json

@@ -1,5 +1,5 @@
 {
-  "description": "request with impression with stored bid response with sensitive bidder name",
+  "description": "request with impression with stored bid response with case matching bidder name",


Nitpick: Typo "case not matching"

bsardo · 2023-10-18T16:36:06Z

endpoints/openrtb2/auction_test.go

 			},
 			expectedErrorCount: 0,
 		},
 		{
 			name:                       "req has two imps with missing stored bid responses",
 			givenRequestBody:           validRequest(t, "req-two-imps-missing-stored-bid-response.json"),
 			expectedStoredBidResponses: nil,
-			expectedErrorCount:         2,
+			expectedErrorCount:         0,


I'm curious what the thought process is here. Why did this behavior change? It looks like both the bidders and the stored response ids in imp.ext.prebid.storedbidresponse are invalid in the corresponding JSON file. Is that intended?

The next test case that involves imp-with-stored-bid-resp-non-existing-bidder-name.json considers it an error if the bidder is invalid but the stored bid response ID is not.

Ok, I see what is happening here. This test executes deps.parseRequest where we parse imp.bidders and imp.storedrequests and validate the result.

Test with req-two-imps-missing-stored-bid-response.json doesn't return errors, because there are no matches here:

"appnexus": { "placementId": 12883451 }, "prebid": { "storedbidresponse": [ {"bidder":"testBidder1", "id": "123"} ] }

In this case result stored responses are empty and actual request is going to the "appnexus" adapter.
We need to confirm if we want this behavior. Before this change if imp.prebid.storedbidresponse was specified in imp, then we required stored responses for every bidder in imp.ext. Now if there are no matches between imp.ext.bidder and bidders in stored responses - we execute this imp as a "usual" imp and bidders send actual http calls to actual bidders

Test with imp-with-stored-bid-resp-non-existing-bidder-name.json returnes a validation error for bidder bidderABC, because it cannot normalize it. This test has predefined deps with this bidder map:
map[string]openrtb_ext.BidderName{"telaria": "telaria", "amx": "amx", "appnexus": "appnexus"},

Gotcha. I can't find any info confirming this but I thought we made a conscious decision a while back to report an error if all bidders called out in imps didn't have a stored bid response when the bid response object is present in the request. I don't recall a specific reason for this implementation but I assume it was to help us avoid confusion in case people think they can mix stored bid responses with real bid requests for an imp, which is something we don't currently support.

I don't think this is critical but I'm having trouble discerning what is driving the change here. Thoughts?

This is good catch. I didn't test the scenario where there are no mathing bidders at all in stored bid responses.
Fixed back: if stored responses are specified, then every bidder (ignoring casing) in imp.ext should have a stored bid response

bsardo · 2023-10-18T16:47:09Z

stored_responses/stored_responses_test.go

+			assert.Equal(t, test.expectedStoredAuctionResponses, storedAuctionResponses, "storedAuctionResponses doesn't match: %s\n", test.description)
+			assert.Equalf(t, test.expectedStoredBidResponses, storedBidResponses, "storedBidResponses doesn't match: %s\n", test.description)
+			assert.Equal(t, test.expectedBidderImpReplaceImpID, bidderImpReplaceImpId, "bidderImpReplaceImpId doesn't match: %s\n", test.description)
+			assert.Nil(t, errorList, "Error should be nil")


Nitpick: you don't need the error message and test description at the end of these assert statements when using t.Run. test.description was passed into t.Run as the first parameter and if one of the assert statements fails, the description will be automatically be output to give context on the error.

bsardo · 2023-10-18T16:48:14Z

stored_responses/stored_responses_test.go

+			rw := openrtb_ext.RequestWrapper{BidRequest: &test.request}
+			_, _, _, errorList := ProcessStoredResponses(nil, &rw, fetcher)
+			for _, err := range test.expectedErrors {
+				assert.Contains(t, errorList, err, "incorrect errors returned: %s", test.description)


Nitpick: we can remove error message from this assert statement.

bsardo · 2023-10-18T16:48:19Z

stored_responses/stored_responses_test.go

-		assert.Equal(t, test.outBidderImpReplaceImpID, actualResult, "Incorrect flipped map for test case %s\n", test.description)
+		t.Run(test.description, func(t *testing.T) {
+			actualResult := flipMap(test.inImpBidderReplaceImpID)
+			assert.Equal(t, test.outBidderImpReplaceImpID, actualResult, "Incorrect flipped map for test case %s\n", test.description)


Nitpick: we can remove error message from this assert statement.

# Conflicts: # exchange/utils.go

…prebid.storedbidresponse

bsardo · 2023-10-19T14:21:41Z

endpoints/openrtb2/auction.go

+	if len(prebid.StoredBidResponse) > 0 || len(storedBidResp) > 0 {
+		// prebid.StoredBidResponse - data from incoming imp.ext.prebid.storedbidresponse
+		// storedBidResp - imp id mapped to bidders from imp.ext (case insensitive) that have matched stored bid response
+		// in case only one of prebid.StoredBidResponse or storedBidResp exist means stored responses are specified
+		// in imp.ext.prebid.storedresponse, but don't match bidders in imp.ext (or imp.ext.prebid.bidders)
+		if err := deps.validateStoredBidRespAndImpExtBidders(prebid.Bidder, storedBidResp, imp.ID); err != nil {


This works but I think the fact that we need this comment is an indication that perhaps things could be simplified.
How about a few slight modifications like this where we always call the validate function which then does the entire analysis including the edge case we are trying to address:

hasPrebidStoredBidResponse := len(prebid.StoredBidResponse) > 0 if err := deps.validateStoredBidRespAndImpExtBidders(hasPrebidStoredBidResponse, prebid.Bidder, storedBidResp, imp.ID); err != nil { return []error{err} }

func (deps *endpointDeps) validateStoredBidRespAndImpExtBidders(hasPrebidStoredBidResponse bool, bidderExts map[string]json.RawMessage, storedBidResp stored_responses.ImpBidderStoredResp, impId string) error { if storedBidResp == nil && !hasPrebidStoredBidResponse{ return nil } if storedBidResp == nil { return generateStoredBidResponseValidationError(impId) } if bidResponses, ok := storedBidResp[impId]; ok { if len(bidResponses) != len(bidderExts) { return generateStoredBidResponseValidationError(impId) } for bidderName := range bidResponses { if _, bidderNameOk := deps.normalizeBidderName(bidderName); !bidderNameOk { return fmt.Errorf(`unrecognized bidder "%v"`, bidderName) } if _, present := bidderExts[bidderName]; !present { return generateStoredBidResponseValidationError(impId) } } } return nil }

Thoughts?

Yes, refactored. I put this part len(prebid.StoredBidResponse) > 0 inside of the validateStoredBidRespAndImpExtBidders function

# Conflicts: # endpoints/openrtb2/auction.go # stored_responses/stored_responses.go

guscarreon

LGTM

Added support for stored bid responses with case-insensitive bidder name

c870c0b

onkarvhanumante self-requested a review October 10, 2023 07:05

onkarvhanumante reviewed Oct 10, 2023

View reviewed changes

bsardo added do not port PBS 2.0 labels Oct 10, 2023

bsardo changed the title ~~Case-insensitive stored bid responses~~ Adapter Name Case Insensitive: Stored Bid Responses Oct 10, 2023

bsardo assigned guscarreon and bsardo Oct 10, 2023

guscarreon reviewed Oct 11, 2023

View reviewed changes

VeronikaSolovei9 added 2 commits October 11, 2023 15:12

Fix for stored bid responses

421e54d

Minor refactoring

3bc4b76

guscarreon previously approved these changes Oct 16, 2023

View reviewed changes

bsardo assigned hhhjort and bsardo and unassigned bsardo and hhhjort Oct 16, 2023

bsardo reviewed Oct 17, 2023

View reviewed changes

Added logic to ensure that the first stored response is used instead …

3ecaeef

…of the last if there are multiple stored responses for the same bidder

VeronikaSolovei9 dismissed guscarreon’s stale review via 3ecaeef October 17, 2023 20:57

bsardo reviewed Oct 18, 2023

View reviewed changes

VeronikaSolovei9 added 4 commits October 18, 2023 13:02

Minor clean up

33187cc

Merge branch 'master' into stroredbidresponses_caseinsensitive

3e2d920

# Conflicts: # exchange/utils.go

Merge fix

d4fb8e0

Validation fix for non-matching bidder names in imp.ext with imp.ext.…

1b53ce4

…prebid.storedbidresponse

bsardo reviewed Oct 19, 2023

View reviewed changes

Code clean up

0a73798

bsardo previously approved these changes Oct 19, 2023

View reviewed changes

Merge branch 'master' into stroredbidresponses_caseinsensitive

01e1a85

# Conflicts: # endpoints/openrtb2/auction.go # stored_responses/stored_responses.go

VeronikaSolovei9 dismissed bsardo’s stale review via 01e1a85 October 19, 2023 19:21

bsardo approved these changes Oct 19, 2023

View reviewed changes

guscarreon approved these changes Oct 20, 2023

View reviewed changes

bsardo merged commit 53e0adc into master Oct 20, 2023
3 checks passed

SyntaxNode deleted the stroredbidresponses_caseinsensitive branch November 17, 2023 22:13

svamiftah pushed a commit to sovrn/prebid-server that referenced this pull request Nov 21, 2023

Adapter Name Case Insensitive: Stored Bid Responses (prebid#3197)

238a2c4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapter Name Case Insensitive: Stored Bid Responses #3197

Adapter Name Case Insensitive: Stored Bid Responses #3197

VeronikaSolovei9 commented Oct 10, 2023 •

edited

Loading

onkarvhanumante Oct 10, 2023

VeronikaSolovei9 Oct 11, 2023 •

edited

Loading

VeronikaSolovei9 Oct 11, 2023

onkarvhanumante Oct 10, 2023

VeronikaSolovei9 Oct 11, 2023

guscarreon Oct 11, 2023

guscarreon left a comment

bsardo Oct 17, 2023

VeronikaSolovei9 Oct 17, 2023 •

edited

Loading

bsardo Oct 17, 2023

VeronikaSolovei9 Oct 17, 2023

bsardo Oct 17, 2023

VeronikaSolovei9 Oct 17, 2023 •

edited

Loading

bsardo Oct 17, 2023

VeronikaSolovei9 Oct 17, 2023

bsardo Oct 17, 2023

VeronikaSolovei9 Oct 17, 2023 •

edited

Loading

bsardo Oct 17, 2023

bsardo Oct 18, 2023

bsardo Oct 18, 2023

VeronikaSolovei9 Oct 18, 2023

bsardo Oct 18, 2023

VeronikaSolovei9 Oct 18, 2023 •

edited

Loading

bsardo Oct 18, 2023

bsardo Oct 18, 2023

bsardo Oct 18, 2023

bsardo Oct 19, 2023

VeronikaSolovei9 Oct 19, 2023 •

edited

Loading

guscarreon left a comment

Adapter Name Case Insensitive: Stored Bid Responses #3197

Adapter Name Case Insensitive: Stored Bid Responses #3197

Conversation

VeronikaSolovei9 commented Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

VeronikaSolovei9 Oct 11, 2023 • 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

guscarreon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeronikaSolovei9 Oct 17, 2023 • 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

VeronikaSolovei9 Oct 17, 2023 • 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

VeronikaSolovei9 Oct 17, 2023 • 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

VeronikaSolovei9 Oct 18, 2023 • 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

VeronikaSolovei9 Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

guscarreon left a comment

Choose a reason for hiding this comment

VeronikaSolovei9 commented Oct 10, 2023 •

edited

Loading

VeronikaSolovei9 Oct 11, 2023 •

edited

Loading

VeronikaSolovei9 Oct 17, 2023 •

edited

Loading

VeronikaSolovei9 Oct 17, 2023 •

edited

Loading

VeronikaSolovei9 Oct 17, 2023 •

edited

Loading

VeronikaSolovei9 Oct 18, 2023 •

edited

Loading

VeronikaSolovei9 Oct 19, 2023 •

edited

Loading