Adnuntius: Add multi-format and native support #4223
Conversation
Code coverage summaryNote:
adnuntiusRefer here for heat map coverage report |
bsardo
changed the title
|
@pm-isha-bharti can you please review? |
adapters/adnuntius/adnuntius.go
Outdated
| adUnit.AdType = "NATIVE" | ||
| nativeRequest := json.RawMessage{} | ||
|
|
||
| if err := json.Unmarshal([]byte(imp.Native.Request), &nativeRequest); err != nil { |
There was a problem hiding this comment.
Please use jsonutil.Unmarshal instead of json.Unmarshal
| var extUser openrtb_ext.ExtUser | ||
| if ortbRequest.User != nil && ortbRequest.User.Ext != nil { | ||
| if err := jsonutil.Unmarshal(ortbRequest.User.Ext, &extUser); err != nil { | ||
| return nil, []error{fmt.Errorf("failed to parse Ext User: %v", err)} |
There was a problem hiding this comment.
Please add a supplemental JSON test case where user.ext is invalid(if not already added).
There was a problem hiding this comment.
@pm-isha-bharti thank you for your comments!
We don't ask to cover unmarshal errors. JSON test should have a valid format to be loaded to the test suit and if JSON itself is invalid it will never reach this point.
We need these error handling for real requests, because user.ext may be untouched and unverified until it reaches the bidder code.
Hope it makes sense :)
| }, | ||
| } | ||
|
|
||
| returnExt, err := json.Marshal(ext) |
There was a problem hiding this comment.
Use jsonutil.Marshal instead of json.Marshal
adapters/adnuntius/adnuntius.go
Outdated
|
|
||
| adBid, err := generateAdResponse(ad, imp, html, bidType, request) | ||
| if err != nil { | ||
| return nil, []error{&errortypes.BadInput{ |
There was a problem hiding this comment.
Here we are ignoring the original error err and returning a new error under the BadInput error type. Is this the intended behaviour?
Additionally, in the generateAdResponse method, many errors are caused by issues in the bidder response, such as an invalid ad.creativeWidth. It seems appropriate for these errors to fall under the BadServerResponse error type. Should we align them accordingly?
There was a problem hiding this comment.
Also, please add supplemental JSON test cases to cover these scenario
There was a problem hiding this comment.
I found it hard to create a test for this since all error scenarios makes it throw an error earlier in the code that's already tested for.
adapters/adnuntius/adnuntius.go
Outdated
|
|
||
| ad := adunit.Ads[0] | ||
| html := adunit.Html | ||
| var bidType openrtb2.MarkupType = 1 |
There was a problem hiding this comment.
Can we rename bidType to mtype to avoid confusion as type defined here is openrtb2.MarkupType?
Alternatively, you can use variable bidType of type openrtb_ext.BidType and later on there is no need to have convertMarkupTypeToBidType method and bidType can be directly used to assign BidType in the bidResponse.Bids on Line no 228.
BidType: bidtype
instead of
BidType: convertMarkupTypeToBidType(bidType),
adapters/adnuntius/adnuntius.go
Outdated
|
|
||
| if native != nil { | ||
| html = string(native) | ||
| bidType = 4 |
There was a problem hiding this comment.
Uss constants here instead of numbers
adapters/adnuntius/adnuntius.go
Outdated
| }) | ||
|
|
||
| for _, deal := range adunit.Deals { | ||
| bidType = 1 |
There was a problem hiding this comment.
Use existing constants instead of numbers
adapters/adnuntius/adnuntius.go
Outdated
| bidType = 1 | ||
| dealBid, err := generateAdResponse(deal, imp, deal.Html, bidType, request) | ||
| if err != nil { | ||
| return nil, []error{&errortypes.BadInput{ |
There was a problem hiding this comment.
Same comment as on Line no. 221
|
|
||
| func makeEndpointUrl(ortbRequest openrtb2.BidRequest, a *adapter, noCookies bool) (string, []error) { | ||
| uri, err := url.Parse(a.endpoint) | ||
| endpointUrl := a.endpoint |
There was a problem hiding this comment.
The url.Parse function is called but the returned uri is not used correctly. The endpointUrl should be constructed using uri.String() instead of a.endpoint.
|
|
||
| gdpr, consent, err := getGDPR(&ortbRequest) | ||
| if err != nil { | ||
| return "", []error{fmt.Errorf("failed to parse Adnuntius endpoint: %v", err)} |
There was a problem hiding this comment.
The error message in the fmt.Errorf call is incorrect. It should indicate that the error occurred while parsing GDPR information, not the Adnuntius endpoint.
| q.Set("noCookies", "true") | ||
| } | ||
|
|
||
| q.Set("tzo", fmt.Sprint(tzo)) |
There was a problem hiding this comment.
The fmt.Sprint function is used to convert tzo to a string. It would be more efficient to use strconv.Itoa for this purpose.
q.Set("tzo", strconv.Itoa(tzo))
|
|
||
| q := uri.Query() | ||
| if gdpr != "" { | ||
| endpointUrl = a.extraInfo |
There was a problem hiding this comment.
Can we validate a.extraInfo as a valid URL before assigning it to endpointUrl.
extraInfoURI, err := url.Parse(a.extraInfo)
if err != nil {
return "", []error{fmt.Errorf("invalid extraInfo URL: %v", err)}
}
endpointUrl = extraInfoURI.String()
| q.Set("tzo", fmt.Sprint(tzo)) | ||
| q.Set("format", "prebidServer") | ||
|
|
||
| url := endpointUrl + "?" + q.Encode() |
There was a problem hiding this comment.
Instead of concatenation use
// Set the query params to the URI
uri.RawQuery = q.Encode()
// Return the correctly formatted URL
return uri.String(), nil
There was a problem hiding this comment.
Here is the modified version:
func makeEndpointUrl(ortbRequest openrtb2.BidRequest, a *adapter, noCookies bool) (string, []error) {
uri, err := url.Parse(a.endpoint)
- endpointUrl := a.endpoint
if err != nil {
return "", []error{fmt.Errorf("failed to parse Adnuntius endpoint: %v", err)}
}
@@ -40,6 +39,15 @@ func makeEndpointUrl(ortbRequest openrtb2.BidRequest, a *adapter, noCookies bool
return "", []error{fmt.Errorf("failed to parse Adnuntius endpoint: %v", err)}
}
+ if gdpr != "" {
+ extraInfoURI, err := url.Parse(a.extraInfo)
+ if err != nil {
+ return "", []error{fmt.Errorf("invalid extraInfo URL: %v", err)}
+ }
+ uri = extraInfoURI
+ }
+
if !noCookies {
var deviceExt extDeviceAdnuntius
if ortbRequest.Device != nil && ortbRequest.Device.Ext != nil {
@@ -58,7 +66,6 @@ func makeEndpointUrl(ortbRequest openrtb2.BidRequest, a *adapter, noCookies bool
q := uri.Query()
if gdpr != "" {
- endpointUrl = a.extraInfo
q.Set("gdpr", gdpr)
}
@@ -73,8 +80,11 @@ func makeEndpointUrl(ortbRequest openrtb2.BidRequest, a *adapter, noCookies bool
q.Set("tzo", fmt.Sprint(tzo))
q.Set("format", "prebidServer")
- url := endpointUrl + "?" + q.Encode()
- return url, nil
+ // Set the query params to the URI
+ uri.RawQuery = q.Encode()
+
+ // Return the correctly formatted URL
+ return uri.String(), nil
}
| gdpr := "" | ||
| var extRegs openrtb_ext.ExtRegs | ||
| if request.Regs != nil && request.Regs.Ext != nil { | ||
| if err := jsonutil.Unmarshal(request.Regs.Ext, &extRegs); err != nil { |
There was a problem hiding this comment.
Can we use jsonparser.GetString instead of unmarshaling regs.ext ?
| consent := "" | ||
| if request.User != nil && request.User.Ext != nil { | ||
| var extUser openrtb_ext.ExtUser | ||
| if err := jsonutil.Unmarshal(request.User.Ext, &extUser); err != nil { |
There was a problem hiding this comment.
Can we use jsonparser.GetString instead of user.ext unmarshal ?
consent, err := jsonparser.GetString(request.User.Ext, "consent")
There was a problem hiding this comment.
I really tried to make this work, but the problem is that if there's no consent added to the request it will throw an error if I use the GetString, making the adapter return a no-bid. And what I have now with the Unmarshal works. Can you provide an example of what it might look like?
|
Oh boy, I will start looking into all of this. :) |
|
I've changed most of the things except for the ones I've commented on above. |
Code coverage summaryNote:
adnuntiusRefer here for heat map coverage report |
| type RequestExt struct { | ||
| Bidder adnAdunit `json:"bidder"` | ||
| } | ||
|
|
||
| const defaultNetwork = "default" | ||
| const defaultSite = "unknown" | ||
| const minutesInHour = 60 |
There was a problem hiding this comment.
I dont see where You use this const in adnuntius.go. Is it necessery?
There was a problem hiding this comment.
adnuntius_utils.go, line 64: tzo := -offset / minutesInHour
Will it make more sense if this constant will be moved to adnuntius_utils.go?
There was a problem hiding this comment.
@VeronikaSolovei9 I'm not sure I 100% follow you here. :)
| /* | ||
| 1 = Banner | ||
| 2 = Video | ||
| 3 = Audio | ||
| 4 = Native | ||
| */ | ||
| func convertMarkupTypeToBidType(markupType openrtb2.MarkupType) openrtb_ext.BidType { | ||
| switch markupType { | ||
| case openrtb2.MarkupBanner: | ||
| return openrtb_ext.BidTypeBanner | ||
| case openrtb2.MarkupVideo: | ||
| return openrtb_ext.BidTypeVideo | ||
| case openrtb2.MarkupAudio: | ||
| return openrtb_ext.BidTypeAudio | ||
| case openrtb2.MarkupNative: | ||
| return openrtb_ext.BidTypeNative | ||
| } | ||
| return openrtb_ext.BidTypeBanner | ||
| } |
There was a problem hiding this comment.
In yaml file You use Banner and Native. Audio and Video are unnecessery.
adapters/adnuntius/adnuntius.go
Outdated
| func (a *adapter) MakeRequests(request *openrtb2.BidRequest, reqInfo *adapters.ExtraRequestInfo) ([]*adapters.RequestData, []error) { | ||
| return a.generateRequests(*request) | ||
| } |
There was a problem hiding this comment.
Put this func before generateRequests
adapters/adnuntius/adnuntius.go
Outdated
| func (a *adapter) MakeBids(request *openrtb2.BidRequest, externalRequest *adapters.RequestData, response *adapters.ResponseData) (*adapters.BidderResponse, []error) { | ||
|
|
||
| if response.StatusCode == http.StatusBadRequest { |
There was a problem hiding this comment.
put this func before generateBidResponse
|
There, should now be addressed. :) |
Code coverage summaryNote:
adnuntiusRefer here for heat map coverage report |
Code coverage summaryNote:
adnuntiusRefer here for heat map coverage report |
__debug_bin2524059436
Outdated
There was a problem hiding this comment.
May you please delete this file?
Code coverage summaryNote:
adnuntiusRefer here for heat map coverage report |
This change is meant to add support for Native format and enable us to add video in the future. We've als made it so that types and helper functions are in their own files.