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

Pass through unknown imp.ext values to adapters #3878

Merged
merged 8 commits into from
Nov 26, 2024
Merged

Pass through unknown imp.ext values to adapters #3878

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
834818f
Pass through unknown imp.ext values to adapters
AntonYPost Aug 1, 2024
fa8fdf7
fix easy code review comments
justadreamer Nov 20, 2024
18b6b7f
more review comments addressed
justadreamer Nov 21, 2024
427eff2
fix req-ext-bidder-params-backward-compatible-merge.json
justadreamer Nov 21, 2024
ce51fda
uitls_test: unknownBidder -> arbitraryField
justadreamer Nov 22, 2024
73e7319
invalid-whole/unknown-bidder -> invalid-whole/imp-ext-prebid-bidder-e…
justadreamer Nov 22, 2024
ef994ed
rename valid-whole/supplementary/req-ext-bidder-params-backward-compa…
justadreamer Nov 22, 2024
9bad784
Merge branch 'master' into unknown-imp-ext-values-support
justadreamer Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions endpoints/openrtb2/auction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4651,7 +4651,7 @@ func TestValidateStoredResp(t *testing.T) {
storedBidResponses: stored_responses.ImpBidderStoredResp{"Some-Imp-ID": {"appnexus": json.RawMessage(`{"test":true}`), "rubicon": json.RawMessage(`{"test":true}`)}},
},
{
description: "One imp with 2 stored bid responses and 1 bidders in imp.ext and 1 in imp.ext.prebid.bidder, expect validate request to throw no errors",
description: "One imp with 1 stored bid response and 1 ignored bidder in imp.ext and 1 included bidder in imp.ext.prebid.bidder, expect validate request to throw no errors",
givenRequestWrapper: &openrtb_ext.RequestWrapper{
BidRequest: &openrtb2.BidRequest{
ID: "Some-ID",
Expand All @@ -4678,7 +4678,7 @@ func TestValidateStoredResp(t *testing.T) {
},
expectedErrorList: []error{},
hasStoredAuctionResponses: false,
storedBidResponses: stored_responses.ImpBidderStoredResp{"Some-Imp-ID": {"appnexus": json.RawMessage(`{"test":true}`), "telaria": json.RawMessage(`{"test":true}`)}},
storedBidResponses: stored_responses.ImpBidderStoredResp{"Some-Imp-ID": {"telaria": json.RawMessage(`{"test":true}`)}},
},
{
description: "One imp with 2 stored bid responses and 1 bidders in imp.ext and 1 in imp.ext.prebid.bidder that is not defined in stored bid responses, expect validate request to throw an error",
Expand Down
2 changes: 1 addition & 1 deletion endpoints/openrtb2/sample-requests/invalid-whole/imp-ext-unknown-bidder.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.prebid.bidder must contain at least one bidder"
}
4 changes: 2 additions & 2 deletions endpoints/openrtb2/sample-requests/invalid-whole/unknown-bidder.json
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

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:

  1. 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
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

  1. done
  2. 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?

Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,6 @@
],
"tmax": 500
},
"expectedReturnCode": 400,
"expectedErrorMessage": "Invalid request"
"expectedReturnCode": 200,
"expectedErrorMessage": "request.imp[0].ext contains unknown bidder: 'unknownbidder', ignoring"
}
3 changes: 0 additions & 3 deletions ...e-requests/valid-whole/supplementary/req-ext-bidder-params-backward-compatible-merge.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,6 @@
"wrapper": {
"profile": 1234
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this test affected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

  1. modify the test fixture and remove an empty prebid.bidder: {}
  2. modify the test fixture and promote the bidders from imp.ext into imp.ext.prebid.bidder
  3. 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.

},
"prebid": {
"bidder": {}
}
}
}
Expand Down
17 changes: 6 additions & 11 deletions exchange/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,20 +709,15 @@ func mergeImpFPD(imp *openrtb2.Imp, fpd json.RawMessage, index int) error {
return nil
}

