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.

Sign up for GitHub

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

Reject request if stored auction or stored bid response not found #2691

Merged
merged 3 commits into from
Apr 18, 2023
Merged

Reject request if stored auction or stored bid response not found #2691

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9689bd6
Reject request if stored auction or stored bid response not found
VeronikaSolovei9 Apr 11, 2023
6038704
Parameters rename
VeronikaSolovei9 Apr 12, 2023
d7b593d
Validation functions refactoring
VeronikaSolovei9 Apr 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 8 additions & 14 deletions endpoints/openrtb2/auction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4305,13 +4305,10 @@ func TestParseRequestStoredResponses(t *testing.T) {
expectedErrorCount: 0,
},
{
name: "req has two imps with missing stored responses",
givenRequestBody: validRequest(t, "req-two-imps-missing-stored-response.json"),
expectedStoredResponses: map[string]json.RawMessage{
"imp-id1": json.RawMessage(`[{"bid": [{"id": "bid_id1"],"seat": "appnexus"}]`),
"imp-id2": json.RawMessage(nil),
},
expectedErrorCount: 0,
name: "req has two imps with missing stored responses",
givenRequestBody: validRequest(t, "req-two-imps-missing-stored-response.json"),
expectedStoredResponses: nil,
expectedErrorCount: 2,
},
{
name: "req has two imps: one with stored response and another imp without stored resp",
Expand Down Expand Up @@ -4404,13 +4401,10 @@ func TestParseRequestStoredBidResponses(t *testing.T) {
expectedErrorCount: 0,
},
{
name: "req has two imps with missing stored bid responses",
givenRequestBody: validRequest(t, "req-two-imps-missing-stored-bid-response.json"),
expectedStoredBidResponses: map[string]map[string]json.RawMessage{
"imp-id1": {"testBidder1": nil},
"imp-id2": {"testBidder2": nil},
},
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,
},
}
for _, test := range tests {
Expand Down
35 changes: 35 additions & 0 deletions stored_responses/stored_responses.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,11 +147,46 @@ func ProcessStoredResponses(ctx context.Context, requestJson []byte, storedRespF
bidderImpIdReplaceImp := flipMap(impBidderReplaceImp)

impIdToStoredResp, impBidderToStoredBidResponse := buildStoredResponsesMaps(storedResponses, impBidderToStoredBidResponseId, impIdToRespId)

errs = validateStoredAuctionResponses(impIdToStoredResp, impIdToRespId)
if len(errs) > 0 {
return nil, nil, nil, errs
}

errs = validateStoredBidResponses(impBidderToStoredBidResponse, impBidderToStoredBidResponseId)
if len(errs) > 0 {
return nil, nil, nil, errs
}

return impIdToStoredResp, impBidderToStoredBidResponse, bidderImpIdReplaceImp, nil
}
return nil, nil, nil, nil
}

func validateStoredAuctionResponses(impIdToStoredResp ImpsWithBidResponses, impIdToRespId ImpsWithAuctionResponseIDs) []error {
var errs []error
for impId, data := range impIdToStoredResp {
if len(data) == 0 {
respId := impIdToRespId[impId]
errs = append(errs, fmt.Errorf("failed to fetch stored auction response for impId = %s and storedAuctionResponse id = %s", impId, respId))
}
}
return errs
}

func validateStoredBidResponses(impBidderToStoredBidResponse ImpBidderStoredResp, impBidderToStoredBidResponseId ImpBiddersWithBidResponseIDs) []error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to think of a way to shorten the variable names. I'm thinking you could remove the word bid from both of these variables and shorten the word response to be resp, so they'd change to impBidderToStoredResp and impBidderToStoredRespId. And we can leverage the name of the function to know that the variables passed in are stored bid responses. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, modified parameters names. About functions names those are two different: validateStoredBidResponses and validateStoredAuctionResponses. I'm open to rename it something that looks better.

var errs []error
for impId, bidderStoredBidResp := range impBidderToStoredBidResponse {
for bidderName, data := range bidderStoredBidResp {
if len(data) == 0 {
respId := impBidderToStoredBidResponseId[impId][bidderName]
errs = append(errs, fmt.Errorf("failed to fetch stored bid response for impId = %s, bidder = %s and storedBidResponse id = %s", impId, bidderName, respId))
}
}
}
return errs
}

