bidRequest.Source.Ext wasn't cloned in prepareSource #2070

bjorn-lw · 2021-11-03T13:39:40Z

When adding bidder specific schains, the last bidder schain being setup was used in all requests due to the bidRequest was only shallow copied in utils.getAuctionBidderRequests.

There may very well be additional problems related to the shallow copy, but the issue with the wrong bidder schain being sent to all but one bidder is fixed with this PR.

Note I'm not a GO programmer so there may be better ways to solve the issue, so please help me out in that case.

I consider this bug to be very serious since bidder specific schains is not doing at all what it is meant to do right now.

guscarreon · 2021-11-04T15:57:20Z

exchange/utils_test.go

+	copy1 := *bidRequest
+	prepareSource(&copy1, "bidder1", bidderSchains)
+	copy2 := *bidRequest
+	prepareSource(&copy2, "bidder2", bidderSchains)


The second call to prepareSource(...) seems to test the exact same scenario than the first. Should we remove?

No, that would destroy the purpose of the test. It’s supposed to test that a sequence of calls to prepareSource will only modify the bidRequest being sent in. Maybe I wasn’t clear about what this bug is all about.

Without the bug fix “copy1” would be affected by the second call with the “copy2” parameter.

This causes bidder specific schains to not work at all and has probably never worked.

i.e. the following would pass where complete is 2 in both cases (which is clearly wrong):

assert.Equal(t, json.RawMessage(`{"schain":{"complete":2,"nodes":null,"ver":""}}`), copy1.Source.Ext, "First schain was overwritten or not set") assert.Equal(t, json.RawMessage(`{"schain":{"complete":2,"nodes":null,"ver":""}}`), copy2.Source.Ext, "Second schain was overwritten or not set")

If newly added lines 307 to 309 get removed TestSourceExtSChainCopied still succeeds, and I believe this happens because in both calls to prepareSource() we pass a bidRequest with a nil Source field. In other words, both calls to prepareSource() seem to take the true branch of the if statement in line 305:

304 // set source 305 if req.Source == nil { 306 req.Source = &openrtb2.Source{} 307 } else { 308 sourceCopy := *req.Source 309 req.Source = &sourceCopy 310 } exchange/utils.go

And the newly added false branch (lines 308 and 309) is not covered by your TestSourceExtSChainCopied. Can you modify?

Without the bug fix “copy1” would be affected by the second call with the “copy2” parameter.

Can we explicitly assert this?

@guscarreon You're right, the test was sloppy. Let me fix this.

@guscarreon please check latest commit

guscarreon

Looks pretty good. Just have one last comment before approving

guscarreon · 2021-11-05T17:27:35Z

exchange/utils_test.go

+	prepareSource(&copy2, "bidder2", bidderSchains)
+
+	assert.Equal(t, json.RawMessage(`{"schain":{"complete":1,"nodes":null,"ver":""}}`), copy1.Source.Ext, "First schain was overwritten or not set")
+	assert.Equal(t, json.RawMessage(`{"schain":{"complete":2,"nodes":null,"ver":""}}`), copy2.Source.Ext, "Second schain was overwritten or not set")


Thank you @bjorn-lw . One last ask: could we also assert that the primitive values are preserved when copying the Source field?

