New Adapter: AdUp Tech #4076

danygielow · 2024-11-26T07:34:10Z

Hello,

We as AdUp Technology want to add our own Prebid Server Adapter.
We already have a PrebidJS Adapter.

github-actions · 2024-11-26T07:34:50Z

adapters/aduptech/aduptech.go

+}
+
+func getBidType(markupType openrtb2.MarkupType) (openrtb_ext.BidType, error) {
+	switch markupType {


Consider this as a suggestion. The current implementation follows an anti-pattern, assumes that if there is a multi-format request, the media type defaults to openrtb_ext.BidTypeBanner. Prebid server expects the media type to be explicitly set in the adapter response. Therefore, we strongly recommend implementing a pattern where the adapter server sets the MType field in the response to accurately determine the media type for the impression.

github-actions · 2024-11-26T07:34:51Z

adapters/aduptech/aduptech.go

+}
+
+func getBidType(markupType openrtb2.MarkupType) (openrtb_ext.BidType, error) {
+	switch markupType {


Consider this as a suggestion. The current implementation follows an anti-pattern, assumes that if there is a multi-format request, the media type defaults to openrtb_ext.BidTypeNative. Prebid server expects the media type to be explicitly set in the adapter response. Therefore, we strongly recommend implementing a pattern where the adapter server sets the MType field in the response to accurately determine the media type for the impression.

github-actions · 2024-11-26T07:34:56Z

Code coverage summary

Note:

Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
Coverage summary encompasses all commits leading up to the latest one, 74112fe

aduptech

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/aduptech/aduptech.go:23:	Builder		100.0%
github.com/prebid/prebid-server/v3/adapters/aduptech/aduptech.go:32:	MakeRequests	53.8%
github.com/prebid/prebid-server/v3/adapters/aduptech/aduptech.go:63:	convertCurrency	0.0%
github.com/prebid/prebid-server/v3/adapters/aduptech/aduptech.go:90:	MakeBids	63.6%
github.com/prebid/prebid-server/v3/adapters/aduptech/aduptech.go:136:	getBidType	75.0%
total:									(statements)	49.1%

github-actions · 2024-11-27T06:32:57Z

adapters/aduptech/aduptech.go

+}
+
+func getBidType(markupType openrtb2.MarkupType) (openrtb_ext.BidType, error) {
+	switch markupType {


Consider this as a suggestion. The current implementation follows an anti-pattern, assumes that if there is a multi-format request, the media type defaults to openrtb_ext.BidTypeBanner. Prebid server expects the media type to be explicitly set in the adapter response. Therefore, we strongly recommend implementing a pattern where the adapter server sets the MType field in the response to accurately determine the media type for the impression.

github-actions · 2024-11-27T06:32:58Z

adapters/aduptech/aduptech.go

+}
+
+func getBidType(markupType openrtb2.MarkupType) (openrtb_ext.BidType, error) {
+	switch markupType {


Consider this as a suggestion. The current implementation follows an anti-pattern, assumes that if there is a multi-format request, the media type defaults to openrtb_ext.BidTypeNative. Prebid server expects the media type to be explicitly set in the adapter response. Therefore, we strongly recommend implementing a pattern where the adapter server sets the MType field in the response to accurately determine the media type for the impression.

What should be changed here? I assume this check is wrong, because we do exactly what the comment suggests.

💯 this suggestion (x3?) is not very helpful

github-actions · 2024-11-27T06:33:05Z

Code coverage summary

Note:

Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
Coverage summary encompasses all commits leading up to the latest one, 8e94b4b

aduptech

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/aduptech/aduptech.go:23:	Builder		100.0%
github.com/prebid/prebid-server/v3/adapters/aduptech/aduptech.go:32:	MakeRequests	84.6%
github.com/prebid/prebid-server/v3/adapters/aduptech/aduptech.go:63:	convertCurrency	75.0%
github.com/prebid/prebid-server/v3/adapters/aduptech/aduptech.go:90:	MakeBids	63.6%
github.com/prebid/prebid-server/v3/adapters/aduptech/aduptech.go:136:	getBidType	75.0%
total:									(statements)	73.6%

bsardo · 2025-01-06T16:46:56Z

@scr-oath can you please review?

bsardo · 2025-01-07T16:20:08Z

@pm-isha-bharti can you please review?

scr-oath · 2025-01-07T19:24:57Z

adapters/aduptech/aduptech.go

+func Builder(bidderName openrtb_ext.BidderName, config config.Adapter, server config.Server) (adapters.Bidder, error) {
+	bidder := &adapter{
+		endpoint:        config.Endpoint,
+		target_currency: "EUR",


question: Can this go into ExtraAdapterInfo (either directly or as json that's parsed or inspected maybe with gjson.Get) rather than being hard coded?

Changed to use ExtraAdapterInfo.

scr-oath · 2025-01-07T19:28:26Z

adapters/aduptech/aduptech.go

+
+			convertedValue, err := a.convertCurrency(imp.BidFloor, imp.BidFloorCur, reqInfo)
+			if err != nil {
+				return nil, err


suggestion: This return doesn't seem right - as it should be []error - perhaps, if convertCurrency returns a []error, it should be named errs rather than err which suggests a single error as the type.

convertCurrency doesn't have to return []error. Refactored to just return a single error.

scr-oath · 2025-01-07T19:29:03Z

adapters/aduptech/aduptech.go

+	}
+
+	requestData := &adapters.RequestData{
+		Method: "POST",


Suggested change

Method: "POST",

Method: http.MethodPost,

scr-oath · 2025-01-07T19:56:59Z

adapters/aduptech/aduptech.go

+
+			// try again by first converting to USD
+			// then convert to target_currency
+			convertedValue, err = reqInfo.ConvertCurrency(value, cur, "USD")


observation we should have constants or an enum of valid currencies… you can ignore for this PR but just making the note; filed #4134

scr-oath · 2025-01-07T20:42:16Z

adapters/aduptech/aduptech.go

+	if responseData.StatusCode == http.StatusNoContent {
+		return nil, nil
+	}
+
+	if responseData.StatusCode == http.StatusBadRequest {
+		err := &errortypes.BadInput{
+			Message: "Unexpected status code: 400. Run with request.debug = 1 for more info.",
+		}
+		return nil, []error{err}
+	}
+
+	if responseData.StatusCode != http.StatusOK {
+		err := &errortypes.BadServerResponse{
+			Message: fmt.Sprintf("Unexpected status code: %d. Run with request.debug = 1 for more info.", responseData.StatusCode),
+		}
+		return nil, []error{err}
+	}


question: would a switch on the statusCode (with empty for ok and default clause for this everything-else case) be more readable?

Refactored into a switch

scr-oath · 2025-01-07T20:46:18Z

adapters/aduptech/aduptech.go

+	for _, seatBid := range response.SeatBid {
+		for i, bid := range seatBid.Bid {
+			bidType, err := getBidType(bid.MType)
+			if err != nil {
+				errs = append(errs, err)
+				continue
+			}
+
+			bidResponse.Bids = append(bidResponse.Bids, &adapters.TypedBid{
+				Bid:     &seatBid.Bid[i],
+				BidType: bidType,
+			})
+		}
+	}


suggestion: I don't like making copies of the seatbid and bid in a loop

Suggested change

for _, seatBid := range response.SeatBid {

for i, bid := range seatBid.Bid {

bidType, err := getBidType(bid.MType)

if err != nil {

errs = append(errs, err)

continue

}

bidResponse.Bids = append(bidResponse.Bids, &adapters.TypedBid{

Bid: &seatBid.Bid[i],

BidType: bidType,

})

}

}

for i := range response.SeatBid {

seatBid = &response.SeatBid[i]

for j := range seatBid.Bid {

bid := &seatBid.Bid[j]

bidType, err := getBidType(bid.MType)

if err != nil {

errs = append(errs, err)

continue

}

bidResponse.Bids = append(bidResponse.Bids, &adapters.TypedBid{

Bid: bid,

BidType: bidType,

})

}

}

Used your suggestion

scr-oath · 2025-01-07T20:48:00Z

adapters/aduptech/aduptech.go

+}
+
+func getBidType(markupType openrtb2.MarkupType) (openrtb_ext.BidType, error) {
+	switch markupType {


💯 this suggestion (x3?) is not very helpful

scr-oath · 2025-01-07T20:48:45Z

adapters/aduptech/aduptech_test.go

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


Suggested change

if buildErr != nil {

t.Fatalf("Builder returned unexpected error %v", buildErr)

}

if buildErr != nil {

assert.NoError(t, buildErr, "Builder returned unexpected error")

}

Ah, good to know. This is our first time working in Go.

suggestion: Change assert to require

For what it's worth, I realize you were calling fatal before so I realize you need the require.NoError - assertions from the assert module continue but the test fails whereas those from require halt immediately. This is kind of like google's gtest with its two variants for each assertion so when you can continue you report more/all things that fail but when you can't, you don't.

scr-oath · 2025-01-07T20:49:54Z

adapters/aduptech/params_test.go

+		if err := validator.Validate(openrtb_ext.BidderAdUpTech, json.RawMessage(invalidParam)); err == nil {
+			t.Errorf("Schema allowed unexpected params: %s", invalidParam)
+		}


Suggested change

if err := validator.Validate(openrtb_ext.BidderAdUpTech, json.RawMessage(invalidParam)); err == nil {

t.Errorf("Schema allowed unexpected params: %s", invalidParam)

}

assert.NoErrorf(t, validator.Validate(openrtb_ext.BidderAdUpTech, json.RawMessage(invalidParam)), "Schema allowed unexpected params: %s", invalidParam)

using assert.Errorf here, but otherwise used the suggestion.

scr-oath

I didn't mean to approve before… I thought I had ticked request changes…

github-actions · 2025-01-08T07:55:13Z

adapters/aduptech/aduptech.go

+}
+
+func getBidType(markupType openrtb2.MarkupType) (openrtb_ext.BidType, error) {
+	switch markupType {


Consider this as a suggestion. The current implementation follows an anti-pattern, assumes that if there is a multi-format request, the media type defaults to openrtb_ext.BidTypeBanner. Prebid server expects the media type to be explicitly set in the adapter response. Therefore, we strongly recommend implementing a pattern where the adapter server sets the MType field in the response to accurately determine the media type for the impression.

github-actions · 2025-01-08T07:55:14Z

adapters/aduptech/aduptech.go

+}
+
+func getBidType(markupType openrtb2.MarkupType) (openrtb_ext.BidType, error) {
+	switch markupType {


Consider this as a suggestion. The current implementation follows an anti-pattern, assumes that if there is a multi-format request, the media type defaults to openrtb_ext.BidTypeNative. Prebid server expects the media type to be explicitly set in the adapter response. Therefore, we strongly recommend implementing a pattern where the adapter server sets the MType field in the response to accurately determine the media type for the impression.

github-actions · 2025-01-08T07:55:37Z

Code coverage summary

Note:

Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
Coverage summary encompasses all commits leading up to the latest one, 2ad858d

aduptech

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/aduptech/aduptech.go:27:	Builder		80.0%
github.com/prebid/prebid-server/v3/adapters/aduptech/aduptech.go:41:	MakeRequests	84.6%
github.com/prebid/prebid-server/v3/adapters/aduptech/aduptech.go:72:	convertCurrency	75.0%
github.com/prebid/prebid-server/v3/adapters/aduptech/aduptech.go:99:	MakeBids	71.4%
github.com/prebid/prebid-server/v3/adapters/aduptech/aduptech.go:145:	getBidType	75.0%
total:									(statements)	76.4%

pm-isha-bharti · 2025-01-08T08:35:54Z

static/bidder-info/aduptech.yaml

+      - banner
+      - native
+userSync:
+  redirect:


@danygielow The usersync URL returned by the /cookie_sync for aduptech is currently throwing 404. Can you please check if the URLs added for userSync are correct?

pm-isha-bharti · 2025-01-08T09:19:19Z

adapters/aduptech/aduptech.go

+func (a *adapter) MakeBids(request *openrtb2.BidRequest, requestData *adapters.RequestData, responseData *adapters.ResponseData) (*adapters.BidderResponse, []error) {
+	switch responseData.StatusCode {
+	case http.StatusNoContent:
+		return nil, nil


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

pm-isha-bharti · 2025-01-08T09:22:19Z

adapters/aduptech/aduptech.go

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


Please add a supplemental JSON test where your mock response returns a bid response that fails to unmarshal successfully. This can be achieved by setting one of the fields to an incorrect data type.

pm-isha-bharti · 2025-01-08T09:24:49Z

adapters/aduptech/aduptech.go

+	switch responseData.StatusCode {
+	case http.StatusNoContent:
+		return nil, nil
+	case http.StatusBadRequest:


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

pm-isha-bharti · 2025-01-08T10:50:50Z

adapters/aduptech/aduptech.go

+func (a *adapter) convertCurrency(value float64, cur string, reqInfo *adapters.ExtraRequestInfo) (float64, error) {
+	convertedValue, err := reqInfo.ConvertCurrency(value, cur, a.extraInfo.TargetCurrency)
+
+	if err != nil {


Please add supplemental JSONs for currency conversion failures

scr-oath · 2025-01-08T15:57:44Z

adapters/aduptech/aduptech_test.go

+	bidder, buildErr := Builder(openrtb_ext.BidderAdUpTech, config.Adapter{
+		Endpoint: "https://example.com/rtb/bid", ExtraAdapterInfo: "{\"target_currency\": \"EUR\"}"}, config.Server{ExternalUrl: "http://hosturl.com", GvlID: 1, DataCenter: "2"})
+
+	assert.NoError(t, buildErr, "Builder returned unexpected error")


Suggested change

assert.NoError(t, buildErr, "Builder returned unexpected error")

require.NoError(t, buildErr, "Builder returned unexpected error")

scr-oath · 2025-01-08T15:58:01Z

adapters/aduptech/aduptech_test.go

+package aduptech
+
+import (
+	"github.com/stretchr/testify/assert"


Suggested change

"github.com/stretchr/testify/assert"

"github.com/stretchr/testify/assert"

"github.com/stretchr/testify/require"

scr-oath · 2025-01-08T15:58:46Z

adapters/aduptech/params_test.go

+	}
+
+	for _, validParam := range validParams {
+		assert.NoErrorf(t, validator.Validate(openrtb_ext.BidderAdUpTech, json.RawMessage(validParam)), "Schema rejected Aduptech params: %s", validParam)


FWIW, assert seems correct here as you want all failing assertions reported

bsardo · 2025-01-08T18:34:00Z

Hi @danygielow, @derfryday, now that the review process has begun, can you please push new commits when making changes instead of rebasing and force pushing? It will save reviewers a lot of time when performing delta reviews. All of the commits will be squashed when we merge. Thanks!

scr-oath · 2025-01-09T04:28:07Z

adapters/aduptech/aduptech.go

+func Builder(bidderName openrtb_ext.BidderName, config config.Adapter, server config.Server) (adapters.Bidder, error) {
+	var extraInfo ExtraInfo
+	if err := jsonutil.Unmarshal([]byte(config.ExtraAdapterInfo), &extraInfo); err != nil {
+		return nil, fmt.Errorf("invalid extra info: %v", err)


suggestion: Use %w instead - see https://pkg.go.dev/fmt#Errorf

Suggested change

return nil, fmt.Errorf("invalid extra info: %v", err)

return nil, fmt.Errorf("invalid extra info: %w", err)

scr-oath · 2025-01-09T04:39:47Z

adapters/aduptech/aduptech.go

+				return nil, []error{err}
+			}
+
+			imp.BidFloorCur = a.extraInfo.TargetCurrency


bug?: There seems like a bug in here somewhere… the convertCurrency has a path that can handle errors and, instead, convert to "USD" - how is that choice made here if you always set the BidFloorCur to a.extraInfo.TargetCurrency - should you pass in a pointer to the extraInfo and set the chosen currency in the convertCurrency method maybe? or return the choice back and set to that? Style is your choice, but I think that's an unhandled case and might deserve some unit tests to prove it works as intended.

scr-oath · 2025-01-09T04:43:43Z

adapters/aduptech/aduptech.go

+
+	if err != nil {
+		var convErr currency.ConversionNotFoundError
+		if errors.As(err, &convErr) {


nitpick/suggestion: In go, there's a pattern that you try to take the "terminal" case first - return/break/panic/exit so that you don't need an else clause - it saves "cognitive overload" because you can just reason that everything after is the "happy path" (not having your brain full with remembering what the condition was when you get to the else way down the screen or having to scroll back up to understand it) - and also saves precious indentation 😄

(so make this !errors.As and pull the else clause into the block and remove the curly braces {} (keeping and unindenting this code))

pm-isha-bharti · 2025-01-10T07:08:42Z

openrtb_ext/imp_aduptech.go

+	Publisher string          `json:"publisher"`
+	Placement string          `json:"placement"`
+	Query     string          `json:"query"`
+	AdTest    string          `json:"adtest"`


Can we use bidrequest.test instead of introducing a separate parameter, considering it is a standard ORTB field already used by Prebid?

github-actions bot reviewed Nov 26, 2024

View reviewed changes

danygielow mentioned this pull request Nov 26, 2024

Updated aduptech.md to add pbs: true prebid/prebid.github.io#5728

Open

8 tasks

danygielow force-pushed the feat/add-aduptech-adapter branch from 74112fe to 8e94b4b Compare November 27, 2024 06:32

github-actions bot reviewed Nov 27, 2024

View reviewed changes

bsardo added the adapter label Jan 6, 2025

bsardo assigned bsardo and przemkaczmarek Jan 7, 2025

scr-oath approved these changes Jan 7, 2025

View reviewed changes

scr-oath mentioned this pull request Jan 8, 2025

Consider making constants for currencies #4134

Open

scr-oath suggested changes Jan 8, 2025

View reviewed changes

New Adapter: AdUp Tech

2ad858d

derfryday force-pushed the feat/add-aduptech-adapter branch from 8e94b4b to 2ad858d Compare January 8, 2025 07:54

github-actions bot reviewed Jan 8, 2025

View reviewed changes

pm-isha-bharti reviewed Jan 8, 2025

View reviewed changes

scr-oath reviewed Jan 8, 2025

View reviewed changes

scr-oath suggested changes Jan 9, 2025

View reviewed changes

scr-oath reviewed Jan 9, 2025

View reviewed changes

pm-isha-bharti reviewed Jan 10, 2025

View reviewed changes

	assert.NoError(t, buildErr, "Builder returned unexpected error")
	require.NoError(t, buildErr, "Builder returned unexpected error")

	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"

	return nil, fmt.Errorf("invalid extra info: %v", err)
	return nil, fmt.Errorf("invalid extra info: %w", err)

New Adapter: AdUp Tech #4076

Are you sure you want to change the base?

New Adapter: AdUp Tech #4076

Conversation

danygielow commented Nov 26, 2024 • edited Loading

github-actions bot Nov 26, 2024

Choose a reason for hiding this comment

github-actions bot Nov 26, 2024

Choose a reason for hiding this comment

github-actions bot commented Nov 26, 2024

Code coverage summary

aduptech

github-actions bot Nov 27, 2024

Choose a reason for hiding this comment

github-actions bot Nov 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 27, 2024

Code coverage summary

aduptech

bsardo commented Jan 6, 2025

bsardo commented Jan 7, 2025

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

scr-oath Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scr-oath left a comment

Choose a reason for hiding this comment

github-actions bot Jan 8, 2025

Choose a reason for hiding this comment

github-actions bot Jan 8, 2025

Choose a reason for hiding this comment

github-actions bot commented Jan 8, 2025

Code coverage summary

aduptech

pm-isha-bharti Jan 8, 2025 • edited Loading

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

pm-isha-bharti Jan 8, 2025 • edited Loading

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 commented Jan 8, 2025

Choose a reason for hiding this comment

scr-oath Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

scr-oath Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

pm-isha-bharti Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

danygielow commented Nov 26, 2024 •

edited

Loading

scr-oath Jan 8, 2025 •

edited

Loading

pm-isha-bharti Jan 8, 2025 •

edited

Loading

pm-isha-bharti Jan 8, 2025 •

edited

Loading

scr-oath Jan 9, 2025 •

edited

Loading

scr-oath Jan 9, 2025 •

edited

Loading

pm-isha-bharti Jan 10, 2025 •

edited

Loading