-
Notifications
You must be signed in to change notification settings - Fork 748
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
New Adapter: Connatix #3916
New Adapter: Connatix #3916
Changes from 18 commits
395401d
714bfe7
51f4e61
215430b
185aeaa
b377051
5ed6570
8c03223
b61d5ff
1c5bcac
2162440
5144c53
08242be
55ec262
b57a08a
28ed966
b5f955d
4c7a620
6b50d55
15bdaa2
c29ca38
9d79042
a69ba3c
ce01457
4be32f5
66d781c
8d50d87
967e769
586d31c
181c4f9
5e74e5c
b04d466
38d26e9
b4b1be2
8fcb657
a8891b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,269 @@ | ||
package connatix | ||
|
||
import ( | ||
"encoding/json" | ||
"fmt" | ||
"net/http" | ||
"net/url" | ||
"strings" | ||
|
||
"github.com/buger/jsonparser" | ||
"github.com/prebid/openrtb/v20/openrtb2" | ||
"github.com/prebid/prebid-server/v2/adapters" | ||
"github.com/prebid/prebid-server/v2/config" | ||
"github.com/prebid/prebid-server/v2/errortypes" | ||
"github.com/prebid/prebid-server/v2/openrtb_ext" | ||
) | ||
|
||
const ( | ||
maxImpsPerReq = 1 | ||
) | ||
|
||
// Builder builds a new instance of the Connatix adapter for the given bidder with the given config. | ||
func Builder(bidderName openrtb_ext.BidderName, config config.Adapter, server config.Server) (adapters.Bidder, error) { | ||
uri, err := url.Parse(config.Endpoint) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
bidder := &adapter{ | ||
uri: *uri, | ||
} | ||
return bidder, nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see any endpoint params. You can simplify this to:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
} | ||
|
||
func (a *adapter) MakeRequests(request *openrtb2.BidRequest, reqInfo *adapters.ExtraRequestInfo) ([]*adapters.RequestData, []error) { | ||
if request.Site == nil && request.App == nil { | ||
return nil, []error{&errortypes.BadInput{ | ||
Message: "Either site or app object is required", | ||
}} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can delete this check. Core will only call your adapter if site or app is present in the request because you have declared support for both site and app in your YAML file and have not declared support for the third option, DOOH. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. handled in 8d50d87 |
||
|
||
if request.Device != nil && request.Device.IP == "" && request.Device.IPv6 == "" { | ||
return nil, []error{&errortypes.BadInput{ | ||
Message: "Device IP is required", | ||
}} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you also want to report this error if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. indeed. thank you! fixed |
||
} | ||
|
||
// connatix adapter expects imp.displaymanagerver to be populated in openrtb2 request | ||
// but some SDKs will put it in imp.ext.prebid instead | ||
displayManagerVer := buildDisplayManageVer(request) | ||
|
||
var errs []error | ||
|
||
validImps := []openrtb2.Imp{} | ||
for i := 0; i < len(request.Imp); i++ { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: I suggest using a range here: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. handled in 967e769 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest adding a multi imp test, perhaps where one is of type banner and another is of type video. You could also include a third imp with an error so you can be sure two imp requests are generated and one error is returned. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. handled in 181c4f9 |
||
impExtIncoming, err := validateAndBuildImpExt(&request.Imp[i]) | ||
if err != nil { | ||
errs = append(errs, err) | ||
continue | ||
} | ||
|
||
if err := buildRequestImp(&request.Imp[i], impExtIncoming, displayManagerVer, reqInfo); err != nil { | ||
errs = append(errs, err) | ||
continue | ||
} | ||
|
||
validImps = append(validImps, request.Imp[i]) | ||
} | ||
request.Imp = validImps | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious if this assignment is necessary. You could simplify a bit by just passing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. handled in 967e769 |
||
|
||
// If all the requests were malformed, don't bother making a server call with no impressions. | ||
if len(request.Imp) == 0 { | ||
return nil, errs | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You might be able to delete this. I think your There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. handled in 967e769 |
||
|
||
// Divide imps to several requests | ||
requests, errors := splitRequests(request.Imp, request, a.uri.String()) | ||
return requests, append(errs, errors...) | ||
} | ||
|
||
func (a *adapter) MakeBids(internalRequest *openrtb2.BidRequest, externalRequest *adapters.RequestData, response *adapters.ResponseData) (*adapters.BidderResponse, []error) { | ||
if adapters.IsResponseStatusCodeNoContent(response) { | ||
return nil, nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a supplemental JSON test for a 204 response. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. handled in 181c4f9 |
||
} | ||
|
||
if err := adapters.CheckResponseStatusCodeForErrors(response); err != nil { | ||
return nil, []error{err} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a supplemental JSON test for a 400 response There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. handled in 181c4f9 |
||
} | ||
|
||
var connatixResponse openrtb2.BidResponse | ||
if err := json.Unmarshal(response.Body, &connatixResponse); err != nil { | ||
return nil, []error{err} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a supplemental JSON test for an invalid response. You should be able to do this easily by having your response include a known field that maps to the struct that is of the wrong type. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. handled in 181c4f9 |
||
} | ||
|
||
var errs []error | ||
bidderResponse := adapters.NewBidderResponseWithBidsCapacity(1) | ||
for _, sb := range connatixResponse.SeatBid { | ||
for i := range sb.Bid { | ||
bid := sb.Bid[i] | ||
var bidExt bidExt | ||
var bidType openrtb_ext.BidType | ||
|
||
if err := json.Unmarshal(bid.Ext, &bidExt); err != nil { | ||
bidType = openrtb_ext.BidTypeBanner | ||
} else { | ||
bidType = getBidType(bidExt) | ||
} | ||
|
||
bidderResponse.Bids = append(bidderResponse.Bids, &adapters.TypedBid{ | ||
Bid: &bid, | ||
BidType: bidType, | ||
}) | ||
} | ||
} | ||
|
||
bidderResponse.Currency = "USD" | ||
|
||
return bidderResponse, errs | ||
} | ||
|
||
func validateAndBuildImpExt(imp *openrtb2.Imp) (impExtIncoming, error) { | ||
var ext impExtIncoming | ||
if err := json.Unmarshal(imp.Ext, &ext); err != nil { | ||
return impExtIncoming{}, err | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a supplemental JSON test where the imp.ext cannot be unmarshaled. You should be able to do this by providing an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. handled in 181c4f9 |
||
} | ||
|
||
if err := validateConnatixExt(&ext.Bidder); err != nil { | ||
return impExtIncoming{}, err | ||
} | ||
|
||
return ext, nil | ||
} | ||
|
||
func validateConnatixExt(cnxExt *openrtb_ext.ExtImpConnatix) error { | ||
if cnxExt.PlacementId == "" { | ||
return &errortypes.BadInput{ | ||
Message: "Placement id is required", | ||
} | ||
} | ||
return nil | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can delete the validation logic here. There's no need to ensure your placement ID is not empty because you have added a rule to your bidder-params JSON file that indicates it has a minimum length of 1. If a request came in with a placement ID of empty string, PBS Core would report this error to the publisher and skip calling your adapter. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. handled in 8d50d87 |
||
|
||
func splitRequests(imps []openrtb2.Imp, request *openrtb2.BidRequest, uri string) ([]*adapters.RequestData, []error) { | ||
var errs []error | ||
// Initial capacity for future array of requests, memory optimization. | ||
// Let's say there are 35 impressions and limit impressions per request equals to 10. | ||
// In this case we need to create 4 requests with 10, 10, 10 and 5 impressions. | ||
// With this formula initial capacity=(35+10-1)/10 = 4 | ||
initialCapacity := (len(imps) + maxImpsPerReq - 1) / maxImpsPerReq | ||
resArr := make([]*adapters.RequestData, 0, initialCapacity) | ||
startInd := 0 | ||
impsLeft := len(imps) > 0 | ||
|
||
headers := http.Header{} | ||
headers.Add("Content-Type", "application/json") | ||
headers.Add("Accept", "application/json") | ||
|
||
if len(request.Device.UA) > 0 { | ||
headers.Add("User-Agent", request.Device.UA) | ||
} | ||
|
||
if len(request.Device.IPv6) > 0 { | ||
headers.Add("X-Forwarded-For", request.Device.IPv6) | ||
} | ||
|
||
if len(request.Device.IP) > 0 { | ||
headers.Add("X-Forwarded-For", request.Device.IP) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. handled in 66d781c |
||
|
||
for impsLeft { | ||
endInd := startInd + maxImpsPerReq | ||
if endInd >= len(imps) { | ||
endInd = len(imps) | ||
impsLeft = false | ||
} | ||
impsForReq := imps[startInd:endInd] | ||
request.Imp = impsForReq | ||
|
||
reqJSON, err := json.Marshal(request) | ||
if err != nil { | ||
errs = append(errs, err) | ||
return nil, errs | ||
} | ||
|
||
resArr = append(resArr, &adapters.RequestData{ | ||
Method: "POST", | ||
Uri: uri, | ||
Body: reqJSON, | ||
Headers: headers, | ||
ImpIDs: openrtb_ext.GetImpIDs(request.Imp), | ||
}) | ||
startInd = endInd | ||
} | ||
return resArr, errs | ||
} | ||
|
||
func buildRequestImp(imp *openrtb2.Imp, ext impExtIncoming, displayManagerVer string, reqInfo *adapters.ExtraRequestInfo) error { | ||
if imp.Video == nil && imp.Banner == nil { | ||
return &errortypes.BadInput{ | ||
Message: "Either video or banner object on impression is required", | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can delete this check. Given that you've declared support for banner and video in your yaml file for both site and app, PBS core will skip calling your adapter if it contains impressions that are not one of those formats. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. handled in 8d50d87 |
||
|
||
if imp.Banner != nil { | ||
bannerCopy := *imp.Banner | ||
|
||
if bannerCopy.W == nil && bannerCopy.H == nil && len(bannerCopy.Format) > 0 { | ||
firstFormat := bannerCopy.Format[0] | ||
bannerCopy.W = &(firstFormat.W) | ||
bannerCopy.H = &(firstFormat.H) | ||
} | ||
imp.Banner = &bannerCopy | ||
} | ||
|
||
// Populate imp.displaymanagerver if the SDK failed to do it. | ||
if len(imp.DisplayManagerVer) == 0 && len(displayManagerVer) > 0 { | ||
imp.DisplayManagerVer = displayManagerVer | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a supplemental test where the computed display manager version is copied to the imp. Perhaps this is the same test that adds the additional coverage I'm asking for in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. handled in 181c4f9 |
||
} | ||
|
||
// Check if imp comes with bid floor amount defined in a foreign currency | ||
if imp.BidFloor > 0 && imp.BidFloorCur != "" && !strings.EqualFold(imp.BidFloorCur, "USD") { | ||
// Convert to US dollars | ||
convertedValue, err := reqInfo.ConvertCurrency(imp.BidFloor, imp.BidFloorCur, "USD") | ||
if err != nil { | ||
return err | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a supplemental JSON test where the currency conversion fails. You should be able to force this by including an invalid bid floor currency in the impression. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. handled in 181c4f9 |
||
} | ||
// Update after conversion. All imp elements inside request.Imp are shallow copies | ||
// therefore, their non-pointer values are not shared memory and are safe to modify. | ||
imp.BidFloorCur = "USD" | ||
imp.BidFloor = convertedValue | ||
} | ||
|
||
impExt := impExt{ | ||
Connatix: impExtConnatix{ | ||
PlacementId: ext.Bidder.PlacementId, | ||
}, | ||
} | ||
|
||
var err error | ||
imp.Ext, err = json.Marshal(impExt) | ||
|
||
return err | ||
} | ||
|
||
func buildDisplayManageVer(req *openrtb2.BidRequest) string { | ||
if req.App == nil { | ||
return "" | ||
} | ||
|
||
source, err := jsonparser.GetString(req.App.Ext, openrtb_ext.PrebidExtKey, "source") | ||
if err != nil { | ||
return "" | ||
} | ||
|
||
version, err := jsonparser.GetString(req.App.Ext, openrtb_ext.PrebidExtKey, "version") | ||
if err != nil { | ||
return "" | ||
} | ||
Comment on lines
+223
to
+225
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a supplemental JSON test where reading the version fails. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. handled in 181c4f9 |
||
|
||
return fmt.Sprintf("%s-%s", source, version) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add test coverage for this line where both source and version exist There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. handled in 181c4f9 |
||
} | ||
|
||
func getBidType(ext bidExt) openrtb_ext.BidType { | ||
if ext.Cnx.MediaType == "video" { | ||
return openrtb_ext.BidTypeVideo | ||
} | ||
|
||
return openrtb_ext.BidTypeBanner | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
package connatix | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/prebid/prebid-server/v2/adapters/adapterstest" | ||
"github.com/prebid/prebid-server/v2/config" | ||
"github.com/prebid/prebid-server/v2/openrtb_ext" | ||
) | ||
|
||
func TestJsonSamples(t *testing.T) { | ||
bidder, buildErr := Builder(openrtb_ext.BidderConnatix, config.Adapter{ | ||
Endpoint: "http://example.com"}, config.Server{ExternalUrl: "http://hosturl.com", GvlID: 1, DataCenter: "2"}) | ||
|
||
if buildErr != nil { | ||
t.Fatalf("Builder returned unexpected error %v", buildErr) | ||
} | ||
|
||
adapterstest.RunJSONBidderTest(t, "connatixtest", bidder) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you have a value of 1 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because right now we only support a single impression per request and it would be much simpler and clearer (for new devs too) to update this constraint when we will support multiple impressions per request