Skip to content

Commit

Permalink
Remove Config Backwards Compatibility: Enable Events (#3135)
Browse files Browse the repository at this point in the history
  • Loading branch information
bsardo authored Sep 27, 2023
1 parent efdda05 commit 0c8f79b
Show file tree
Hide file tree
Showing 10 changed files with 17 additions and 221 deletions.
16 changes: 0 additions & 16 deletions account/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,6 @@ func GetAccount(ctx context.Context, cfg *config.Configuration, fetcher stored_r
account.Privacy.IPv4Config.AnonKeepBits = iputil.IPv4DefaultMaskingBitSize
}

// set the value of events.enabled field based on deprecated events_enabled field and ensure backward compatibility
deprecateEventsEnabledField(account)

return account, nil
}

Expand Down Expand Up @@ -264,16 +261,3 @@ func useGDPRChannelEnabled(account *config.Account) bool {
func useCCPAChannelEnabled(account *config.Account) bool {
return account.CCPA.ChannelEnabled.IsSet() && !account.CCPA.IntegrationEnabled.IsSet()
}

// deprecateEventsEnabledField is responsible for ensuring backwards compatibility of "events_enabled" field.
// This function favors "events.enabled" field over deprecated "events_enabled" field, if values for both are set.
// If only deprecated "events_enabled" field is set then it sets the same value to "events.enabled" field.
func deprecateEventsEnabledField(account *config.Account) {
if account != nil {
if account.Events.Enabled == nil {
account.Events.Enabled = account.EventsEnabled
}
// assign the old value to the new value so old and new are always the same even though the new value is what is used in the application code.
account.EventsEnabled = account.Events.Enabled
}
}
64 changes: 0 additions & 64 deletions account/account_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import (
"github.com/prebid/prebid-server/openrtb_ext"
"github.com/prebid/prebid-server/stored_requests"
"github.com/prebid/prebid-server/util/iputil"
"github.com/prebid/prebid-server/util/ptrutil"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
)
Expand Down Expand Up @@ -590,66 +589,3 @@ func TestAccountUpgradeStatusGetAccount(t *testing.T) {
})
}
}

