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 all 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
21 changes: 15 additions & 6 deletions endpoints/openrtb2/auction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3977,6 +3977,7 @@ func TestParseRequestMergeBidderParams(t *testing.T) {
expectedImpExt json.RawMessage
expectedReqExt json.RawMessage
expectedErrorCount int
expectedErrors []error
}{
{
name: "add missing bidder-params from req.ext.prebid.bidderparams to imp[].ext.prebid.bidder",
Expand All @@ -3994,10 +3995,16 @@ func TestParseRequestMergeBidderParams(t *testing.T) {
},
{
name: "add missing bidder-params from req.ext.prebid.bidderparams to imp[].ext for backward compatibility",
givenRequestBody: validRequest(t, "req-ext-bidder-params-backward-compatible-merge.json"),
expectedImpExt: getObject(t, "req-ext-bidder-params-backward-compatible-merge.json", "expectedImpExt"),
expectedReqExt: getObject(t, "req-ext-bidder-params-backward-compatible-merge.json", "expectedReqExt"),
expectedErrorCount: 0,
givenRequestBody: validRequest(t, "req-ext-bidder-params-promotion.json"),
expectedImpExt: getObject(t, "req-ext-bidder-params-promotion.json", "expectedImpExt"),
expectedReqExt: getObject(t, "req-ext-bidder-params-promotion.json", "expectedReqExt"),
expectedErrorCount: 1,
expectedErrors: []error{
&errortypes.Warning{
WarningCode: 0,
Message: "request.imp[0].ext contains unknown bidder: 'arbitraryObject', ignoring",
},
},
},
}
for _, test := range tests {
Expand Down Expand Up @@ -4054,6 +4061,8 @@ func TestParseRequestMergeBidderParams(t *testing.T) {
assert.Equal(t, eReqE, reqE, "req.Ext should match")

assert.Len(t, errL, test.expectedErrorCount, "error length should match")

assert.Equal(t, errL, test.expectedErrors)
})
}
}
Expand Down Expand Up @@ -4658,7 +4667,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 @@ -4685,7 +4694,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
5 changes: 4 additions & 1 deletion ...equests/invalid-whole/unknown-bidder.json → ...id-whole/imp-ext-prebid-bidder-empty.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
]
},
"ext": {
"prebid": {
"bidder": {}
},
"appnexus": {
"placementId": 12883451
},
Expand All @@ -34,5 +37,5 @@
"tmax": 500
},
"expectedReturnCode": 400,
"expectedErrorMessage": "Invalid request"
"expectedErrorMessage": "Invalid request: request.imp[0].ext.prebid.bidder must contain at least one bidder\n"
}
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"
}
7 changes: 5 additions & 2 deletions ...der-params-backward-compatible-merge.json → ...tary/req-ext-bidder-params-promotion.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@
"profile": 1234
}
},
"prebid": {
"bidder": {}
"arbitraryObject": {
"arbitraryField": 1232
}
}
}
Expand Down Expand Up @@ -107,6 +107,9 @@
}
}
}
},
"arbitraryObject": {
"arbitraryField": 1232
}
},
"expectedReturnCode": 200
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 @@ -718,20 +718,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 @@ -753,8 +748,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"`),
"arbitraryField": json.RawMessage(`"arbitraryValue"`),
"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"`),
"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 + 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"`),
"arbitraryField": json.RawMessage(`"arbitraryValue"`),
"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"`),
"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 + 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"`),
"arbitraryField": json.RawMessage(`"arbitraryValue"`),
"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"`),
"arbitraryField": json.RawMessage(`"arbitraryValue"`),
"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