var allowedImpExtFields = map[string]interface{}{
openrtb_ext.AuctionEnvironmentKey: struct{}{},
openrtb_ext.FirstPartyDataExtKey: struct{}{},
openrtb_ext.FirstPartyDataContextExtKey: struct{}{},
openrtb_ext.GPIDKey: struct{}{},
openrtb_ext.SKAdNExtKey: struct{}{},
openrtb_ext.TIDKey: struct{}{},
}

var allowedImpExtPrebidFields = map[string]interface{}{
openrtb_ext.IsRewardedInventoryKey: struct{}{},
openrtb_ext.OptionsKey: struct{}{},
}

var deniedImpExtFields = map[string]interface{}{
openrtb_ext.PrebidExtKey: struct{}{},
}

func createSanitizedImpExt(impExt, impExtPrebid map[string]json.RawMessage) (map[string]json.RawMessage, error) {
sanitizedImpExt := make(map[string]json.RawMessage, 6)
sanitizedImpPrebidExt := make(map[string]json.RawMessage, 2)
Expand All @@ -744,8 +739,8 @@ func createSanitizedImpExt(impExt, impExtPrebid map[string]json.RawMessage) (map
}

// copy reserved imp[].ext fields known to not be bidder names
for k := range allowedImpExtFields {
if v, exists := impExt[k]; exists {
for k, v := range impExt {
if _, exists := deniedImpExtFields[k]; !exists {
sanitizedImpExt[k] = v
}
}
Expand Down
100 changes: 52 additions & 48 deletions exchange/utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,93 +607,97 @@ func TestCreateSanitizedImpExt(t *testing.T) {
{
description: "imp.ext",
givenImpExt: map[string]json.RawMessage{
"anyBidder": json.RawMessage(`"anyBidderValues"`),
"data": json.RawMessage(`"anyData"`),
"context": json.RawMessage(`"anyContext"`),
"skadn": json.RawMessage(`"anySKAdNetwork"`),
"gpid": json.RawMessage(`"anyGPID"`),
"tid": json.RawMessage(`"anyTID"`),
"arbitraryField": json.RawMessage(`"arbitraryValue"`),
"data": json.RawMessage(`"anyData"`),
"context": json.RawMessage(`"anyContext"`),
"skadn": json.RawMessage(`"anySKAdNetwork"`),
"gpid": json.RawMessage(`"anyGPID"`),
"tid": json.RawMessage(`"anyTID"`),
},
givenImpExtPrebid: map[string]json.RawMessage{},
expected: map[string]json.RawMessage{
"data": json.RawMessage(`"anyData"`),
"context": json.RawMessage(`"anyContext"`),
"skadn": json.RawMessage(`"anySKAdNetwork"`),
"gpid": json.RawMessage(`"anyGPID"`),
"tid": json.RawMessage(`"anyTID"`),
"arbitraryField": json.RawMessage(`"arbitraryValue"`),
"data": json.RawMessage(`"anyData"`),
"context": json.RawMessage(`"anyContext"`),
"skadn": json.RawMessage(`"anySKAdNetwork"`),
"gpid": json.RawMessage(`"anyGPID"`),
"tid": json.RawMessage(`"anyTID"`),
},
expectedError: "",
},
{
description: "imp.ext + imp.ext.prebid - Prebid Bidder Only",
givenImpExt: map[string]json.RawMessage{
"anyBidder": json.RawMessage(`"anyBidderValues"`),
"prebid": json.RawMessage(`"ignoredInFavorOfSeparatelyUnmarshalledImpExtPrebid"`),
"data": json.RawMessage(`"anyData"`),
"context": json.RawMessage(`"anyContext"`),
"skadn": json.RawMessage(`"anySKAdNetwork"`),
"gpid": json.RawMessage(`"anyGPID"`),
"tid": json.RawMessage(`"anyTID"`),
"unknownBidder": json.RawMessage(`"anyUnknownBidderValues"`),
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should be addressed now in utils_test.go

"prebid": json.RawMessage(`"ignoredInFavorOfSeparatelyUnmarshalledImpExtPrebid"`),
"data": json.RawMessage(`"anyData"`),
"context": json.RawMessage(`"anyContext"`),
"skadn": json.RawMessage(`"anySKAdNetwork"`),
"gpid": json.RawMessage(`"anyGPID"`),
"tid": json.RawMessage(`"anyTID"`),
},
givenImpExtPrebid: map[string]json.RawMessage{
"bidder": json.RawMessage(`"anyBidder"`),
},
expected: map[string]json.RawMessage{
"data": json.RawMessage(`"anyData"`),
"context": json.RawMessage(`"anyContext"`),
"skadn": json.RawMessage(`"anySKAdNetwork"`),
"gpid": json.RawMessage(`"anyGPID"`),
"tid": json.RawMessage(`"anyTID"`),
"unknownBidder": json.RawMessage(`"anyUnknownBidderValues"`),
"data": json.RawMessage(`"anyData"`),
"context": json.RawMessage(`"anyContext"`),
"skadn": json.RawMessage(`"anySKAdNetwork"`),
"gpid": json.RawMessage(`"anyGPID"`),
"tid": json.RawMessage(`"anyTID"`),
},
expectedError: "",
},
{
description: "imp.ext + imp.ext.prebid - Prebid Bidder + Other Forbidden Value",
givenImpExt: map[string]json.RawMessage{
"anyBidder": json.RawMessage(`"anyBidderValues"`),
"prebid": json.RawMessage(`"ignoredInFavorOfSeparatelyUnmarshalledImpExtPrebid"`),
"data": json.RawMessage(`"anyData"`),
"context": json.RawMessage(`"anyContext"`),
"skadn": json.RawMessage(`"anySKAdNetwork"`),
"gpid": json.RawMessage(`"anyGPID"`),
"tid": json.RawMessage(`"anyTID"`),
"unknownBidder": json.RawMessage(`"anyUnknownBidderValues"`),
"prebid": json.RawMessage(`"ignoredInFavorOfSeparatelyUnmarshalledImpExtPrebid"`),
"data": json.RawMessage(`"anyData"`),
"context": json.RawMessage(`"anyContext"`),
"skadn": json.RawMessage(`"anySKAdNetwork"`),
"gpid": json.RawMessage(`"anyGPID"`),
"tid": json.RawMessage(`"anyTID"`),
},
givenImpExtPrebid: map[string]json.RawMessage{
"bidder": json.RawMessage(`"anyBidder"`),
"forbidden": json.RawMessage(`"anyValue"`),
},
expected: map[string]json.RawMessage{
"data": json.RawMessage(`"anyData"`),
"context": json.RawMessage(`"anyContext"`),
"skadn": json.RawMessage(`"anySKAdNetwork"`),
"gpid": json.RawMessage(`"anyGPID"`),
"tid": json.RawMessage(`"anyTID"`),
"unknownBidder": json.RawMessage(`"anyUnknownBidderValues"`),
"data": json.RawMessage(`"anyData"`),
"context": json.RawMessage(`"anyContext"`),
"skadn": json.RawMessage(`"anySKAdNetwork"`),
"gpid": json.RawMessage(`"anyGPID"`),
"tid": json.RawMessage(`"anyTID"`),
},
expectedError: "",
},
{
description: "imp.ext + imp.ext.prebid - Prebid Bidder + Other Allowed Values",
givenImpExt: map[string]json.RawMessage{
"anyBidder": json.RawMessage(`"anyBidderValues"`),
"prebid": json.RawMessage(`"ignoredInFavorOfSeparatelyUnmarshalledImpExtPrebid"`),
"data": json.RawMessage(`"anyData"`),
"context": json.RawMessage(`"anyContext"`),
"skadn": json.RawMessage(`"anySKAdNetwork"`),
"gpid": json.RawMessage(`"anyGPID"`),
"tid": json.RawMessage(`"anyTID"`),
"unknownBidder": json.RawMessage(`"anyUnknownBidderValues"`),
"prebid": json.RawMessage(`"ignoredInFavorOfSeparatelyUnmarshalledImpExtPrebid"`),
"data": json.RawMessage(`"anyData"`),
"context": json.RawMessage(`"anyContext"`),
"skadn": json.RawMessage(`"anySKAdNetwork"`),
"gpid": json.RawMessage(`"anyGPID"`),
"tid": json.RawMessage(`"anyTID"`),
},
givenImpExtPrebid: map[string]json.RawMessage{
"bidder": json.RawMessage(`"anyBidder"`),
"is_rewarded_inventory": json.RawMessage(`"anyIsRewardedInventory"`),
"options": json.RawMessage(`"anyOptions"`),
},
expected: map[string]json.RawMessage{
"prebid": json.RawMessage(`{"is_rewarded_inventory":"anyIsRewardedInventory","options":"anyOptions"}`),
"data": json.RawMessage(`"anyData"`),
"context": json.RawMessage(`"anyContext"`),
"skadn": json.RawMessage(`"anySKAdNetwork"`),
"gpid": json.RawMessage(`"anyGPID"`),
"tid": json.RawMessage(`"anyTID"`),
"unknownBidder": json.RawMessage(`"anyUnknownBidderValues"`),
"prebid": json.RawMessage(`{"is_rewarded_inventory":"anyIsRewardedInventory","options":"anyOptions"}`),
"data": json.RawMessage(`"anyData"`),
"context": json.RawMessage(`"anyContext"`),
"skadn": json.RawMessage(`"anySKAdNetwork"`),
"gpid": json.RawMessage(`"anyGPID"`),
"tid": json.RawMessage(`"anyTID"`),
},
expectedError: "",
},
Expand Down
28 changes: 19 additions & 9 deletions ortb/request_validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,22 +88,27 @@ func (srv *standardRequestValidator) validateImpExt(imp *openrtb_ext.ImpWrapper,
prebid := impExt.GetOrCreatePrebid()
prebidModified := false

bidderPromote := false

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

ext := impExt.GetExt()
extModified := false

// 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
}
}
Comment on lines 101 to 113
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

Copy link
Contributor

@SyntaxNode SyntaxNode Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.


Expand All @@ -117,15 +122,15 @@ func (srv *standardRequestValidator) validateImpExt(imp *openrtb_ext.ImpWrapper,

errL := []error{}

for bidder, ext := range prebid.Bidder {
for bidder, val := range prebid.Bidder {
coreBidder, _ := openrtb_ext.NormalizeBidderName(bidder)
if tmp, isAlias := aliases[bidder]; isAlias {
coreBidder = openrtb_ext.BidderName(tmp)
}

if coreBidderNormalized, isValid := srv.bidderMap[coreBidder.String()]; isValid {
if !cfg.SkipBidderParams {
if err := srv.paramsValidator.Validate(coreBidderNormalized, ext); err != nil {
if err := srv.paramsValidator.Validate(coreBidderNormalized, val); err != nil {
return []error{fmt.Errorf("request.imp[%d].ext.prebid.bidder.%s failed validation.\n%v", impIndex, bidder, err)}
}
}
Expand All @@ -134,6 +139,11 @@ func (srv *standardRequestValidator) validateImpExt(imp *openrtb_ext.ImpWrapper,
errL = append(errL, &errortypes.BidderTemporarilyDisabled{Message: msg})
delete(prebid.Bidder, bidder)
prebidModified = true
} else if bidderPromote {
errL = append(errL, &errortypes.Warning{Message: fmt.Sprintf("request.imp[%d].ext contains unknown bidder: '%s', ignoring", impIndex, bidder)})
ext[bidder] = val
delete(prebid.Bidder, bidder)
prebidModified = true
} else {
return []error{fmt.Errorf("request.imp[%d].ext.prebid.bidder contains unknown bidder: %s. Did you forget an alias in request.ext.prebid.aliases?", impIndex, bidder)}
}
Expand Down
Loading
Loading