2547 func TestSourceExtSChainCopied(t *testing.T) { 2548 bidRequest := newBidRequest(t) 2549 2550 bidderSchains := map[string]*openrtb_ext.ExtRequestPrebidSChainSChain{ 2551 "bidder1": { 2552 Complete: 1, 2553 }, 2554 "bidder2": { 2555 Complete: 2, 2556 }, 2557 } 2558 2559 copy1 := *bidRequest 2560 prepareSource(&copy1, "bidder1", bidderSchains) 2561 copy2 := *bidRequest + copy2.Source.TID = "Different value" 2562 prepareSource(&copy2, "bidder2", bidderSchains) 2563 2564 assert.Equal(t, json.RawMessage(`{"schain":{"complete":1,"nodes":null,"ver":""}}`), copy1.Source.Ext, "First schain was overwritten or not set") 2565 assert.Equal(t, json.RawMessage(`{"schain":{"complete":2,"nodes":null,"ver":""}}`), copy2.Source.Ext, "Second schain was overwritten or not set") + + // Assert copy1.Source was copied inside prepareSource() and primitive values were not overwritten + assert.NotEqual(t, copy1.Source.TID, copy2.Source.TID, "Primitive values inside copy1.Source are not supposed get overwritten") 2566 } exchange/utils_test.go

@guscarreon Extended the test as you asked for.

guscarreon

LGTM. Thank you very much @bjorn-lw for correcting this bug and addressing my PR feedback.

bjorn-lw · 2021-11-14T19:21:05Z

Are we waiting for @bsardo for approving before merge?

bsardo · 2021-11-15T17:42:19Z

exchange/utils_test.go

@@ -2609,8 +2609,48 @@ func TestBuildXPrebidHeader(t *testing.T) {
 	}
 }

-func TestApplyFPD(t *testing.T) {
+func TestSourceExtSChainCopied(t *testing.T) {


Great catch finding this bug. This test works but what are you thoughts on adding a higher level test instead that isn't specifically focused on the copy function but rather verifies schains are correctly added to the bid requests when there are multiple bidders:

func TestCleanOpenRTBRequestsSChainMultipleBidders(t *testing.T) { req := &openrtb2.BidRequest{ Site: &openrtb2.Site{}, Source: &openrtb2.Source{ TID: "61018dc9-fa61-4c41-b7dc-f90b9ae80e87", }, Imp: []openrtb2.Imp{{ Ext: json.RawMessage(`{"appnexus": {"placementId": 1}, "axonix": { "supplyId": "123"}}`), }}, Ext: json.RawMessage(`{"prebid":{"schains":[{ "bidders":["appnexus"],"schain":{"complete":1,"nodes":[{"asi":"directseller1.com","sid":"00001","rid":"BidRequest1","hp":1}],"ver":"1.0"}}, {"bidders":["axonix"],"schain":{"complete":1,"nodes":[{"asi":"directseller2.com","sid":"00002","rid":"BidRequest2","hp":1}],"ver":"1.0"}}]}}`), } unmarshaledExt, err := extractBidRequestExt(req) assert.NoErrorf(t, err, "Error unmarshaling inExt") extRequest := unmarshaledExt auctionReq := AuctionRequest{ BidRequest: req, UserSyncs: &emptyUsersync{}, } bidderToSyncerKey := map[string]string{} permissions := permissionsMock{allowAllBidders: true, passGeo: true, passID: true} metrics := metrics.MetricsEngineMock{} bidderRequests, _, errs := cleanOpenRTBRequests(context.Background(), auctionReq, extRequest, bidderToSyncerKey, &permissions, &metrics, gdpr.SignalNo, config.Privacy{}, nil) assert.Nil(t, errs) assert.Len(t, bidderRequests, 2, "Bid request count is not 2") bidRequestSourceExts := map[openrtb_ext.BidderName]json.RawMessage{} for _, bidderRequest := range bidderRequests{ bidRequestSourceExts[bidderRequest.BidderName] = bidderRequest.BidRequest.Source.Ext } appnexusPrebidSchainsSchain := json.RawMessage(`{"schain":{"complete":1,"nodes":[{"asi":"directseller1.com","sid":"00001","rid":"BidRequest1","hp":1}],"ver":"1.0"}}`) axonixPrebidSchainsSchain := json.RawMessage(`{"schain":{"complete":1,"nodes":[{"asi":"directseller2.com","sid":"00002","rid":"BidRequest2","hp":1}],"ver":"1.0"}}`) assert.Equal(t, appnexusPrebidSchainsSchain, bidRequestSourceExts["appnexus"], "Incorrect appnexus bid request schain in source.ext") assert.Equal(t, axonixPrebidSchainsSchain, bidRequestSourceExts["axonix"], "Incorrect axonix bid request schain in source.ext") }

I stole some of this setup from TestCleanOpenRTBRequestsSChain.

@bsardo I don't have a strong opinion on that, and I frankly don't know the code well enough to make a good decision :) I do however think a unit tests like the one I wrote is more to the point perhaps. But I don't mind at all replacing my test with the one you proposed if you think that is the better choice!

I tried the test @bsardo proposes above in my local machine and it works as it is. @bjorn-lw can we add it as part of your PR?

@guscarreon I tested it as well, it worked right away :) I can add it to the PR.

