Add GPID Reserved Bidder Name #2107

SyntaxNode · 2021-12-09T08:36:30Z

Implements #2088

I needed to refactor existing code for handling bidderparams due to a parsing problem it introduced. That code failed if any field in imp.ext is not an embedded object type, which is a problem since imp.ext.gpid is a string. Since this is code is upstream from validation (which makes sense as it could cause an invalid request to become valid) it should not "step on the toes" of the validator. If there is a problem with the request, we should let the validate method continue to handle validation issues.

SyntaxNode · 2021-12-09T08:36:56Z

adapters/openrtb_util.go

 	"github.com/mxmCherry/openrtb/v15/openrtb2"
 	"github.com/prebid/prebid-server/openrtb_ext"
 )

-func ExtractAdapterReqBidderParams(bidRequest *openrtb2.BidRequest) (map[string]json.RawMessage, error) {
+func ExtractReqExtBidderParamsMap(bidRequest *openrtb2.BidRequest) (map[string]json.RawMessage, error) {


Provided a more precise name to better align with the other method in this file.

SyntaxNode · 2021-12-09T08:37:35Z

adapters/openrtb_util_test.go

-func TestExtractAdapterReqBidderParams(t *testing.T) {
-	type args struct {
-		bidRequest *openrtb2.BidRequest
-	}


Refactors to remove the args object, since it only contains a single argument I don't see the need for a separate structure.

SyntaxNode · 2021-12-09T08:38:07Z

adapters/openrtb_util_test.go

+			name:            "nil req",
+			givenBidRequest: nil,
+			want:            nil,
+			wantErr:         errors.New("error bidRequest should not be nil"),


It's better to compare the actual error message rather than verify the count of errors.

SyntaxNode · 2021-12-09T08:38:33Z

endpoints/openrtb2/auction.go

+	}
+
+	if len(reqBidderParams) == 0 {
+		return nil
 	}


Optimization to avoid the expensive operations of the loop if there are no bidderparams to apply.

Good catch. Do you think checking err != nil and len(reqBidderParams) == 0 separately make for a better read? Or can we compact?

338 func mergeBidderParams(req *openrtb_ext.RequestWrapper) error { 339 reqBidderParams, err := adapters.ExtractReqExtBidderParamsEmbeddedMap(req.BidRequest) 340 - if err != nil { + if err != nil || len(reqBidderParams) == 0 { 341 return nil 342 } 343 - 344 - if len(reqBidderParams) == 0 { 345 - return nil 346 - } 347 endpoints/openrtb2/auction.go

I have no preference. I can combine them if you prefer.

that's fine. No need to hold this PR over this.

SyntaxNode · 2021-12-09T08:39:08Z

endpoints/openrtb2/auction.go

-		iExt, err := json.Marshal(impExt)
-		if err != nil {
-			return fmt.Errorf("error marshalling imp[].ext : %s", err.Error())
+		if impExtModified || impExtPrebidModified {


Slight optimization to avoid marshaling if no changes were made, such as the edge case when all values are more specifically defined in imp.ext.

SyntaxNode · 2021-12-09T08:41:02Z

endpoints/openrtb2/auction.go

 	case openrtb_ext.BidderReservedPrebid:
 		return false
 	case openrtb_ext.BidderReservedSKAdN:
 		return false
-	case openrtb_ext.BidderReservedBidder:
-		return false


The bidderparams feature does not make special use of "bidder", so I don't see why it needed to add this reserved bidder name. Perhaps it did in an earlier version? Removed.

SyntaxNode · 2021-12-09T08:41:31Z

endpoints/openrtb2/auction_test.go

@@ -1146,6 +1146,301 @@ func TestStoredRequests(t *testing.T) {
 	}
 }

+func TestMergeBidderParams(t *testing.T) {


Added extensive unit tests for the merge methods. I left the json based integration tests alone.

SyntaxNode · 2021-12-09T08:41:57Z

endpoints/openrtb2/sample-requests/invalid-whole/cache-nothing.json

@@ -27,5 +27,5 @@
    }
  },
  "expectedReturnCode": 400,
-  "expectedErrorMessage": "Invalid request: error decoding Request.ext : request.ext.prebid.cache requires one of the \"bids\" or \"vastxml\" properties\n"
+  "expectedErrorMessage": "Invalid request: request.ext is invalid: request.ext.prebid.cache requires one of the \"bids\" or \"vastxml\" properties\n"


Reverted. The bidderparams feature should not alter the validation flow.

SyntaxNode · 2021-12-09T08:42:01Z

endpoints/openrtb2/sample-requests/invalid-whole/imp-ext-invalid-params.json

@@ -20,5 +20,5 @@
    }
  },
  "expectedReturnCode": 400,
-  "expectedErrorMessage": "Invalid request: json: cannot unmarshal string into Go value of type map[string]json.RawMessage"
+  "expectedErrorMessage": "Invalid request: request.imp[0].ext.appnexus failed validation.\n(root): Invalid type. Expected: object, given: string\n"


Reverted. The bidderparams feature should not alter the validation flow.

SyntaxNode · 2021-12-09T08:42:27Z

openrtb_ext/bidders_test.go

@@ -104,15 +104,15 @@ func TestIsBidderNameReserved(t *testing.T) {
 		{"general", true},
 		{"gEnErAl", true},
 		{"GENERAL", true},
-		{"skadn", true},
-		{"skADN", true},
-		{"SKADN", true},


Ordered alphabetically.

SyntaxNode · 2021-12-09T08:51:12Z

endpoints/openrtb2/auction_test.go

-			},
-			expectedImpExt: getObject(t, "req-ext-bidder-params-merge-reserved-keyword.json", "expectedImpExt"),
-			expectedReqExt: getObject(t, "req-ext-bidder-params-merge-reserved-keyword.json", "expectedReqExt"),
-			errL:           0,


Removed. I don't understand the use case of a reserved bidder keyword and I didn't see mention of special handling in the issue. Please correct me if I missed something.

guscarreon

Left some questions below

guscarreon · 2021-12-13T18:19:06Z

endpoints/openrtb2/auction.go

+	}
+
+	if len(reqBidderParams) == 0 {
+		return nil
 	}


Good catch. Do you think checking err != nil and len(reqBidderParams) == 0 separately make for a better read? Or can we compact?

338 func mergeBidderParams(req *openrtb_ext.RequestWrapper) error { 339 reqBidderParams, err := adapters.ExtractReqExtBidderParamsEmbeddedMap(req.BidRequest) 340 - if err != nil { + if err != nil || len(reqBidderParams) == 0 { 341 return nil 342 } 343 - 344 - if len(reqBidderParams) == 0 { 345 - return nil 346 - } 347 endpoints/openrtb2/auction.go

guscarreon · 2021-12-13T23:16:03Z

endpoints/openrtb2/auction.go

+		if err := json.Unmarshal(bidderExt, &params); err != nil {
+			// bidder should be parsable, but we haven't validated yet so there may be another issue. ignore it here
+			// and let the validation later on catch it.
+			continue


Correct me if I'm wrong, but by unmarshaling to a map[string]json.RawMessage in line 357 inside mergeBidderParams(req *openrtb_ext.RequestWrapper) we are effectively moving that additional json.Unmarshal call down here, right? Are we doing this to filter out the malformed imp[].ext fields and salvage the good ones? If so, I think downstream the entire imp gets discarded anyways, even if only one of the entries is found to not pass validation:

1152 for bidder, ext := range bidderExts { 1153 if isBidderToValidate(bidder) { 1154 coreBidder := bidder 1155 if tmp, isAlias := aliases[bidder]; isAlias { 1156 coreBidder = tmp 1157 } 1158 if bidderName, isValid := deps.bidderMap[coreBidder]; isValid { 1159 if err := deps.paramsValidator.Validate(bidderName, ext); err != nil { 1160 /*DISCARDS IMP-->*/ return []error{fmt.Errorf("request.imp[%d].ext.%s failed validation.\n%v", impIndex, coreBidder, err)} 1161 } 1162 } else { 1163 if msg, isDisabled := deps.disabledBidders[bidder]; isDisabled { 1164 errL = append(errL, &errortypes.BidderTemporarilyDisabled{Message: msg}) 1165 disabledBidders = append(disabledBidders, bidder) 1166 } else { 1167 return []error{fmt.Errorf("request.imp[%d].ext contains unknown bidder: %s. Did you forget an alias in request.ext.prebid.aliases?", impIndex, bidder)} 1168 } 1169 } 1170 } else { 1171 otherExtElements++ 1172 } 1173 } endpoints/openrtb2/auction.go

We can verify this behavior if we slightly modify endpoints/openrtb2/sample-requests/invalid-whole/imp-ext-invalid-params.json to include a valid bidder ext. Notice how the test still passes, in other words, the entire imp[0] element gets discarded:

1 { 2 "description": "Bid request's sole imp element has invalid ext value", 3 "mockBidRequest": { 4 "id": "req-id", 5 "imp": [ 6 { 7 "id": "imp-id", 8 "audio": { 9 "mimes": [ 10 "video/mp4" 11 ] 12 }, 13 "ext": { + "aja": { + "asi": "test-asi" + }, 14 "appnexus": "invalidParams" 15 } 16 } 17 ], 18 "app": { 19 "id": "app_001" 20 } 21 }, 22 "expectedReturnCode": 400, 23 "expectedErrorMessage": "Invalid request: request.imp[0].ext.appnexus failed validation.\n(root): Invalid type. Expected: object, given: string\n" 24 } endpoints/openrtb2/sample-requests/invalid-whole/imp-ext-invalid-params.json

If, on the other hand, the test is modified so the "aja" entry fails before the "appnexus" one does, the test does not pass because the error message mentions aja and not appnexus:

1 { 2 "description": "Bid request's sole imp element has invalid ext value", 3 "mockBidRequest": { 4 "id": "req-id", 5 "imp": [ 6 { 7 "id": "imp-id", 8 "audio": { 9 "mimes": [ 10 "video/mp4" 11 ] 12 }, 13 "ext": { + "aja": "malformed", 14 "appnexus": "invalidParams" 15 } 16 } 17 ], 18 "app": { 19 "id": "app_001" 20 } 21 }, 22 "expectedReturnCode": 400, 23 "expectedErrorMessage": "Invalid request: request.imp[0].ext.appnexus failed validation.\n(root): Invalid type. Expected: object, given: string\n" 24 } endpoints/openrtb2/sample-requests/invalid-whole/imp-ext-invalid-params.json

Test failure message:

endpoints/openrtb2/auction_test.go|132| || Error Trace: auction_test.go:169 || auction_test.go:132 || Error: Should be true || Test: TestJsonSampleRequests || Messages: Actual: Invalid request: request.imp[0].ext.aja failed validation. || (root): Invalid type. Expected: object, given: string || || Expected: Invalid request: request.imp[0].ext.appnexus failed validation. || (root): Invalid type. Expected: object, given: string || . Filename: sample-requests/invalid-whole/imp-ext-invalid-params.json

If at the end we are going to fail fast anyways, should go back to unmarshaling the entire thing in mergeBidderParams(req *openrtb_ext.RequestWrapper)?

Are we doing this to filter out the malformed imp[].ext fields and salvage the good ones?

No. There is no validation intended or performed by the merge method. The map[string]map[string]json.RawMessage is not a valid data structure for imp.ext since it forces all fields to be an object or fails to parse. This PR adds support for imp.ext.gpid which is a string. That's the reason for the refactor.

should go back to unmarshaling the entire thing in mergeBidderParams(req *openrtb_ext.RequestWrapper)?

We cannot. If we do, then imp.ext.gpid will always cause a validation error in mergeBidderParams.

guscarreon · 2021-12-14T00:21:17Z

endpoints/openrtb2/auction.go

 		}

 		//merges bidder parameters passed at req.ext level with imp[].ext.prebid.bidder level.
-		err = addMissingReqExtParamsInImpExtPrebid(impExt, reqBidderParams)
+		impExtPrebidModified, err := addMissingReqExtParamsInImpExtPrebid(impExt, reqBidderParams)


Given that request.imp.ext.prebid.bidder.BIDDER takes precedence over request.imp.ext.BIDDER, does this become an "either-or" situation? If a given imp.ext comes with a prebid field, should we only call addMissingReqExtParamsInImpExtPrebid and not both addMissingReqExtParamsInImpExtPrebid and addMissingReqExtParamsInImpExt? If the imp.exts that come with a prebid field will ignore request.imp.ext.BIDDER values anyways, maybe it'd be a desirable optimization to not copy them to request.imp.ext.BIDDER at all.

1124 func (deps *endpointDeps) validateImpExt(imp *openrtb2.Imp, aliases map[string]string, impIndex int) []error { 1125 errL := []error{} . . 1134 1135 // Prefer bidder params from request.imp.ext.prebid.bidder.BIDDER over request.imp.ext.BIDDER 1136 // to avoid confusion beteween prebid specific adapter config and other ext protocols. 1137 if extPrebidJSON, ok := bidderExts[openrtb_ext.PrebidExtKey]; ok { 1138 var extPrebid openrtb_ext.ExtImpPrebid 1139 if err := json.Unmarshal(extPrebidJSON, &extPrebid); err == nil && extPrebid.Bidder != nil { 1140 for bidder, ext := range extPrebid.Bidder { 1141 if ext == nil { 1142 continue 1143 } 1144 bidderExts[bidder] = ext 1145 } 1146 } 1147 } 1148 1149 /* Process all the bidder exts in the request */ endpoints/openrtb2/auction.go

If implemented, the change would make mergeBidderParams behave differently, and a test case like the one shown below would expect a different outcome:

1149 func TestMergeBidderParams(t *testing.T) { 1150 testCases := []struct { 1151 description string 1152 givenRequest openrtb2.BidRequest 1153 expectedRequestImps []openrtb2.Imp 1154 }{ 1155 *- 55 lines: {------------------------------------------------------------------------------- 1210 { 1211 - description: "One Imp - imp.ext + imp.ext.prebid.bidder Modified - Same Bidder", + description: "One Imp - modify imp.ext.prebid.bidder ONLY - Same Bidder", 1212 givenRequest: openrtb2.BidRequest{ 1213 Imp: []openrtb2.Imp{{ID: "1", Ext: json.RawMessage(`{"bidder1":{"a":1},"prebid":{"bidder":{"bidder1":{"a":"one"}}}}`)}}, 1214 Ext: json.RawMessage(`{"prebid":{"bidderparams":{"bidder1":{"b":2}}}}`), 1215 }, 1216 - expectedRequestImps: []openrtb2.Imp{{ID: "1", Ext: json.RawMessage(`{"bidder1":{"a":1,"b":2},"prebid":{"bidder":{"bidder1":{"a":"one","b":2}}}}`)}}, + expectedRequestImps: []openrtb2.Imp{{ID: "1", Ext: json.RawMessage(`{"bidder1":{"a":1},"prebid":{"bidder":{"bidder1":{"a":"one","b":2}}}}`)}}, 1217 }, endpoints/openrtb2/auction_test.go

Unless I'm missing something and those values are still needed in request.imp.ext.BIDDER. Thoughts?

Given that request.imp.ext.prebid.bidder.BIDDER takes precedence over request.imp.ext.BIDDER, does this become an "either-or" situation?

Yes. Later code will pick one or the other, but not both. Referencing the precedence logic here would increase it's complexity for IMHO minimal processing savings.

I'm not sure why it was designed this way, but separating the merging pf params from the decision on which params to use seems like a good isolation of responsibilities.

This is pre-existing behavior. The reason I didn't refactor that in this PR is that it's not required for imp.ext.gpid to function, which is the goal of this PR.

makes sense. It can be refactored separately

guscarreon

LGTM

guscarreon · 2021-12-14T17:28:46Z

endpoints/openrtb2/auction.go

+	}
+
+	if len(reqBidderParams) == 0 {
+		return nil
 	}


that's fine. No need to hold this PR over this.

guscarreon · 2021-12-14T17:30:52Z

endpoints/openrtb2/auction.go

 		}

 		//merges bidder parameters passed at req.ext level with imp[].ext.prebid.bidder level.
-		err = addMissingReqExtParamsInImpExtPrebid(impExt, reqBidderParams)
+		impExtPrebidModified, err := addMissingReqExtParamsInImpExtPrebid(impExt, reqBidderParams)


makes sense. It can be refactored separately

guscarreon · 2021-12-14T17:31:32Z

endpoints/openrtb2/auction.go

+		if err := json.Unmarshal(bidderExt, &params); err != nil {
+			// bidder should be parsable, but we haven't validated yet so there may be another issue. ignore it here
+			// and let the validation later on catch it.
+			continue


* '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) ...

SyntaxNode added 6 commits December 8, 2021 23:26

Add GPID Reserved Bidder Name

a7c935d

Simplified Test Structure

8f042f9

Fix Missing Error Handler

ceae767

Refactor

f78fbfb

Fix Implementation Of Bidder Params

e29a322

Remove Reserved Bidder Name + Passthrough GPID

686d6fb

SyntaxNode commented Dec 9, 2021

View reviewed changes

Removed Bidder Reserved Test

78b137e

SyntaxNode commented Dec 9, 2021

View reviewed changes

mansinahar requested review from mansinahar and guscarreon December 9, 2021 18:19

mansinahar assigned mansinahar and guscarreon Dec 9, 2021

guscarreon reviewed Dec 14, 2021

View reviewed changes

guscarreon previously approved these changes Dec 14, 2021

View reviewed changes

Updated Comment

b6d8b2a

SyntaxNode dismissed guscarreon’s stale review via b6d8b2a December 15, 2021 16:18

mansinahar approved these changes Dec 15, 2021

View reviewed changes

mansinahar merged commit a60dfe2 into prebid:master Dec 16, 2021

SyntaxNode deleted the gpid branch March 21, 2022 20:30

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

Add GPID Reserved Bidder Name (prebid#2107)

61ccbc4

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

Add GPID Reserved Bidder Name (prebid#2107)

0d71b11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GPID Reserved Bidder Name #2107

Add GPID Reserved Bidder Name #2107

SyntaxNode commented Dec 9, 2021

SyntaxNode Dec 9, 2021

SyntaxNode Dec 9, 2021

SyntaxNode Dec 9, 2021

SyntaxNode Dec 9, 2021

guscarreon Dec 13, 2021

SyntaxNode Dec 14, 2021

guscarreon Dec 14, 2021

SyntaxNode Dec 9, 2021

SyntaxNode Dec 9, 2021

SyntaxNode Dec 9, 2021

SyntaxNode Dec 9, 2021

SyntaxNode Dec 9, 2021

SyntaxNode Dec 9, 2021

SyntaxNode Dec 9, 2021

guscarreon left a comment

guscarreon Dec 13, 2021

guscarreon Dec 13, 2021

SyntaxNode Dec 14, 2021

guscarreon Dec 14, 2021

guscarreon Dec 14, 2021

SyntaxNode Dec 14, 2021

guscarreon Dec 14, 2021

guscarreon left a comment

guscarreon Dec 14, 2021

guscarreon Dec 14, 2021

guscarreon Dec 14, 2021

Add GPID Reserved Bidder Name #2107

Add GPID Reserved Bidder Name #2107

Conversation

SyntaxNode commented Dec 9, 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

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

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

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

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

Choose a reason for hiding this comment