Allow Host to Add SChain Node #2272

AlexBVolcy · 2022-06-13T16:19:44Z

This PR addresses this issue #1491

The goal is to give the host a way to optionally define an schain_node at the config level. In config.go, there's a new member of the Configuration struct, SChainNode of type *openrtb_ext.ExtRequestPrebidSChainSChainNode. And there's a corresponding SetDefault, which will allow the host to define their schain_node

If this new schain node is set, the node should be appended to the existing schain nodes that are tied to a specific bidder request at source.ext.schain. If source.ext.schain is not defined, it needs to be created, and the host node needs to be the first node of that path. The main implementation of this part of the issue is found in schainwriter.go

Unit tests were added to accompany the changes

End to end json tests were added as well in exchange_test.json

mansinahar · 2022-06-14T16:05:53Z

config/config.go

@@ -91,7 +91,8 @@ type Configuration struct {
 	//When true, new bid id will be generated in seatbid[].bid[].ext.prebid.bidid and used in event urls instead
 	GenerateBidID bool `mapstructure:"generate_bid_id"`
 	// GenerateRequestID overrides the bidrequest.id in an AMP Request or an App Stored Request with a generated UUID if set to true. The default is false.
-	GenerateRequestID bool `mapstructure:"generate_request_id"`
+	GenerateRequestID bool                                          `mapstructure:"generate_request_id"`
+	SChainNode        *openrtb_ext.ExtRequestPrebidSChainSChainNode `mapstructure:"schain_node"`


I think what you need here is to be this config option of type string that would read a json string and unmarshal it as ExtRequestPrebidSChainSChainNode and then append it to source.ext.schain.nodes. This is because the way Viper config works is that if you specify an object here, you'd have to specify a configuration for each of the object's field. For example, if you look at the Debug config, you'd have to have debug_timeout_notification and debug_override_token in your config file.

Also, please add tests for your config changes. One of the many advantages of doing that is that you probably would have caught this problem when writing and executing the unit tests :)

mansinahar · 2022-06-14T16:07:38Z

endpoints/openrtb2/amp_auction.go

