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

Changed FS bidder names to mixed case #2390

Merged
merged 6 commits into from
Sep 29, 2022
Merged
Show file tree
Hide file tree
Changes from 5 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
65 changes: 50 additions & 15 deletions config/bidderinfo.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/prebid/prebid-server/util/sliceutil"
"io/ioutil"
"log"
"path/filepath"
"strings"
"text/template"

Expand Down Expand Up @@ -170,38 +171,66 @@ func (bi BidderInfo) IsEnabled() bool {
return !bi.Disabled
}

// LoadBidderInfo parses all static/bidder-info/{bidder}.yaml files from the file system.
func LoadBidderInfo(path string) (BidderInfos, error) {
bidderConfigs, err := ioutil.ReadDir(path)
type InfoReader interface {
Read() (map[string][]byte, error)
}

type InfoReaderFromDisk struct {
Path string
}

func (r InfoReaderFromDisk) Read() (map[string][]byte, error) {
bidderConfigs, err := ioutil.ReadDir(r.Path)
if err != nil {
log.Fatal(err)
}

infos := BidderInfos{}

bidderInfos := make(map[string][]byte)
for _, bidderConfig := range bidderConfigs {
if bidderConfig.IsDir() {
continue //ignore directories
}
fileName := bidderConfig.Name()
filePath := fmt.Sprintf("%v/%v", path, fileName)
filePath := filepath.Join(r.Path, fileName)
data, err := ioutil.ReadFile(filePath)
if err != nil {
return nil, err
}
bidderInfos[fileName] = data
}
return bidderInfos, nil
}

func LoadBidderInfoFromDisk(path string) (BidderInfos, error) {
bidderInfoReader := InfoReaderFromDisk{Path: path}
return LoadBidderInfo(bidderInfoReader)
}

func LoadBidderInfo(reader InfoReader) (BidderInfos, error) {
return processBidderInfos(reader, openrtb_ext.NormalizeBidderName)
}

func processBidderInfos(reader InfoReader, normalizeBidderName func(string) (openrtb_ext.BidderName, bool)) (BidderInfos, error) {
bidderConfigs, err := reader.Read()
if err != nil {
return nil, fmt.Errorf("error loading bidders data")
}

infos := BidderInfos{}

for fileName, data := range bidderConfigs {
info := BidderInfo{}
if err := yaml.Unmarshal(data, &info); err != nil {
return nil, fmt.Errorf("error parsing yaml for bidder %s: %v", fileName, err)
return nil, fmt.Errorf("error parsing config for bidder %s: %v", fileName, err)
}

bidderName := strings.Split(fileName, ".")
if len(bidderName) == 2 && bidderName[1] == "yaml" {
// all bidder names loaded from the file system should be in lowercase
// to match bidder names in Viper config bidder infos
// this will prevent from missing bidder config overrides
// in func applyBidderInfoConfigOverrides
infos[(strings.ToLower(bidderName[0]))] = info
normalizedBidderName, bidderNameExists := normalizeBidderName(bidderName[0])
if !bidderNameExists {
return nil, fmt.Errorf("error parsing config for bidder %s: unknown bidder", fileName)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

A map check is less expensive than unmarshalling the yaml file. A small optimization would occur if we switch the order of the operations:

213   func processBidderInfos(reader InfoReader, normalizeBidderName func(string) (openrtb_ext.BidderName, bool)) (BidderInfos, error) {
214       bidderConfigs, err := reader.Read()
215       if err != nil {
216           return nil, fmt.Errorf("error loading bidders data")
217       }
218   
219       infos := BidderInfos{}
220   
221       for fileName, data := range bidderConfigs {
222 -         info := BidderInfo{}
223 -         if err := yaml.Unmarshal(data, &info); err != nil {
224 -             return nil, fmt.Errorf("error parsing config for bidder %s: %v", fileName, err)
225 -         }
226 - 
227           bidderName := strings.Split(fileName, ".")
228           if len(bidderName) == 2 && bidderName[1] == "yaml" {
229               normalizedBidderName, bidderNameExists := normalizeBidderName(bidderName[0])
230               if !bidderNameExists {
231                   return nil, fmt.Errorf("error parsing config for bidder %s: unknown bidder", fileName)
232               }
    + 
    +             info := BidderInfo{}
    +             if err := yaml.Unmarshal(data, &info); err != nil {
    +                 return nil, fmt.Errorf("error parsing config for bidder %s: %v", fileName, err)
    +             }
    + 
233               infos[string(normalizedBidderName)] = info
234           }
235       }
236       return infos, nil
config/bidderinfo.go

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like a good idea. I would like to mention this code runs once at startup so performance is not a critical concern with this flow.

infos[string(normalizedBidderName)] = info
}
}
return infos, nil
Expand Down Expand Up @@ -349,9 +378,13 @@ func validateSyncer(bidderInfo BidderInfo) error {
return nil
}

func applyBidderInfoConfigOverrides(configBidderInfos BidderInfos, fsBidderInfos BidderInfos) (BidderInfos, error) {
func applyBidderInfoConfigOverrides(configBidderInfos BidderInfos, fsBidderInfos BidderInfos, normalizeBidderName func(string) (openrtb_ext.BidderName, bool)) (BidderInfos, error) {
for bidderName, bidderInfo := range configBidderInfos {
if fsBidderCfg, exists := fsBidderInfos[bidderName]; exists {
normalizedBidderName, bidderNameExists := normalizeBidderName(bidderName)
if !bidderNameExists {
return nil, fmt.Errorf("error setting configuration for bidder %s: unknown bidder", bidderName)
}
if fsBidderCfg, exists := fsBidderInfos[string(normalizedBidderName)]; exists {
bidderInfo.Syncer = bidderInfo.Syncer.Override(fsBidderCfg.Syncer)

if bidderInfo.Endpoint == "" && len(fsBidderCfg.Endpoint) > 0 {
Expand Down Expand Up @@ -436,7 +469,9 @@ func applyBidderInfoConfigOverrides(configBidderInfos BidderInfos, fsBidderInfos
glog.Warningf("adapters.%s.usersync_url is deprecated and will be removed in a future version, please update to the latest user sync config values", strings.ToLower(bidderName))
}

fsBidderInfos[bidderName] = bidderInfo
fsBidderInfos[string(normalizedBidderName)] = bidderInfo
} else {
return nil, fmt.Errorf("error finding configuration for bidder %s: unknown bidder", bidderName)
}
}
return fsBidderInfos, nil
Expand Down
123 changes: 114 additions & 9 deletions config/bidderinfo_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,12 @@ import (
"github.com/stretchr/testify/assert"
)

const testInfoFilesPath = "./test/bidder-info"
const testInfoFilesPathValid = "./test/bidder-info-valid"
const testSimpleYAML = `
maintainer:
email: "[email protected]"
gvlVendorID: 42
`
const fullBidderYAMLConfig = `
maintainer:
email: "[email protected]"
Expand All @@ -36,14 +41,14 @@ endpointCompression: "GZIP"
`

func TestLoadBidderInfoFromDisk(t *testing.T) {
// should appear in result in lowercase
bidder := "somebidder"
// should appear in result in mixed case
bidder := "stroeerCore"
Copy link
Contributor

@SyntaxNode SyntaxNode Sep 28, 2022

Choose a reason for hiding this comment

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

For future contributors: stroeerCore was chosen at random just because we needed a real mixed case bidder name. There is no other significance to this choice. Our usual real bidder choices do not have mixed upper and lower case in their name.

trueValue := true

adapterConfigs := make(map[string]Adapter)
adapterConfigs[strings.ToLower(bidder)] = Adapter{}

infos, err := LoadBidderInfo(testInfoFilesPath)
infos, err := LoadBidderInfoFromDisk(testInfoFilesPathValid)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -84,6 +89,80 @@ func TestLoadBidderInfoFromDisk(t *testing.T) {
assert.Equal(t, expected, infos)
}

func TestProcessBidderInfo(t *testing.T) {
testCases := []struct {
description string
bidderInfos map[string][]byte
expectedBidderInfos BidderInfos
expectError string
}{
{
description: "Valid bidder info",
bidderInfos: map[string][]byte{
"bidderA.yaml": []byte(testSimpleYAML),
},
expectedBidderInfos: BidderInfos{
"bidderA": BidderInfo{
Maintainer: &MaintainerInfo{
Email: "[email protected]",
},
GVLVendorID: 42,
},
},
expectError: "",
},
{
description: "Bidder doesn't exist in bidder info list",
bidderInfos: map[string][]byte{
"unknown.yaml": []byte(testSimpleYAML),
},
expectedBidderInfos: nil,
expectError: "error parsing config for bidder unknown.yaml",
},
{
description: "Invalid bidder config",
bidderInfos: map[string][]byte{
"bidderA.yaml": []byte("invalid bidder config"),
},
expectedBidderInfos: nil,
expectError: "error parsing config for bidder bidderA.yaml",
},
}
for _, test := range testCases {
reader := StubInfoReader{test.bidderInfos}
bidderInfos, err := processBidderInfos(reader, mockNormalizeBidderName)
if test.expectError != "" {
assert.ErrorContains(t, err, test.expectError, "")
} else {
assert.Equal(t, test.expectedBidderInfos, bidderInfos, "incorrect bidder infos for test case: %s", test.description)
}

}

}

type StubInfoReader struct {
mockBidderInfos map[string][]byte
}

func (r StubInfoReader) Read() (map[string][]byte, error) {
return r.mockBidderInfos, nil
}

var testBidderNames = map[string]openrtb_ext.BidderName{
"biddera": openrtb_ext.BidderName("bidderA"),
"bidderb": openrtb_ext.BidderName("bidderB"),
"bidder1": openrtb_ext.BidderName("bidder1"),
"bidder2": openrtb_ext.BidderName("bidder2"),
"a": openrtb_ext.BidderName("a"),
}

func mockNormalizeBidderName(name string) (openrtb_ext.BidderName, bool) {
nameLower := strings.ToLower(name)
bidderName, exists := testBidderNames[nameLower]
return bidderName, exists
}

func TestToGVLVendorIDMap(t *testing.T) {
givenBidderInfos := BidderInfos{
"bidderA": BidderInfo{Disabled: false, GVLVendorID: 0},
Expand All @@ -105,7 +184,7 @@ const bidderInfoRelativePath = "../static/bidder-info"
// TestBidderInfoFiles ensures each bidder has a valid static/bidder-info/bidder.yaml file. Validation is performed directly
// against the file system with separate yaml unmarshalling from the LoadBidderInfo func.
func TestBidderInfoFiles(t *testing.T) {
_, err := LoadBidderInfo(bidderInfoRelativePath)
_, err := LoadBidderInfoFromDisk(bidderInfoRelativePath)
if err != nil {
assert.Fail(t, err.Error(), "Errors in bidder info files")
}
Expand Down Expand Up @@ -687,7 +766,7 @@ func TestApplyBidderInfoConfigSyncerOverrides(t *testing.T) {
}

for _, test := range testCases {
bidderInfos, resultErr := applyBidderInfoConfigOverrides(test.givenConfigBidderInfos, test.givenFsBidderInfos)
bidderInfos, resultErr := applyBidderInfoConfigOverrides(test.givenConfigBidderInfos, test.givenFsBidderInfos, mockNormalizeBidderName)
if test.expectedError == "" {
assert.NoError(t, resultErr, test.description+":err")
assert.Equal(t, test.expectedBidderInfos, bidderInfos, test.description+":result")
Expand Down Expand Up @@ -849,17 +928,43 @@ func TestApplyBidderInfoConfigOverrides(t *testing.T) {
},
}
for _, test := range testCases {
bidderInfos, resultErr := applyBidderInfoConfigOverrides(test.givenConfigBidderInfos, test.givenFsBidderInfos)
bidderInfos, resultErr := applyBidderInfoConfigOverrides(test.givenConfigBidderInfos, test.givenFsBidderInfos, mockNormalizeBidderName)
assert.NoError(t, resultErr, test.description+":err")
assert.Equal(t, test.expectedBidderInfos, bidderInfos, test.description+":result")
}
}

func TestApplyBidderInfoConfigOverridesInvalid(t *testing.T) {
var testCases = []struct {
description string
givenFsBidderInfos BidderInfos
givenConfigBidderInfos BidderInfos
expectedError string
expectedBidderInfos BidderInfos
}{
{
description: "Bidder doesn't exists in bidder list",
givenConfigBidderInfos: BidderInfos{"unknown": {Syncer: &Syncer{Key: "override"}}},
expectedError: "error setting configuration for bidder unknown: unknown bidder",
},
{
description: "Bidder doesn't exists in file system",
givenFsBidderInfos: BidderInfos{"unknown": {Endpoint: "original"}},
givenConfigBidderInfos: BidderInfos{"bidderA": {Syncer: &Syncer{Key: "override"}}},
expectedError: "error finding configuration for bidder bidderA: unknown bidder",
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use unique descriptions so we know which test fails from the error message.

}
for _, test := range testCases {
_, err := applyBidderInfoConfigOverrides(test.givenConfigBidderInfos, test.givenFsBidderInfos, mockNormalizeBidderName)
assert.ErrorContains(t, err, test.expectedError, test.description+":err")
}
}

func TestReadFullYamlBidderConfig(t *testing.T) {
bidder := "someBidder"
bidder := "bidderA"
bidderInf := BidderInfo{}
err := yaml.Unmarshal([]byte(fullBidderYAMLConfig), &bidderInf)
actualBidderInfo, err := applyBidderInfoConfigOverrides(BidderInfos{bidder: bidderInf}, BidderInfos{bidder: {Syncer: &Syncer{Supports: []string{"iframe"}}}})
actualBidderInfo, err := applyBidderInfoConfigOverrides(BidderInfos{bidder: bidderInf}, BidderInfos{bidder: {Syncer: &Syncer{Supports: []string{"iframe"}}}}, mockNormalizeBidderName)

assert.NoError(t, err, "Error wasn't expected")

Expand Down
4 changes: 2 additions & 2 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -617,7 +617,7 @@ func (cfg *TimeoutNotification) validate(errs []error) []error {
}

// New uses viper to get our server configurations.
func New(v *viper.Viper, bidderInfos BidderInfos) (*Configuration, error) {
func New(v *viper.Viper, bidderInfos BidderInfos, normalizeBidderName func(string) (openrtb_ext.BidderName, bool)) (*Configuration, error) {
var c Configuration
if err := v.Unmarshal(&c); err != nil {
return nil, fmt.Errorf("viper failed to unmarshal app config: %v", err)
Expand Down Expand Up @@ -700,7 +700,7 @@ func New(v *viper.Viper, bidderInfos BidderInfos) (*Configuration, error) {
// Migrate combo stored request config to separate stored_reqs and amp stored_reqs configs.
resolvedStoredRequestsConfig(&c)

mergedBidderInfos, err := applyBidderInfoConfigOverrides(c.BidderInfos, bidderInfos)
mergedBidderInfos, err := applyBidderInfoConfigOverrides(c.BidderInfos, bidderInfos, normalizeBidderName)
if err != nil {
return nil, err
}
Expand Down
10 changes: 5 additions & 5 deletions config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -442,7 +442,7 @@ func TestFullConfig(t *testing.T) {
SetupViper(v, "", bidderInfos)
v.SetConfigType("yaml")
v.ReadConfig(bytes.NewBuffer(fullConfig))
cfg, err := New(v, bidderInfos)
cfg, err := New(v, bidderInfos, mockNormalizeBidderName)
assert.NoError(t, err, "Setting up config should work but it doesn't")
cmpStrings(t, "cookie domain", cfg.HostCookie.Domain, "cookies.prebid.org")
cmpStrings(t, "cookie name", cfg.HostCookie.CookieName, "userid")
Expand Down Expand Up @@ -691,7 +691,7 @@ func TestMigrateConfig(t *testing.T) {
v.SetConfigType("yaml")
v.ReadConfig(bytes.NewBuffer(oldStoredRequestsConfig))
migrateConfig(v)
cfg, err := New(v, bidderInfos)
cfg, err := New(v, bidderInfos, mockNormalizeBidderName)
assert.NoError(t, err, "Setting up config should work but it doesn't")
cmpBools(t, "stored_requests.filesystem.enabled", true, cfg.StoredRequests.Files.Enabled)
cmpStrings(t, "stored_requests.filesystem.path", "/somepath", cfg.StoredRequests.Files.Path)
Expand Down Expand Up @@ -1753,7 +1753,7 @@ func TestNewCallsRequestValidation(t *testing.T) {
`request_validation:
ipv4_private_networks: ` + test.privateIPNetworks)))

result, resultErr := New(v, bidderInfos)
result, resultErr := New(v, bidderInfos, mockNormalizeBidderName)

if test.expectedError == "" {
assert.NoError(t, resultErr, test.description+":err")
Expand Down Expand Up @@ -1788,7 +1788,7 @@ func newDefaultConfig(t *testing.T) (*Configuration, *viper.Viper) {
SetupViper(v, "", bidderInfos)
v.Set("gdpr.default_value", "0")
v.SetConfigType("yaml")
cfg, err := New(v, bidderInfos)
cfg, err := New(v, bidderInfos, mockNormalizeBidderName)
assert.NoError(t, err, "Setting up config should work but it doesn't")
return cfg, v
}
Expand Down Expand Up @@ -1858,7 +1858,7 @@ func TestSpecialFeature1VendorExceptionMap(t *testing.T) {
SetupViper(v, "", bidderInfos)
v.SetConfigType("yaml")
v.ReadConfig(bytes.NewBuffer(config))
cfg, err := New(v, bidderInfos)
cfg, err := New(v, bidderInfos, mockNormalizeBidderName)
assert.NoError(t, err, "Setting up config error", tt.description)

assert.Equal(t, tt.wantVendorExceptions, cfg.GDPR.TCF2.SpecialFeature1.VendorExceptions, tt.description)
Expand Down
Loading