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 5 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 @@ -130,7 +130,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))
if reject != nil {
return nil, extraBidderRespInfo{}, []error{reject}
}
Expand All @@ -142,6 +143,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
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 @@ -160,7 +160,7 @@ func (e *hookExecutor) ExecuteProcessedAuctionStage(request *openrtb_ext.Request

stageName := hooks.StageProcessedAuctionRequest.String()
executionCtx := e.newContext(stageName)
payload := hookstage.ProcessedAuctionRequestPayload{RequestWrapper: request}
payload := hookstage.ProcessedAuctionRequestPayload{BidRequest: request}

outcome, _, contexts, reject := executeStage(executionCtx, plan, payload, handler, e.metricEngine)
outcome.Entity = entityAuctionRequest
Expand All @@ -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 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
8 changes: 4 additions & 4 deletions hooks/hookexecution/mocks_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ func (e mockTimeoutHook) HandleProcessedAuctionHook(_ context.Context, _ hooksta
time.Sleep(20 * time.Millisecond)
c := hookstage.ChangeSet[hookstage.ProcessedAuctionRequestPayload]{}
c.AddMutation(func(payload hookstage.ProcessedAuctionRequestPayload) (hookstage.ProcessedAuctionRequestPayload, error) {
payload.RequestWrapper.User.CustomData = "some-custom-data"
payload.BidRequest.User.CustomData = "some-custom-data"
return payload, nil
}, hookstage.MutationUpdate, "bidRequest", "user.customData")

Expand Down Expand Up @@ -305,8 +305,8 @@ func (e mockUpdateBidRequestHook) HandleProcessedAuctionHook(_ context.Context,
c := hookstage.ChangeSet[hookstage.ProcessedAuctionRequestPayload]{}
c.AddMutation(
func(payload hookstage.ProcessedAuctionRequestPayload) (hookstage.ProcessedAuctionRequestPayload, error) {
payload.RequestWrapper.User.Yob = 2000
userExt, err := payload.RequestWrapper.GetUserExt()
payload.BidRequest.User.Yob = 2000
userExt, err := payload.BidRequest.GetUserExt()
if err != nil {
return payload, err
}
Expand All @@ -318,7 +318,7 @@ func (e mockUpdateBidRequestHook) HandleProcessedAuctionHook(_ context.Context,
}, hookstage.MutationUpdate, "bidRequest", "user.yob",
).AddMutation(
func(payload hookstage.ProcessedAuctionRequestPayload) (hookstage.ProcessedAuctionRequestPayload, error) {
payload.RequestWrapper.User.Consent = "true"
payload.BidRequest.User.Consent = "true"
return payload, nil
}, hookstage.MutationUpdate, "bidRequest", "user.consent",
)
Expand Down
5 changes: 2 additions & 3 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
BidRequest *openrtb_ext.RequestWrapper
Bidder string
}
10 changes: 5 additions & 5 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,10 +31,10 @@ 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 {
return nil, errors.New("empty BidRequest provided")
return nil, errors.New("payload contains a nil bid request")
}
return payload.BidRequest, nil
}
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
2 changes: 1 addition & 1 deletion hooks/hookstage/processedauctionrequest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,5 @@ type ProcessedAuctionRequest interface {
// ProcessedAuctionRequestPayload consists of the openrtb_ext.RequestWrapper object.
// Hooks are allowed to modify openrtb_ext.RequestWrapper using mutations.
type ProcessedAuctionRequestPayload struct {
RequestWrapper *openrtb_ext.RequestWrapper
BidRequest *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.

I think naming BidRequest to request wrapper object will be misleading as openrtb_ext.RequestWrapper also have bidRequest object embedded
What are your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

The Request Wrapper is an internal concept for optimization. I don't find the Bid Request misleading, since the RequestWrapper embeds the BidRequest it acts as a drop-in replacement with enhanced functionality.

Would it be better to name this simply Request instead of BidRequest?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I prefer it being named RequestWrapper since when we actually go ahead and access the BidRequest within the wrapper, right now it looks like payload.BidRequest.BidRequest, and I think it's clearer for me to read something like payload.RequestWrapper.BidRequest. I'm also fine with it being payload.Request.BidRequest

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep Request will be fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}
6 changes: 3 additions & 3 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 {
return result, hookexecution.NewFailure("empty BidRequest provided")
if payload.BidRequest == nil || payload.BidRequest.BidRequest == nil {
return result, hookexecution.NewFailure("empty input provided")
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about we use the same error message we now use in the mutation code: "payload contains a nil bid request"?

}

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

Expand Down
9 changes: 6 additions & 3 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 @@ -497,7 +498,7 @@ func TestHandleBidderRequestHook(t *testing.T) {
bidRequest: nil,
expectedBidRequest: nil,
expectedHookResult: hookstage.HookResult[hookstage.BidderRequestPayload]{},
expectedError: hookexecution.NewFailure("empty BidRequest provided"),
expectedError: hookexecution.NewFailure("empty input provided"),
},
{
description: "Expect baadv error if bidders and media_types not defined in config conditions",
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, BidRequest: &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.BidRequest.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