-
Notifications
You must be signed in to change notification settings - Fork 750
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.
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
Fix imp ext prebid adunitcode drop #4064
Fix imp ext prebid adunitcode drop #4064
Conversation
@@ -168,7 +168,7 @@ func moveRewardedFromPrebidExtTo26(i *ImpWrapper) { | |||
// read and clear prebid ext | |||
impExt, _ := i.GetImpExt() | |||
rwddPrebidExt := (*int8)(nil) | |||
if p := impExt.GetPrebid(); p != nil { | |||
if p := impExt.GetPrebid(); p != nil && p.IsRewardedInventory != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: This is an optimization unrelated to the fix.
Functionally, it doesn't matter if IsRewardedInventory
is nil
in the ext since it's always set to nil
in the upconvert process. There is a side affect that calling SetPrebid
will eventually force a re-marshal, so if there is no change then it's a slight performance optimization to skip.
openrtb_ext/request_wrapper_test.go
Outdated
@@ -234,6 +236,13 @@ func TestRebuildImp(t *testing.T) { | |||
expectedRequest: openrtb2.BidRequest{Imp: []openrtb2.Imp{{ID: "1"}, {ID: "2", Ext: prebidJson}}}, | |||
expectedAccessed: true, | |||
}, | |||
{ | |||
description: "Many - Accessed - Dirty / Not Dirty", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: The name is a copy / paste typo. Consider calling this "One - Accessed - Dirty - AdUnitCode" and move so it's grouped below "One - Accessed - Dirty".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the sharp eye, I just fixed it
@@ -43,6 +43,8 @@ type ExtImpPrebid struct { | |||
|
|||
Options *Options `json:"options,omitempty"` | |||
|
|||
AdUnitCode string `json:"adunitcode,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: This is the fix. For some reason, this supported prebid extension was not adde to the structure which caused it to be ignored when marshalling this struct. Confirmed all other imp[].ext.prebid values from the docs are included here.
7b5b9d8
to
0db4d27
Compare
#4060