-
Notifications
You must be signed in to change notification settings - Fork 753
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
Changes from 4 commits
c870c0b
421e54d
3bc4b76
3ecaeef
33187cc
3e2d920
d4fb8e0
1b53ce4
0a73798
01e1a85
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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 { | ||
|
@@ -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} | ||
} | ||
} | ||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 The next test case that involves There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I see what is happening here. This test executes
"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.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
}, | ||
{ | ||
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 { | ||
|
@@ -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}, | ||
|
@@ -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") | ||
|
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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: Typo "case not matching" |
||
"mockBidRequest": { | ||
"id": "request-with-stored-resp", | ||
"site": { | ||
|
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 | ||
} |
There was a problem hiding this comment.
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 nameThere was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.