Add ORTB 2.4 schain support #2108

bsardo · 2021-12-09T17:57:41Z

This PR adds ORTB 2.4 schain support as described in issue #2080.

If an schain is defined in one of the ORTB 2.5 locations (.source.ext.schain or .ext.prebid.schains) and in the ORTB 2.4 location (.ext.schain), the 2.5 schains take precedence so the ORTB 2.4 schain is ignored.
If an schain is defined in the ORTB 2.4 location (.ext.schain) but not in one of the ORTB 2.5 schain locations, the ORTB 2.4 schain is copied to the ORTB 2.5 location (.source.ext.schain) for each bid request.

The PR refactors the schain logic as part of introducing ORTB 2.4 schain support. An SChainWriter interface is introduced with concrete ORTB 2.4 and 2.5 implementations. Each concrete type's Write method modifies the request .source.ext.schain accordingly ensuring that any fields defined in .source are copied (this was a recently fixed bug).

It should also be noted that the function BidderToPrebidSChains in schain/schain.go and its corresponding tests were copied verbatim from exchange/utils.go.

bsardo · 2021-12-13T21:08:02Z

exchange/utils_test.go

@@ -2734,47 +2623,6 @@ func TestBuildXPrebidHeader(t *testing.T) {
 	}
 }

-func TestSourceExtSChainCopied(t *testing.T) {


This test was deleted because the corresponding logic was moved into the schain writer implementation Write methods which have tests that cover this logic.

bsardo · 2021-12-13T21:08:45Z

exchange/utils.go

 	return json.Marshal(extCopy)
 }