// flipMap takes map[impID][bidderName]replaceImpId and modifies it to map[bidderName][impId]replaceImpId
func flipMap(impBidderReplaceImpId ImpBidderReplaceImpID) BidderImpReplaceImpID {
flippedMap := BidderImpReplaceImpID{}
Expand Down
210 changes: 210 additions & 0 deletions stored_responses/stored_responses_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,216 @@ func TestProcessStoredAuctionAndBidResponses(t *testing.T) {

}

func TestProcessStoredResponsesNotFoundResponse(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd be nice to have one test case where we expect no errors with valid stored response information. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, agree. It exists already: TestProcessStoredAuctionAndBidResponses. Before this change we didn't throw an error and this functionality is covered. With this change valid cases are still valid and this specific test was added to cover error cases only.

bidderMap := map[string]openrtb_ext.BidderName{"bidderA": "bidderA", "bidderB": "bidderB"}
bidStoredResp1 := json.RawMessage(`[{"bid": [{"id": "bid_id1"],"seat": "bidderA"}]`)
bidStoredResp2 := json.RawMessage(`[{"bid": [{"id": "bid_id2"],"seat": "bidderB"}]`)
mockStoredResponses := map[string]json.RawMessage{
"1": bidStoredResp1,
"2": bidStoredResp2,
"3": nil,
"4": nil,
}
fetcher := &mockStoredBidResponseFetcher{mockStoredResponses}

testCases := []struct {
description string
requestJson []byte
expectedErrors []error
}{
{
description: "Stored bid response with nil data, one bidder one imp",
requestJson: []byte(`{"imp": [
{
"id": "imp-id1",
"ext": {
"appnexus": {
"placementId": 123
},
"prebid": {
"storedbidresponse": [
{"bidder":"bidderB", "id": "3"}
]
}
}
}
]}`),
expectedErrors: []error{
errors.New("failed to fetch stored bid response for impId = imp-id1, bidder = bidderB and storedBidResponse id = 3"),
},
},
{
description: "Stored bid response with nil data, one bidder, two imps, one with correct stored response",
requestJson: []byte(`{"imp": [
{
"id": "imp-id1",
"ext": {
"appnexus": {
"placementId": 123
},
"prebid": {
"storedbidresponse": [
{"bidder":"bidderB", "id": "1"}
]
}
}
},
{
"id": "imp-id2",
"ext": {
"appnexus": {
"placementId": 123
},
"prebid": {
"storedbidresponse": [
{"bidder":"bidderB", "id": "3"}
]
}
}
}
]}`),
expectedErrors: []error{
errors.New("failed to fetch stored bid response for impId = imp-id2, bidder = bidderB and storedBidResponse id = 3"),
},
},
{
description: "Stored bid response with nil data, one bidder, two imps, both with correct stored response",
requestJson: []byte(`{"imp": [
{
"id": "imp-id1",
"ext": {
"appnexus": {
"placementId": 123
},
"prebid": {
"storedbidresponse": [
{"bidder":"bidderB", "id": "4"}
]
}
}
},
{
"id": "imp-id2",
"ext": {
"appnexus": {
"placementId": 123
},
"prebid": {
"storedbidresponse": [
{"bidder":"bidderB", "id": "3"}
]
}
}
}
]}`),
expectedErrors: []error{
errors.New("failed to fetch stored bid response for impId = imp-id1, bidder = bidderB and storedBidResponse id = 4"),
errors.New("failed to fetch stored bid response for impId = imp-id2, bidder = bidderB and storedBidResponse id = 3"),
},
},
{
description: "Stored auction response with nil data and one imp",
requestJson: []byte(`{"imp": [
{
"id": "imp-id1",
"ext": {
"appnexus": {
"placementId": 123
},
"prebid": {
"storedauctionresponse": {
"id": "4"
}
}
}
}
]}`),
expectedErrors: []error{
errors.New("failed to fetch stored auction response for impId = imp-id1 and storedAuctionResponse id = 4"),
},
},
{
description: "Stored auction response with nil data, and two imps with nil responses",
requestJson: []byte(`{"imp": [
{
"id": "imp-id1",
"ext": {
"appnexus": {
"placementId": 123
},
"prebid": {
"storedauctionresponse": {
"id": "4"
}
}
}
},
{
"id": "imp-id2",
"ext": {
"appnexus": {
"placementId": 123
},
"prebid": {
"storedauctionresponse": {
"id": "3"
}
}
}
}
]}`),
expectedErrors: []error{
errors.New("failed to fetch stored auction response for impId = imp-id1 and storedAuctionResponse id = 4"),
errors.New("failed to fetch stored auction response for impId = imp-id2 and storedAuctionResponse id = 3"),
},
},
{
description: "Stored auction response with nil data, two imps, one with nil responses",
requestJson: []byte(`{"imp": [
{
"id": "imp-id1",
"ext": {
"appnexus": {
"placementId": 123
},
"prebid": {
"storedauctionresponse": {
"id": "2"
}
}
}
},
{
"id": "imp-id2",
"ext": {
"appnexus": {
"placementId": 123
},
"prebid": {
"storedauctionresponse": {
"id": "3"
}
}
}
}
]}`),
expectedErrors: []error{
errors.New("failed to fetch stored auction response for impId = imp-id2 and storedAuctionResponse id = 3"),
},
},
}

for _, test := range testCases {
storedAuctionResponses, storedBidResponses, bidderImpReplaceImpId, errorList := ProcessStoredResponses(nil, test.requestJson, fetcher, bidderMap)
for _, err := range test.expectedErrors {
assert.Contains(t, errorList, err, "incorrect errors returned: %s", test.description)
}
assert.Nil(t, storedAuctionResponses, "storedAuctionResponses doesn't match: %s\n", test.description)
assert.Nil(t, storedBidResponses, "storedBidResponses doesn't match: %s\n", test.description)
assert.Nil(t, bidderImpReplaceImpId, "bidderImpReplaceImpId doesn't match: %s\n", test.description)
}
}

func TestFlipMap(t *testing.T) {
testCases := []struct {
description string
Expand Down