Correct GetCpmStringValue's second return value (#1520)

endpoints/auction.go

@@ -310,10 +310,7 @@ func sortBidsAddKeywordsMobile(bids pbs.PBSBidSlice, pbs_req *pbs.PBSRequest, pr
 		// after sorting we need to add the ad targeting keywords
 		for i, bid := range bar {
 			// We should eventually check for the error and do something.
-			roundedCpm, err := exchange.GetCpmStringValue(bid.Price, openrtb_ext.PriceGranularityFromString(priceGranularitySetting))
-			if err != nil {
-				glog.Error(err.Error())
-			}
+			roundedCpm := exchange.GetPriceBucket(bid.Price, openrtb_ext.PriceGranularityFromString(priceGranularitySetting))
 
 			hbSize := ""
 			if bid.Width != 0 && bid.Height != 0 {

exchange/auction.go

@@ -11,7 +11,6 @@ import (
 	"time"
 
 	uuid "github.com/gofrs/uuid"
-	"github.com/golang/glog"
 	"github.com/mxmCherry/openrtb"
 	"github.com/prebid/prebid-server/config"
 	"github.com/prebid/prebid-server/openrtb_ext"
@@ -129,11 +128,7 @@ func (a *auction) setRoundedPrices(priceGranularity openrtb_ext.PriceGranularity
 	roundedPrices := make(map[*pbsOrtbBid]string, 5*len(a.winningBids))
 	for _, topBidsPerImp := range a.winningBidsByBidder {
 		for _, topBidPerBidder := range topBidsPerImp {
-			roundedPrice, err := GetCpmStringValue(topBidPerBidder.bid.Price, priceGranularity)
-			if err != nil {
-				glog.Errorf(`Error rounding price according to granularity. This shouldn't happen unless /openrtb2 input validation is buggy. Granularity was "%v".`, priceGranularity)
-			}
-			roundedPrices[topBidPerBidder] = roundedPrice
+			roundedPrices[topBidPerBidder] = GetPriceBucket(topBidPerBidder.bid.Price, priceGranularity)
 		}
 	}
 	a.roundedPrices = roundedPrices

exchange/exchange.go

@@ -603,7 +603,7 @@ func applyCategoryMapping(ctx context.Context, requestExt *openrtb_ext.ExtReques
 
 			// TODO: consider should we remove bids with zero duration here?
 
-			pb, _ = GetCpmStringValue(bid.bid.Price, targData.priceGranularity)
+			pb = GetPriceBucket(bid.bid.Price, targData.priceGranularity)
 
 			newDur := duration
 			if len(requestExt.Prebid.Targeting.DurationRangeSec) > 0 {

exchange/price_granularity.go

@@ -7,33 +7,32 @@ import (
 	"github.com/prebid/prebid-server/openrtb_ext"
 )
 
-// DEFAULT_PRECISION should be taken care of in openrtb_ext/request.go, but throwing an additional safety check here.
-
-// GetCpmStringValue is the externally facing function for computing CPM buckets
-func GetCpmStringValue(cpm float64, config openrtb_ext.PriceGranularity) (string, error) {
+// GetPriceBucket is the externally facing function for computing CPM buckets
+func GetPriceBucket(cpm float64, config openrtb_ext.PriceGranularity) string {
 	cpmStr := ""
 	bucketMax := 0.0
 	increment := 0.0
 	precision := config.Precision
-	// calculate max of highest bucket
+
 	for i := 0; i < len(config.Ranges); i++ {
 		if config.Ranges[i].Max > bucketMax {
 			bucketMax = config.Ranges[i].Max
 		}
-	} // calculate which bucket cpm is in
-	if cpm > bucketMax {
-		// If we are over max, just return that
-		return strconv.FormatFloat(bucketMax, 'f', precision, 64), nil
-	}
-	for i := 0; i < len(config.Ranges); i++ {
+		// find what range cpm is in
 		if cpm >= config.Ranges[i].Min && cpm <= config.Ranges[i].Max {
 			increment = config.Ranges[i].Increment
 		}
 	}
-	if increment > 0 {
+
+	if cpm > bucketMax {
+		// We are over max, just return that
+		cpmStr = strconv.FormatFloat(bucketMax, 'f', precision, 64)
+	} else if increment > 0 {
+		// If increment exists, get cpm string value
 		cpmStr = getCpmTarget(cpm, increment, precision)
 	}
-	return cpmStr, nil
+
+	return cpmStr
 }
 
 func getCpmTarget(cpm float64, increment float64, precision int) string {

exchange/price_granularity_test.go

@@ -1,9 +1,11 @@
 package exchange
 
 import (
+	"math"
 	"testing"
 
 	"github.com/prebid/prebid-server/openrtb_ext"
+	"github.com/stretchr/testify/assert"
 )
 
 func TestGetPriceBucketString(t *testing.T) {
@@ -28,32 +30,105 @@ func TestGetPriceBucketString(t *testing.T) {
 		},
 	}
 
-	price := 1.87
-	getOnePriceBucket(t, "low", low, price, "1.50")
-	getOnePriceBucket(t, "medium", medium, price, "1.80")
-	getOnePriceBucket(t, "high", high, price, "1.87")
-	getOnePriceBucket(t, "auto", auto, price, "1.85")
-	getOnePriceBucket(t, "dense", dense, price, "1.87")
-	getOnePriceBucket(t, "custom1", custom1, price, "1.86")
-
-	// test a cpm above the max in low price bucket
-	price = 5.72
-	getOnePriceBucket(t, "low", low, price, "5.00")
-	getOnePriceBucket(t, "medium", medium, price, "5.70")
-	getOnePriceBucket(t, "high", high, price, "5.72")
-	getOnePriceBucket(t, "auto", auto, price, "5.70")
-	getOnePriceBucket(t, "dense", dense, price, "5.70")
-	getOnePriceBucket(t, "custom1", custom1, price, "5.70")
+	// Define test cases
+	type aTest struct {
+		granularityId       string
+		granularity         openrtb_ext.PriceGranularity
+		expectedPriceBucket string
+	}
+	testGroups := []struct {
+		groupDesc string
+		cpm       float64
+		testCases []aTest
+	}{
+		{
+			groupDesc: "cpm below the max in every price bucket",
+			cpm:       1.87,
+			testCases: []aTest{
+				{"low", low, "1.50"},
+				{"medium", medium, "1.80"},
+				{"high", high, "1.87"},
+				{"auto", auto, "1.85"},
+				{"dense", dense, "1.87"},
+				{"custom1", custom1, "1.86"},
+			},
+		},
+		{
+			groupDesc: "cpm above the max in low price bucket",
+			cpm:       5.72,
+			testCases: []aTest{
+				{"low", low, "5.00"},
+				{"medium", medium, "5.70"},
+				{"high", high, "5.72"},
+				{"auto", auto, "5.70"},
+				{"dense", dense, "5.70"},
+				{"custom1", custom1, "5.70"},
+			},
+		},
+		{
+			groupDesc: "Precision value corner cases",
+			cpm:       1.876,
+			testCases: []aTest{
+				{
+					"Negative precision defaults to number of digits already in CPM float",
+					openrtb_ext.PriceGranularity{Precision: -1, Ranges: []openrtb_ext.GranularityRange{{Max: 5, Increment: 0.05}}},
+					"1.85",
+				},
+				{
+					"Precision value equals zero, we expect to round up to the nearest integer",
+					openrtb_ext.PriceGranularity{Ranges: []openrtb_ext.GranularityRange{{Max: 5, Increment: 0.05}}},
+					"2",
+				},
+				{
+					"Largest precision value PBS supports 15",
+					openrtb_ext.PriceGranularity{Precision: 15, Ranges: []openrtb_ext.GranularityRange{{Max: 5, Increment: 0.05}}},
+					"1.850000000000000",
+				},
+			},
+		},
+		{
+			groupDesc: "Increment value corner cases",
+			cpm:       1.876,
+			testCases: []aTest{
+				{
+					"Negative increment, return empty string",
+					openrtb_ext.PriceGranularity{Precision: 2, Ranges: []openrtb_ext.GranularityRange{{Max: 5, Increment: -0.05}}},
+					"",
+				},
+				{
+					"Zero increment, return empty string",
+					openrtb_ext.PriceGranularity{Precision: 2, Ranges: []openrtb_ext.GranularityRange{{Max: 5}}},
+					"",
+				},
+				{
+					"Increment value is greater than CPM itself, return zero float value",
+					openrtb_ext.PriceGranularity{Precision: 2, Ranges: []openrtb_ext.GranularityRange{{Max: 5, Increment: 1.877}}},
+					"0.00",
+				},
+			},
+		},
+		{
+			groupDesc: "Negative Cpm, return empty string since it does not belong into any range",
+			cpm:       -1.876,
+			testCases: []aTest{{"low", low, ""}},
+		},
+		{
+			groupDesc: "Zero value Cpm, return the same, only in string format",
+			cpm:       0,
+			testCases: []aTest{{"low", low, "0.00"}},
+		},
+		{
+			groupDesc: "Large Cpm, return bucket Max",
+			cpm:       math.MaxFloat64,
+			testCases: []aTest{{"low", low, "5.00"}},
+		},
+	}
 
-}
+	for _, testGroup := range testGroups {
+		for _, test := range testGroup.testCases {
+			priceBucket := GetPriceBucket(testGroup.cpm, test.granularity)
 
-func getOnePriceBucket(t *testing.T, name string, granularity openrtb_ext.PriceGranularity, price float64, expected string) {
-	t.Helper()
-	priceBucket, err := GetCpmStringValue(price, granularity)
-	if err != nil {
-		t.Errorf("Granularity: %s :: GetPriceBucketString: %s", name, err.Error())
-	}
-	if priceBucket != expected {
-		t.Errorf("Granularity: %s :: Expected %s, got %s from %f", name, expected, priceBucket, price)
+			assert.Equal(t, test.expectedPriceBucket, priceBucket, "Group: %s Granularity: %s :: Expected %s, got %s from %f", testGroup.groupDesc, test.granularityId, test.expectedPriceBucket, priceBucket, testGroup.cpm)
+		}
 	}
 }

openrtb_ext/request.go

@@ -8,6 +8,7 @@ import (
 // FirstPartyDataContextExtKey defines the field name within bidrequest.ext reserved
 // for first party data support.
 const FirstPartyDataContextExtKey string = "context"
+const MaxDecimalFigures int = 15
 
 // ExtRequest defines the contract for bidrequest.ext
 type ExtRequest struct {
@@ -179,6 +180,9 @@ func (pg *PriceGranularity) UnmarshalJSON(b []byte) error {
 	if pgraw.Precision < 0 {
 		return errors.New("Price granularity error: precision must be non-negative")
 	}
+	if pgraw.Precision > MaxDecimalFigures {
+		return errors.New("Price granularity error: precision of more than 15 significant figures is not supported")
+	}
 	if len(pgraw.Ranges) > 0 {
 		var prevMax float64 = 0
 		for i, gr := range pgraw.Ranges {
@@ -190,9 +194,6 @@ func (pg *PriceGranularity) UnmarshalJSON(b []byte) error {
 			}
 			// Enforce that we don't read "min" from the request
 			pgraw.Ranges[i].Min = prevMax
-			if pgraw.Ranges[i].Min < prevMax {
-				return errors.New("Price granularity error: overlapping granularity ranges")
-			}
 			prevMax = gr.Max
 		}
 		*pg = PriceGranularity(pgraw)

openrtb_ext/request_test.go

@@ -190,21 +190,43 @@ var validGranularityTests []granularityTestData = []granularityTestData{
 }
 
 func TestGranularityUnmarshalBad(t *testing.T) {
-	tests := [][]byte{
-		[]byte(`[]`),
-		[]byte(`{"precision": -1, "ranges": [{"max":20, "increment":0.5}]}`),
-		[]byte(`{"ranges":[{"max":20, "increment": -1}]}`),
-		[]byte(`{"ranges":[{"max":"20", "increment": "0.1"}]}`),
-		[]byte(`{"ranges":[{"max":20, "increment":0.1}. {"max":10, "increment":0.02}]}`),
-		[]byte(`{"ranges":[{"max":20, "min":10, "increment": 0.1}, {"max":10, "min":0, "increment":0.05}]}`),
-		[]byte(`{"ranges":[{"max":1.0, "increment": 0.07}, {"max" 1.0, "increment": 0.03}]}`),
+	testCases := []struct {
+		description          string
+		jsonPriceGranularity []byte
+	}{
+		{
+			"Malformed",
+			[]byte(`[]`),
+		},
+		{
+			"Negative precision",
+			[]byte(`{"precision": -1, "ranges": [{"max":20, "increment":0.5}]}`),
+		},
+		{
+			"Precision greater than MaxDecimalFigures supported",
+			[]byte(`{"precision": 16, "ranges": [{"max":20, "increment":0.5}]}`),
+		},
+		{
+			"Negative increment",
+			[]byte(`{"ranges":[{"max":20, "increment": -1}]}`),
+		},
+		{
+			"Range with non float64 max value",
+			[]byte(`{"ranges":[{"max":"20", "increment": "0.1"}]}`),
+		},
+		{
+			"Ranges in decreasing order",
+			[]byte(`{"ranges":[{"max":20, "increment":0.1}. {"max":10, "increment":0.02}]}`),
+		},
+		{
+			"Max equal to previous max",
+			[]byte(`{"ranges":[{"max":1.0, "increment": 0.07}, {"max" 1.0, "increment": 0.03}]}`),
+		},
 	}
-	var resolved PriceGranularity
-	for _, b := range tests {
-		resolved = PriceGranularity{}
-		err := json.Unmarshal(b, &resolved)
-		if err == nil {
-			t.Errorf("Invalid granularity unmarshalled without error.\nJSON was: %s\n Resolved to: %v", string(b), resolved)
-		}
+
+	for _, test := range testCases {
+		resolved := PriceGranularity{}
+		err := json.Unmarshal(test.jsonPriceGranularity, &resolved)
+		assert.Errorf(t, err, "Invalid granularity unmarshalled without error.\nJSON was: %s\n Resolved to: %v. Test: %s", string(test.jsonPriceGranularity), resolved, test.description)
 	}
 }