Adnuntius Adapter: Set no cookies as a parameter

[mikael-lundin]

This will allow prebid server partners to be able to tell the ad server not to set cookies as a parameter with the ad request.

[guscarreon]

Looks pretty good. Left a couple of optimization nitpicks

[guscarreon]

Given that extDeviceAdnuntius only comes with the NoCookies field:

type extDeviceAdnuntius struct {
	NoCookies bool `json:"noCookies,omitempty"`
}
adapters/adnuntius/adnuntius.go

we may not need to unmarshal at all if noCookies is true. Do you think that condition is worth adding as a small optimization? I guess something similar to:

 98   func makeEndpointUrl(ortbRequest openrtb2.BidRequest, a *adapter, noCookies bool) (string, []error) {
 99       uri, err := url.Parse(a.endpoint)
100       if err != nil {
101           return "", []error{fmt.Errorf("failed to parse Adnuntius endpoint: %v", err)}
102       }
103  
104       gdpr, consent, err := getGDPR(&ortbRequest)
105       if err != nil {
106           return "", []error{fmt.Errorf("failed to parse Adnuntius endpoint: %v", err)}
107       }
108  
109 -     var deviceExt extDeviceAdnuntius
110 -     if ortbRequest.Device != nil && ortbRequest.Device.Ext != nil {
111 -         if err := json.Unmarshal(ortbRequest.Device.Ext, &deviceExt); err != nil {
112 -             return "", []error{fmt.Errorf("failed to parse Adnuntius endpoint: %v", err)}
113 -         }
114 -     }
115  
116       _, offset := a.time.Now().Zone()
117       tzo := -offset / minutesInHour
118  
119       q := uri.Query()
120       if gdpr != "" && consent != "" {
121           q.Set("gdpr", gdpr)
122           q.Set("consentString", consent)
123       }
124  
125 -     if deviceExt.NoCookies || noCookies {
    +     cookiesEnabled := noCookes
    +     if !cookiesEnabled {
    +         var deviceExt extDeviceAdnuntius
    +         if ortbRequest.Device != nil && ortbRequest.Device.Ext != nil {
    +             if err := json.Unmarshal(ortbRequest.Device.Ext, &deviceExt); err != nil {
    +                 return "", []error{fmt.Errorf("failed to parse Adnuntius endpoint: %v", err)}
    +             }
    +         }
    +         cookiesEnabled = deviceExt.NoCookies
    +     }
    +     
    +     if cookiesEnabled {
126           q.Set("noCookies", "true")
127       }
128  
129       q.Set("tzo", fmt.Sprint(tzo))
adapters/adnuntius/adnuntius.go

Thouhgts?

[mikael-lundin]

Generally the idea was that you could set noCookies it in both places, but maybe I should just get rid of the deviceExt part. No one is using it as of now. will change that. Just a general question since I'm pretty new to golang, is unmarshaling crazy expensive or just a bit expensive?

[guscarreon]

Unmarshal library we are currently using is very robust and reliable, but expensive. extDeviceAdnuntius is not a huge struct so I don't think it'd be crazy expensive, but if noCookies is already true, there seems to be no reason to unmarshal deviceExt. I think it doesn't hurt to set noCookies in either place. For backwards compatibility I think it makes sense to leave deviceExt.

I just think that if noCookies is true, the OR in line 125 would evaluate to true regardless of the value of deviceExt.NoCookies. Therefore, I think it makes sense to unmarshal deviceExt only if needed. That is, only if noCookies is false. I leave it up to you.

[guscarreon]

As of this version of the code, noCookies doesn't seem to get modified anywhere else. Can we discard it altogether?

159   func (a *adapter) generateRequests(ortbRequest openrtb2.BidRequest) ([]*adapters.RequestData, []error) {
160       var requestData []*adapters.RequestData
161       networkAdunitMap := make(map[string][]adnAdunit)
162       headers := setHeaders(ortbRequest)
163 -     var noCookies bool = false
164
165       for _, imp := range ortbRequest.Imp {
166           if imp.Banner == nil {
167               return nil, []error{&errortypes.BadInput{
168                   Message: fmt.Sprintf("ignoring imp id=%s, Adnuntius supports only Banner", imp.ID),
169               }}
170           }
*-- 11 lines: 171           var bidderExt adapters.ExtImpBidder--------------------------------------------------------------
182               }}
183           }
184
185 -         if adnuntiusExt.NoCookies == true {
186 -             noCookies = true
187 -         }
188
189           network := defaultNetwork
190           if adnuntiusExt.Network != "" {
*--  8 lines: 191               network = adnuntiusExt.Network---------------------------------------------------------------
199                   Dimensions: getImpSizes(imp),
200               })
201       }
202
203 -     endpoint, err := makeEndpointUrl(ortbRequest, a, noCookies)
    +     endpoint, err := makeEndpointUrl(ortbRequest, a, adnuntiusExt.NoCookies)
204       if err != nil {
205           return nil, []error{&errortypes.BadInput{
206               Message: fmt.Sprintf("failed to parse URL: %s", err),
207           }}
208       }
adapters/adnuntius/adnuntius.go

[mikael-lundin]

The idea here is that the noCookies is set as per imp. that means that there might be 3 imps that where only one have the noCookies option. that means that the entire request to the endpoint would need the parameter set in the url. That's kind of why I opted to set a variable before the for loop so that if there's one we set it for all.

If that makes sense :D

[guscarreon]

Ohhh I missed it is outside of the for loop. It does make sense, my bad. Yep, let's leaving as is.

[AlexBVolcy]

Looks good, one minor comment

[AlexBVolcy]

I noticed this error case isn't covered by tests, would it be possible to update your tests for this fail to parse URL case?

[mikael-lundin]

Sure, will add that.

[mikael-lundin]

I couldn't find a way to trigger this, so I removed the check.

[AlexBVolcy]

I see, I think we should still be handling the error regardless. So even if you can't cover it for a test, we still want to have the error check if you can re-add it please.

[guscarreon]

Looks good. I believe the error check mentioned below should be added back. Other than that I agree with these changes.

[guscarreon]

Why was the error check removed? Can we add it again?

[mikael-lundin]

Adding it back in! :)

[guscarreon]

LGTM