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

Modularity: Make request wrapper accessible in bidder request hook #3096

Merged
merged 9 commits into from
Nov 16, 2023
Merged

Modularity: Make request wrapper accessible in bidder request hook #3096

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
79ac96c
Replaced BidRequest to RequestWrapper in BidderRequestPayload
VeronikaSolovei9 Sep 12, 2023
cdc8f22
Removed test config
VeronikaSolovei9 Sep 12, 2023
39f39d4
Added RebuildRequest
VeronikaSolovei9 Sep 12, 2023
80daab8
Code clean up
VeronikaSolovei9 Sep 24, 2023
64ca130
Merge branch 'master' into bidderRequestPayload_RequestWrapper
VeronikaSolovei9 Sep 24, 2023
358c3b4
Code clean up
VeronikaSolovei9 Sep 28, 2023
d841a8a
Code clean up
VeronikaSolovei9 Oct 5, 2023
37a5e5a
Merge branch 'master' into bidderRequestPayload_RequestWrapper
VeronikaSolovei9 Oct 23, 2023
c5a7249
Merge fixes
VeronikaSolovei9 Oct 23, 2023
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
7 changes: 6 additions & 1 deletion exchange/bidder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,8 @@ type bidderAdapterConfig struct {
}

func (bidder *bidderAdapter) requestBid(ctx context.Context, bidderRequest BidderRequest, conversions currency.Conversions, reqInfo *adapters.ExtraRequestInfo, adsCertSigner adscert.Signer, bidRequestOptions bidRequestOptions, alternateBidderCodes openrtb_ext.ExtAlternateBidderCodes, hookExecutor hookexecution.StageExecutor, ruleToAdjustments openrtb_ext.AdjustmentsByDealID) ([]*entities.PbsOrtbSeatBid, extraBidderRespInfo, []error) {
reject := hookExecutor.ExecuteBidderRequestStage(bidderRequest.BidRequest, string(bidderRequest.BidderName))
brw := openrtb_ext.RequestWrapper{BidRequest: bidderRequest.BidRequest}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a variable name other than brw we could use? I'd prefer something like reqWrapper for clarity, but if you prefer brw I'm fine to keep it like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed to request, similar to other renames

reject := hookExecutor.ExecuteBidderRequestStage(&brw, string(bidderRequest.BidderName))
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
if reject != nil {
return nil, extraBidderRespInfo{}, []error{reject}
}
Expand All @@ -144,6 +145,10 @@ func (bidder *bidderAdapter) requestBid(ctx context.Context, bidderRequest Bidde
extraRespInfo extraBidderRespInfo
)

// rebuild request after modules execution
brw.RebuildRequest()
bidderRequest.BidRequest = brw.BidRequest

//check if real request exists for this bidder or it only has stored responses
dataLen := 0
if len(bidderRequest.BidRequest.Imp) > 0 {
Expand Down
4 changes: 2 additions & 2 deletions exchange/exchange_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5622,12 +5622,12 @@ func (e mockUpdateBidRequestHook) HandleBidderRequestHook(_ context.Context, mct
c := hookstage.ChangeSet[hookstage.BidderRequestPayload]{}
c.AddMutation(
func(payload hookstage.BidderRequestPayload) (hookstage.BidderRequestPayload, error) {
payload.BidRequest.Site.Name = "test"
payload.RequestWrapper.Site.Name = "test"
return payload, nil
}, hookstage.MutationUpdate, "bidRequest", "site.name",
).AddMutation(
func(payload hookstage.BidderRequestPayload) (hookstage.BidderRequestPayload, error) {
payload.BidRequest.Site.Domain = "test.com"
payload.RequestWrapper.Site.Domain = "test.com"
return payload, nil
}, hookstage.MutationUpdate, "bidRequest", "site.domain",
)
Expand Down
8 changes: 4 additions & 4 deletions hooks/hookexecution/executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ type StageExecutor interface {
ExecuteEntrypointStage(req *http.Request, body []byte) ([]byte, *RejectError)
ExecuteRawAuctionStage(body []byte) ([]byte, *RejectError)
ExecuteProcessedAuctionStage(req *openrtb_ext.RequestWrapper) error
ExecuteBidderRequestStage(req *openrtb2.BidRequest, bidder string) *RejectError
ExecuteBidderRequestStage(req *openrtb_ext.RequestWrapper, bidder string) *RejectError
ExecuteRawBidderResponseStage(response *adapters.BidderResponse, bidder string) *RejectError
ExecuteAllProcessedBidResponsesStage(adapterBids map[openrtb_ext.BidderName]*entities.PbsOrtbSeatBid)
ExecuteAuctionResponseStage(response *openrtb2.BidResponse)
Expand Down Expand Up @@ -177,7 +177,7 @@ func (e *hookExecutor) ExecuteProcessedAuctionStage(request *openrtb_ext.Request
return reject
}

func (e *hookExecutor) ExecuteBidderRequestStage(req *openrtb2.BidRequest, bidder string) *RejectError {
func (e *hookExecutor) ExecuteBidderRequestStage(req *openrtb_ext.RequestWrapper, bidder string) *RejectError {
plan := e.planBuilder.PlanForBidderRequestStage(e.endpoint, e.account)
if len(plan) == 0 {
return nil
Expand All @@ -194,7 +194,7 @@ func (e *hookExecutor) ExecuteBidderRequestStage(req *openrtb2.BidRequest, bidde

stageName := hooks.StageBidderRequest.String()
executionCtx := e.newContext(stageName)
payload := hookstage.BidderRequestPayload{BidRequest: req, Bidder: bidder}
payload := hookstage.BidderRequestPayload{RequestWrapper: req, Bidder: bidder}
outcome, payload, contexts, reject := executeStage(executionCtx, plan, payload, handler, e.metricEngine)
outcome.Entity = entity(bidder)
outcome.Stage = stageName
Expand Down Expand Up @@ -332,7 +332,7 @@ func (executor EmptyHookExecutor) ExecuteProcessedAuctionStage(_ *openrtb_ext.Re
return nil
}

func (executor EmptyHookExecutor) ExecuteBidderRequestStage(_ *openrtb2.BidRequest, bidder string) *RejectError {
func (executor EmptyHookExecutor) ExecuteBidderRequestStage(_ *openrtb_ext.RequestWrapper, bidder string) *RejectError {
return nil
}

Expand Down
4 changes: 2 additions & 2 deletions hooks/hookexecution/executor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ func TestEmptyHookExecutor(t *testing.T) {
entrypointBody, entrypointRejectErr := executor.ExecuteEntrypointStage(req, body)
rawAuctionBody, rawAuctionRejectErr := executor.ExecuteRawAuctionStage(body)
processedAuctionRejectErr := executor.ExecuteProcessedAuctionStage(&openrtb_ext.RequestWrapper{BidRequest: &openrtb2.BidRequest{}})
bidderRequestRejectErr := executor.ExecuteBidderRequestStage(bidderRequest, "bidder-name")
bidderRequestRejectErr := executor.ExecuteBidderRequestStage(&openrtb_ext.RequestWrapper{BidRequest: bidderRequest}, "bidder-name")
executor.ExecuteAuctionResponseStage(&openrtb2.BidResponse{})

outcomes := executor.GetOutcomes()
Expand Down Expand Up @@ -1171,7 +1171,7 @@ func TestExecuteBidderRequestStage(t *testing.T) {
exec := NewHookExecutor(test.givenPlanBuilder, EndpointAuction, &metricsConfig.NilMetricsEngine{})
exec.SetAccount(test.givenAccount)

reject := exec.ExecuteBidderRequestStage(test.givenBidderRequest, bidderName)
reject := exec.ExecuteBidderRequestStage(&openrtb_ext.RequestWrapper{BidRequest: test.givenBidderRequest}, bidderName)

assert.Equal(t, test.expectedReject, reject, "Unexpected stage reject.")
assert.Equal(t, test.expectedBidderRequest, test.givenBidderRequest, "Incorrect bidder request.")
Expand Down
6 changes: 3 additions & 3 deletions hooks/hookexecution/mocks_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ func (e mockTimeoutHook) HandleBidderRequestHook(_ context.Context, _ hookstage.
time.Sleep(20 * time.Millisecond)
c := hookstage.ChangeSet[hookstage.BidderRequestPayload]{}
c.AddMutation(func(payload hookstage.BidderRequestPayload) (hookstage.BidderRequestPayload, error) {
payload.BidRequest.User.CustomData = "some-custom-data"
payload.RequestWrapper.User.CustomData = "some-custom-data"
return payload, nil
}, hookstage.MutationUpdate, "bidRequest", "user.customData")

Expand Down Expand Up @@ -330,12 +330,12 @@ func (e mockUpdateBidRequestHook) HandleBidderRequestHook(_ context.Context, _ h
c := hookstage.ChangeSet[hookstage.BidderRequestPayload]{}
c.AddMutation(
func(payload hookstage.BidderRequestPayload) (hookstage.BidderRequestPayload, error) {
payload.BidRequest.User.Yob = 2000
payload.RequestWrapper.User.Yob = 2000
return payload, nil
}, hookstage.MutationUpdate, "bidRequest", "user.yob",
).AddMutation(
func(payload hookstage.BidderRequestPayload) (hookstage.BidderRequestPayload, error) {
payload.BidRequest.User.Consent = "true"
payload.RequestWrapper.User.Consent = "true"
return payload, nil
}, hookstage.MutationUpdate, "bidRequest", "user.consent",
)
Expand Down
7 changes: 3 additions & 4 deletions hooks/hookstage/bidderrequest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@ package hookstage

import (
"context"

"github.com/prebid/openrtb/v19/openrtb2"
"github.com/prebid/prebid-server/openrtb_ext"
)

// BidderRequest hooks are invoked for each bidder participating in auction.
Expand All @@ -25,6 +24,6 @@ type BidderRequest interface {
// distilled for the particular bidder.
// Hooks are allowed to modify openrtb2.BidRequest using mutations.
type BidderRequestPayload struct {
BidRequest *openrtb2.BidRequest
Bidder string
RequestWrapper *openrtb_ext.RequestWrapper
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO the RequestWrapper is an abstraction on top of the bid request, but it still represents a bid request. I recommend leaving this variable name (and others) as "BidRequest" of type RequestWrapper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed. Also renamed RequestWrapper to BidRequest in ProcessedAuctionRequestPayload

Bidder string
}
12 changes: 6 additions & 6 deletions hooks/hookstage/bidderrequest_mutations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ package hookstage

import (
"errors"
"github.com/prebid/prebid-server/openrtb_ext"

"github.com/prebid/openrtb/v19/adcom1"
"github.com/prebid/openrtb/v19/openrtb2"
)

func (c *ChangeSet[T]) BidderRequest() ChangeSetBidderRequest[T] {
Expand All @@ -31,12 +31,12 @@ func (c ChangeSetBidderRequest[T]) BApp() ChangeSetBApp[T] {
return ChangeSetBApp[T]{changeSetBidderRequest: c}
}

func (c ChangeSetBidderRequest[T]) castPayload(p T) (*openrtb2.BidRequest, error) {
func (c ChangeSetBidderRequest[T]) castPayload(p T) (*openrtb_ext.RequestWrapper, error) {
if payload, ok := any(p).(BidderRequestPayload); ok {
if payload.BidRequest == nil {
if payload.RequestWrapper == nil {
return nil, errors.New("empty BidRequest provided")
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: We should update the error messages in this method to not refer to the specific argument. Perhaps messages like "payload contains a nil bid request" and "failed to cast payload".

}
return payload.BidRequest, nil
return payload.RequestWrapper, nil
}
return nil, errors.New("failed to cast BidderRequestPayload")
}
Expand All @@ -47,9 +47,9 @@ type ChangeSetBAdv[T any] struct {

func (c ChangeSetBAdv[T]) Update(badv []string) {
c.changeSetBidderRequest.changeSet.AddMutation(func(p T) (T, error) {
bidRequest, err := c.changeSetBidderRequest.castPayload(p)
bidRequestWrapper, err := c.changeSetBidderRequest.castPayload(p)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps the return value from castPayload should still be referred to as bidRequest as @SyntaxNode mentioned earlier.

if err == nil {
bidRequest.BAdv = badv
bidRequestWrapper.BAdv = badv
Copy link
Collaborator

Choose a reason for hiding this comment

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

I imagine it is guaranteed that if we got this far bidRequestWrapper.BidRequest is not nil, in which case this assignment to BAdv will never fail, is that correct? I'm bringing this up because previously we were checking if the ortb2.BidRequest was nil in the call to castPayload but now that function just checks if the wrapper is nil, not if the wrapper's ortb2.BidRequest is nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically auction and mutation functions should be never executed with nil bid request. parseRequest function sets this explicitly:

req = &openrtb_ext.RequestWrapper{}
req.BidRequest = &openrtb2.BidRequest{}

I added extra nil check in castPayload just to be safe:

if payload.BidRequest == nil || payload.BidRequest.BidRequest == nil {...}

}
return p, err
}, MutationUpdate, "bidrequest", "badv")
Expand Down
18 changes: 9 additions & 9 deletions modules/prebid/ortb2blocking/hook_bidderrequest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@ func handleBidderRequestHook(
cfg config,
payload hookstage.BidderRequestPayload,
) (result hookstage.HookResult[hookstage.BidderRequestPayload], err error) {
if payload.BidRequest == nil {
if payload.RequestWrapper == nil || payload.RequestWrapper.BidRequest == nil {
return result, hookexecution.NewFailure("empty BidRequest provided")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the error message to a more generic wording, to no longer reference a specific variable name.

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's harder than it seems. Is "empty input provided" ok?

}

mediaTypes := mediaTypesFrom(payload.BidRequest)
mediaTypes := mediaTypesFrom(payload.RequestWrapper.BidRequest)
changeSet := hookstage.ChangeSet[hookstage.BidderRequestPayload]{}
blockingAttributes := blockingAttributes{}

Expand Down Expand Up @@ -60,7 +60,7 @@ func updateBAdv(
result *hookstage.HookResult[hookstage.BidderRequestPayload],
changeSet *hookstage.ChangeSet[hookstage.BidderRequestPayload],
) (err error) {
if len(payload.BidRequest.BAdv) > 0 {
if len(payload.RequestWrapper.BAdv) > 0 {
return nil
}

Expand All @@ -87,7 +87,7 @@ func updateBApp(
result *hookstage.HookResult[hookstage.BidderRequestPayload],
changeSet *hookstage.ChangeSet[hookstage.BidderRequestPayload],
) (err error) {
if len(payload.BidRequest.BApp) > 0 {
if len(payload.RequestWrapper.BApp) > 0 {
return nil
}

Expand All @@ -114,7 +114,7 @@ func updateBCat(
result *hookstage.HookResult[hookstage.BidderRequestPayload],
changeSet *hookstage.ChangeSet[hookstage.BidderRequestPayload],
) (err error) {
if len(payload.BidRequest.BCat) > 0 {
if len(payload.RequestWrapper.BCat) > 0 {
return nil
}

Expand Down Expand Up @@ -191,7 +191,7 @@ func updateCatTax(
attributes *blockingAttributes,
changeSet *hookstage.ChangeSet[hookstage.BidderRequestPayload],
) {
if payload.BidRequest.CatTax > 0 {
if payload.RequestWrapper.CatTax > 0 {
return
}

Expand Down Expand Up @@ -226,7 +226,7 @@ func mutationForImp(
impUpdater impUpdateFunc,
) hookstage.MutationFunc[hookstage.BidderRequestPayload] {
return func(payload hookstage.BidderRequestPayload) (hookstage.BidderRequestPayload, error) {
for i, imp := range payload.BidRequest.Imp {
for i, imp := range payload.RequestWrapper.Imp {
if values, ok := valuesByImp[imp.ID]; ok {
if len(values) == 0 {
continue
Expand All @@ -236,7 +236,7 @@ func mutationForImp(
imp.Banner = &openrtb2.Banner{}
}

payload.BidRequest.Imp[i] = impUpdater(imp, values)
payload.RequestWrapper.Imp[i] = impUpdater(imp, values)
}
}
return payload, nil
Expand Down Expand Up @@ -310,7 +310,7 @@ func findImpressionOverrides(
overrides := map[string][]int{}
messages := []string{}

for _, imp := range payload.BidRequest.Imp {
for _, imp := range payload.RequestWrapper.Imp {
// do not add override for attribute if it already exists in request
if isAttrPresent(imp) {
continue
Expand Down
7 changes: 5 additions & 2 deletions modules/prebid/ortb2blocking/module_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"encoding/json"
"errors"
"github.com/prebid/prebid-server/openrtb_ext"
"testing"

"github.com/prebid/openrtb/v19/adcom1"
Expand Down Expand Up @@ -566,7 +567,8 @@ func TestHandleBidderRequestHook(t *testing.T) {

for _, test := range testCases {
t.Run(test.description, func(t *testing.T) {
payload := hookstage.BidderRequestPayload{Bidder: test.bidder, BidRequest: test.bidRequest}
brw := openrtb_ext.RequestWrapper{BidRequest: test.bidRequest}
payload := hookstage.BidderRequestPayload{Bidder: test.bidder, RequestWrapper: &brw}

result, err := Builder(nil, moduledeps.ModuleDeps{})
assert.NoError(t, err, "Failed to build module.")
Expand All @@ -590,7 +592,8 @@ func TestHandleBidderRequestHook(t *testing.T) {
_, err := mut.Apply(payload)
assert.NoError(t, err)
}
assert.Equal(t, test.expectedBidRequest, payload.BidRequest, "Invalid BidRequest after executing BidderRequestHook.")

assert.Equal(t, test.expectedBidRequest, payload.RequestWrapper.BidRequest, "Invalid BidRequest after executing BidderRequestHook.")

// reset ChangeSet not to break hookResult assertion, we validated ChangeSet separately
hookResult.ChangeSet = hookstage.ChangeSet[hookstage.BidderRequestPayload]{}
Expand Down
Loading