Reject request if stored auction or stored bid response not found

[VeronikaSolovei9]

Fix for #2487

[AlexBVolcy]

Looks good, just left a couple of comments

[AlexBVolcy]

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?

[VeronikaSolovei9]

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.

[AlexBVolcy]

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?

[VeronikaSolovei9]

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

[guscarreon]

As discussed offline, we could check the lengths of the json.RawMessage elements in the buildStoredResponsesMaps function so we don't have to traverse impIdToStoredResp and impBidderToStoredBidResponse again downstream inside functions validateStoredAuctionResponses and validateStoredBidResponses. WIth this change, we gould get rid of said funtions entirely

204 - func buildStoredResponsesMaps(storedResponses StoredResponseIdToStoredResponse, impBidderToStoredBidResponseId ImpBiddersWithBidResponseIDs, impIdToRespId ImpsWithAuctionResponseIDs) (ImpsWithBidResponses, ImpBidderStoredResp) {
    + func buildStoredResponsesMaps(storedResponses StoredResponseIdToStoredResponse, impBidderToStoredBidResponseId ImpBiddersWithBidResponseIDs, impIdToRespId ImpsWithAuctionResponseIDs) (ImpsWithBidResponses, ImpBidderStoredResp, []error) {
205       //imp id to stored resp body
206       impIdToStoredResp := ImpsWithBidResponses{}
207       //stored bid responses: imp id to bidder to stored response body
208       impBidderToStoredBidResponse := ImpBidderStoredResp{}
    +     var errs []error
209
210       for impId, respId := range impIdToRespId {
    +         // This is equivalent to the `len(data) == 0` check inside `validateStoredAuctionResponses`
    +         // we can append to an error array here and get rid of `validateStoredAuctionResponses` entirely
    +         if len(storedResponses[respId]) == 0 {
    +             errs = append(errs, fmt.Errorf("failed to fetch stored auction response for impId = %s and storedAuctionResponse id = %s", impId, respId))
    +         }
211           impIdToStoredResp[impId] = storedResponses[respId]
212       }
213
214       for impId, bidderStoredResp := range impBidderToStoredBidResponseId {
215           bidderStoredResponses := StoredResponseIdToStoredResponse{}
216           for bidderName, id := range bidderStoredResp {
    +             // Same thing here: this check is equivalent to the `len(data) == 0` inside `validateStoredBidResponses`
    +             if len(storedResponses[id]) == 0 {
    +                 errs = append(errs, fmt.Errorf("failed to fetch stored auction response for impId = %s and storedAuctionResponse id = %s", impId, respId))
    +             }
217               bidderStoredResponses[bidderName] = storedResponses[id]
218           }
219           impBidderToStoredBidResponse[impId] = bidderStoredResponses
220       }
221 -     return impIdToStoredResp, impBidderToStoredBidResponse
    +     return impIdToStoredResp, impBidderToStoredBidResponse, errs
222   }
stored_responses/stored_responses.go

[VeronikaSolovei9]

@guscarreon Good point Gus, I refactored functions as discussed. Looks cleaner now. Please re-review

[AlexBVolcy]

LGTM

[guscarreon]

LGTM