@guscarreon Github says I dismissed your review. I guess my new commit caused that..

guscarreon

Reapproved @bjorn-lw . Thank you for addressing our feedback

bsardo

LGTM

bidRequest.Source.Ext wasn't cloned in prepareSource

c878098

mansinahar assigned bsardo and guscarreon Nov 3, 2021

mansinahar requested review from bsardo and guscarreon November 3, 2021 17:34

guscarreon reviewed Nov 4, 2021

View reviewed changes

bjorn-lw added 2 commits November 4, 2021 17:35

Test fix

4eb11e5

Merge branch 'master' of https://github.com/prebid/prebid-server

a831f99

guscarreon reviewed Nov 5, 2021

View reviewed changes

Extended schain copied test

3d33426

guscarreon previously approved these changes Nov 11, 2021

View reviewed changes

bsardo reviewed Nov 15, 2021

View reviewed changes

Additional bidder schain test on higher level

46e5b47

bjorn-lw dismissed guscarreon’s stale review via 46e5b47 November 15, 2021 21:45

bjorn-lw requested review from guscarreon and bsardo November 15, 2021 22:03

guscarreon approved these changes Nov 15, 2021

View reviewed changes

bsardo approved these changes Nov 16, 2021

View reviewed changes

bsardo merged commit 36e6b40 into prebid:master Nov 16, 2021

mansinahar pushed a commit to mansinahar/prebid-server that referenced this pull request Jan 18, 2022

bidRequest.Source.Ext wasn't cloned in prepareSource (prebid#2070)

214d70c

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

bidRequest.Source.Ext wasn't cloned in prepareSource (prebid#2070)

9070ddd

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

bidRequest.Source.Ext wasn't cloned in prepareSource (prebid#2070)

015ad58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bidRequest.Source.Ext wasn't cloned in prepareSource #2070

bidRequest.Source.Ext wasn't cloned in prepareSource #2070

bjorn-lw commented Nov 3, 2021 •

edited

guscarreon Nov 4, 2021

bjorn-lw Nov 4, 2021 •

edited

bjorn-lw Nov 4, 2021 •

edited

guscarreon Nov 4, 2021

bjorn-lw Nov 4, 2021

bjorn-lw Nov 4, 2021

guscarreon left a comment

guscarreon Nov 5, 2021

bjorn-lw Nov 8, 2021

guscarreon left a comment

bjorn-lw commented Nov 14, 2021

bsardo Nov 15, 2021 •

edited

bjorn-lw Nov 15, 2021

guscarreon Nov 15, 2021

bjorn-lw Nov 15, 2021

bjorn-lw Nov 15, 2021 •

edited

guscarreon left a comment

bsardo left a comment

bidRequest.Source.Ext wasn't cloned in prepareSource #2070

bidRequest.Source.Ext wasn't cloned in prepareSource #2070

Conversation

bjorn-lw commented Nov 3, 2021 • edited

Choose a reason for hiding this comment

bjorn-lw Nov 4, 2021 • edited

Choose a reason for hiding this comment

bjorn-lw Nov 4, 2021 • edited

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

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

bjorn-lw commented Nov 14, 2021

bsardo Nov 15, 2021 • edited

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

bjorn-lw Nov 15, 2021 • edited

Choose a reason for hiding this comment

guscarreon left a comment

Choose a reason for hiding this comment

bsardo left a comment

Choose a reason for hiding this comment

bjorn-lw commented Nov 3, 2021 •

edited

bjorn-lw Nov 4, 2021 •

edited

bjorn-lw Nov 4, 2021 •

edited

bsardo Nov 15, 2021 •

edited

bjorn-lw Nov 15, 2021 •

edited