Skip to content
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: Insticator #3806

Merged
merged 19 commits into from
Dec 12, 2024
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
328 changes: 328 additions & 0 deletions adapters/insticator/insticator.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,328 @@
package insticator

import (
"fmt"
"net/http"
"strings"

"github.com/prebid/openrtb/v20/openrtb2"
"github.com/prebid/prebid-server/v3/adapters"
"github.com/prebid/prebid-server/v3/config"
"github.com/prebid/prebid-server/v3/errortypes"
"github.com/prebid/prebid-server/v3/openrtb_ext"
"github.com/prebid/prebid-server/v3/util/jsonutil"
"github.com/prebid/prebid-server/v3/util/mathutil"
)

type ext struct {
Insticator impInsticatorExt `json:"insticator"`
}

type impInsticatorExt struct {
AdUnitId string `json:"adUnitId"`
PublisherId string `json:"publisherId"`
}

type adapter struct {
endpoint string
}

type reqExt struct {
Insticator *reqInsticatorExt `json:"insticator,omitempty"`
}

type reqInsticatorExt struct {
Caller []insticatorCaller `json:"caller,omitempty"`
}

type insticatorCaller struct {
Name string `json:"name,omitempty"`
Version string `json:"version,omitempty"`
}

// caller Info used to track Prebid Server
// as one of the hops in the request to exchange
var caller = insticatorCaller{"Prebid-Server", "n/a"}

type bidExt struct {
Insticator bidInsticatorExt `json:"insticator,omitempty"`
}

type bidInsticatorExt struct {
MediaType string `json:"mediaType,omitempty"`
}

// Builder builds a new instance of the Insticator adapter with the given config.
func Builder(bidderName openrtb_ext.BidderName, config config.Adapter, server config.Server) (adapters.Bidder, error) {
bidder := &adapter{
endpoint: config.Endpoint,
}
return bidder, nil
}

// getMediaTypeForBid figures out which media type this bid is for
func getMediaTypeForBid(bid *openrtb2.Bid) openrtb_ext.BidType {
switch bid.MType {
case openrtb2.MarkupBanner:
return openrtb_ext.BidTypeBanner
case openrtb2.MarkupVideo:
return openrtb_ext.BidTypeVideo
default:
return openrtb_ext.BidTypeBanner
}
}

func (a *adapter) MakeRequests(request *openrtb2.BidRequest, requestInfo *adapters.ExtraRequestInfo) ([]*adapters.RequestData, []error) {
var errs []error
var adapterRequests []*adapters.RequestData
var groupedImps = make(map[string][]openrtb2.Imp)

reqExt, err := makeReqExt(request)
if err != nil {
errs = append(errs, err)
}

request.Ext = reqExt
shubhamc-ins marked this conversation as resolved.
Show resolved Hide resolved

// Create a copy of the request to avoid modifying the original request
requestCopy := *request

for i := 0; i < len(request.Imp); i++ {
impCopy, impKey, err := makeImps(request.Imp[i])
if err != nil {
errs = append(errs, err)
continue
}

// Populate site.publisher.id from imp extension
if requestCopy.Site != nil || requestCopy.App != nil {
if err := populatePublisherId(&impCopy, &requestCopy); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will unmarshal the imp.ext of the current imp and update the site/app object's publisher ID to match. It will repeat this process for every imp in the request. It would be much more efficient to remove this step from the loop, and just execute this for the last imp in the list, as it will have the same result and only consume 1/len(Imp) as many cycles.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I know. I was doing the same previously but someone previously asked to update it.

#3806 (comment)

@hhhjort

errs = append(errs, err)
continue
}
}

resolvedBidFloor, errFloor := resolveBidFloor(impCopy.BidFloor, impCopy.BidFloorCur, requestInfo)
if errFloor != nil {
errs = append(errs, &errortypes.BadInput{
Message: fmt.Sprintf("Error in converting the provided bid floor currency from %s to USD",
impCopy.BidFloorCur),
})
continue
}
if resolvedBidFloor > 0 {
impCopy.BidFloor = resolvedBidFloor
impCopy.BidFloorCur = "USD"
}

groupedImps[impKey] = append(groupedImps[impKey], impCopy)
}

for _, impList := range groupedImps {
if adapterReq, err := a.makeRequest(&requestCopy, impList); err == nil {
adapterRequests = append(adapterRequests, adapterReq)
} else {
errs = append(errs, err)
}
}
return adapterRequests, errs
}

func (a *adapter) makeRequest(request *openrtb2.BidRequest, impList []openrtb2.Imp) (*adapters.RequestData, error) {
request.Imp = impList

reqJSON, err := jsonutil.Marshal(request)
if err != nil {
return nil, err
}

headers := http.Header{}
headers.Add("Content-Type", "application/json;charset=utf-8")
headers.Add("Accept", "application/json")

if request.Device != nil {
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)
headers.Add("IP", request.Device.IP)
}
Copy link
Collaborator

