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

Adapter Name Case Insensitive: Stored Bid Responses #3197

Merged
merged 10 commits into from
Oct 20, 2023
2 changes: 1 addition & 1 deletion endpoints/openrtb2/amp_auction.go
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,7 @@ func (deps *endpointDeps) loadRequestJSONForAmp(httpRequest *http.Request) (req
return
}

storedAuctionResponses, storedBidResponses, bidderImpReplaceImp, errs = stored_responses.ProcessStoredResponses(ctx, requestJSON, deps.storedRespFetcher, deps.bidderMap)
storedAuctionResponses, storedBidResponses, bidderImpReplaceImp, errs = stored_responses.ProcessStoredResponses(ctx, &openrtb_ext.RequestWrapper{BidRequest: req}, deps.storedRespFetcher)
if err != nil {
errs = []error{err}
return
Expand Down
24 changes: 11 additions & 13 deletions endpoints/openrtb2/auction.go
Original file line number Diff line number Diff line change
Expand Up @@ -516,12 +516,6 @@ func (deps *endpointDeps) parseRequest(httpRequest *http.Request, labels *metric
return
}

//Stored auction responses should be processed after stored requests due to possible impression modification
storedAuctionResponses, storedBidResponses, bidderImpReplaceImpId, errs = stored_responses.ProcessStoredResponses(ctx, requestJson, deps.storedRespFetcher, deps.bidderMap)
if len(errs) > 0 {
return nil, nil, nil, nil, nil, nil, errs
}

if err := json.Unmarshal(requestJson, req.BidRequest); err != nil {
errs = []error{err}
return
Expand All @@ -547,6 +541,12 @@ func (deps *endpointDeps) parseRequest(httpRequest *http.Request, labels *metric

lmt.ModifyForIOS(req.BidRequest)

//Stored auction responses should be processed after stored requests due to possible impression modification
storedAuctionResponses, storedBidResponses, bidderImpReplaceImpId, errs = stored_responses.ProcessStoredResponses(ctx, req, deps.storedRespFetcher)
if len(errs) > 0 {
return nil, nil, nil, nil, nil, nil, errs
}

hasStoredResponses := len(storedAuctionResponses) > 0
errL := deps.validateRequest(req, false, hasStoredResponses, storedBidResponses, hasStoredBidRequest)
if len(errL) > 0 {
Expand Down Expand Up @@ -1525,7 +1525,7 @@ func (deps *endpointDeps) validateImpExt(imp *openrtb_ext.ImpWrapper, aliases ma
}

if len(storedBidResp) > 0 {
if err := validateStoredBidRespAndImpExtBidders(prebid.Bidder, storedBidResp, imp.ID); err != nil {
if err := deps.validateStoredBidRespAndImpExtBidders(prebid.Bidder, storedBidResp, imp.ID); err != nil {
return []error{err}
}
}
Expand Down Expand Up @@ -2492,19 +2492,17 @@ func checkIfAppRequest(request []byte) (bool, error) {
return false, nil
}

func validateStoredBidRespAndImpExtBidders(bidderExts map[string]json.RawMessage, storedBidResp stored_responses.ImpBidderStoredResp, impId string) error {
func (deps *endpointDeps) validateStoredBidRespAndImpExtBidders(bidderExts map[string]json.RawMessage, storedBidResp stored_responses.ImpBidderStoredResp, impId string) error {
if bidResponses, ok := storedBidResp[impId]; ok {
if len(bidResponses) != len(bidderExts) {
return generateStoredBidResponseValidationError(impId)
}

for bidderName := range bidResponses {
bidder := bidderName
normalizedCoreBidder, ok := openrtb_ext.NormalizeBidderName(bidder)
if ok {
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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@VeronikaSolovei9 VeronikaSolovei9 Oct 11, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2023-10-11 at 3 41 07 PM

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@VeronikaSolovei9 VeronikaSolovei9 Oct 17, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

return generateStoredBidResponseValidationError(impId)
}
}
Expand Down
37 changes: 26 additions & 11 deletions endpoints/openrtb2/auction_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4990,8 +4990,8 @@ func TestParseRequestStoredResponses(t *testing.T) {
}

func TestParseRequestStoredBidResponses(t *testing.T) {
bidRespId1 := json.RawMessage(`{"id": "resp_id1", "seatbid": [{"bid": [{"id": "bid_id1"}], "seat": "testBidder1"}], "bidid": "123", "cur": "USD"}`)
bidRespId2 := json.RawMessage(`{"id": "resp_id2", "seatbid": [{"bid": [{"id": "bid_id2"}], "seat": "testBidder2"}], "bidid": "124", "cur": "USD"}`)
bidRespId1 := json.RawMessage(`{"id": "resp_id1", "seatbid": [{"bid": [{"id": "bid_id1"}], "seat": "telaria"}], "bidid": "123", "cur": "USD"}`)
bidRespId2 := json.RawMessage(`{"id": "resp_id2", "seatbid": [{"bid": [{"id": "bid_id2"}], "seat": "amx"}], "bidid": "124", "cur": "USD"}`)
bidRespId3 := json.RawMessage(`{"id": "resp_id3", "seatbid": [{"bid": [{"id": "bid_id3"}], "seat": "APPNEXUS"}], "bidid": "125", "cur": "USD"}`)
mockStoredBidResponses := map[string]json.RawMessage{
"bidResponseId1": bidRespId1,
Expand All @@ -5010,40 +5010,54 @@ func TestParseRequestStoredBidResponses(t *testing.T) {
name: "req imp has valid stored bid response",
givenRequestBody: validRequest(t, "imp-with-stored-bid-resp.json"),
expectedStoredBidResponses: map[string]map[string]json.RawMessage{
"imp-id1": {"testBidder1": bidRespId1},
"imp-id1": {"telaria": bidRespId1},
},
expectedErrorCount: 0,
},
{
name: "req imp has valid stored bid response with case insensitive bidder name",
givenRequestBody: validRequest(t, "imp-with-stored-bid-resp-insensitive-bidder-name.json"),
name: "req imp has valid stored bid response with case not-matching bidder name",
givenRequestBody: validRequest(t, "imp-with-stored-bid-resp-case-not-matching-bidder-name.json"),
expectedStoredBidResponses: map[string]map[string]json.RawMessage{
"imp-id3": {"APPNEXUS": bidRespId3},
"imp-id3": {"appnexus": bidRespId3},
},
expectedErrorCount: 0,
},
{
name: "req imp has valid stored bid response with case matching bidder name",
givenRequestBody: validRequest(t, "imp-with-stored-bid-resp-case-matching-bidder-name.json"),
expectedStoredBidResponses: map[string]map[string]json.RawMessage{
"imp-id3": {"appnexus": bidRespId3},
},
expectedErrorCount: 0,
},
{
name: "req has two imps with valid stored bid responses",
givenRequestBody: validRequest(t, "req-two-imps-stored-bid-responses.json"),
expectedStoredBidResponses: map[string]map[string]json.RawMessage{
"imp-id1": {"testBidder1": bidRespId1},
"imp-id2": {"testBidder2": bidRespId2},
"imp-id1": {"telaria": bidRespId1},
"imp-id2": {"amx": bidRespId2},
},
expectedErrorCount: 0,
},
{
name: "req has two imps one with valid stored bid responses and another one without stored bid responses",
givenRequestBody: validRequest(t, "req-two-imps-with-and-without-stored-bid-responses.json"),
expectedStoredBidResponses: map[string]map[string]json.RawMessage{
"imp-id2": {"testBidder2": bidRespId2},
"imp-id2": {"amx": bidRespId2},
},
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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

  1. 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

  1. 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"},

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@VeronikaSolovei9 VeronikaSolovei9 Oct 18, 2023

Choose a reason for hiding this comment

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

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

},
{
name: "req imp has valid stored bid response with non existing bidder name",
givenRequestBody: validRequest(t, "imp-with-stored-bid-resp-non-existing-bidder-name.json"),
expectedStoredBidResponses: nil,
expectedErrorCount: 1,
},
}
for _, test := range tests {
Expand All @@ -5062,7 +5076,7 @@ func TestParseRequestStoredBidResponses(t *testing.T) {
map[string]string{},
false,
[]byte{},
map[string]openrtb_ext.BidderName{"testBidder1": "testBidder1", "testBidder2": "testBidder2", "appnexus": "appnexus"},
map[string]openrtb_ext.BidderName{"telaria": "telaria", "amx": "amx", "appnexus": "appnexus"},
nil,
nil,
hardcodedResponseIPValidator{response: true},
Expand All @@ -5077,6 +5091,7 @@ func TestParseRequestStoredBidResponses(t *testing.T) {
req := httptest.NewRequest("POST", "/openrtb2/auction", strings.NewReader(test.givenRequestBody))
_, _, _, storedBidResponses, _, _, errL := deps.parseRequest(req, &metrics.Labels{}, hookExecutor)
if test.expectedErrorCount == 0 {
assert.Empty(t, errL)
assert.Equal(t, test.expectedStoredBidResponses, storedBidResponses, "stored responses should match")
} else {
assert.Contains(t, errL[0].Error(), test.expectedError, "error should match")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
{
"description": "request with impression with stored bid response with case matching bidder name",
"mockBidRequest": {
"id": "request-with-stored-resp",
"site": {
"page": "test.somepage.com"
},
"imp": [
{
"id": "imp-id3",
"banner": {
"format": [
{
"w": 300,
"h": 600
}
]
},
"ext": {
"appnexus": {
"placementId": 12883451
},
"prebid": {
"storedbidresponse": [
{
"bidder": "appnexus",
"id": "bidResponseId3"
}
]
}
}
}
],
"user": {
"yob": 1989
}
},
"expectedReturnCode": 200
}
Original file line number Diff line number Diff line change
@@ -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",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: Typo "case not matching"

"mockBidRequest": {
"id": "request-with-stored-resp",
"site": {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
{
"description": "request with impression with stored bid response",
"mockBidRequest": {
"id": "request-with-stored-resp",
"site": {
"page": "test.somepage.com"
},
"imp": [
{
"id": "imp-id1",
"banner": {
"format": [
{
"w": 300,
"h": 600
}
]
},
"ext": {
"bidderABC": {},
"prebid": {
"storedbidresponse": [
{"bidder":"bidderABC", "id": "bidResponseId1"}
]
}
}
}
],
"user": {
"yob": 1989
}
},
"expectedReturnCode": 200
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,10 @@
]
},
"ext": {
"appnexus": {
"placementId": 12883451
},
"telaria": {},
"prebid": {
"storedbidresponse": [
{"bidder":"testBidder1", "id": "bidResponseId1"}
{"bidder":"telaria", "id": "bidResponseId1"}
]
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@
]
},
"ext": {
"appnexus": {
"telaria": {
"placementId": 12883451
},
"prebid": {
"storedbidresponse": [
{"bidder":"testBidder1", "id": "bidResponseId1"}
{"bidder":"telaria", "id": "bidResponseId1"}
]
}
}
Expand All @@ -38,12 +38,10 @@
]
},
"ext": {
"appnexus": {
"placementId": 12883451
},
"amx": {},
"prebid": {
"storedbidresponse": [
{"bidder":"testBidder2", "id": "bidResponseId2"}
{"bidder":"amx", "id": "bidResponseId2"}
]
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,12 @@
]
},
"ext": {
"appnexus": {
"amx": {
"placementId": 12883451
},
"prebid": {
"storedbidresponse": [
{"bidder":"testBidder2", "id": "bidResponseId2"}
{"bidder":"amx", "id": "bidResponseId2"}
]
}
}
Expand Down
2 changes: 1 addition & 1 deletion exchange/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -930,7 +930,7 @@ func buildBidResponseRequest(req *openrtb2.BidRequest,
BidderCoreName: resolvedBidder,
BidderName: bidderName,
BidderStoredResponses: impResps,
ImpReplaceImpId: bidderImpReplaceImpID[string(resolvedBidder)],
ImpReplaceImpId: bidderImpReplaceImpID[string(bidderName)],
BidderLabels: metrics.AdapterLabels{Adapter: resolvedBidder},
}
}
Expand Down
23 changes: 23 additions & 0 deletions exchange/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"encoding/json"
"errors"
"fmt"
"github.com/prebid/prebid-server/stored_responses"
"sort"
"testing"

Expand Down Expand Up @@ -4679,3 +4680,25 @@ func TestApplyBidAdjustmentToFloor(t *testing.T) {
})
}
}

func TestBuildBidResponseRequestBidderName(t *testing.T) {
bidderImpResponses := stored_responses.BidderImpsWithBidResponses{
openrtb_ext.BidderName("appnexus"): {"impId1": json.RawMessage(`{}`), "impId2": json.RawMessage(`{}`)},
openrtb_ext.BidderName("appneXUS"): {"impId3": json.RawMessage(`{}`), "impId4": json.RawMessage(`{}`)},
}

bidderImpReplaceImpID := stored_responses.BidderImpReplaceImpID{
"appnexus": {"impId1": true, "impId2": false},
"appneXUS": {"impId3": true, "impId4": false},
}
result := buildBidResponseRequest(nil, bidderImpResponses, nil, bidderImpReplaceImpID)

resultAppnexus := result["appnexus"]
assert.Equal(t, resultAppnexus.BidderName, openrtb_ext.BidderName("appnexus"))
assert.Equal(t, resultAppnexus.ImpReplaceImpId, map[string]bool{"impId1": true, "impId2": false})

resultAppneXUS := result["appneXUS"]
assert.Equal(t, resultAppneXUS.BidderName, openrtb_ext.BidderName("appneXUS"))
assert.Equal(t, resultAppneXUS.ImpReplaceImpId, map[string]bool{"impId3": true, "impId4": false})

}
Loading
Loading