@@ -204,6 +204,7 @@ func (deps *endpointDeps) AmpAuction(w http.ResponseWriter, r *http.Request, _ h
 		StoredAuctionResponses:     storedAuctionResponses,
 		StoredBidResponses:         storedBidResponses,
 		PubID:                      labels.PubID,
+		HostSChainNode:             deps.cfg.SChainNode,


I don't think AuctionRequest is the best place for passing in the host schain node because AuctionRequest is supposed to store all information about a specific request vs host schain node is going to be common across all requests.

I think the best way to do this would be to store the host schain config in the exchange object, similar to privacyConfig and then pass that information downstream to getAuctionBidderRequests

bsardo · 2022-06-17T17:27:01Z

config/config.go

+
+	// Where host can define an schain node
+	v.SetDefault("host_schain_node", "")


Nitpick: I think you can delete the comment as this code is pretty self explanatory.

Also can you move this SetDefault somewhere up above the env var support and migrate function calls on lines 1149-1160? If you notice the comments on lines 1162-1164, there's a specific reason a handful of SetDefault calls are made after the migrate logic; the rest of the SetDefault calls are before it. I'd tend to put this just after v.SetDefault("host_cookie.max_cookie_size_bytes", 0) but I'm good as long as it's somewhere above the env var and migrate logic.

bsardo · 2022-06-17T17:35:12Z

config/config.go

@@ -91,7 +91,8 @@ type Configuration struct {
 	//When true, new bid id will be generated in seatbid[].bid[].ext.prebid.bidid and used in event urls instead
 	GenerateBidID bool `mapstructure:"generate_bid_id"`
 	// GenerateRequestID overrides the bidrequest.id in an AMP Request or an App Stored Request with a generated UUID if set to true. The default is false.
-	GenerateRequestID bool `mapstructure:"generate_request_id"`
+	GenerateRequestID bool   `mapstructure:"generate_request_id"`
+	HostSChainNode    string `mapstructure:"host_schain_node"`


I see that the requirements define this as a string but it is really stringified JSON. Are there any complications with this when the value is read from the host config by Viper?

@mansinahar Would it be better if the type was a struct similar to ExtRequestPrebidSChainSChainNode defined in openrtb_ext.request.go? I know you advised defining it as a string up above, and that is what the spec says to do, but isn't that format making things more complicated for us downstream? We would then need to parse the string and perhaps validate it. I guess there might still be a complication there too though because the ext field is json.RawMessage...
As we always say, we don't have to follow what PBS-Java does so does it make more sense for this to be a struct?

bsardo · 2022-06-17T17:46:08Z

schain/schainwriter.go

@@ -33,16 +33,16 @@ type SChainWriter struct {
 // Write selects an schain from the multi-schain ORTB 2.5 location (req.ext.prebid.schains) for the specified bidder
 // and copies it to the ORTB 2.5 location (req.source.ext). If no schain exists for the bidder in the multi-schain
 // location and no wildcard schain exists, the request is not modified.
-func (w SChainWriter) Write(req *openrtb2.BidRequest, bidder string) {
+func (w SChainWriter) Write(req *openrtb2.BidRequest, bidder string, hostSChainInfo string) error {


I suggest removing this new function parameter and instead passing hostSChainInfo into the NewSChainWriter function which instantiates a new writer object since this is config information that will not change throughout the writer object's lifecycle.

I agree with @bsardo's comment here. We should move the hostSchainInfo to the SchainWriter struct as that is going to be the same for each bidder request.

bsardo · 2022-06-17T17:51:43Z

schain/schainwriter.go

-	schain := openrtb_ext.ExtRequestPrebidSChain{
-		SChain: *selectedSChain,
+
+	schain := openrtb_ext.ExtRequestPrebidSChain{}
+
+	if selectedSChain != nil {
+		schain.SChain = *selectedSChain


After looking more closely, I see why we now have this nil check but I think we should work to eliminate it as I think it makes things a little more complicated than they need to be. How about we guarantee that selectedSChain is set (not nil) by the time we try to assign it to schain.SChain? Something like:

selectedSChain = &openrtb_ext.ExtRequestPrebidSChainSChain{} if bidderSChain != nil { selectedSChain = bidderSChain } else if wildCardSChain != nil { selectedSChain = wildCardSChain } schain := openrtb_ext.ExtRequestPrebidSChain{ SChain: *selectedSChain, }

This is a mix of the logic on lines 48-52 and this nil check logic but effectively eliminates the nil check.

bsardo · 2022-06-17T18:43:56Z

schain/schainwriter.go

+
+	if hostSChainInfo != "" {
+		var hostSChainNode *openrtb_ext.ExtRequestPrebidSChainSChainNode
+		if err := json.Unmarshal([]byte(hostSChainInfo), &hostSChainNode); err != nil {


I see we are unmarshalling the schain node because it was defined as a string. This leads me to believe it would be better if we defined the schain in the config as a struct. Alternatively we could allow the host to still define it as a string and unmarshal it into a struct at startup which would then be used here so we aren't performing an inefficient unmarshal on every request. I tend to lean though towards allowing the host to define it as a struct in their config. I think it results in leaner config code and is easier for the host to write. @mansinahar thoughts?

Just to keep this updated, we talked during team time a couple days ago, and the type of the host schain node has now been reverted from string to be type *openrtb_ext.ExtRequestPrebidSChainSChainNode

bsardo · 2022-06-17T18:46:55Z

exchange/utils.go

+		if err != nil {
+			return nil, []error{err}
+		}


I'm pretty sure we intentionally left out this error handling because we didn't want invalid schains killing the auction. I'll need to check on that.

bsardo · 2022-06-17T18:56:56Z

schain/schainwriter.go

 	}
+
+	if hostSChainInfo != "" {


For the scenario where the lone schain node is the host defined node, we set the schain complete field to 0, which is the default value for that field when an instance of openrtb_ext.ExtRequestPrebidSChain is created. The ver field is set to "" by default but I believe that is required as you can see here OpenRTB Object: SupplyChain. The question is what should it be?

After further research, it appears that ver should be set to "1.0" which is the latest and sole version of sellers.json spec which is linked to from here. The change log on sellers.json shows 1.0.

mansinahar · 2022-06-23T13:52:59Z

config/config.go

@@ -773,6 +774,7 @@ func SetupViper(v *viper.Viper, filename string) {
 	v.SetDefault("host_cookie.value", "")
 	v.SetDefault("host_cookie.ttl_days", 90)
 	v.SetDefault("host_cookie.max_cookie_size_bytes", 0)
+	v.SetDefault("host_schain_node", nil)


Please add assertions for the default and full config tests in the config_test.go file

mansinahar · 2022-06-23T14:21:38Z

schain/schainwriter.go

 	}

+	selectedSChain = &openrtb_ext.ExtRequestPrebidSChainSChain{}


Why not just set the default version here when initializing this struct?

mansinahar · 2022-06-23T14:27:24Z

schain/schainwriter.go

@@ -33,37 +33,49 @@ type SChainWriter struct {
 // Write selects an schain from the multi-schain ORTB 2.5 location (req.ext.prebid.schains) for the specified bidder
 // and copies it to the ORTB 2.5 location (req.source.ext). If no schain exists for the bidder in the multi-schain
 // location and no wildcard schain exists, the request is not modified.
-func (w SChainWriter) Write(req *openrtb2.BidRequest, bidder string) {
+func (w SChainWriter) Write(req *openrtb2.BidRequest, bidder string, hostSChainNode *openrtb_ext.ExtRequestPrebidSChainSChainNode) error {


Why add error as a return parameter here? There doesn't seem to be any case where this function will return an error

You're right, there shouldn't be any error handling in this function

bsardo

This is looking good -- just a couple of minor comments.

bsardo · 2022-06-28T02:04:31Z

exchange/exchange_test.go

+	schainFilename := strings.Contains(filename, "schain")
+	hostSChainNode := &openrtb_ext.ExtRequestPrebidSChainSChainNode{}
+
+	if schainFilename {
+		hostSChainNode = &openrtb_ext.ExtRequestPrebidSChainSChainNode{
+			ASI: "pbshostcompany.com", SID: "00001", RID: "BidRequest", HP: 1,
+		}
+	} else {
+		hostSChainNode = nil
+	}
+


I see this is working off of the filename and I'm sure it works. However, I'd like to propose a different approach that is more explicit and in line with how other JSON-based exchange tests work when something needs to be configured for a handful of tests to force PBS down a particular code path.

Perhaps take a look at the struct exchangeSpec and how a field like GDPREnabled is used. This struct field maps to the JSON test field gdpr_enabled. In runSpec we pass this value into newExchangeForTests which we then use to set gdprDefaultValue on the returned exchange. You could do something similar by adding a bool field named something like host_schain_node to exchangeSpec which ultimately ends up being used here in newExchangeForTests to set hostSChainNode.

bsardo · 2022-06-28T02:07:10Z

exchange/exchangetest/schain-also-in-request.json

@@ -0,0 +1,93 @@
+{


Nitpick: can we rename this to something like schain-host-and-request.json so it isn't implied that this test also verifies the host schain?

bsardo · 2022-06-28T03:03:51Z

schain/schainwriter_test.go

+		{
+			description: "Schain in request, host schain defined, source.ext for bidder request should update with appended host schain",
+			giveRequest: openrtb2.BidRequest{
+				Ext:    json.RawMessage(`{"prebid":{"schains":[{"bidders":["testbidder"],"schain":{"complete":1,"nodes":[{"asi":"appnexus.com","sid":"1792","rid":"6a586290-ae54-44e9-8985-d60821f7bb07","hp":1}],"ver":"1.0"}}]}}`),
+				Source: nil,
+			},
+			giveBidder: "testbidder",
+			giveHostSChain: &openrtb_ext.ExtRequestPrebidSChainSChainNode{
+				ASI: "pbshostcompany.com", SID: "00001", RID: "BidRequest", HP: 1,
+			},
+			wantRequest: openrtb2.BidRequest{
+				Ext: json.RawMessage(`{"prebid":{"schains":[{"bidders":["testbidder"],"schain":{"complete":1,"nodes":[{"asi":"appnexus.com","sid":"1792","rid":"6a586290-ae54-44e9-8985-d60821f7bb07","hp":1}],"ver":"1.0"}}]}}`),
+				Source: &openrtb2.Source{
+					Ext: json.RawMessage(`{"schain":{"complete":1,"nodes":[{"asi":"appnexus.com","sid":"1792","rid":"6a586290-ae54-44e9-8985-d60821f7bb07","hp":1},{"asi":"pbshostcompany.com","sid":"00001","rid":"BidRequest","hp":1}],"ver":"1.0"}}`),
+				},
+			},
+		},
+		{
+			description: "No Schain in request, host schain defined, source.ext for bidder request should have just the host schain",
+			giveRequest: openrtb2.BidRequest{
+				Ext:    nil,
+				Source: nil,
+			},
+			giveBidder: "testbidder",
+			giveHostSChain: &openrtb_ext.ExtRequestPrebidSChainSChainNode{
+				ASI: "pbshostcompany.com", SID: "00001", RID: "BidRequest", HP: 1,
+			},
+			wantRequest: openrtb2.BidRequest{
+				Ext: nil,
+				Source: &openrtb2.Source{
+					Ext: json.RawMessage(`{"schain":{"complete":0,"nodes":[{"asi":"pbshostcompany.com","sid":"00001","rid":"BidRequest","hp":1}],"ver":"1.0"}}`),
+				},
+			},
+		},


Nitpick: to improve readability and continue with the style of this test, I'd suggest defining a couple of schain node constants at the top of this test like so:

const hostNode string = `{"asi":"pbshostcompany.com","sid":"00001","rid":"BidRequest","hp":1}` const seller1Node string = `{"asi":"directseller1.com","sid":"00001","rid":"BidRequest1","hp":1}`

and then using them here so we don't have long inline JSON strings that are hard to read and digest for someone unfamiliar with the logic:

@@ -157,7 +160,7 @@ func TestSChainWriter(t *testing.T) { { description: "Schain in request, host schain defined, source.ext for bidder request should update with appended host schain", giveRequest: openrtb2.BidRequest{ - Ext: json.RawMessage(`{"prebid":{"schains":[{"bidders":["testbidder"],"schain":{"complete":1,"nodes":[{"asi":"appnexus.com","sid":"1792","rid":"6a586290-ae54-44e9-8985-d60821f7bb07","hp":1}],"ver":"1.0"}}]}}`), + Ext: json.RawMessage(`{"prebid":{"schains":[{"bidders":["testbidder"],"schain":{"complete":1,"nodes":[` + seller1Node + `],"ver":"1.0"}}]}}`), Source: nil, }, giveBidder: "testbidder", @@ -165,9 +168,9 @@ func TestSChainWriter(t *testing.T) { ASI: "pbshostcompany.com", SID: "00001", RID: "BidRequest", HP: 1, }, wantRequest: openrtb2.BidRequest{ - Ext: json.RawMessage(`{"prebid":{"schains":[{"bidders":["testbidder"],"schain":{"complete":1,"nodes":[{"asi":"appnexus.com","sid":"1792","rid":"6a586290-ae54-44e9-8985-d60821f7bb07","hp":1}],"ver":"1.0"}}]}}`), + Ext: json.RawMessage(`{"prebid":{"schains":[{"bidders":["testbidder"],"schain":{"complete":1,"nodes":[` + seller1Node + `],"ver":"1.0"}}]}}`), Source: &openrtb2.Source{ - Ext: json.RawMessage(`{"schain":{"complete":1,"nodes":[{"asi":"appnexus.com","sid":"1792","rid":"6a586290-ae54-44e9-8985-d60821f7bb07","hp":1},{"asi":"pbshostcompany.com","sid":"00001","rid":"BidRequest","hp":1}],"ver":"1.0"}}`), + Ext: json.RawMessage(`{"schain":{"complete":1,"nodes":[` + seller1Node +`,` + hostNode + `],"ver":"1.0"}}`), }, }, }, @@ -184,7 +187,7 @@ func TestSChainWriter(t *testing.T) { wantRequest: openrtb2.BidRequest{ Ext: nil, Source: &openrtb2.Source{ - Ext: json.RawMessage(`{"schain":{"complete":0,"nodes":[{"asi":"pbshostcompany.com","sid":"00001","rid":"BidRequest","hp":1}],"ver":"1.0"}}`), + Ext: json.RawMessage(`{"schain":{"complete":0,"nodes":[` + hostNode + `],"ver":"1.0"}}`), }, }, }

bsardo

LGTM

SChain Code

3d4e23e

mansinahar requested review from bsardo and mansinahar June 13, 2022 17:49

mansinahar assigned bsardo and mansinahar Jun 13, 2022

mansinahar reviewed Jun 14, 2022

View reviewed changes

AlexBVolcy added 2 commits June 15, 2022 15:21

Change schain config type, address all feedback

c35a382

Remove unused code line

e8a36fe

bsardo reviewed Jun 17, 2022

View reviewed changes

AlexBVolcy added 2 commits June 20, 2022 09:20

SChain version handling, address some comments

ca0d421

Change type of host schain node

817f275

mansinahar reviewed Jun 23, 2022

View reviewed changes

AlexBVolcy added 2 commits June 23, 2022 13:41

End to end json tests

c0e4016

Address all comments

2615015

bsardo reviewed Jun 28, 2022

View reviewed changes

Tweaked tests based on feedback

4a81cd7

bsardo approved these changes Jun 28, 2022

View reviewed changes

mansinahar approved these changes Jul 5, 2022

View reviewed changes

mansinahar merged commit f1002e5 into prebid:master Jul 5, 2022

nayan9229 pushed a commit to infytvcode/prebid-server-go that referenced this pull request Sep 5, 2022

Allow Host to Add SChain Node (prebid#2272)

59e99bc

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

Allow Host to Add SChain Node (prebid#2272)

70b9fdf

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

Allow Host to Add SChain Node (prebid#2272)

d1cf3fd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Host to Add SChain Node #2272

Allow Host to Add SChain Node #2272

AlexBVolcy commented Jun 13, 2022 •

edited

Loading

mansinahar Jun 14, 2022

mansinahar Jun 14, 2022

bsardo Jun 17, 2022

bsardo Jun 17, 2022

bsardo Jun 17, 2022

mansinahar Jun 23, 2022

bsardo Jun 17, 2022

bsardo Jun 17, 2022

AlexBVolcy Jun 23, 2022

bsardo Jun 17, 2022

bsardo Jun 17, 2022

bsardo Jun 17, 2022 •

edited

Loading

mansinahar Jun 23, 2022

mansinahar Jun 23, 2022

AlexBVolcy Jun 23, 2022

mansinahar Jun 23, 2022

AlexBVolcy Jun 23, 2022

bsardo left a comment

bsardo Jun 28, 2022

bsardo Jun 28, 2022

bsardo Jun 28, 2022

bsardo left a comment


		// Where host can define an schain node
		v.SetDefault("host_schain_node", "")

		}

		selectedSChain = &openrtb_ext.ExtRequestPrebidSChainSChain{}

Allow Host to Add SChain Node #2272

Allow Host to Add SChain Node #2272

Conversation

AlexBVolcy commented Jun 13, 2022 • 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

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

bsardo Jun 17, 2022 • 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

Choose a reason for hiding this comment

bsardo left a 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

bsardo left a comment

Choose a reason for hiding this comment

AlexBVolcy commented Jun 13, 2022 •

edited

Loading

bsardo Jun 17, 2022 •

edited

Loading