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

Updated FloorsSchemaVersion data type from string to int #3592

Merged
merged 2 commits into from
Apr 2, 2024

Conversation

pm-jaydeep-mohite
Copy link
Contributor

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

@@ -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 {
Copy link
Contributor

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:

  1. According to the docs, the default value of data.floorsSchemaVersion is 1 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 condition extFloorRules.Data.FloorsSchemaVersion != 2 evaluates to true?
    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
  2. If we indeed not support version 1, does FloorsSchemaVersion value of 0 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

Copy link
Contributor Author

@pm-jaydeep-mohite pm-jaydeep-mohite Mar 29, 2024

Choose a reason for hiding this comment

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

  1. 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)

  2. 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.

Copy link
Contributor

@guscarreon guscarreon left a 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.

@bsardo bsardo merged commit 47c9434 into prebid:master Apr 2, 2024
3 checks passed
@pm-nilesh-chate pm-nilesh-chate deleted the floors_schemaVersion branch April 8, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants