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
33 changes: 19 additions & 14 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 @@ -1531,8 +1531,12 @@ func (deps *endpointDeps) validateImpExt(imp *openrtb_ext.ImpWrapper, aliases ma
return []error{fmt.Errorf("request validation failed. The StoredAuctionResponse.ID field must be completely present with, or completely absent from, all impressions in request. No StoredAuctionResponse data found for request.imp[%d].ext.prebid \n", impIndex)}
}

if len(storedBidResp) > 0 {
if err := validateStoredBidRespAndImpExtBidders(prebid.Bidder, storedBidResp, imp.ID); err != nil {
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 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@VeronikaSolovei9 VeronikaSolovei9 Oct 19, 2023

Choose a reason for hiding this comment

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

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

return []error{err}
}
}
Expand Down Expand Up @@ -2501,19 +2505,20 @@ 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 storedBidResp == nil {
return generateStoredBidResponseValidationError(impId)
}
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
43 changes: 30 additions & 13 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: 1,
},
{
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 Expand Up @@ -5652,8 +5667,10 @@ func TestValidateStoredResp(t *testing.T) {
}

for _, test := range testCases {
errorList := deps.validateRequest(test.givenRequestWrapper, false, test.hasStoredAuctionResponses, test.storedBidResponses, false)
assert.Equalf(t, test.expectedErrorList, errorList, "Error doesn't match: %s\n", test.description)
t.Run(test.description, func(t *testing.T) {
errorList := deps.validateRequest(test.givenRequestWrapper, false, test.hasStoredAuctionResponses, test.storedBidResponses, false)
assert.Equalf(t, test.expectedErrorList, errorList, "Error doesn't match: %s\n", test.description)
})
}
}

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 not matching bidder name",
"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 @@ -957,7 +957,7 @@ func buildBidResponseRequest(req *openrtb2.BidRequest,
BidderCoreName: resolvedBidder,
BidderName: bidderName,
BidderStoredResponses: impResps,
ImpReplaceImpId: bidderImpReplaceImpID[string(resolvedBidder)],
ImpReplaceImpId: bidderImpReplaceImpID[string(bidderName)],
IsRequestAlias: isRequestAlias,
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 @@ -4769,3 +4770,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