Add AMX RTB adapter

[nickjacob]

Adding AMX RTB adapter

Server uses standard OpenRTB 2.5. This adapter adds support for overriding the publisher id using imp.ext.amx.tagId, although this is optional. The parameter is named to match the bid param of our prebid.js adapter

Maintainer email: prebid@amxrtb.com

[bretg]

docs PR prebid/prebid.github.io#2437

[SyntaxNode]

Thank you for your contribution. Very glad to see the docs PR already open. :)

[nickjacob]

Sorry for some reason github won't let me resolve the comments or reply here—but thanks for the great CR! I made the changes you noted. Our bids do always have `startdelay` in bid.ext when video, so I used that instead to determine the type. For the GVL—737 is correct thanks I renamed everything to AMX (from Amx). Let me know if there's anything else Thanks!

…

On 26 Oct 2020, at 16:15, Scott Kay wrote: @SyntaxNode commented on this pull request. Thank you for your contribution. Very glad to see the docs PR already open. :) > + "net/http" + "strings" +) + +const vastImpressionFormat = "<Impression><![CDATA[%s]></Impression>" +const vastSearchPoint = "</Impression>" + +// AmxAdapter is the AMX bid adapter +type AmxAdapter struct { + endpoint string +} + +// NewAmxBidder creates an AmxAdapter +func NewAmxBidder(endpoint string) *AmxAdapter { + return &AmxAdapter{endpoint: endpoint} +} FYI: We'll update the `NewAmxBidder` for you in the coming weeks with the #1532 PR. > + +func getPublisherID(imps []openrtb.Imp) *string { + for _, imp := range imps { + paramsSource := (*json.RawMessage)(&imp.Ext) + var params amxExt + if err := json.Unmarshal(*paramsSource, &params); err == nil { + if params.Bidder.PublisherID != "" { + return &params.Bidder.PublisherID + } + } else { + fmt.Printf("unable to decode imp.ext.bidder: %v", err) + } + } + + return nil +} Logic looks good, but the use of pointers is largely unnecessary. It's common in Go to return a second boolean to indiciate if the string was found or not rather than using nil. Something like this would be more idiomatic Go. ``` func getPublisherID(imps []openrtb.Imp) (string, bool) { for _, imp := range imps { var params amxExt if err := json.Unmarshal(imp.Ext, &params); err == nil { if params.Bidder.PublisherID != "" { return params.Bidder.PublisherID, true } } else { fmt.Printf("unable to decode imp.ext.bidder: %v", err) } } return "", false } ``` > + } + + return nil +} + +func ensurePublisherWithID(pub *openrtb.Publisher, publisherID string) *openrtb.Publisher { + if pub == nil { + pub = &openrtb.Publisher{} + } + pub.ID = publisherID + return pub +} + +// MakeRequests creates AMX adapter requests +func (adapter *AmxAdapter) MakeRequests(request *openrtb.BidRequest, req *adapters.ExtraRequestInfo) (reqsBidder []*adapters.RequestData, errs []error) { + publisherID := getPublisherID(request.Imp) With the other suggested change, this would become: ``` if publisherID, ok := getPublisherID(request.Imp); ok { if request.App != nil { request.App.Publisher = ensurePublisherWithID(request.App.Publisher, publisherID) } if request.Site != nil { request.Site.Publisher = ensurePublisherWithID(request.Site.Publisher, publisherID) } } ``` > +} + +type amxParams struct { + PublisherID string `json:"tagId,omitempty"` +} + +func getPublisherID(imps []openrtb.Imp) *string { + for _, imp := range imps { + paramsSource := (*json.RawMessage)(&imp.Ext) + var params amxExt + if err := json.Unmarshal(*paramsSource, &params); err == nil { + if params.Bidder.PublisherID != "" { + return &params.Bidder.PublisherID + } + } else { + fmt.Printf("unable to decode imp.ext.bidder: %v", err) There is no consequence to this print statement, neither in the response or the in the log. The chance of this happening is also likely 0% due to preprocessing by the "core" logic. I recommend removing the print statement since the publisher id is optional for your adapter. > + return pub +} + +// MakeRequests creates AMX adapter requests +func (adapter *AmxAdapter) MakeRequests(request *openrtb.BidRequest, req *adapters.ExtraRequestInfo) (reqsBidder []*adapters.RequestData, errs []error) { + publisherID := getPublisherID(request.Imp) + if publisherID != nil { + if request.App != nil { + request.App.Publisher = ensurePublisherWithID(request.App.Publisher, *publisherID) + } + if request.Site != nil { + request.Site.Publisher = ensurePublisherWithID(request.Site.Publisher, *publisherID) + } + } + + encoded, err := json.Marshal(request) Nitpick: In Go, it's common to remove the line break after the `json.Marshal` call to group it with the error handling code. > + request.Site.Publisher = > ensurePublisherWithID(request.Site.Publisher, *publisherID) + } + } + + encoded, err := json.Marshal(request) + + if err != nil { + errs = append(errs, err) + return nil, errs + } + + headers := http.Header{} + headers.Add("Content-Type", "application/json;charset=utf-8") + reqBidder := &adapters.RequestData{ + Method: "POST", + Uri: adapter.endpoint + "?v=pbs1.0", Please consider adding this query parameter safely using Go's url methods within the `NewAmxBidder` method. > +// MakeBids will parse the bids from the AMX server +func (adapter *AmxAdapter) MakeBids(request *openrtb.BidRequest, externalRequest *adapters.RequestData, response *adapters.ResponseData) (*adapters.BidderResponse, []error) { + var errs []error + + if http.StatusNoContent == response.StatusCode { + return nil, nil + } + + if http.StatusBadRequest == response.StatusCode { + return nil, []error{&errortypes.BadInput{ + Message: fmt.Sprint("Invalid request: 400"), + }} + } + + if http.StatusOK != response.StatusCode { + return nil, []error{&errortypes.BadInput{ Since this error would likely mean the server responded back with a bad response, it's common to use `BadServerResponse` here instead. > + +func getMediaTypeForImp(impID string, imps []openrtb.Imp) (openrtb_ext.BidType, error) { + mediaType := openrtb_ext.BidTypeBanner + for _, imp := range imps { + if imp.ID == impID { + if imp.Banner == nil && imp.Video != nil { + mediaType = openrtb_ext.BidTypeVideo + } + return mediaType, nil + } + } + + return "", &errortypes.BadInput{ + Message: fmt.Sprintf("No impression for: %s", impID), + } +} Does your bid response include the bid type? Although the code you have here is common in the project, there's been issues reported (see #862) that this approach isn't always reliable for multiformat impressions. Is there enough information to detect the type based on the bid response? > @@ -0,0 +1,13 @@ +package amx + +import ( + "text/template" + + "github.com/prebid/prebid-server/adapters" + "github.com/prebid/prebid-server/usersync" +) + +// NewAmxSyncer produces an AMX RTB usersyncer +func NewAmxSyncer(temp *template.Template) usersync.Usersyncer { + return adapters.NewSyncer("amx", 737, temp, adapters.SyncTypeRedirect) GVL ID is registered to `Monet Engine Inc`. Is that correct for AMX? > +) + +const ( + testSyncEndpoint = "http://pbs.amxrtb.com/cchain/0" +) + +func TestDmxSyncer(t *testing.T) { + temp := template.Must(template.New("sync-template").Parse(testSyncEndpoint)) + syncer := NewAmxSyncer(temp) + syncInfo, err := syncer.GetUsersyncInfo(privacy.Policies{}) + assert.NoError(t, err) + assert.Equal(t, "redirect", syncInfo.Type) + assert.Equal(t, testSyncEndpoint, syncInfo.URL) + assert.Equal(t, false, syncInfo.SupportCORS) + assert.EqualValues(t, 737, syncer.GDPRVendorID()) +} Please follow the format form the other syncer tests. > @@ -99,6 +100,7 @@ func NewSyncerMap(cfg *config.Configuration) > map[openrtb_ext.BidderName]usersync insertIntoMap(cfg, syncers, openrtb_ext.BidderAdtelligent, adtelligent.NewAdtelligentSyncer) insertIntoMap(cfg, syncers, openrtb_ext.BidderAdvangelists, advangelists.NewAdvangelistsSyncer) insertIntoMap(cfg, syncers, openrtb_ext.BidderAJA, aja.NewAJASyncer) + insertIntoMap(cfg, syncers, openrtb_ext.BidderAmx, amx.NewAmxSyncer) Per Go naming convention, this should be `BidderAMX`, `NewAMXSyncer`, etc... > +// AmxAdapter is the AMX bid adapter +type AmxAdapter struct { + endpoint string +} + +// NewAmxBidder creates an AmxAdapter +func NewAmxBidder(endpoint string) *AmxAdapter { + return &AmxAdapter{endpoint: endpoint} +} + +type amxExt struct { + Bidder amxParams `json:"bidder"` +} + +type amxParams struct { + PublisherID string `json:"tagId,omitempty"` Please move `amxParams` to the `openrtb_ext` package in the file openrtb_ext/imp_amx.go with the name `ExtImpAmx` to match our standard convention. -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #1549 (review)

[SyntaxNode]

Thank you for the quick update from our feedback. I took a closer look into the tests and found a few items to address.

[SyntaxNode]

This will slightly improve performance as the new string is concacted only once an app startup instead of during each request.

[nickjacob]

Thanks @SyntaxNode -- I made the requested CR changes; refactored to use assert which did really improve the tests, adding some more test coverage and "supplemental" JSON tests, expectedBidResponse & expectedMakeBidsErrors in JSON tests

[nickjacob]

Thanks @mansinahar -- I made those changes & rebased against master

[mansinahar]

Please add a params_test.go file as well to test the bidder params. For reference you can check this one for 33across: https://github.com/prebid/prebid-server/blob/master/adapters/33across/params_test.go