-func prepareSource(req *openrtb2.BidRequest, bidder string, sChainsByBidder map[string]*openrtb_ext.ExtRequestPrebidSChainSChain) {


This logic has been moved to the schain writer implementation Write methods.

bsardo · 2021-12-13T21:10:15Z

exchange/utils.go

@@ -31,23 +32,6 @@ var integrationTypeMap = map[metrics.RequestType]config.IntegrationType{

 const unknownBidder string = ""

-func BidderToPrebidSChains(sChains []*openrtb_ext.ExtRequestPrebidSChain) (map[string]*openrtb_ext.ExtRequestPrebidSChainSChain, error) {


This function was copied to schain/schain.go.

bsardo · 2021-12-13T21:17:06Z

schain/schainwriter.go

+func extPrebidSChainExists(reqExt *openrtb_ext.ExtRequest) bool {
+	if reqExt == nil {
+		return false
+	}
+	if reqExt.Prebid.SChains == nil {
+		return false
+	}
+	return true
+}


This function was written this way instead of a simple one liner of return reqExt != nil && reqExt.Prebid.SChains != nil because I think it helps us ensure code coverage of each condition when running the tests with code coverage enabled. The same applies to the two helper functions right after it.
If anyone prefers I use a one liner LMK. I thought the benefits here outweighed the slight increase in LOC.

I was going to write a comment about this :)
I only think it's better to check reqExt.Prebid.SChains != nil. In this case returning false by default sounds more reasonable to me.

I think we also need to check if reqExt is nil otherwise we will get an error when attempting to access the Prebid field on a nil reqExt.

VeronikaSolovei9 · 2021-12-20T09:18:37Z

While reviewing this PR and reading description at the 100th time, I started to think there is no need of ORTBTwoFourSChainWriter. If an schain is defined in the ORTB 2.4 location (.ext.schain) but not in one of the ORTB 2.5 schain locations, the ORTB 2.4 schain is copied to the ORTB 2.5 location (.source.ext.schain) for each bid request. -> sounds there will always be ORTB 2.5.
Also code in func (w ORTBTwoFourSChainWriter) Write(req *openrtb2.BidRequest, bidder string) duplicates code in func (w ORTBTwoFiveSChainWriter) Write(req *openrtb2.BidRequest, bidder string).
Also you execute sChainWriter.Write(&reqCopy, bidder) inside of the loop, so ORTBTwoFourSChainWriter still writes data to each bidder request (even if it's the same) and bidder param doesn't have any usages.
Would it be reasonable to have just one SChainWriter?
Please let me know if I miss something

bsardo · 2021-12-20T14:49:11Z

An schain can be defined in the ORTB 2.4 or the ORTB 2.5 location of the original request. Each of the writers is used to set the schain in the ORTB 2.5 location of each bid request. If no ORTB 2.5 schain is defined but an ORTB 2.4 schain is defined, the ORTB 2.4 schain writer is what is instantiated and then used to write the 2.4 schain to the 2.5 location. So yes, there will always be an schain in the 2.5 location of each bid request assuming an schain was defined somewhere in the original request.
I created two separate writers, each of which writes to the 2.5 location, because IMO it is better design (smaller functions, easier to test, more extensible) to separate the 2.4 and 2.5 logic than to mix that logic together. I agree there is a little code duplication in the Write function. That could be extracted into a common function.
Also, yes, the bidder isn't used in the 2.4 writer Write function; it is there to satisfy the interface contract at this time. If we have some concerns about the fact that the 2.4 writer is called in a loop and just does the same thing for each bidder, the marshalling could happen on instantiation to address any performance concerns.
Thoughts @VeronikaSolovei9 @mansinahar?

stale · 2022-01-08T22:57:35Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

mansinahar · 2022-01-11T17:20:20Z

openrtb_ext/request_wrapper.go

@@ -456,6 +511,19 @@ func (re *RequestExt) SetPrebid(prebid *ExtRequestPrebid) {
 	re.prebidDirty = true
 }

+func (re *RequestExt) GetSChain() *ExtRequestPrebidSChainSChain {


Can we please add a comment here saying that these Schain functions on the request ext are only for backwards compatibility for openrtb 2.4 and shouldn't really be used for anything else. Only source.ext.schain or reques.ext.prebid.schain should be used?

mansinahar · 2022-01-11T17:25:25Z

endpoints/openrtb2/auction_test.go

+			if tt.wantReqExtSChain == nil {
+				assert.Nil(t, reqExtSChain, tt.description)
+			} else {
+				assert.Equal(t, tt.wantReqExtSChain, reqExtSChain, tt.description)


Wouldn't this assert.Equal call take care of the nil case as well? Do we need the if-else conditional here? Similar for the source ext below

VeronikaSolovei9

Thank you for changing the code, LGTM

* 'master' of https://github.com/wwwyyy/prebid-server: GDPR: pass geo based on special feature 1 opt-in (prebid#2122) Adyoulike: Fix adapter errors if no content (prebid#2134) Update adyoulike.yaml (prebid#2131) Add ORTB 2.4 schain support (prebid#2108) Stored response fetcher (with new func for stored resp) (prebid#2099) Adot: Add OpenRTB macros resolution (prebid#2118) Rubicon adapter: accept accountId, siteId, zoneId as strings (prebid#2110) Operaads: support multiformat request (prebid#2121) Update go-gdpr to v1.11.0 (prebid#2125) Adgeneration: Update request to include device and app related info in headers and query params (prebid#2109) Adnuntius: Read device information from ortb Request (prebid#2112) New Adapter: apacdex and Remove Adapter: valueimpression (prebid#2097) New Adapter: Compass (prebid#2102) Orbidder: bidfloor currency handling (prebid#2093)

mansinahar requested review from mansinahar and VeronikaSolovei9 December 9, 2021 18:22

mansinahar assigned VeronikaSolovei9 and mansinahar Dec 9, 2021

bsardo added 5 commits December 13, 2021 09:57

Add ORTB 2.4 schain support

330ea15

go fmt

47b2aa2

Refactor schain writer tests and increase test coverage

efccf8b

Update exchange utils schain test

97cecff

Remove test covering logic in writer

d778631

bsardo force-pushed the PBS-1033 branch from 25c8038 to d778631 Compare December 13, 2021 20:53

bsardo commented Dec 13, 2021

View reviewed changes

bsardo marked this pull request as ready for review December 13, 2021 21:09

bsardo commented Dec 13, 2021

View reviewed changes

stale bot added the stale label Jan 8, 2022

Map ORTB 2.4 schain to ORTB 2.5 location upstream in validation step

e70939d

stale bot removed the stale label Jan 10, 2022

Add request wrapper test coverage to push package coverage over 30%

f3040ba

mansinahar reviewed Jan 11, 2022

View reviewed changes

Add wrapper schain method comment and minor test improvement

b09cf34

VeronikaSolovei9 previously approved these changes Jan 11, 2022

View reviewed changes

Remove warnings added when debugging

14b3346

bsardo dismissed VeronikaSolovei9’s stale review via 14b3346 January 12, 2022 02:15

Remove glog import

bb83cc6

VeronikaSolovei9 approved these changes Jan 12, 2022

View reviewed changes

mansinahar approved these changes Jan 13, 2022

View reviewed changes

mansinahar merged commit 9f908f6 into prebid:master Jan 13, 2022

ramyferjaniadot pushed a commit to adotmob/prebid-server that referenced this pull request Feb 2, 2022

Add ORTB 2.4 schain support (prebid#2108)

86b0a23

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

Add ORTB 2.4 schain support (prebid#2108)

8a5d5f0

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

Add ORTB 2.4 schain support (prebid#2108)

6e76b57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ORTB 2.4 schain support #2108

Add ORTB 2.4 schain support #2108

bsardo commented Dec 9, 2021 •

edited

Loading

bsardo Dec 13, 2021

bsardo Dec 13, 2021

bsardo Dec 13, 2021 •

edited

Loading

bsardo Dec 13, 2021

VeronikaSolovei9 Dec 20, 2021 •

edited

Loading

bsardo Dec 20, 2021

VeronikaSolovei9 commented Dec 20, 2021 •

edited

Loading

bsardo commented Dec 20, 2021

stale bot commented Jan 8, 2022

mansinahar Jan 11, 2022

mansinahar Jan 11, 2022

VeronikaSolovei9 left a comment

		@@ -31,23 +32,6 @@ var integrationTypeMap = map[metrics.RequestType]config.IntegrationType{

		const unknownBidder string = ""

		func BidderToPrebidSChains(sChains []openrtb_ext.ExtRequestPrebidSChain) (map[string]openrtb_ext.ExtRequestPrebidSChainSChain, error) {

Add ORTB 2.4 schain support #2108

Add ORTB 2.4 schain support #2108

Conversation

bsardo commented Dec 9, 2021 • edited Loading

bsardo Dec 13, 2021

Choose a reason for hiding this comment

bsardo Dec 13, 2021

Choose a reason for hiding this comment

bsardo Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

bsardo Dec 13, 2021

Choose a reason for hiding this comment

VeronikaSolovei9 Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

bsardo Dec 20, 2021

Choose a reason for hiding this comment

VeronikaSolovei9 commented Dec 20, 2021 • edited Loading

bsardo commented Dec 20, 2021

stale bot commented Jan 8, 2022

mansinahar Jan 11, 2022

Choose a reason for hiding this comment

mansinahar Jan 11, 2022

Choose a reason for hiding this comment

VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

bsardo commented Dec 9, 2021 •

edited

Loading

bsardo Dec 13, 2021 •

edited

Loading

VeronikaSolovei9 Dec 20, 2021 •

edited

Loading

VeronikaSolovei9 commented Dec 20, 2021 •

edited

Loading