-
Notifications
You must be signed in to change notification settings - Fork 763
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
Updated FloorsSchemaVersion data type from string to int #3592
Conversation
floors/validate.go
Outdated
@@ -55,7 +55,7 @@ func validateFloorRulesAndLowerValidRuleKey(schema openrtb_ext.PriceFloorSchema, | |||
|
|||
// validateFloorParams validates SchemaVersion, SkipRate and FloorMin | |||
func validateFloorParams(extFloorRules *openrtb_ext.PriceFloorRules) error { | |||
if extFloorRules.Data != nil && len(extFloorRules.Data.FloorsSchemaVersion) > 0 && extFloorRules.Data.FloorsSchemaVersion != "2" { | |||
if extFloorRules.Data != nil && extFloorRules.Data.FloorsSchemaVersion > 0 && extFloorRules.Data.FloorsSchemaVersion != 2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reading the price floor schema 2 documentation that you referenced in the description above, I have two questions:
- According to the docs, the default value of
data.floorsSchemaVersion
is1
and "if no schema version is provided, the module will assume version 1". Is PBS intended to not support version 1? Or why are we erroring out in line 59 when conditionextFloorRules.Data.FloorsSchemaVersion != 2
evaluates totrue
?
Extract from the docs:Param Type Description Default data.floorsSchemaVersion integer The module supports two version of the data schema. Version 1 allows for only one model to be applied in a given data set, whereas Version 2 allows you to sample multiple models selected by supplied weights. If no schema version is provided, the module will assume version 1 for the sake of backwards compatiblity. 1 - If we indeed not support version 1, does
FloorsSchemaVersion
value of0
have a special meaning? If not, can we simplify the condition?
56 // validateFloorParams validates SchemaVersion, SkipRate and FloorMin
57 func validateFloorParams(extFloorRules *openrtb_ext.PriceFloorRules) error {
58 - if extFloorRules.Data != nil && extFloorRules.Data.FloorsSchemaVersion > 0 && extFloorRules.Data.FloorsSchemaVersion != 2 {
+ if extFloorRules.Data != nil && extFloorRules.Data.FloorsSchemaVersion != 2 {
59 return fmt.Errorf("Invalid FloorsSchemaVersion = '%v', supported version 2", extFloorRules.Data.FloorsSchemaVersion)
60 }
61
floors/validate.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
It seems to be https://docs.prebid.org/dev-docs/modules/floors.html#schema-2 needs to be updated for default value as 2, as its Schema Version 2 (Reference https://prebid.slack.com/archives/C7TLBVAH3/p1711357926151519)
-
As per Floors PRD Floors Feature PRD, Requirements for validation is
Schema version validation. The feature should validate that if defined, floorsSchemaVersion is 2. This field may be missing, and if so, it's assumed that it will conform to schema 2. Unknown fields may be ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @pm-jaydeep-mohite thank you for your contribution.
Prebid Documentation [https://docs.prebid.org/dev-docs/modules/floors.html#schema-2] is updated for data type of data.floorsSchemaVersion from string to integer