Changed FS bidder names to mixed case

[VeronikaSolovei9]

No description provided.

[SyntaxNode]

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.

[SyntaxNode]

Nitpick: Please rename to either StubInfoReader or FakeInfoReader (technically, this is a stub). Mock implies we can define expectations on the object, control the object responses, and assert expectations later.

[SyntaxNode]

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

[guscarreon]

Looks pretty good. I left a couple of questions but feel like approving either way.

[guscarreon]

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

[SyntaxNode]

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.

[guscarreon]

Do we need to pass openrtb_ext.NormalizeBidderName from main.go all the way down to config/bidderinfo.go's applyBidderInfoConfigOverrides(...) function? I see the need for bidderInfos to be passed from main.go and also the convenience of passing a mock function to applyBidderInfoConfigOverrides(...) for testing purposes but, I'm wondering if maybe we can simplify config.New(...) by not adding that third paramenter:

619   // New uses viper to get our server configurations.
620 - 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) {
621       var c Configuration
622       if err := v.Unmarshal(&c); err != nil {
623   *-- 78 lines: return nil, fmt.Errorf("viper failed to unmarshal app config: %v", err)------------------------------------
701       resolvedStoredRequestsConfig(&c)
702   
703 -     mergedBidderInfos, err := applyBidderInfoConfigOverrides(c.BidderInfos, bidderInfos, normalizeBidderName)
    +     mergedBidderInfos, err := applyBidderInfoConfigOverrides(c.BidderInfos, bidderInfos, openrtb_ext.NormalizeBidderName)
704       if err != nil {
705           return nil, err
706       }
config/config.go

Therefore we could rollback to:

60   func loadConfig(bidderInfos config.BidderInfos) (*config.Configuration, error) {
61       v := viper.New()
62       config.SetupViper(v, configFileName, bidderInfos)
63 -     return config.New(v, bidderInfos, openrtb_ext.NormalizeBidderName)
   +     return config.New(v, bidderInfos)
64   }
main.go

Unless passing it from main.go makes something happen behind the scenes that I'm missing.

[SyntaxNode]

It is necessary for New to accept the normalize function so unit tests are free to use fake bidder names instead of being forced to use real ones. The bidder name flow will continue to evolve and in the near future the normalize func will likely no longer be required. We're focused on restoring functionality to fix the bug and will design a better solution as a follow-up.

[VeronikaSolovei9]

Yes, I was thinking about this too. We also need this parameter in config_test.go -> newDefaultConfig

[guscarreon]

LGTM