func TestDeprecateEventsEnabledField(t *testing.T) {
testCases := []struct {
name string
account *config.Account
want *bool
}{
{
name: "account is nil",
account: nil,
want: nil,
},
{
name: "account.EventsEnabled is nil, account.Events.Enabled is nil",
account: &config.Account{
EventsEnabled: nil,
Events: config.Events{
Enabled: nil,
},
},
want: nil,
},
{
name: "account.EventsEnabled is nil, account.Events.Enabled is non-nil",
account: &config.Account{
EventsEnabled: nil,
Events: config.Events{
Enabled: ptrutil.ToPtr(true),
},
},
want: ptrutil.ToPtr(true),
},
{
name: "account.EventsEnabled is non-nil, account.Events.Enabled is nil",
account: &config.Account{
EventsEnabled: ptrutil.ToPtr(true),
Events: config.Events{
Enabled: nil,
},
},
want: ptrutil.ToPtr(true),
},
{
name: "account.EventsEnabled is non-nil, account.Events.Enabled is non-nil",
account: &config.Account{
EventsEnabled: ptrutil.ToPtr(false),
Events: config.Events{
Enabled: ptrutil.ToPtr(true),
},
},
want: ptrutil.ToPtr(true),
},
}

for _, test := range testCases {
t.Run(test.name, func(t *testing.T) {
deprecateEventsEnabledField(test.account)
if test.account != nil {
assert.Equal(t, test.want, test.account.Events.Enabled)
}
})
}
}
1 change: 0 additions & 1 deletion config/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ type Account struct {
ID string `mapstructure:"id" json:"id"`
Disabled bool `mapstructure:"disabled" json:"disabled"`
CacheTTL DefaultTTLs `mapstructure:"cache_ttl" json:"cache_ttl"`
EventsEnabled *bool `mapstructure:"events_enabled" json:"events_enabled"` // Deprecated: Use events.enabled instead.
CCPA AccountCCPA `mapstructure:"ccpa" json:"ccpa"`
GDPR AccountGDPR `mapstructure:"gdpr" json:"gdpr"`
DebugAllow bool `mapstructure:"debug_allow" json:"debug_allow"`
Expand Down
36 changes: 5 additions & 31 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (
"github.com/prebid/openrtb/v19/openrtb2"
"github.com/prebid/prebid-server/errortypes"
"github.com/prebid/prebid-server/openrtb_ext"
"github.com/prebid/prebid-server/util/ptrutil"
"github.com/spf13/viper"
)

Expand Down Expand Up @@ -141,12 +140,12 @@ func (cfg *Configuration) validate(v *viper.Viper) []error {
glog.Warning(`With account_defaults.disabled=true, host-defined accounts must exist and have "disabled":false. All other requests will be rejected.`)
}

if cfg.PriceFloors.Enabled {
glog.Warning(`cfg.PriceFloors.Enabled will currently not do anything as price floors feature is still under development.`)
if cfg.AccountDefaults.Events.Enabled {
glog.Warning(`account_defaults.events has no effect as the feature is under development.`)
}

if len(cfg.AccountDefaults.Events.VASTEvents) > 0 {
errs = append(errs, errors.New("account_defaults.Events.VASTEvents has no effect as the feature is under development."))
if cfg.PriceFloors.Enabled {
glog.Warning(`cfg.PriceFloors.Enabled will currently not do anything as price floors feature is still under development.`)
}

errs = cfg.Experiment.validate(errs)
Expand Down Expand Up @@ -695,9 +694,6 @@ func New(v *viper.Viper, bidderInfos BidderInfos, normalizeBidderName func(strin
// Update account defaults and generate base json for patch
c.AccountDefaults.CacheTTL = c.CacheURL.DefaultTTLs // comment this out to set explicitly in config

// Update the deprecated and new events enabled values for account defaults.
c.AccountDefaults.EventsEnabled, c.AccountDefaults.Events.Enabled = migrateConfigEventsEnabled(c.AccountDefaults.EventsEnabled, c.AccountDefaults.Events.Enabled)

if err := c.MarshalAccountDefaults(); err != nil {
return nil, err
}
Expand Down Expand Up @@ -1019,7 +1015,6 @@ func SetupViper(v *viper.Viper, filename string, bidderInfos BidderInfos) {
v.SetDefault("account_defaults.price_floors.use_dynamic_data", false)
v.SetDefault("account_defaults.price_floors.max_rules", 100)
v.SetDefault("account_defaults.price_floors.max_schema_dims", 3)
v.SetDefault("account_defaults.events_enabled", false)
v.SetDefault("account_defaults.privacy.ipv6.anon_keep_bits", 56)
v.SetDefault("account_defaults.privacy.ipv4.anon_keep_bits", 24)

Expand Down Expand Up @@ -1118,6 +1113,7 @@ func SetupViper(v *viper.Viper, filename string, bidderInfos BidderInfos) {

// Defaults for account_defaults.events.default_url
v.SetDefault("account_defaults.events.default_url", "https://PBS_HOST/event?t=##PBS-EVENTTYPE##&vtype=##PBS-VASTEVENT##&b=##PBS-BIDID##&f=i&a=##PBS-ACCOUNTID##&ts=##PBS-TIMESTAMP##&bidder=##PBS-BIDDER##&int=##PBS-INTEGRATION##&mt=##PBS-MEDIATYPE##&ch=##PBS-CHANNEL##&aid=##PBS-AUCTIONID##&l=##PBS-LINEID##")
v.SetDefault("account_defaults.events.enabled", false)

v.SetDefault("experiment.adscert.mode", "off")
v.SetDefault("experiment.adscert.inprocess.origin", "")
Expand Down Expand Up @@ -1396,28 +1392,6 @@ func migrateConfigDatabaseConnection(v *viper.Viper) {
}
}

// migrateConfigEventsEnabled is responsible for ensuring backward compatibility of events_enabled field.
// This function copies the value of newField "events.enabled" and set it to the oldField "events_enabled".
// This is necessary to achieve the desired order of precedence favoring the account values over the host values
// given the account fetcher JSON merge mechanics.
func migrateConfigEventsEnabled(oldFieldValue *bool, newFieldValue *bool) (updatedOldFieldValue, updatedNewFieldValue *bool) {
newField := "account_defaults.events.enabled"
oldField := "account_defaults.events_enabled"

updatedOldFieldValue = oldFieldValue
if oldFieldValue != nil {
glog.Warningf("%s is deprecated and should be changed to %s", oldField, newField)
}
if newFieldValue != nil {
if oldFieldValue != nil {
glog.Warningf("using %s and ignoring deprecated %s", newField, oldField)
}
updatedOldFieldValue = ptrutil.ToPtr(*newFieldValue)
}

return updatedOldFieldValue, nil
}

func isConfigInfoPresent(v *viper.Viper, prefix string, fields []string) bool {
prefix = prefix + "."
for _, field := range fields {
Expand Down
57 changes: 2 additions & 55 deletions config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (

"github.com/prebid/go-gdpr/consentconstants"
"github.com/prebid/prebid-server/openrtb_ext"
"github.com/prebid/prebid-server/util/ptrutil"
"github.com/spf13/viper"
"github.com/stretchr/testify/assert"
)
Expand Down Expand Up @@ -191,8 +190,7 @@ func TestDefaults(t *testing.T) {
cmpBools(t, "account_defaults.price_floors.use_dynamic_data", false, cfg.AccountDefaults.PriceFloors.UseDynamicData)
cmpInts(t, "account_defaults.price_floors.max_rules", 100, cfg.AccountDefaults.PriceFloors.MaxRule)
cmpInts(t, "account_defaults.price_floors.max_schema_dims", 3, cfg.AccountDefaults.PriceFloors.MaxSchemaDims)
cmpBools(t, "account_defaults.events_enabled", *cfg.AccountDefaults.EventsEnabled, false)
cmpNils(t, "account_defaults.events.enabled", cfg.AccountDefaults.Events.Enabled)
cmpBools(t, "account_defaults.events.enabled", false, cfg.AccountDefaults.Events.Enabled)

cmpBools(t, "hooks.enabled", false, cfg.Hooks.Enabled)
cmpStrings(t, "validations.banner_creative_max_size", "skip", cfg.Validations.BannerCreativeMaxSize)
Expand Down Expand Up @@ -469,7 +467,6 @@ hooks:
price_floors:
enabled: true
account_defaults:
events_enabled: false
events:
enabled: true
price_floors:
Expand Down Expand Up @@ -582,8 +579,7 @@ func TestFullConfig(t *testing.T) {
cmpBools(t, "account_defaults.price_floors.use_dynamic_data", true, cfg.AccountDefaults.PriceFloors.UseDynamicData)
cmpInts(t, "account_defaults.price_floors.max_rules", 120, cfg.AccountDefaults.PriceFloors.MaxRule)
cmpInts(t, "account_defaults.price_floors.max_schema_dims", 5, cfg.AccountDefaults.PriceFloors.MaxSchemaDims)
cmpBools(t, "account_defaults.events_enabled", *cfg.AccountDefaults.EventsEnabled, true)
cmpNils(t, "account_defaults.events.enabled", cfg.AccountDefaults.Events.Enabled)
cmpBools(t, "account_defaults.events.enabled", true, cfg.AccountDefaults.Events.Enabled)

cmpInts(t, "account_defaults.privacy.ipv6.anon_keep_bits", 50, cfg.AccountDefaults.Privacy.IPv6Config.AnonKeepBits)
cmpInts(t, "account_defaults.privacy.ipv4.anon_keep_bits", 20, cfg.AccountDefaults.Privacy.IPv4Config.AnonKeepBits)
Expand Down Expand Up @@ -3287,52 +3283,3 @@ func TestTCF2FeatureOneVendorException(t *testing.T) {
assert.Equal(t, tt.wantIsVendorException, value, tt.description)
}
}

func TestMigrateConfigEventsEnabled(t *testing.T) {
testCases := []struct {
name string
oldFieldValue *bool
newFieldValue *bool
expectedOldFieldValue *bool
expectedNewFieldValue *bool
}{
{
name: "Both old and new fields are nil",
oldFieldValue: nil,
newFieldValue: nil,
expectedOldFieldValue: nil,
expectedNewFieldValue: nil,
},
{
name: "Only old field is set",
oldFieldValue: ptrutil.ToPtr(true),
newFieldValue: nil,
expectedOldFieldValue: ptrutil.ToPtr(true),
expectedNewFieldValue: nil,
},
{
name: "Only new field is set",
oldFieldValue: nil,
newFieldValue: ptrutil.ToPtr(true),
expectedOldFieldValue: ptrutil.ToPtr(true),
expectedNewFieldValue: nil,
},
{
name: "Both old and new fields are set, override old field with new field value",
oldFieldValue: ptrutil.ToPtr(false),
newFieldValue: ptrutil.ToPtr(true),
expectedOldFieldValue: ptrutil.ToPtr(true),
expectedNewFieldValue: nil,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
updatedOldFieldValue, updatedNewFieldValue := migrateConfigEventsEnabled(tc.oldFieldValue, tc.newFieldValue)

assert.Equal(t, tc.expectedOldFieldValue, updatedOldFieldValue)
assert.Nil(t, updatedNewFieldValue)
assert.Nil(t, tc.expectedNewFieldValue)
})
}
}
9 changes: 2 additions & 7 deletions config/events.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,14 @@ type VASTEvent struct {
// within the VAST XML
// Don't enable this feature. It is still under developmment. Please follow https://github.com/prebid/prebid-server/issues/1725 for more updates
type Events struct {
Enabled *bool `mapstructure:"enabled" json:"enabled"`
Enabled bool `mapstructure:"enabled" json:"enabled"`
DefaultURL string `mapstructure:"default_url" json:"default_url"`
VASTEvents []VASTEvent `mapstructure:"vast_events" json:"vast_events,omitempty"`
}

// validate verifies the events object and returns error if at least one is invalid.
func (e Events) validate(errs []error) []error {
if e.IsEnabled() {
if e.Enabled {
if !isValidURL(e.DefaultURL) {
return append(errs, errors.New("Invalid events.default_url"))
}
Expand Down Expand Up @@ -147,8 +147,3 @@ func isValidURL(eventURL string) bool {
func (e VASTEvent) isTrackingEvent() bool {
return e.CreateElement == TrackingVASTElement
}

// IsEnabled function returns the value of events.enabled field
func (e Events) IsEnabled() bool {
return e.Enabled != nil && *e.Enabled
}
47 changes: 4 additions & 43 deletions config/events_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package config
import (
"testing"

"github.com/prebid/prebid-server/util/ptrutil"
"github.com/stretchr/testify/assert"
)

Expand Down Expand Up @@ -262,37 +261,30 @@ func TestValidate(t *testing.T) {
{
description: "Empty default URL",
events: Events{
Enabled: ptrutil.ToPtr(true),
Enabled: true,
},
expectErr: true,
},
{
description: "Events are disabled. Skips validations",
events: Events{
Enabled: ptrutil.ToPtr(false),
Enabled: false,
DefaultURL: "",
},
expectErr: false,
},
{
description: "Events are nil. Skip validations",
events: Events{
Enabled: nil,
},
expectErr: false,
},
{
description: "No VAST Events and default URL present",
events: Events{
Enabled: ptrutil.ToPtr(true),
Enabled: true,
DefaultURL: "http://prebid.org",
},
expectErr: false,
},
{
description: "Invalid VAST Event",
events: Events{
Enabled: ptrutil.ToPtr(true),
Enabled: true,
DefaultURL: "http://prebid.org",
VASTEvents: []VASTEvent{
{},
Expand Down Expand Up @@ -335,34 +327,3 @@ func TestValidateVASTEvents(t *testing.T) {
assert.Equal(t, !test.expectErr, err == nil, test.description)
}
}

func TestIsEnabled(t *testing.T) {
testCases := []struct {
name string
events Events
expected bool
}{
{
name: "nil pointer",
events: Events{},
expected: false,
},
{
name: "event false",
events: Events{Enabled: ptrutil.ToPtr(false)},
expected: false,
},
{
name: "event true",
events: Events{Enabled: ptrutil.ToPtr(true)},
expected: true,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
actual := tc.events.IsEnabled()
assert.Equal(t, tc.expected, actual)
})
}
}
2 changes: 1 addition & 1 deletion endpoints/events/event.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ func (e *eventEndpoint) Handle(w http.ResponseWriter, r *http.Request, _ httprou
}

// Check if events are enabled for the account
if !account.Events.IsEnabled() {
if !account.Events.Enabled {
w.WriteHeader(http.StatusUnauthorized)
w.Write([]byte(fmt.Sprintf("Account '%s' doesn't support events", eventRequest.AccountID)))
return
Expand Down
Loading

0 comments on commit 0c8f79b

Please sign in to comment.