From 2d1e5a7e15bc5735975b0f47370a00e04817bfe0 Mon Sep 17 00:00:00 2001 From: Mansi Nahar Date: Thu, 16 Apr 2020 10:35:27 -0400 Subject: [PATCH] Add nil check errors when setting native asset types (#1260) --- exchange/bidder.go | 39 ++++++++--- exchange/bidder_test.go | 144 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 172 insertions(+), 11 deletions(-) diff --git a/exchange/bidder.go b/exchange/bidder.go index 8e95835ffba..7a53db5ee97 100644 --- a/exchange/bidder.go +++ b/exchange/bidder.go @@ -224,26 +224,43 @@ func addNativeTypes(bid *openrtb.Bid, request *openrtb.BidRequest) (*nativeRespo } for _, asset := range nativeMarkup.Assets { - setAssetTypes(asset, nativePayload) + if err := setAssetTypes(asset, nativePayload); err != nil { + errs = append(errs, err) + } } return nativeMarkup, errs } -func setAssetTypes(asset nativeResponse.Asset, nativePayload nativeRequests.Request) { +func setAssetTypes(asset nativeResponse.Asset, nativePayload nativeRequests.Request) error { if asset.Img != nil { - tempAsset := getAssetByID(asset.ID, nativePayload.Assets) - if tempAsset.Img.Type != 0 { - asset.Img.Type = tempAsset.Img.Type + if tempAsset, err := getAssetByID(asset.ID, nativePayload.Assets); err == nil { + if tempAsset.Img != nil { + if tempAsset.Img.Type != 0 { + asset.Img.Type = tempAsset.Img.Type + } + } else { + return fmt.Errorf("Response has an Image asset with ID:%d present that doesn't exist in the request", asset.ID) + } + } else { + return err } } if asset.Data != nil { - tempAsset := getAssetByID(asset.ID, nativePayload.Assets) - if tempAsset.Data.Type != 0 { - asset.Data.Type = tempAsset.Data.Type + if tempAsset, err := getAssetByID(asset.ID, nativePayload.Assets); err == nil { + if tempAsset.Data != nil { + if tempAsset.Data.Type != 0 { + asset.Data.Type = tempAsset.Data.Type + } + } else { + return fmt.Errorf("Response has a Data asset with ID:%d present that doesn't exist in the request", asset.ID) + } + } else { + return err } } + return nil } func getNativeImpByImpID(impID string, request *openrtb.BidRequest) (*openrtb.Native, error) { @@ -255,13 +272,13 @@ func getNativeImpByImpID(impID string, request *openrtb.BidRequest) (*openrtb.Na return nil, errors.New("Could not find native imp") } -func getAssetByID(id int64, assets []nativeRequests.Asset) nativeRequests.Asset { +func getAssetByID(id int64, assets []nativeRequests.Asset) (nativeRequests.Asset, error) { for _, asset := range assets { if id == asset.ID { - return asset + return asset, nil } } - return nativeRequests.Asset{} + return nativeRequests.Asset{}, fmt.Errorf("Unable to find asset with ID:%d in the request", id) } // makeExt transforms information about the HTTP call into the contract class for the PBS response. diff --git a/exchange/bidder_test.go b/exchange/bidder_test.go index 46f63cc66c4..f20b431c13a 100644 --- a/exchange/bidder_test.go +++ b/exchange/bidder_test.go @@ -15,6 +15,9 @@ import ( "github.com/prebid/prebid-server/currencies" "github.com/prebid/prebid-server/openrtb_ext" "github.com/stretchr/testify/assert" + + nativeRequests "github.com/mxmCherry/openrtb/native/request" + nativeResponse "github.com/mxmCherry/openrtb/native/response" ) // TestSingleBidder makes sure that the following things work if the Bidder needs only one request. @@ -1083,6 +1086,147 @@ func TestErrorReporting(t *testing.T) { } } +func TestSetAssetTypes(t *testing.T) { + testCases := []struct { + respAsset nativeResponse.Asset + nativeReq nativeRequests.Request + expectedErr string + desc string + }{ + { + respAsset: nativeResponse.Asset{ + ID: 1, + Img: &nativeResponse.Image{ + URL: "http://some-url", + }, + }, + nativeReq: nativeRequests.Request{ + Assets: []nativeRequests.Asset{ + { + ID: 1, + Img: &nativeRequests.Image{ + Type: 2, + }, + }, + { + ID: 2, + Data: &nativeRequests.Data{ + Type: 4, + }, + }, + }, + }, + expectedErr: "", + desc: "Matching image asset exists in the request and asset type is set correctly", + }, + { + respAsset: nativeResponse.Asset{ + ID: 2, + Data: &nativeResponse.Data{ + Label: "some label", + }, + }, + nativeReq: nativeRequests.Request{ + Assets: []nativeRequests.Asset{ + { + ID: 1, + Img: &nativeRequests.Image{ + Type: 2, + }, + }, + { + ID: 2, + Data: &nativeRequests.Data{ + Type: 4, + }, + }, + }, + }, + expectedErr: "", + desc: "Matching data asset exists in the request and asset type is set correctly", + }, + { + respAsset: nativeResponse.Asset{ + ID: 1, + Img: &nativeResponse.Image{ + URL: "http://some-url", + }, + }, + nativeReq: nativeRequests.Request{ + Assets: []nativeRequests.Asset{ + { + ID: 2, + Img: &nativeRequests.Image{ + Type: 2, + }, + }, + }, + }, + expectedErr: "Unable to find asset with ID:1 in the request", + desc: "Matching image asset with the same ID doesn't exist in the request", + }, + { + respAsset: nativeResponse.Asset{ + ID: 2, + Data: &nativeResponse.Data{ + Label: "some label", + }, + }, + nativeReq: nativeRequests.Request{ + Assets: []nativeRequests.Asset{ + { + ID: 2, + Img: &nativeRequests.Image{ + Type: 2, + }, + }, + }, + }, + expectedErr: "Response has a Data asset with ID:2 present that doesn't exist in the request", + desc: "Assets with same ID in the req and resp are of different types", + }, + { + respAsset: nativeResponse.Asset{ + ID: 1, + Img: &nativeResponse.Image{ + URL: "http://some-url", + }, + }, + nativeReq: nativeRequests.Request{ + Assets: []nativeRequests.Asset{ + { + ID: 1, + Data: &nativeRequests.Data{ + Type: 2, + }, + }, + }, + }, + expectedErr: "Response has an Image asset with ID:1 present that doesn't exist in the request", + desc: "Assets with same ID in the req and resp are of different types", + }, + } + + for _, test := range testCases { + err := setAssetTypes(test.respAsset, test.nativeReq) + if len(test.expectedErr) != 0 { + assert.EqualError(t, err, test.expectedErr, "Test Case: %s", test.desc) + continue + } else { + assert.NoError(t, err, "Test Case: %s", test.desc) + } + + for _, asset := range test.nativeReq.Assets { + if asset.Img != nil && test.respAsset.Img != nil { + assert.Equal(t, asset.Img.Type, test.respAsset.Img.Type, "Asset type not set correctly. Test Case: %s", test.desc) + } + if asset.Data != nil && test.respAsset.Data != nil { + assert.Equal(t, asset.Data.Type, test.respAsset.Data.Type, "Asset type not set correctly. Test Case: %s", test.desc) + } + } + } +} + type goodSingleBidder struct { bidRequest *openrtb.BidRequest httpRequest *adapters.RequestData