@bsardo bsardo Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're missing some test coverage here. I suggest the following:

  1. Either update an existing exemplary test or add a new supplemental JSON test where .device.ua, .device.ipv6 and .device.ip are set
  2. Add a supplemental JSON test where .device is not nil but .device.ua, device.ipv6 and .device.ip are of zero length.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a new adapters/insticator/insticatortest/supplemental/device-validation.json and added device object in one or two old files too.

}

return &adapters.RequestData{
Method: "POST",
Uri: a.endpoint,
Body: reqJSON,
Headers: headers,
ImpIDs: openrtb_ext.GetImpIDs(request.Imp),
}, nil
}

func (a *adapter) MakeBids(request *openrtb2.BidRequest, requestData *adapters.RequestData, responseData *adapters.ResponseData) (*adapters.BidderResponse, []error) {
if adapters.IsResponseStatusCodeNoContent(responseData) {
return nil, nil
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a supplemental JSON test where your mock response returns a 204 status code.

Copy link
Contributor Author

@shubhamc-ins shubhamc-ins Dec 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

if err := adapters.CheckResponseStatusCodeForErrors(responseData); err != nil {
return nil, []error{err}
}

var response openrtb2.BidResponse
if err := jsonutil.Unmarshal(responseData.Body, &response); err != nil {
return nil, []error{err}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a supplemental JSON test where your mock response returns a bid response that cannot be successuflly unmarshalled. This can be accomplished by making one of the fields the wrong type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

bidResponse := adapters.NewBidderResponseWithBidsCapacity(len(request.Imp))
if response.Cur != "" {
bidResponse.Currency = response.Cur
}
for _, seatBid := range response.SeatBid {
for i := range seatBid.Bid {
bid := &seatBid.Bid[i]
bidType := getMediaTypeForBid(bid)
b := &adapters.TypedBid{
Bid: &seatBid.Bid[i],
BidType: bidType,
}
bidResponse.Bids = append(bidResponse.Bids, b)
}
}
return bidResponse, nil
}

func makeImps(imp openrtb2.Imp) (openrtb2.Imp, string, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may make sense to return the publisher ID here, since you unmarshal the imp.ext here. This would save you from unmarshalling it again in populatePublisherID(). Or possibly move the unmarshal into the Imp loop and pass the unmarshalled structure into this function.

Copy link
Contributor Author

@shubhamc-ins shubhamc-ins Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see @hhhjort

what do you think of this ?

for i := 0; i < len(request.Imp); i++ {
                // I will return publisherId from the makeImps, and use it in next step
		impCopy, impKey, publisherId, err := makeImps(request.Imp[i])
		if err != nil {
			errs = append(errs, err)
			continue
		}

		// Populate publisher.id from imp extension only once for the first imp
		if (requestCopy.Site != nil || requestCopy.App != nil) && i == 0  {
                        // use publisherId here
			if err := populatePublisherId(publisherId, &requestCopy); err != nil {
				errs = append(errs, err)
				continue
			}
		}

... rest code

and I will remove the Unmarshal from populatePublisherId

func populatePublisherId(imp *openrtb2.Imp, request *openrtb2.BidRequest) error {
	// this will be removed
       //  var ext ext

	// Unmarshal the imp extension to get the publisher ID
	// if err := jsonutil.Unmarshal(imp.Ext, &ext); err != nil {
	// 	return &errortypes.BadInput{Message: "Error unmarshalling imp extension"}
	// }

	// Populate site.publisher.id if request.Site is not nil
	if request.Site != nil {

.... Rest code

var bidderExt adapters.ExtImpBidder
if err := jsonutil.Unmarshal(imp.Ext, &bidderExt); err != nil {
return openrtb2.Imp{}, "", &errortypes.BadInput{
Message: err.Error(),
}
}

var insticatorExt openrtb_ext.ExtImpInsticator
if err := jsonutil.Unmarshal(bidderExt.Bidder, &insticatorExt); err != nil {
return openrtb2.Imp{}, "", &errortypes.BadInput{
Message: err.Error(),
}
}

// Directly construct the impExt
impExt := ext{
Insticator: impInsticatorExt{
AdUnitId: insticatorExt.AdUnitId,
PublisherId: insticatorExt.PublisherId,
},
}

impExtJSON, err := jsonutil.Marshal(impExt)
if err != nil {
return openrtb2.Imp{}, "", &errortypes.BadInput{
Message: err.Error(),
}
}
imp.Ext = impExtJSON

// Validate Video if it exists
if imp.Video != nil {
if err := validateVideoParams(imp.Video); err != nil {
return openrtb2.Imp{}, insticatorExt.AdUnitId, &errortypes.BadInput{
Message: err.Error(),
}
}
}

// Return the imp, AdUnitId, and no error
return imp, insticatorExt.AdUnitId, nil
}

func makeReqExt(request *openrtb2.BidRequest) ([]byte, error) {
var reqExt reqExt

if len(request.Ext) > 0 {
if err := jsonutil.Unmarshal(request.Ext, &reqExt); err != nil {
return nil, err
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a supplemental JSON test case where attempting to unmarshal request.ext fails. You should be able to force this by simply setting request.ext.insticator in your JSON test to an invalid type which would be something other than an object (e.g. a string).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added adapters/insticator/insticatortest/supplemental/request-ext-unmarhal-fail.json

}
}

if reqExt.Insticator == nil {
reqExt.Insticator = &reqInsticatorExt{}
}

if reqExt.Insticator.Caller == nil {
reqExt.Insticator.Caller = make([]insticatorCaller, 0)
}

reqExt.Insticator.Caller = append(reqExt.Insticator.Caller, caller)

return jsonutil.Marshal(reqExt)
}

func resolveBidFloor(bidFloor float64, bidFloorCur string, reqInfo *adapters.ExtraRequestInfo) (float64, error) {
if bidFloor > 0 && bidFloorCur != "" && strings.ToUpper(bidFloorCur) != "USD" {
floor, err := reqInfo.ConvertCurrency(bidFloor, bidFloorCur, "USD")
return mathutil.RoundTo4Decimals(floor), err
Comment on lines +266 to +267
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're missing test coverage here. Please add the following supplemental JSON test cases:

  1. currency conversion is successful resulting in a bid floor that is greater than 0
  2. currency conversion fails

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

return bidFloor, nil
}

func validateVideoParams(video *openrtb2.Video) error {
if (video.W == nil || *video.W == 0) || (video.H == nil || *video.H == 0) || video.MIMEs == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: you should be able to delete the parentheses

return &errortypes.BadInput{
Message: "One or more invalid or missing video field(s) w, h, mimes",
}
}

return nil
}

// populatePublisherId function populates site.publisher.id or app.publisher.id
func populatePublisherId(imp *openrtb2.Imp, request *openrtb2.BidRequest) error {
var ext ext

// Unmarshal the imp extension to get the publisher ID
if err := jsonutil.Unmarshal(imp.Ext, &ext); err != nil {
return &errortypes.BadInput{Message: "Error unmarshalling imp extension"}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a supplemental JSON test case where attempting to unmarshal imp.ext fails. You should be able to force this by simply setting imp.ext.insticator in your JSON test to an invalid type which would be something other than an object (e.g. a string).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this should never be non-empty. The ext definition you are using here is looking for the field name 'insticator' rather than bidder which is always translated to bidder by PBS core.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This unmarshal step has been removed.

}

// Populate site.publisher.id if request.Site is not nil
if request.Site != nil {
// Make a shallow copy of Site if it already exists
siteCopy := *request.Site
request.Site = &siteCopy

// Make a shallow copy of Publisher if it already exists
if request.Site.Publisher != nil {
publisherCopy := *request.Site.Publisher
request.Site.Publisher = &publisherCopy
} else {
request.Site.Publisher = &openrtb2.Publisher{}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a supplemental JSON test case where .site is present but .site.publisher is absent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

request.Site.Publisher.ID = ext.Insticator.PublisherId
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
}

// Populate app.publisher.id if request.App is not nil
if request.App != nil {
// Make a shallow copy of App if it already exists
appCopy := *request.App
request.App = &appCopy

// Make a shallow copy of Publisher if it already exists
if request.App.Publisher != nil {
publisherCopy := *request.App.Publisher
request.App.Publisher = &publisherCopy
} else {
request.App.Publisher = &openrtb2.Publisher{}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a supplemental JSON test case where .app is present but .app.publisher is absent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

request.App.Publisher.ID = ext.Insticator.PublisherId
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
}

return nil
}
21 changes: 21 additions & 0 deletions adapters/insticator/insticator_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package insticator

import (
"testing"

"github.com/prebid/prebid-server/v3/adapters/adapterstest"
"github.com/prebid/prebid-server/v3/config"
"github.com/prebid/prebid-server/v3/openrtb_ext"
)

func TestJsonSamples(t *testing.T) {
bidder, buildErr := Builder(openrtb_ext.BidderInsticator, config.Adapter{
Endpoint: "https://insticator.example.com/v1/pbs"},
config.Server{ExternalUrl: "https://insticator.example.com/v1/pbs", GvlID: 1, DataCenter: "2"})

if buildErr != nil {
t.Fatalf("Builder returned unexpected error %v", buildErr)
}

adapterstest.RunJSONBidderTest(t, "insticatortest", bidder)
}
Loading
Loading