Unruly r1 consolidated adapter #1997

opeledtremor · 2021-09-14T08:04:00Z

This is a consolidated adapter between the Unruly adapter and the RhythmOne Adapter.
Now the unruly adapter use the new endpoint and has the same support as the RhythmOne Adapter with the relevant params.

…rt the new consolidated adapter changes

* remove S2S param * remove usersync since we do this on our server side now

* change the siteid type to be int instead of string

guscarreon · 2021-09-16T06:02:24Z

Please resolve merge conflicts on file usersync/usersyncers/syncer.go

# Conflicts: # usersync/usersyncers/syncer.go

opeledtremor · 2021-09-19T07:15:11Z

I believe I resolved the conflicts.

guscarreon

Left some comments below

guscarreon · 2021-09-21T16:02:12Z

openrtb_ext/imp_unruly.go

@@ -1,6 +1,5 @@
 package openrtb_ext

 type ExtImpUnruly struct {
-	UUID   string `json:"uuid"`
-	SiteID string `json:"siteid"`
+	SiteID int `json:"siteid"`


Has the param list been updated in the docs PR?
https://github.com/prebid/prebid.github.io/blob/master/dev-docs/bidders/unruly.md

yes it is updated.

adapters/unruly/params_test.go

guscarreon · 2021-09-21T16:13:55Z

adapters/unruly/unruly.go

@@ -13,136 +13,136 @@ import (
 )

 type UnrulyAdapter struct {


Can we please rename to simply adapter? If not, could we avoid capitalization? There's no need to export this particular strucrture.

guscarreon · 2021-09-21T16:49:07Z

adapters/unruly/unruly.go

+	for i := 0; i < numRequests; i++ {
+		imp := req.Imp[i]
+		var bidderExt adapters.ExtImpBidder
+		err := json.Unmarshal(imp.Ext, &bidderExt)
 		if err != nil {


Can we contract the Unmarshal call error checks in lines 59-60 and 68-69?

58 var bidderExt adapters.ExtImpBidder 59 - err := json.Unmarshal(imp.Ext, &bidderExt) 60 - if err != nil { + if err := json.Unmarshal(imp.Ext, &bidderExt); err != nil { 61 err = &errortypes.BadInput{ 62 Message: fmt.Sprintf("ext data not provided in imp id=%s. Abort all Request", imp.ID), 63 } 64 errors = append(errors, err) 65 return nil, "", errors 66 } adapters/unruly/unruly.go

guscarreon · 2021-09-21T16:56:38Z

adapters/unruly/unruly.go

 	}
+
+	var errs []error


I notice that the errs array doesn't get populated in the rest of the function. Is it still needed? Should we return an empty error array in line 132 instead?

132 - return bidResponse, errs + return bidResponse, []error{}

changed both this to return the errors from the changes from the next comment

guscarreon · 2021-09-21T17:10:32Z

adapters/unruly/unruly.go

+				mediaType = openrtb_ext.BidTypeBanner
+			} else if imp.Video != nil {
+				mediaType = openrtb_ext.BidTypeVideo
+			}


Should we code a bit more defensively and return an error in case a given impId doesn't match with any of the imp.IDs in the array or if a given imp comes with both a imp.Banner == nil and imp.Video == nil? Maybe the error that getMediaTypeForImp returns could end up in MakeBids' unused errs array.

done, I hope this is what you meant.

I did. Thank you

mansinahar · 2021-09-28T17:27:14Z

@opeledtremor Just a friendly ping about the comments above.

guscarreon · 2021-09-30T17:08:37Z

adapters/unruly/unruly.go

+		}
+		var unrulyExt openrtb_ext.ExtImpUnruly
+		//err = json.Unmarshal(bidderExt.Bidder, &unrulyExt)
+		//if err != nil {


I believe these commented lines are a left-over

guscarreon · 2021-09-30T17:12:13Z

adapters/unruly/unruly.go

 	}
+
+	var errs []error


guscarreon

It looks pretty good. I have two last minor nitpicks and after those get resolved, I think I'll be ready to approve.

guscarreon · 2021-09-30T17:15:05Z

adapters/unruly/unruly.go

 	for _, sb := range bidResp.SeatBid {
 		for i := range sb.Bid {
-			bidType, err := getMediaTypeForImpWithId(sb.Bid[i].ImpID, internalRequest.Imp)
+			var bla, err = getMediaTypeForImp(sb.Bid[i].ImpID, internalRequest.Imp)


Nitpick: Can we rename bla to bidType or something more descriptive?

bsardo · 2021-10-01T19:26:36Z

adapters/unruly/unruly.go

+		unrulyExtCopy, err := json.Marshal(&unrulyExt)
+		if err != nil {
+			errors = append(errors, err)
+			return nil, "", errors
+		}


If you want a copy of the unrulyExt as JSON, why marshal what was just unmarshaled? At this point, you've verified unrulyExt is valid since it was successfully unmarshaled and it was not mutated so couldn't you just make a copy of bidderExt.Bidder instead and avoid the additional marshal call? Or are you leaning on the marshal call here to remove data other than siteid?

yes we are leaning on the marshal call here to remove data other than siteid

bsardo · 2021-10-01T19:37:35Z

adapters/unruly/unruly.go

+		if uri == "" {
+			uri = fmt.Sprintf("%s", a.endPoint)
 		}


It seems like this should be outside of the for loop.

removed since we removed the uri according to the next comment

bsardo · 2021-10-01T20:05:47Z

adapters/unruly/unruly.go

-	if err != nil {
-		return nil, []error{err}
+	var uri string
+	request, uri, errs = a.preProcess(request, errs)


It looks like you could simplify the preProcess method by eliminating the url return value and just looking at a.endpoint here by changing line 38 in this function from to if a.endpoint != "" and then changing line 44 to Uri: a.endpoint

bsardo · 2021-10-01T20:31:31Z

adapters/unruly/unruly.go

+		} else {
+			errs = append(errs, fmt.Errorf("impId doesn't match with any of the imp.ID"))


It looks like this else statement could execute multiple times and if imp.ID == impId is never true, the error slice will contain this same error once for every imp in the original request.

changed it to be only 1 error with all the non matching imps

bsardo · 2021-10-01T20:44:40Z

adapters/unruly/unruly.go

-	return imp, nil
-}
+func getMediaTypeForImp(impId string, imps []openrtb2.Imp) (openrtb_ext.BidType, []error) {
+	var errs []error


This function never returns errs

bsardo · 2021-10-01T20:48:39Z

adapters/unruly/unruly.go

-			return openrtb_ext.BidTypeVideo, nil
-		}
+func (a *adapter) MakeBids(internalRequest *openrtb2.BidRequest, externalRequest *adapters.RequestData, response *adapters.ResponseData) (*adapters.BidderResponse, []error) {
+	if response.StatusCode == http.StatusNoContent {


Please add a test for when the status code is 204.

bsardo · 2021-10-01T20:49:06Z

adapters/unruly/unruly.go

-	return "", &errortypes.BadInput{
-		Message: fmt.Sprintf("Failed to find impression \"%s\" ", impID),
+
+	if response.StatusCode == http.StatusBadRequest {


Please add a test for when the status code is 400.

bsardo · 2021-10-01T20:50:06Z

adapters/unruly/unruly.go

-		return &errortypes.BadServerResponse{
-			Message: fmt.Sprintf("Unexpected status code: %d. Run with request.debug = 1 for more info", response.StatusCode),
-		}
+		return nil, []error{&errortypes.BadServerResponse{


Please add a test with another error code so that the BadServerResponse is returned.

added another BadServerResponse for the 401 response

bsardo

This is looking good; I have just a few more comments on getMediaTypeForImp.

bsardo · 2021-10-04T02:53:54Z

adapters/unruly/unruly.go

+			} else {
+				errs = append(errs, fmt.Errorf("bid responses mediaType didn't match supported mediaTypes"))
+			}
+			return mediaType, nil


Shouldn't this be return mediaType, errs so the "didn't match supported mediaTypes" error is returned?

bsardo · 2021-10-04T02:55:57Z

adapters/unruly/unruly.go

-func (a *UnrulyAdapter) MakeBids(internalRequest *openrtb2.BidRequest, externalRequest *adapters.RequestData, response *adapters.ResponseData) (*adapters.BidderResponse, []error) {
-	if err := CheckResponse(response); err != nil {
-		return nil, []error{err}
+	if len(noMatchingImps) > 0 {


Given that you always return if the imp is found, I don't think you need this conditional check as it appears the only way you would reach this part of the function is if the bid response imp ID was not found in the bid request.

If I understood you correctly it's done (removed the condition)

bsardo · 2021-10-04T03:00:59Z

adapters/unruly/unruly.go

-	if err := CheckResponse(response); err != nil {
-		return nil, []error{err}
+	if len(noMatchingImps) > 0 {
+		errs = append(errs, fmt.Errorf("the following imps id's %v didn't match impId %s", noMatchingImps, impId))


Perhaps this error should be a little more generic and not include a variable name like impId in it. How about something like: "Bid response imp ID %s not found in bid request containing imps %v\n"?

guscarreon

LGTM

opeledtremor added 5 commits August 15, 2021 14:25

Changing the unruly adapter to be like the rhytemone adapter to suppo…

4e43686

…rt the new consolidated adapter changes

* fix typo

29f7ebd

* remove S2S param * remove usersync since we do this on our server side now

* remove the usersync

6aa1b15

* change the siteid type to be int instead of string

* change the maintainer email to the correct one

d178122

* change the gvlVendorID according to our legal department

7b0df81

mansinahar requested review from bsardo and guscarreon September 14, 2021 18:03

mansinahar assigned bsardo and guscarreon Sep 14, 2021

Merge branch 'master' into unruly-r1-consolidated-adapter

8b54e72

# Conflicts: # usersync/usersyncers/syncer.go

guscarreon reviewed Sep 21, 2021

View reviewed changes

changes made after code review

c4f9153

guscarreon reviewed Sep 30, 2021

View reviewed changes

bsardo reviewed Oct 2, 2021

View reviewed changes

changes made after code review

54cfd26

bsardo reviewed Oct 4, 2021

View reviewed changes

changes made after code review

d8be2d5

bsardo approved these changes Oct 4, 2021

View reviewed changes

guscarreon approved these changes Oct 4, 2021

View reviewed changes

guscarreon merged commit f81b8dd into prebid:master Oct 4, 2021

jizeyopera pushed a commit to operaads/prebid-server that referenced this pull request Oct 13, 2021

Unruly r1 consolidated adapter (prebid#1997)

b5e0bee

jorgeluisrocha pushed a commit to jwplayer/prebid-server that referenced this pull request Sep 28, 2022

Unruly r1 consolidated adapter (prebid#1997)

22f99bb

shunj-nb pushed a commit to ParticleMedia/prebid-server that referenced this pull request Nov 8, 2022

Unruly r1 consolidated adapter (prebid#1997)

60c36dd

		} else {
		errs = append(errs, fmt.Errorf("impId doesn't match with any of the imp.ID"))

Unruly r1 consolidated adapter #1997

Unruly r1 consolidated adapter #1997

Conversation

opeledtremor commented Sep 14, 2021

guscarreon commented Sep 16, 2021

opeledtremor commented Sep 19, 2021

guscarreon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mansinahar commented Sep 28, 2021

Choose a reason for hiding this comment

opeledtremor Oct 3, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guscarreon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsardo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guscarreon left a comment

Choose a reason for hiding this comment

opeledtremor Oct 3, 2021 •

edited