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.

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

Media type price granularity #2721

Merged
merged 18 commits into from
Jun 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions endpoints/openrtb2/amp_auction_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1720,7 +1720,7 @@ func TestBuildAmpObject(t *testing.T) {
},
AT: 1,
TMax: 500,
Ext: json.RawMessage(`{"prebid":{"cache":{"bids":{}},"channel":{"name":"amp","version":""},"targeting":{"pricegranularity":{"precision":2,"ranges":[{"min":0,"max":20,"increment":0.1}]},"includewinners":true,"includebidderkeys":true}}}`),
Ext: json.RawMessage(`{"prebid":{"cache":{"bids":{}},"channel":{"name":"amp","version":""},"targeting":{"pricegranularity":{"precision":2,"ranges":[{"min":0,"max":20,"increment":0.1}]},"mediatypepricegranularity":{},"includewinners":true,"includebidderkeys":true}}}`),
},
AuctionResponse: &openrtb2.BidResponse{
SeatBid: []openrtb2.SeatBid{{
Expand Down Expand Up @@ -1774,7 +1774,7 @@ func TestBuildAmpObject(t *testing.T) {
},
AT: 1,
TMax: 500,
Ext: json.RawMessage(`{"prebid":{"cache":{"bids":{}},"channel":{"name":"amp","version":""},"targeting":{"pricegranularity":{"precision":2,"ranges":[{"min":0,"max":20,"increment":0.1}]},"includewinners":true,"includebidderkeys":true}}}`),
Ext: json.RawMessage(`{"prebid":{"cache":{"bids":{}},"channel":{"name":"amp","version":""},"targeting":{"pricegranularity":{"precision":2,"ranges":[{"min":0,"max":20,"increment":0.1}]},"mediatypepricegranularity":{},"includewinners":true,"includebidderkeys":true}}}`),
},
AuctionResponse: &openrtb2.BidResponse{
SeatBid: []openrtb2.SeatBid{{
Expand Down
55 changes: 38 additions & 17 deletions endpoints/openrtb2/auction.go
Original file line number Diff line number Diff line change
Expand Up @@ -1584,29 +1584,50 @@ func validateTargeting(t *openrtb_ext.ExtRequestTargeting) error {
}

if t.PriceGranularity != nil {
pg := t.PriceGranularity
if err := validatePriceGranularity(t.PriceGranularity); err != nil {
return err
}
}

if pg.Precision == nil {
return errors.New("Price granularity error: precision is required")
} else if *pg.Precision < 0 {
return errors.New("Price granularity error: precision must be non-negative")
} else if *pg.Precision > openrtb_ext.MaxDecimalFigures {
return fmt.Errorf("Price granularity error: precision of more than %d significant figures is not supported", openrtb_ext.MaxDecimalFigures)
if t.MediaTypePriceGranularity.Video != nil {
if err := validatePriceGranularity(t.MediaTypePriceGranularity.Video); err != nil {
return err
}
}
if t.MediaTypePriceGranularity.Banner != nil {
if err := validatePriceGranularity(t.MediaTypePriceGranularity.Banner); err != nil {
return err
}
}
if t.MediaTypePriceGranularity.Native != nil {
if err := validatePriceGranularity(t.MediaTypePriceGranularity.Native); err != nil {
return err
}
}

var prevMax float64 = 0
for _, gr := range pg.Ranges {
if gr.Max <= prevMax {
return errors.New(`Price granularity error: range list must be ordered with increasing "max"`)
}
return nil
}

if gr.Increment <= 0.0 {
return errors.New("Price granularity error: increment must be a nonzero positive number")
}
prevMax = gr.Max
}
func validatePriceGranularity(pg *openrtb_ext.PriceGranularity) error {
if pg.Precision == nil {
return errors.New("Price granularity error: precision is required")
Copy link
Contributor

Choose a reason for hiding this comment

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

Will we ever get to a situation where this will be nil? Validate is called after SetDefaults and as part of set defaults we ensure that precision is set to the default value if not specified. See here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed offline. No changes needed.

} else if *pg.Precision < 0 {
return errors.New("Price granularity error: precision must be non-negative")
} else if *pg.Precision > openrtb_ext.MaxDecimalFigures {
return fmt.Errorf("Price granularity error: precision of more than %d significant figures is not supported", openrtb_ext.MaxDecimalFigures)
}

var prevMax float64 = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a Go preference for how this should be initialized?

e.g: prevMax := 0.0 vs. var prevMax float64 = 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code was moved without modifications from func validateTargeting(t *openrtb_ext.ExtRequestTargeting) error to it's own func validatePriceGranularity(pg *openrtb_ext.PriceGranularity) error.
For this case I think type was declared explicitly like this to show this is a price comparison.
We can discuss it with the team.

for _, gr := range pg.Ranges {
if gr.Max <= prevMax {
return errors.New(`Price granularity error: range list must be ordered with increasing "max"`)
}

if gr.Increment <= 0.0 {
return errors.New("Price granularity error: increment must be a nonzero positive number")
}
prevMax = gr.Max
}
return nil
}

Expand Down
234 changes: 210 additions & 24 deletions endpoints/openrtb2/auction_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1863,60 +1863,172 @@ func TestValidateTargeting(t *testing.T) {
expectedError: nil,
},
{
name: "price granularity empty",
name: "price granularity ranges out of order",
givenTargeting: &openrtb_ext.ExtRequestTargeting{
IncludeWinners: ptrutil.ToPtr(true),
PriceGranularity: &openrtb_ext.PriceGranularity{},
IncludeWinners: ptrutil.ToPtr(true),
PriceGranularity: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 1.0, Max: 2.0, Increment: 0.2},
{Min: 0.0, Max: 1.0, Increment: 0.5},
},
},
},
expectedError: errors.New("Price granularity error: precision is required"),
expectedError: errors.New(`Price granularity error: range list must be ordered with increasing "max"`),
},
{
name: "price granularity negative",
name: "media type price granularity video correct",
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we have separated out price granularity validation in a separate method, you think we can add another unit test specifically for logic in validatePriceGranularity function and move some of the test cases pertaining to that logic from this test to the new test?

This allows detailed test cases to be separated out from higher level function test cases. Plus, the test cases in this test have become too large and this will help keep them to a manageable number as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! Some tests are gone due to duplicated tests with validatePriceGranularity. I left some of previously added tests where we test more than one price granularities where all are incorrect/correct/or some correct and some incorrect. I'm open to remove some of them if you think they are redundant

givenTargeting: &openrtb_ext.ExtRequestTargeting{
IncludeWinners: ptrutil.ToPtr(true),
PriceGranularity: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(-1),
MediaTypePriceGranularity: openrtb_ext.MediaTypePriceGranularity{
Video: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 10.0, Increment: 1},
},
},
},
},
expectedError: errors.New("Price granularity error: precision must be non-negative"),
expectedError: nil,
},
{
name: "price granularity greater than max",
name: "media type price granularity banner correct",
givenTargeting: &openrtb_ext.ExtRequestTargeting{
IncludeWinners: ptrutil.ToPtr(true),
PriceGranularity: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(openrtb_ext.MaxDecimalFigures + 1),
MediaTypePriceGranularity: openrtb_ext.MediaTypePriceGranularity{
Banner: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 10.0, Increment: 1},
},
},
},
},
expectedError: errors.New("Price granularity error: precision of more than 15 significant figures is not supported"),
expectedError: nil,
},
{
name: "price granularity ranges out of order",
name: "media type price granularity native correct",
givenTargeting: &openrtb_ext.ExtRequestTargeting{
IncludeWinners: ptrutil.ToPtr(true),
PriceGranularity: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 1.0, Max: 2.0, Increment: 0.2},
{Min: 0.0, Max: 1.0, Increment: 0.5},
MediaTypePriceGranularity: openrtb_ext.MediaTypePriceGranularity{
Native: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 20.0, Increment: 1},
},
},
},
},
expectedError: errors.New(`Price granularity error: range list must be ordered with increasing "max"`),
expectedError: nil,
},
{
name: "price granularity negative increment",
name: "media type price granularity video and banner correct",
givenTargeting: &openrtb_ext.ExtRequestTargeting{
IncludeWinners: ptrutil.ToPtr(true),
PriceGranularity: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 1.0, Increment: -0.1},
MediaTypePriceGranularity: openrtb_ext.MediaTypePriceGranularity{
Banner: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 10.0, Increment: 1},
},
},
Video: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 10.0, Increment: 1},
},
},
},
},
expectedError: nil,
},
{
name: "media type price granularity video incorrect",
givenTargeting: &openrtb_ext.ExtRequestTargeting{
IncludeWinners: ptrutil.ToPtr(true),
MediaTypePriceGranularity: openrtb_ext.MediaTypePriceGranularity{
Video: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 10.0, Increment: -1},
},
},
},
},
expectedError: errors.New("Price granularity error: increment must be a nonzero positive number"),
},
{
name: "media type price granularity banner incorrect",
givenTargeting: &openrtb_ext.ExtRequestTargeting{
IncludeWinners: ptrutil.ToPtr(true),
MediaTypePriceGranularity: openrtb_ext.MediaTypePriceGranularity{
Banner: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 0.0, Increment: 1},
},
},
},
},
expectedError: errors.New("Price granularity error: range list must be ordered with increasing \"max\""),
},
{
name: "media type price granularity native incorrect",
givenTargeting: &openrtb_ext.ExtRequestTargeting{
IncludeWinners: ptrutil.ToPtr(true),
MediaTypePriceGranularity: openrtb_ext.MediaTypePriceGranularity{
Native: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 0.0, Increment: 1},
},
},
},
},
expectedError: errors.New("Price granularity error: range list must be ordered with increasing \"max\""),
},
{
name: "media type price granularity video correct and banner incorrect",
givenTargeting: &openrtb_ext.ExtRequestTargeting{
IncludeWinners: ptrutil.ToPtr(true),
MediaTypePriceGranularity: openrtb_ext.MediaTypePriceGranularity{
Banner: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 10.0, Increment: -1},
},
},
Video: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 0.0, Increment: 1},
},
},
},
},
expectedError: errors.New("Price granularity error: range list must be ordered with increasing \"max\""),
},
{
name: "media type price granularity native incorrect and banner correct",
givenTargeting: &openrtb_ext.ExtRequestTargeting{
IncludeWinners: ptrutil.ToPtr(true),
MediaTypePriceGranularity: openrtb_ext.MediaTypePriceGranularity{
Native: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 10.0, Increment: -1},
},
},
Video: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 0.0, Increment: 1},
},
},
},
},
expectedError: errors.New("Price granularity error: range list must be ordered with increasing \"max\""),
},
}

