Adform adapter lacked gross/net parameter support #2084

bjorn-lw · 2021-11-15T22:21:13Z

The client version of the Adform adapter supports choosing between returning gross or net via the priceType parameter.

As the new "adf" adapter defaults to net while the old "adform" adapter defaulted to gross, this can make the transition to the new adapter become quite a challenge if you are using plenty of bid conversion functions and the PBS adapter doesn't support selecting gross or net.

This PR adds the same priceType support as the client has.

…Type as in client adapter.

bjorn-lw · 2021-11-16T21:29:45Z

@braizhas maybe you want to have a look at this as well.

VeronikaSolovei9 · 2021-11-17T00:46:14Z

adapters/adf/adf.go

+				}
+			} else {
+				requestExt = adfRequestExt{}
+			}


else statement can be omitted if you init requestExt like this: requestExt := adfRequestExt{} (line 57)

Thanks! It will be initiated even if it may not be needed but no harm done in that I guess.

VeronikaSolovei9 · 2021-11-17T01:38:50Z

adapters/adf/adf.go

+			requestExt.PriceType = adfImpExt.PriceType
+
+			var err error
+			if request.Ext, err = json.Marshal(&requestExt); err != nil {


Line 60: you marshal req.Ext as many times as many imps are there in reqiest - it's located inside for _, imp := range request.Imp {. This should be moved ouside the for loop.
Line 71 sets new request.ext on every iteration over imp array. It means req.ext.PriceType will have the value from last imp in loop.
How will it work on your side if there are two or more imp in request with different PriceType?

All priceTypes need to be the same, but really only the first one should be used and not the last one if they should differ.
I took a bad shortcut there. Me programming Go is also a bit like https://youtu.be/C1Sw0PDgHU4.

VeronikaSolovei9 · 2021-11-17T01:52:17Z

adapters/adf/adftest/exemplary/single-banner-pricetype-gross-extend-ext.json

+        "bidder": {
+          "mid": "828782",
+          "priceType": "gross"
+        }


Please add a test case where there are 2 imps in request with different priceType

Not sure if that is really needed but I can add one.

Done. Thank you @VeronikaSolovei9 for the feedback!

VeronikaSolovei9 · 2021-11-17T18:26:54Z

Please update adapter documentation with the description of new parameter

bjorn-lw · 2021-11-17T18:35:31Z

Please update adapter documentation with the description of new parameter

prebid/prebid.github.io#3417

VeronikaSolovei9

Thank you for the fixes, LGTM :)

braizhas · 2021-11-18T12:14:43Z

@braizhas maybe you want to have a look at this as well.

@bjorn-lw thanks for the notification.
Changes are legit and would enable gross price bids from Adform.

Please also update adform bidder params as now it acts as an alias to adf bidder. https://github.com/prebid/prebid-server/blob/063990f8cd8b43b5c8d6d25c1a4ac449350710f7/static/bidder-params/adform.json

bjorn-lw · 2021-11-18T16:14:35Z

Please also update adform bidder params as now it acts as an alias to adf bidder. https://github.com/prebid/prebid-server/blob/063990f8cd8b43b5c8d6d25c1a4ac449350710f7/static/bidder-params/adform.json

Fixed, thank you!

hhhjort · 2021-11-22T20:02:25Z

adapters/adf/adf.go

+		requestExt.PriceType = priceType
+
+		var err error
+		if request.Ext, err = json.Marshal(&requestExt); err != nil {


At this point, you are remarshalling requestExt, which may not be properly populated if the above unmarshal failed. This should be very unlikely, perhaps impossible, but you may want to consider if you want the request to progress through this stage if an error happened.

@hhhjort Thanks! It should be be fixed, but I don't like the many nested if's. I would probably have done it a little different in a language I'm more familiar with. If you can give me a hint on how to make it look nicer, please do.

Actually this doesn't fix it. If you hit line 69, having encountered and error with unmarshalling the .ext, you still drop down and overwrite the original .ext on line 75. You would have to break out of the section, perhaps returning an error from MakeRequests() at that line, to avoid this issue. I am not terribly concerned with this, as it is something that should really never happen in the first place. And if it does somehow happen, it just means that the request.ext you are sending to your server may have missing/corrupt data in it. I really just wanted to highlight it so you could decide if it is a worry for you and change your handling if it is. If you reply that you are good with it as is, or just do nothing, I am still good with approving this PR. Just giving you some time to respond before it is merged.

Ack, nevermind, it does fix it. I just did not read the code properly.

@hhhjort I was frantically reading trying to understand what I missed :) But I think the code is ugly. Can it be written better? You don't have to reply, I could take that online course on Go if I can just get some additional hours in a day :)

I don't have any specific recommendations to make this code prettier, it seems normal enough go to me. Well, one thing that might improve the view...

In the initial loop where you try to find the price type, you are only interested in the first value you find. So when you do find it, you could instead call setRequestPriceType(request *openrtb2.BidRequest, priceType string), which would be the following block of code that sets the price type in request.ext. Then in that function, you could return immediately if the unmarshal fails, making it more obvious that a failure aborts the rest of price type processing. Moving the code to its own function isolates it as its own step, leaving the main function cleaner in its logic. You coud also put the loop to find the price type in the function, making it easier to break out of the loop and further focusing the main function's code.

