Add nil check errors when setting native asset types (prebid#1260)

flux-dev-team · Apr 16, 2020 · 2d1e5a7 · 2d1e5a7
1 parent 255ae54
commit 2d1e5a7
Show file tree

Hide file tree

Showing 2 changed files with 172 additions and 11 deletions.
diff --git 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
@@ -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