-
Notifications
You must be signed in to change notification settings - Fork 762
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 8 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 { | ||
|
@@ -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 { | ||
return []error{err} | ||
} | ||
} | ||
|
@@ -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 { | ||
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. Mostly for my education. Why are we not using normalised bidder name to lookup on 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 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 commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
---|---|---|
@@ -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 |
---|---|---|
@@ -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.
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:
Thoughts?
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.
Yes, refactored. I put this part
len(prebid.StoredBidResponse) > 0
inside of thevalidateStoredBidRespAndImpExtBidders
function