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.

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

Upgrade Bid Adjustments #2678

Merged
merged 32 commits into from
May 4, 2023
Merged

Conversation

AlexBVolcy
Copy link
Contributor

@AlexBVolcy AlexBVolcy commented Apr 3, 2023

Addresses this feature: #2085

The goal of this feature is to add support for bid adjustments that works differently and is more complex than the existing bidadjustmentfactors.

This feature allows bidadjustments to be defined on the request at the path ext.prebid.bidadjustments. Within this definition, bidadjustments will specify the mediatype, the bidder name, the dealID, and finally an adjustment array.

This adjustment array will contain information that'll be used to the adjust the bid price of a particular bid that matches the given mediatype, bidder name, and dealID.

An entry in the adjustment array will include the following information: an adjtype, a value, and sometimes a currency

Adjtype refers to the type of adjustment that'll be occurring. A mulitplier adjtype will result in the bid price being multiplied by the given value. A static adjtype will result in the bid price being overwritten by the given value (after currency conversions). A cpm adjtype, will result in the given value being subtracted from the bid price.

@bsardo bsardo self-assigned this Apr 3, 2023
Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

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

Local testing looks good (except of warnings), code looks good overall. Added some comments.

exchange/exchange.go Outdated Show resolved Hide resolved
exchange/exchange.go Outdated Show resolved Hide resolved
exchange/exchange_test.go Outdated Show resolved Hide resolved
openrtb_ext/request.go Outdated Show resolved Hide resolved
openrtb_ext/request.go Outdated Show resolved Hide resolved
exchange/bidder_test.go Outdated Show resolved Hide resolved
originalBidCpm := 0.0
if bidResponse.Bids[i].Bid != nil {
originalBidCpm = bidResponse.Bids[i].Bid.Price
bidResponse.Bids[i].Bid.Price = bidResponse.Bids[i].Bid.Price * adjustmentFactor * conversionRate
bidResponse.Bids[i].Bid.Price = applyAdjustmentArray(adjArray, bidResponse.Bids[i].Bid.Price, bidResponse.Currency, reqInfo)
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about this idea: combine GetAdjustmentArray and applyAdjustmentArray in one function like processBidAdjustments?
Can you store all bid adjustments related functions in a separated package (except of validation maybe)?
I had similar comment about ad server targeting recently :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that idea! I think I'd want to bring it up during team time first before going through with this :)

Copy link
Contributor Author

@AlexBVolcy AlexBVolcy Apr 12, 2023

Choose a reason for hiding this comment

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

I realized after thinking about this, and I think we need to keep these two functions separate. The GetAdjustmentArray function is needed on it's own to satisfy the requirement of:

"Any module that needs to reverse the adjustment (e.g. floors), will need a way to get the relevant adjustment array and they can walk the adjustments backwards. This implies that there should be a function similar to the Floors getFloor() function that allows callers to supply dimension values and get the adjustments array."

Copy link
Contributor

Choose a reason for hiding this comment

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

You can still have GetAdjustmentArray as a separate function and call it first in the new function that will consolidate
GetAdjustmentArray and applyAdjustmentArray. It seems like a logical way to do this, it will minimize functions calls from requestBid, it's already complicated. And also adjArray is only used in applyAdjustmentArray.

openrtb_ext/request.go Outdated Show resolved Hide resolved
openrtb_ext/request.go Outdated Show resolved Hide resolved
endpoints/openrtb2/auction.go Outdated Show resolved Hide resolved
Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

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

Thank you for addressing comments, looks good! Added some more comments and I also missed the case with decimal places while testing it previous time.

Comment on lines 280 to 287
mergedBidAdj, err := mergeBidAdjustments(r.BidRequestWrapper, r.Account.BidAdjustments)
if err != nil {
return nil, err
}
if valid := mergedBidAdj.ValidateBidAdjustments(); !valid {
mergedBidAdj = nil
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think this can be combined in one function?

Copy link
Contributor

Choose a reason for hiding this comment

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

By "combining" I don't mean to literally merge the code from both of this functions. You can still call them separately, but from another consolidates function. Hope it makes sense.
Something like this (or maybe you will come up with better solution while coding it):

func ProcessBidAdjustments(...){
     mergeBidAdjustments()
    validateBidAdjustments()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this idea, I'll update to combine them!

if err != nil {
return originalBidPrice
}
bidPrice = bidPrice - convertedVal
Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 Apr 13, 2023

Choose a reason for hiding this comment

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

Is this addressed?
Also result should be rounded to 2 decimal digits, right?
In my test I have a bid price = 12.00001,
Adjustment:

 {
             "adjtype": "cpm",
              "value": 12,
               "currency": "USD"
}

And result turns out to be this: "price": 0.000009999999999621423,

UPD: from requirements: Round to 4 decimal places.

originalBidCpm := 0.0
if bidResponse.Bids[i].Bid != nil {
originalBidCpm = bidResponse.Bids[i].Bid.Price
bidResponse.Bids[i].Bid.Price = bidResponse.Bids[i].Bid.Price * adjustmentFactor * conversionRate
bidResponse.Bids[i].Bid.Price = applyAdjustmentArray(adjArray, bidResponse.Bids[i].Bid.Price, bidResponse.Currency, reqInfo)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can still have GetAdjustmentArray as a separate function and call it first in the new function that will consolidate
GetAdjustmentArray and applyAdjustmentArray. It seems like a logical way to do this, it will minimize functions calls from requestBid, it's already complicated. And also adjArray is only used in applyAdjustmentArray.

@VeronikaSolovei9
Copy link
Contributor

About bidPrice = bidPrice - convertedVal
Round the final number to 4 decimal digits first and then check if it's if bidPrice <= 0 {.
In my example result after subtraction is 0.000009999999999621423 which rounds to 0.0000 and it leads to bid rejection and this message in debug logs:

"errors": {
            "appnexus": [
                {
                    "code": 999,
                    "message": "Bid \"9109836616608581859\" does not contain positive 'price' which is required since there is no deal set for this bid"
                }
            ]
        },

Also roundTo := math.Pow(10, amountOfDecimalPlaces) looks like a good candidate to be a constant:
const roundTo = 10000

Here is my final end of this function:

...
rounded := math.Round(bidPrice*roundTo) / roundTo // Returns Bid Price rounded to 4 decimal places

if rounded <= 0 {
	return originalBidPrice
}
return rounded

@VeronikaSolovei9
Copy link
Contributor

Do you think it makes sense to move all new functions related to bid adjustments from bidder.go and exchange.go to a separate file in exchange directory?

@AlexBVolcy
Copy link
Contributor Author

@VeronikaSolovei9 just pushed a commit that moves adjustment functions that were present in bidder.go and exchange.go into it's own file in exchange directory. Also moved tests tied to those functions into their own file as well. I also updated the rounding logic!

@AlexBVolcy
Copy link
Contributor Author

AlexBVolcy commented Apr 17, 2023

Getting this failure on the new test file I added:

exchange/bidadjustments_test.go:6:2: no required module provides package github.com/prebid/openrtb/v17/openrtb2; to add it: go get github.com/prebid/openrtb/v17/openrtb2

Just want to highlight this for tomorrow's team time

@VeronikaSolovei9
Copy link
Contributor

It feels like you need to merge your branch with latest master. Even if there are no conflicts.
There are should not be any usages of github.com/prebid/openrtb/v17/openrtb2

import (
"testing"

"github.com/prebid/openrtb/v17/openrtb2"
Copy link
Contributor

Choose a reason for hiding this comment

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

Use "github.com/prebid/openrtb/v19/openrtb2" instead

@VeronikaSolovei9
Copy link
Contributor

Thank you for addressing comments! Changes look good :)

endpoints/openrtb2/auction.go Outdated Show resolved Hide resolved
openrtb_ext/request.go Outdated Show resolved Hide resolved
endpoints/openrtb2/test_utils.go Outdated Show resolved Hide resolved
Comment on lines 349 to 377
func (bidAdj *ExtRequestPrebidBidAdjustments) GetAdjustmentArray(bidType BidType, bidderName BidderName, dealID string) []Adjustments {
if bidAdj.MediaType.Banner != nil && bidType == BidTypeBanner {
if adjArray := getAdjustmentArrayForMediaType(bidAdj.MediaType.Banner, bidderName.String(), dealID); adjArray != nil {
return adjArray
}
}
if bidAdj.MediaType.Video != nil && bidType == BidTypeVideo {
if adjArray := getAdjustmentArrayForMediaType(bidAdj.MediaType.Video, bidderName.String(), dealID); adjArray != nil {
return adjArray
}
}
if bidAdj.MediaType.Audio != nil && bidType == BidTypeAudio {
if adjArray := getAdjustmentArrayForMediaType(bidAdj.MediaType.Audio, bidderName.String(), dealID); adjArray != nil {
return adjArray
}

}
if bidAdj.MediaType.Native != nil && bidType == BidTypeNative {
if adjArray := getAdjustmentArrayForMediaType(bidAdj.MediaType.Native, bidderName.String(), dealID); adjArray != nil {
return adjArray
}
}
if bidAdj.MediaType.WildCard != nil {
if adjArray := getAdjustmentArrayForMediaType(bidAdj.MediaType.WildCard, bidderName.String(), dealID); adjArray != nil {
return adjArray
}
}
return nil
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: I think you can reduce the verbosity here significantly by introducing a local variable, let's call it adjustments, that is set based on your five if conditions.
Then finish with something like:

if adjustments == nil {
	return nil
}
return getAdjustmentArrayForMediaType(adjustments, bidderName.String(), dealID)

Edit: my comment here may be moot given the potential design modifications you'll have to make for rule selection given specificity.

// #2: Are able to match bidderName and dealID field is WildCard
// #3: Bidder field is WildCard and are able to match DealID
// #4: Wildcard bidder and wildcard dealID
func getAdjustmentArrayForMediaType(bidAdjMap map[string]map[string][]Adjustments, bidderName string, dealID string) []Adjustments {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function looks good. However, I think we might have to make a modification to this and/or the calling function GetAdjustmentArray to satisfy the requirements.

From the requirements: "If multiple adjustment arrays match, choose the one that has the least number of wildcards in the path". In other words, choose the rule that is the most specific.
In that case, a rule of *, bidderA, dealID1 would win over banner, *, *.

There's also a scenario where multiple rules match and have the same level of specificity. In that case, I'd also probably include logic similar to what we have in floors where if two rules have the same amount of specificity, the rule that has the most specific left-most value wins.
In that case, banner, *, dealID1 would win over *, bidderA, dealID1.

openrtb_ext/request.go Outdated Show resolved Hide resolved
openrtb_ext/request.go Outdated Show resolved Hide resolved
@@ -290,6 +290,11 @@ func (e *exchange) HoldAuction(ctx context.Context, r AuctionRequest, debugLog *
r.FirstPartyData = resolvedFPD
}

mergedBidAdj, err := processBidAdjustments(r.BidRequestWrapper, r.Account.BidAdjustments)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If bid adjustments aren't successfully merged we will get an error and we'll halt processing right? In this scenario, the problem must have existed in the account config because the request config already passed validation. If the account config was malformed, we would have already halted processing when it was fetched. In that case, an error could only happen here if we have a code problem right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alex and I chatted offline. No action is required here. Yes we will halt processing if the merge fails but this technically should never happen.

Copy link
Contributor

@SyntaxNode SyntaxNode left a comment

Choose a reason for hiding this comment

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

Looks great. I have nitpicks, but nothing major.

bidadjustments/bidadjustments.go Outdated Show resolved Hide resolved
bidadjustments/bidadjustments.go Outdated Show resolved Hide resolved
bidadjustments/bidadjustments.go Outdated Show resolved Hide resolved
bidadjustments/bidadjustments.go Outdated Show resolved Hide resolved
func applyAdjustmentArray(adjArray []openrtb_ext.Adjustments, bidPrice float64, currency string, reqInfo *adapters.ExtraRequestInfo) (float64, string) {
if adjArray == nil {
return bidPrice, currency
}
Copy link
Contributor

Choose a reason for hiding this comment

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

A nil array is treated different from an empty array. Is this behavior intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just want to check that if there are no adjustments to apply, to return the original bid price and currency. So i can add a length check here as well I think.

if adjustment.Value >= 0 && adjustment.Value < 100 {
return true
}
adjustment.Currency = ""
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 the currency set to empty after the validation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially did this because of the line from the issue that says during validation, if adjtype is "multiplier", ignore currency.

But reading this again, ignoring currency is accomplished with how the apply()method works, where if the adjtype is multiplier, I don't utilize currency at all in the bid price calculation.

So i think this line could be removed

// Overall: BidderName maps to a DealID that maps to an Adjustments Array
type MediaType struct {
Banner map[string]map[string][]Adjustments `json:"banner,omitempty"`
Video map[string]map[string][]Adjustments `json:"video,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

It's hard to understand map[string]map[string][]Adjustments and the comment is evidence of it. Could you use type aliases? Such as...

type AdjusmentsByDealID map[string][]Adjustments

map[BidderName]AdjusmentsByDealID

openrtb_ext/request.go Outdated Show resolved Hide resolved
openrtb_ext/request.go Outdated Show resolved Hide resolved
openrtb_ext/request.go Outdated Show resolved Hide resolved
// BidderName maps to a DealID that maps to the Adjustments
type MediaType struct {
Banner map[BidderName]AdjusmentsByDealID `json:"banner,omitempty"`
Video map[BidderName]AdjusmentsByDealID `json:"video,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

It appears we will be moving from video to video-instream and video-outstream for the bid adjustment rules so we should support both of those here instead of video. I need to investigate this further before we make changes.

bidadjustment/validate.go Outdated Show resolved Hide resolved
bidadjustment/validate_test.go Show resolved Hide resolved
bidadjustment/validate.go Outdated Show resolved Hide resolved
bidadjustment/validate_test.go Outdated Show resolved Hide resolved
bidadjustment/bidadjustment.go Outdated Show resolved Hide resolved
bidadjustment/bidadjustment.go Outdated Show resolved Hide resolved
bidadjustment/bidadjustment.go Outdated Show resolved Hide resolved
return reqAdjMap
}

func PopulateMap(bidAdjustments *openrtb_ext.ExtRequestPrebidBidAdjustments, ruleToAdjustments map[string][]openrtb_ext.Adjustment) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thoughts on changing this signature so ruleToAdjustments is returned instead of passed in? We're just passing in an empty map so I suggest generating that here and returning it.
Also maybe call this something like BuildRules instead?

bidadjustment/bidadjustment.go Outdated Show resolved Hide resolved
exchange/exchange.go Outdated Show resolved Hide resolved
bidadjustment/get.go Outdated Show resolved Hide resolved
bidadjustment/get.go Outdated Show resolved Hide resolved
func GetAndApplyAdjustments(ruleToAdjustments map[string][]openrtb_ext.Adjustment, bidInfo *adapters.TypedBid, bidderName openrtb_ext.BidderName, currency string, reqInfo *adapters.ExtraRequestInfo) (float64, string) {
adjustments := []openrtb_ext.Adjustment{}
if ruleToAdjustments != nil {
adjustments = get(ruleToAdjustments, string(bidInfo.BidType), string(bidderName), bidInfo.Bid.DealID)
Copy link
Collaborator

Choose a reason for hiding this comment

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

TODO: I think we should come up with names for the two types of rules: rules and ruleKeys? providerRules and adContextRules? Not sure what to use...

Copy link
Collaborator

@bsardo bsardo left a comment

Choose a reason for hiding this comment

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

Looks really good. Mostly nitpicky comments.

Also can you add doc comments to all public functions at a minimum and use your own discretion on whether to add comments for private functions?

if err != nil {
return nil, err
}
if !Validate(mergedBidAdj) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably return a warning here as well if this validation fails.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Discussed offline. We will stick with a warning but will need to adjust the warning message to indicate the issue is in the account instead of the request. We'll also need to append the error to the errors slice if the error returned by Merge is non-fatal.

exchange/bidder.go Outdated Show resolved Hide resolved
openrtb_ext/request.go Show resolved Hide resolved
@@ -1554,6 +1555,15 @@ func validateRequestExt(req *openrtb_ext.RequestWrapper) []error {
reqExt.SetPrebid(prebid)
}

if !bidadjustment.Validate(prebid.BidAdjustments) {
prebid.BidAdjustments = nil
reqExt.SetPrebid(prebid)
Copy link
Collaborator

@bsardo bsardo May 2, 2023

Choose a reason for hiding this comment

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

Double check that we don't need to call RebuildRequest after setting prebid on the wrapper.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No action required here. We shouldn't need to call RebuildRequest at this point. The call to SetPrebid is modifying the request ext object on the wrapper and marking it as dirty which is sufficient. The code that follows operates off of the request ext object and does not need access to a marshaled ext. In HoldAuction, we rebuild the request prior to cleaning and splitting the request by bidder to ensure that each bidder uses a copy of the mutated request including any modifications made to the request ext through calls to methods like SetPrebid.

bidadjustment/validate.go Outdated Show resolved Hide resolved
Comment on lines 55 to 57
if roundedBidPrice <= 0 {
return originalBidPrice, originalCurrency
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious, It might make sense to have this logic but where did it come from? I don't see it in the requirements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It came as a response to this comment #2678 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Apparently a zero bid is considered valid for deals only, which makes me wonder if we should add the following at the end of Apply and remove this conditional here:

minBid := 0.1??
adjustedPrice, adjustedCurrency := apply(adjustments, bidInfo.Bid.Price, currency, reqInfo)

if bidInfo.Bid.DealID != "" && adjustedPrice < 0 {
	return 0, currency
}
if bidInfo.Bid.DealID == "" && adjustedPrice <= 0 {
	return minBid, currency
}
return adjustedPrice, adjustedCurrency

bidadjustment/build_rules_test.go Show resolved Hide resolved
bidadjustment/apply_test.go Show resolved Hide resolved
bidadjustment/apply_test.go Show resolved Hide resolved
exchange/bidder.go Show resolved Hide resolved
Comment on lines 55 to 57
if roundedBidPrice <= 0 {
return originalBidPrice, originalCurrency
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Apparently a zero bid is considered valid for deals only, which makes me wonder if we should add the following at the end of Apply and remove this conditional here:

minBid := 0.1??
adjustedPrice, adjustedCurrency := apply(adjustments, bidInfo.Bid.Price, currency, reqInfo)

if bidInfo.Bid.DealID != "" && adjustedPrice < 0 {
	return 0, currency
}
if bidInfo.Bid.DealID == "" && adjustedPrice <= 0 {
	return minBid, currency
}
return adjustedPrice, adjustedCurrency

if err != nil {
return nil, err
}
if !Validate(mergedBidAdj) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Discussed offline. We will stick with a warning but will need to adjust the warning message to indicate the issue is in the account instead of the request. We'll also need to append the error to the errors slice if the error returned by Merge is non-fatal.

Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

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

LGTM, great work!

EventsEnabled *bool `mapstructure:"events_enabled" json:"events_enabled"` // Deprecated: Use events.enabled instead.
CCPA AccountCCPA `mapstructure:"ccpa" json:"ccpa"`
GDPR AccountGDPR `mapstructure:"gdpr" json:"gdpr"`
DebugAllow bool `mapstructure:"debug_allow" json:"debug_allow"`
DefaultIntegration string `mapstructure:"default_integration" json:"default_integration"`
CookieSync CookieSync `mapstructure:"cookie_sync" json:"cookie_sync"`
Events Events `mapstructure:"events" json:"events"` // Don't enable this feature. It is still under developmment - https://github.com/prebid/prebid-server/issues/1725
TruncateTargetAttribute *int `mapstructure:"truncate_target_attr" json:"truncate_target_attr"`
AlternateBidderCodes *openrtb_ext.ExtAlternateBidderCodes `mapstructure:"alternatebiddercodes" json:"alternatebiddercodes"`
Hooks AccountHooks `mapstructure:"hooks" json:"hooks"`
PriceFloors AccountPriceFloors `mapstructure:"price_floors" json:"price_floors"`
Validations Validations `mapstructure:"validations" json:"validations"`
DefaultBidLimit int `mapstructure:"default_bid_limit" json:"default_bid_limit"`
ID string `mapstructure:"id" json:"id"`
Disabled bool `mapstructure:"disabled" json:"disabled"`
CacheTTL DefaultTTLs `mapstructure:"cache_ttl" json:"cache_ttl"`
EventsEnabled *bool `mapstructure:"events_enabled" json:"events_enabled"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you put the // Deprecated: Use events.enabled instead. comment back on the EventsEnabled line as it is currently on master? I think this got lost during your most recent merge with master.

@@ -1554,6 +1555,15 @@ func validateRequestExt(req *openrtb_ext.RequestWrapper) []error {
reqExt.SetPrebid(prebid)
}

if !bidadjustment.Validate(prebid.BidAdjustments) {
prebid.BidAdjustments = nil
reqExt.SetPrebid(prebid)
Copy link
Collaborator

Choose a reason for hiding this comment

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

No action required here. We shouldn't need to call RebuildRequest at this point. The call to SetPrebid is modifying the request ext object on the wrapper and marking it as dirty which is sufficient. The code that follows operates off of the request ext object and does not need access to a marshaled ext. In HoldAuction, we rebuild the request prior to cleaning and splitting the request by bidder to ensure that each bidder uses a copy of the mutated request including any modifications made to the request ext through calls to methods like SetPrebid.

extPrebid.BidAdjustments.MediaType.VideoOutstream = mergeForMediaType(extPrebid.BidAdjustments.MediaType.VideoOutstream, acct.MediaType.VideoOutstream)
extPrebid.BidAdjustments.MediaType.WildCard = mergeForMediaType(extPrebid.BidAdjustments.MediaType.WildCard, acct.MediaType.WildCard)

return extPrebid.BidAdjustments, nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

No action required here; I think this is ok. SetPrebid is creating a copy of the the request.ext.prebid object which is then modified here and returned where it is passed downstream and utilized when processing the bid responses. I don't believe there is any value in modifying the request unless our intention is to make the information visible to the publisher as part of the resolved request, but even so, that would be a deviation from our current approach and is something we should address holistically in a separate PR.

Comment on lines 322 to 328
if err != nil {
if !errortypes.ContainsFatalError([]error{err}) {
errs = append(errs, err)
} else {
return nil, err
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Supernitpick driven by go docs (https://go.dev/doc/effective_go): "In the Go libraries, you'll find that when an if statement doesn't flow into the next statement—that is, the body ends in break, continue, goto, or return—the unnecessary else is omitted."

You can rewrite this to eliminate the else by instead checking if it contains a fatal error and early returning.

Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

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

LGTM!

@bsardo bsardo merged commit e05d429 into prebid:master May 4, 2023
gargcreation1992 pushed a commit that referenced this pull request May 16, 2023
blueseasx pushed a commit to blueseasx/prebid-server that referenced this pull request Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants