Pass through unknown imp.ext values to adapters #3878

justadreamer · 2024-08-22T11:01:13Z

Addresses "Stop hard-failing on unknown imp.ext values" #3735
The behavior is as described in the issue:

If imp.ext.prebid.bidder exists, then do nothing. The requestor has moved to the new syntax where bidders are in imp.ext.prebid and there's no need to check imp.ext anymore. Let it through.
If imp.ext.prebid.bidder doesn't exist, then it's ok to warn about unknown values, but the request should otherwise proceed.

In any case imp.ext.foo will be let through, in the second case with a warning.

NOTE: with "test":1 flag present in the request payload - the debug.resolvedrequest will contain the passed imp.ext.foo, however many bidder adapters do their own sanitization of imp.ext.* fields and as a result many will not send it to their servers. The example of one that transparently passes imp.ext fields was aax - so can be used for testing to see what actual bidder sends.

osazos · 2024-11-12T15:05:51Z

Hi, as the issue addressed here seems quite impacting, is there a reason to keep this PR without activity since 2 months?

shahinrahbariasl · 2024-11-14T20:33:18Z

Hi, as the issue addressed here seems quite impacting, is there a reason to keep this PR without activity since 2 months?

Agreed, this is particularly pressing as Privacy Sandbox support for the latest PBS Go versions is currently affected by this unresolved issue.

exchange/utils.go

ortb/request_validator.go

SyntaxNode · 2024-11-15T22:37:42Z

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

@@ -22,5 +22,5 @@
    }
  },
  "expectedReturnCode": 400,
-  "expectedErrorMessage": "Invalid request: request.imp[0].ext.prebid.bidder contains unknown bidder: noBidderShouldEverHaveThisName. Did you forget an alias in request.ext.prebid.aliases?\n"
+  "expectedErrorMessage": "Invalid request: request.imp[0].ext contains unknown bidder: 'noBidderShouldEverHaveThisName', ignoring"


Interesting. Since the bidder name check is just a warning, this should fail because there are no bidders. Why is there not an error message with lack of bidders found?

the test uses assert.Contains() checking that expectedErrorMessage is within actualErrorMessage.
actualErrorMessage contained 2 error messages (this one was added as errortypes.Warning) and one of them was added as a proper error: Invalid request: request.imp[0].ext.prebid.bidder must contain at least one bidder - so request failed with this one. I've resurrected proper expected error message in this test, as the request indeed fails because of no bidders

SyntaxNode · 2024-11-15T22:39:16Z

...mple-requests/valid-whole/supplementary/req-ext-bidder-params-backward-compatible-merge.json

+                  "profile": 1234
+                }
+              }
+            }


Why is this test affected?

This test was affected because it had an empty imp.ext.prebid.bidder object defined in the request, and all the bidders were specified outside of it. According to the new logic if there is an imp.ext.prebid.bidder object (even if it is empty one) - we must consider that the publisher has switched to the new way of specifying bidders and ignore the bidders in imp.ext - treat them as arbitrary fields and pass along as is. Thus we had 3 options:

modify the test fixture and remove an empty prebid.bidder: {}

modify the test fixture and promote the bidders from imp.ext into imp.ext.prebid.bidder

Modify the condition of promotion in request_validator.go: instead of promoting only when prebid.Bidder==nil here:

if prebid.Bidder == nil { prebid.Bidder = make(map[string]json.RawMessage) bidderPromote = true }

also promote in case it is empty: if prebid.Bidder == nil || len(prebid.Bidder) == 0
but this would disrupt the other tests logic.

So we took option 2. I think route 1 is less invasive, so I am adding a lighter modification to this test fixture - where it will only remove an empty prebid.bidder: {} object.

SyntaxNode · 2024-11-15T22:44:02Z

exchange/utils_test.go

-				"skadn":     json.RawMessage(`"anySKAdNetwork"`),
-				"gpid":      json.RawMessage(`"anyGPID"`),
-				"tid":       json.RawMessage(`"anyTID"`),
+				"unknownBidder": json.RawMessage(`"anyUnknownBidderValues"`),


Nitpick: unknownBidder doesn't seem right for these test cases, as it's now interpreted as just an unknown value and is not checked for as a bidder.

changed to arbitraryKey: arbitraryValue

SyntaxNode · 2024-11-15T22:47:22Z

ortb/request_validator_test.go

@@ -140,6 +154,12 @@ func TestValidateImpExt(t *testing.T) {
 					expectedImpExt: `{"prebid":{"bidder":{"appnexus":{"placement_id":555}}}}`,
 					expectedErrs:   []error{},
 				},
+				{
+					description:    "Valid bidder root ext + Empty Prebid Bidder",


Please add a comment to explain the valid bidder is ignored because prebid.bidder is present.

instead of promoting bidders from imp.ext to imp.ext.prebid.bidder, just remove empty imp.ext.prebid.bidder

justadreamer · 2024-11-21T13:33:36Z

Hi @SyntaxNode the review comments above were addressed.

bsardo

This is looking good; I have just a couple of minor comments. Just a reminder, please push up new commits going forward instead of rebasing and force pushing to make delta reviews easier 😄

bsardo · 2024-11-21T20:28:21Z

ortb/request_validator.go

 	// promote imp[].ext.BIDDER to newer imp[].ext.prebid.bidder.BIDDER location, with the later taking precedence
-	for k, v := range ext {
-		if openrtb_ext.IsPotentialBidder(k) {
-			if _, exists := prebid.Bidder[k]; !exists {
-				prebid.Bidder[k] = v
-				prebidModified = true
+	if bidderPromote {
+		for k, v := range ext {
+			if openrtb_ext.IsPotentialBidder(k) {
+				if _, exists := prebid.Bidder[k]; !exists {
+					prebid.Bidder[k] = v
+					prebidModified = true
+				}
+				delete(ext, k)
+				extModified = true
 			}
-			delete(ext, k)
-			extModified = true
 		}
 	}


@SyntaxNode with this change we will now only attempt to promote from imp[].ext to imp[].ext.bidder if imp[].ext.bidder does not exist. This is what is called out in the issue and what PBS-Java does. Just confirming that we are ok with that approach now.

One more option would be to allow for an empty imp.ext.prebid.bidder object i.e. {} - and promote in that case too, but Perhaps helps in case someone was unintentionally adding this object into requests, but not filling it, but I don't think there are clients that do this...

I vote to keep this as-is.

This is what is called out in the issue and what PBS-Java does. Just confirming that we are ok with that approach now.

Yes. This is the design of the feature and the next phase of deprecating imp[].ext for bidder parameters. It should've never been designed this way, but here we are.

but I don't think there are clients that do this...

Right. Let's not code for unknown use cases.

ortb/request_validator.go

bsardo · 2024-11-21T21:05:59Z

endpoints/openrtb2/sample-requests/invalid-whole/unknown-bidder.json

The way you've modified this test it is now a valid test case instead of an invalid test case and is in the wrong directory. How about doing the following instead:

Rename this test to imp-ext-prebid-bidder-empty.json and modify this test case to result in a 400 status code with imp.ext.bidder being empty

Rename endpoints/openrtb2/sample-requests/valid-whole/supplementary/req-ext-bidder-params-backward-compatible-merge.json to endpoints/openrtb2/sample-requests/valid-whole/supplementary/req-ext-bidder-params-promotion.json and include a field that is not a bidder ensuring the warning is reported in addition to the known bidders being promoted.

This makes sense, sorry did not pay attention to the semantics expressed by directory names.

done

done, but now that I have added this test for the warning, I see that the warning code we have introduced is 0, perhaps it should be either unknownWarningCode or some new warning code for this specific warning?

bsardo · 2024-11-22T02:18:16Z

exchange/utils_test.go

-				"skadn":     json.RawMessage(`"anySKAdNetwork"`),
-				"gpid":      json.RawMessage(`"anyGPID"`),
-				"tid":       json.RawMessage(`"anyTID"`),
+				"unknownBidder": json.RawMessage(`"anyUnknownBidderValues"`),


As mentioned in an earlier comment I think it makes more sense if all of these test cases in TestCreateSanitizedImpExt with key "unknownBidder" use "arbitraryField" instead.

this should be addressed now in utils_test.go

…mpty

…tible-merge.json->req-ext-bidder-params-promotion.json also add arbitraryObject that issues a warning that it is potentially an unknown bidder

hhhjort

LGTM

Co-authored-by: AntonYPost <[email protected]>

bsardo added the do not port label Sep 4, 2024

bsardo assigned bsardo and przemkaczmarek Sep 9, 2024

SyntaxNode reviewed Nov 15, 2024

View reviewed changes

AntonYPost and others added 2 commits November 20, 2024 15:36

Pass through unknown imp.ext values to adapters

834818f

fix easy code review comments

fa8fdf7

justadreamer force-pushed the unknown-imp-ext-values-support branch from b82c1e3 to fa8fdf7 Compare November 20, 2024 15:26

justadreamer added 2 commits November 21, 2024 14:18

more review comments addressed

18b6b7f

fix req-ext-bidder-params-backward-compatible-merge.json

427eff2

instead of promoting bidders from imp.ext to imp.ext.prebid.bidder, just remove empty imp.ext.prebid.bidder

bsardo mentioned this pull request Nov 21, 2024

Stop hard-failing on unknown imp.ext values #3735

Closed

bsardo reviewed Nov 22, 2024

View reviewed changes

justadreamer added 4 commits November 22, 2024 14:09

uitls_test: unknownBidder -> arbitraryField

ce51fda

invalid-whole/unknown-bidder -> invalid-whole/imp-ext-prebid-bidder-e…

73e7319

…mpty

rename valid-whole/supplementary/req-ext-bidder-params-backward-compa…

ef994ed

…tible-merge.json->req-ext-bidder-params-promotion.json also add arbitraryObject that issues a warning that it is potentially an unknown bidder

Merge branch 'master' into unknown-imp-ext-values-support

9bad784

bsardo approved these changes Nov 25, 2024

View reviewed changes

SyntaxNode mentioned this pull request Nov 25, 2024

Reserve IGS name #4073

Merged

hhhjort approved these changes Nov 26, 2024

View reviewed changes

bsardo merged commit 56b72c1 into prebid:master Nov 26, 2024
3 checks passed

scr-oath pushed a commit to scr-oath/prebid-server that referenced this pull request Dec 16, 2024

Pass through unknown imp.ext values to adapters (prebid#3878)

1e1cce0

Co-authored-by: AntonYPost <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass through unknown imp.ext values to adapters #3878

Pass through unknown imp.ext values to adapters #3878

justadreamer commented Aug 22, 2024

osazos commented Nov 12, 2024

shahinrahbariasl commented Nov 14, 2024 •

edited

Loading

SyntaxNode Nov 15, 2024

justadreamer Nov 20, 2024 •

edited

Loading

SyntaxNode Nov 15, 2024

justadreamer Nov 21, 2024

SyntaxNode Nov 15, 2024

justadreamer Nov 21, 2024

SyntaxNode Nov 15, 2024

justadreamer Nov 21, 2024

justadreamer commented Nov 21, 2024

bsardo left a comment

bsardo Nov 21, 2024

justadreamer Nov 22, 2024

SyntaxNode Nov 25, 2024 •

edited

Loading

bsardo Nov 21, 2024

justadreamer Nov 22, 2024

bsardo Nov 22, 2024

justadreamer Nov 22, 2024

hhhjort left a comment

Pass through unknown imp.ext values to adapters #3878

Pass through unknown imp.ext values to adapters #3878

Conversation

justadreamer commented Aug 22, 2024

osazos commented Nov 12, 2024

shahinrahbariasl commented Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

justadreamer Nov 20, 2024 • 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

justadreamer commented Nov 21, 2024

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

SyntaxNode Nov 25, 2024 • 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

hhhjort left a comment

Choose a reason for hiding this comment

shahinrahbariasl commented Nov 14, 2024 •

edited

Loading

justadreamer Nov 20, 2024 •

edited

Loading

SyntaxNode Nov 25, 2024 •

edited

Loading