@hhhjort yes, common clean code sense. I may leave it as is for now if you don't mind. I might get back to this at a later time if I come up with something else that needs to be done in the adapter. Is that OK?

That is fine, I have already approved this PR

hhhjort

LGTM

* 'master' of https://github.com/wwwyyy/prebid-server: (23 commits) Add GPID Reserved Bidder Name (prebid#2107) Marsmedia adapter - param zoneId can get string and int (prebid#2105) Algorix: add Server Region Support (prebid#2096) New Adapter: Bizzclick (prebid#2056) Collect Prometheus Go process metrics (prebid#2101) Added channel support (prebid#2037) Rubicon: update fpd fields resolving (prebid#2091) Beachfront - Currency conversion (prebid#2078) New Adapter: Groupm (Alias Of PubMatic) (prebid#2042) Adform adapter lacked gross/net parameter support (prebid#2084) add iframe sync for openx (prebid#2094) changes to the correct user sync url (prebid#2092) Smaato: Add instl to tests (prebid#2089) Correct MakeRequests() output assertion in adapter JSON tests (prebid#2087) Adview: adapter currency fix. (prebid#2060) New Adapter: Coinzilla (prebid#2074) Syncer Level ExternalURL Override (prebid#2072) 33Across: Enable Support for SRA requests (prebid#2079) Currency conversion on adapter JSON tests (prebid#2071) New Adapter: VideoByte (prebid#2058) ...

Adform adapter lacked gross/net parameter support. Implementing price…

d2b553c

…Type as in client adapter.

bjorn-lw closed this Nov 15, 2021

bjorn-lw reopened this Nov 15, 2021

Extend request.ext

05d9d4b

mansinahar requested review from bsardo and VeronikaSolovei9 November 16, 2021 18:34

mansinahar assigned bsardo and VeronikaSolovei9 Nov 16, 2021

VeronikaSolovei9 reviewed Nov 17, 2021

View reviewed changes

bjorn-lw added 2 commits November 17, 2021 11:41

Some code optimizations

1504712

Extended test

063990f

bjorn-lw mentioned this pull request Nov 17, 2021

Adf Support priceType in Prebid Server prebid/prebid.github.io#3417

Merged

VeronikaSolovei9 previously approved these changes Nov 18, 2021

View reviewed changes

Duplicated adf documentation in adform docs

a3bfae6

bjorn-lw dismissed VeronikaSolovei9’s stale review via a3bfae6 November 18, 2021 16:14

mansinahar assigned hhhjort Nov 22, 2021

mansinahar requested a review from hhhjort November 22, 2021 18:20

hhhjort reviewed Nov 22, 2021

View reviewed changes

Robustness

fffd6b6

hhhjort approved these changes Nov 23, 2021

View reviewed changes

VeronikaSolovei9 approved these changes Nov 29, 2021

View reviewed changes

mansinahar merged commit cc4a376 into prebid:master Nov 29, 2021

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

Adform adapter lacked gross/net parameter support (prebid#2084)

babac28

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

Adform adapter lacked gross/net parameter support (prebid#2084)

4bcfb7d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adform adapter lacked gross/net parameter support #2084

Adform adapter lacked gross/net parameter support #2084

bjorn-lw commented Nov 15, 2021

bjorn-lw commented Nov 16, 2021

VeronikaSolovei9 Nov 17, 2021

bjorn-lw Nov 17, 2021

VeronikaSolovei9 Nov 17, 2021 •

edited

Loading

bjorn-lw Nov 17, 2021

VeronikaSolovei9 Nov 17, 2021

bjorn-lw Nov 17, 2021

bjorn-lw Nov 17, 2021

VeronikaSolovei9 commented Nov 17, 2021

bjorn-lw commented Nov 17, 2021

VeronikaSolovei9 left a comment

braizhas commented Nov 18, 2021

bjorn-lw commented Nov 18, 2021

hhhjort Nov 22, 2021

bjorn-lw Nov 23, 2021

hhhjort Nov 23, 2021

hhhjort Nov 23, 2021

bjorn-lw Nov 23, 2021

hhhjort Nov 24, 2021

bjorn-lw Nov 24, 2021

hhhjort Nov 24, 2021

hhhjort left a comment

Adform adapter lacked gross/net parameter support #2084

Adform adapter lacked gross/net parameter support #2084

Conversation

bjorn-lw commented Nov 15, 2021

bjorn-lw commented Nov 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeronikaSolovei9 Nov 17, 2021 • 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

Choose a reason for hiding this comment

VeronikaSolovei9 commented Nov 17, 2021

bjorn-lw commented Nov 17, 2021

VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

braizhas commented Nov 18, 2021

bjorn-lw commented Nov 18, 2021

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

hhhjort left a comment

Choose a reason for hiding this comment

VeronikaSolovei9 Nov 17, 2021 •

edited

Loading