for _, tc := range testCases {
Expand All @@ -1926,6 +2038,80 @@ func TestValidateTargeting(t *testing.T) {
}
}

func TestValidatePriceGranularity(t *testing.T) {
testCases := []struct {
description string
givenPriceGranularity *openrtb_ext.PriceGranularity
expectedError error
}{
{
description: "Precision is nil",
givenPriceGranularity: &openrtb_ext.PriceGranularity{
Precision: nil,
},
expectedError: errors.New("Price granularity error: precision is required"),
},
{
description: "Precision is negative",
givenPriceGranularity: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(-1),
},
expectedError: errors.New("Price granularity error: precision must be non-negative"),
},
{
description: "Precision is too big",
givenPriceGranularity: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(20),
},
expectedError: errors.New("Price granularity error: precision of more than 15 significant figures is not supported"),
},
{
description: "price granularity ranges out of order",
givenPriceGranularity: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 1.0, Max: 2.0, Increment: 0.2},
{Min: 0.0, Max: 1.0, Increment: 0.5},
},
},
expectedError: errors.New(`Price granularity error: range list must be ordered with increasing "max"`),
},
{
description: "price granularity negative increment",
givenPriceGranularity: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 1.0, Increment: -0.1},
},
},
expectedError: errors.New("Price granularity error: increment must be a nonzero positive number"),
},
{
description: "price granularity correct",
givenPriceGranularity: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
Ranges: []openrtb_ext.GranularityRange{
{Min: 0.0, Max: 10.0, Increment: 1},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for separating the test cases out. This looks great. Can you please just add one more test case where a valid precision value is present but ranges is empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

},
expectedError: nil,
},
{
description: "price granularity with correct precision and ranges not specified",
givenPriceGranularity: &openrtb_ext.PriceGranularity{
Precision: ptrutil.ToPtr(2),
},
expectedError: nil,
},
}

for _, tc := range testCases {
t.Run(tc.description, func(t *testing.T) {
assert.Equal(t, tc.expectedError, validatePriceGranularity(tc.givenPriceGranularity))
})
}
}

func TestValidateOrFillChannel(t *testing.T) {
testCases := []struct {
description string
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,8 @@
]
},
"includewinners": true,
"includebidderkeys": true
"includebidderkeys": true,
"mediatypepricegranularity": {}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,8 @@
]
},
"includewinners": true,
"includebidderkeys": true
"includebidderkeys": true,
"mediatypepricegranularity": {}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,8 @@
]
},
"includewinners": true,
"includebidderkeys": true
"includebidderkeys": true,
"mediatypepricegranularity": {}
}
}
}
Expand Down
Loading