Pubmatic: Pass imp.ext.data to SSP

[pm-isha-bharti]

Pubmatic adapter: Read imp.ext.data and pass all the key-values to SSP at imp.ext.key_val

[hhhjort]

To avoid needing the extra loop done by calling isStringArray(), you could have an else section of this if return nil if v is not of type string. It may be a minor performance enhancement.

It is also a little odd that isStringArray() takes an argument of []interface{}, while getStringArray() takes interface{}, but both are passed the same value. The case statement above insures the value is of type []interface{} which happens to also be a valid interface{}, so it works, but stuffs the typedValue into another interface abstraction which you then have to get out of.

[pm-isha-bharti]

Removed isStringArray and reusing getStringArray for the same.
Changed argument type of getStringArray from interface{} to []interface{}

[hhhjort]

LGTM

[guscarreon]

Looks pretty good. Left a couple of nitpicks

[guscarreon]

I realize that an error here is highly unlikely but, can we still include a check?

554         switch typedValue := val.(type) {
555         case string:
556 -           fmt.Fprintf(&dctr, "%s=%s", key, strings.TrimSpace(typedValue))
    +           if _, err := fmt.Fprintf(&dctr, "%s=%s", key, strings.TrimSpace(typedValue)); err != nil {
    +               continue
    +           }
557
558         case float64, bool:
559 -           fmt.Fprintf(&dctr, "%s=%v", key, typedValue)
    +           if _, err := fmt.Fprintf(&dctr, "%s=%v", key, typedValue); err != nil {
    +               continue
    +           }
560
561         case []interface{}:
562             if valStrArr := getStringArray(typedValue); valStrArr != nil && len(valStrArr) > 0 {
563                 valStr := strings.Join(valStrArr[:], ",")
564 -               fmt.Fprintf(&dctr, "%s=%s", key, valStr)
    +               if _, err := fmt.Fprintf(&dctr, "%s=%s", key, valStr); err != nil {
    +                   continue
    +               }
565             }
566         }
adapters/pubmatic/pubmatic.go

[guscarreon]

Specially in the case of float, would formatting with their specific formatters be desirable here?

554         switch typedValue := val.(type) {
555         case string:
556             fmt.Fprintf(&dctr, "%s=%s", key, strings.TrimSpace(typedValue))
557
558 -       case float64, bool:
559 -           fmt.Fprintf(&dctr, "%s=%v", key, typedValue)
    +       case float64:
    +           fmt.Fprintf(&dctr, "%s=%4.2f", key, typedValue)
    +
    +       case bool:
    +           fmt.Fprintf(&dctr, "%s=%t", key, typedValue)
560 
561         case []interface{}:
562             if valStrArr := getStringArray(typedValue); valStrArr != nil && len(valStrArr) > 0 {
563                 valStr := strings.Join(valStrArr[:], ",")
564                 fmt.Fprintf(&dctr, "%s=%s", key, valStr)
565             }
566         }
adapters/pubmatic/pubmatic.go

[pm-isha-bharti]

Hi @guscarreon Adding a specific format(%4.2f) for float, converts integer values to float too. For instance, if the input string is age=20, the output would be age=20.00 which is not what we desire here.
A single case statement for float64, bool handles all the values of types float, integer, and bool

case float64, bool:
			fmt.Fprintf(&dctr, "%s=%v", key, typedValue)

[guscarreon]

Ohh I see. Thanks for clarifying.

[guscarreon]

len() covers both nil and non-nil cases.

561         case []interface{}:
562 -           if valStrArr := getStringArray(typedValue); valStrArr != nil && len(valStrArr) > 0 {
    +           if valStrArr := getStringArray(typedValue); len(valStrArr) > 0 {
563                 valStr := strings.Join(valStrArr[:], ",")
564                 fmt.Fprintf(&dctr, "%s=%s", key, valStr)
565             }
adapters/pubmatic/pubmatic.go

[pm-isha-bharti]

Done

[guscarreon]

Nitpick: could we use the backward tick so we don't have to escape the double quotes?

688         {
689             name:  "Valid JSON",
690 -           input: json.RawMessage("{\"buyid\":\"testBuyId\"}"),
    +           input: json.RawMessage(`{"buyid":"testBuyId"}`),
691             output: map[string]interface{}{
692                 "buyid": "testBuyId",
693             },
694         },
695         {
696             name:   "Invalid JSON",
697 -           input:  json.RawMessage("{\"buyid\":}"),
    +           input:  json.RawMessage(`{"buyid":}`),
698             output: nil,
699         },
adapters/pubmatic/pubmatic_test.go

[pm-isha-bharti]

Done

[guscarreon]

Small optimization: instead of returning a string, we could simply read the len of the strings.Builder buffer.

546         //separate key-val pairs in dctr string by pipe(|)
547 -       if dctr.String() != "" {
    +       if dctr.Len() != 0 {
548             dctr.WriteString("|")
549         }
adapters/pubmatic/pubmatic.go

[pm-isha-bharti]

Done

[guscarreon]

Nitpick: Can we use a fake url here? Something that at first sight is easily understood to be a URL for testing purposes.

[pm-isha-bharti]

This is the expected SSP URL. I can see that rest of the adapters also use original URL.
Do I need to change

prebid-server/adapters/pubmatic/pubmatic_test.go

Line 20 in 2eb2dee

Endpoint: "https://hbopenbid.pubmatic.com/translator?source=prebid-server"}, config.Server{ExternalUrl: "http://hosturl.com", GvlID: 1, DataCenter: "2"})

for this, this will require changes in all the existing JSON files?

[guscarreon]

It's ok if we leave it as is

[bsardo]

@pm-isha-bharti just a friendly reminder. @guscarreon left a few comments to address when you get some time.

[bsardo]

Hi @pm-isha-bharti, can you merge with master when you get a chance? Your code is failing the go 1.19 validation checks due to go fmt changes that were introduced with the recent upgrade to go 1.19 on master.

[guscarreon]

@pm-isha-bharti our pipeline tests seem to still be failing. We recently updated to Go 1.19 and gofmt needed to be updated on my local computer as well. I think that might be the issue with the fourth test:

Run ./validate.sh --nofmt --cov --race 10
gofmt needs to be run, 1 files have issues.  Below is a list of files to review:
adapters/pubmatic/pubmatic.go
Error: Process completed with exit code 1.

This will probably will go away if you update gofmt and run it on adapters/pubmatic/pubmatic.go

[guscarreon]

LGTM