Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 5 commits into from
Nov 16, 2021
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c878098
bidRequest.Source.Ext wasn't cloned in prepareSource
bjorn-lw Nov 3, 2021
4eb11e5
Test fix
bjorn-lw Nov 4, 2021
a831f99
Merge branch 'master' of https://github.com/prebid/prebid-server
bjorn-lw Nov 4, 2021
3d33426
Extended schain copied test
bjorn-lw Nov 8, 2021
46e5b47
Additional bidder schain test on higher level
bjorn-lw Nov 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions exchange/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,9 @@ func prepareSource(req *openrtb2.BidRequest, bidder string, sChainsByBidder map[
// set source
if req.Source == nil {
req.Source = &openrtb2.Source{}
} else {
sourceCopy := *req.Source
req.Source = &sourceCopy
}
schain := openrtb_ext.ExtRequestPrebidSChain{
SChain: *selectedSChain,
Expand Down
42 changes: 41 additions & 1 deletion exchange/utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2609,8 +2609,48 @@ func TestBuildXPrebidHeader(t *testing.T) {
}
}

func TestApplyFPD(t *testing.T) {
func TestSourceExtSChainCopied(t *testing.T) {
Copy link
Contributor

@bsardo bsardo Nov 15, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@bjorn-lw bjorn-lw Nov 15, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

bidRequest := newBidRequest(t)

bidderSchains := map[string]*openrtb_ext.ExtRequestPrebidSChainSChain{
"bidder1": {
Ver: "1.0",
Complete: 1,
Nodes: []*openrtb_ext.ExtRequestPrebidSChainSChainNode{
{
ASI: "bidder1.com",
SID: "0001",
HP: 1,
},
},
},
"bidder2": {
Ver: "1.0",
Complete: 1,
Nodes: []*openrtb_ext.ExtRequestPrebidSChainSChainNode{
{
ASI: "bidder2.com",
SID: "0002",
HP: 1,
},
},
},
}

copy1 := *bidRequest
originalTid := copy1.Source.TID
prepareSource(&copy1, "bidder1", bidderSchains)
copy2 := *bidRequest
copy2.Source.TID = "new TID"
prepareSource(&copy2, "bidder2", bidderSchains)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@bjorn-lw bjorn-lw Nov 4, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@bjorn-lw bjorn-lw Nov 4, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guscarreon please check latest commit


assert.Equal(t, json.RawMessage(`{"schain":{"complete":1,"nodes":[{"asi":"bidder1.com","sid":"0001","hp":1}],"ver":"1.0"}}`), copy1.Source.Ext, "First schain was overwritten or not set")
assert.Equal(t, originalTid, copy1.Source.TID, "Original TID was overwritten")
assert.Equal(t, json.RawMessage(`{"schain":{"complete":1,"nodes":[{"asi":"bidder2.com","sid":"0002","hp":1}],"ver":"1.0"}}`), copy2.Source.Ext, "Second schain was overwritten or not set")
assert.Equal(t, "new TID", copy2.Source.TID, "New TID was not set")
}

func TestApplyFPD(t *testing.T) {
fpdBidderTest := map[openrtb_ext.BidderName]*firstpartydata.ResolvedFirstPartyData{}
bidderTest := openrtb_ext.BidderName("test")
fpdBidderTest[bidderTest] = &firstpartydata.ResolvedFirstPartyData{Site: nil, App: nil, User: nil}
Expand Down