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

VeronikaSolovei9 · 2023-09-12T03:47:00Z

Replaced BidRequest to RequestWrapper in BidderRequestPayload

exchange/bidder.go

hhhjort

LGTM

SyntaxNode · 2023-09-19T20:57:10Z

hooks/hookstage/bidderrequest_mutations.go

 	if payload, ok := any(p).(BidderRequestPayload); ok {
-		if payload.BidRequest == nil {
+		if payload.RequestWrapper == nil {
 			return nil, errors.New("empty BidRequest provided")


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".

SyntaxNode · 2023-09-19T20:58:15Z

hooks/hookstage/bidderrequest.go

@@ -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


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.

Renamed. Also renamed RequestWrapper to BidRequest in ProcessedAuctionRequestPayload

SyntaxNode · 2023-09-19T20:58:59Z

modules/prebid/ortb2blocking/hook_bidderrequest.go

@@ -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")


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

It's harder than it seems. Is "empty input provided" ok?

bsardo · 2023-09-28T15:38:31Z

hooks/hookstage/bidderrequest_mutations.go

 		if err == nil {
-			bidRequest.BAdv = badv
+			bidRequestWrapper.BAdv = badv


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.

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 {...}

bsardo · 2023-09-28T15:57:03Z

hooks/hookstage/bidderrequest_mutations.go

@@ -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)


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

bsardo · 2023-09-28T16:01:25Z

modules/prebid/ortb2blocking/hook_bidderrequest.go

-	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")


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

hhhjort

LGTM

pm-nikhil-vaidya · 2023-10-04T06:41:57Z

hooks/hookstage/processedauctionrequest.go

@@ -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


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

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?

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

Yep Request will be fine

AlexBVolcy · 2023-10-04T21:59:13Z

exchange/bidder.go

@@ -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}


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.

renamed to request, similar to other renames

AlexBVolcy · 2023-10-04T22:02:17Z

hooks/hookstage/processedauctionrequest.go

@@ -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


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

SyntaxNode · 2023-10-23T01:33:59Z

Prebid Server 2.0 has been released and Go Module name has changed from github.com/prebid/prebid-server to github.com/prebid/prebid-server/v2, per Go versioning conventions.

Please merge the master branch and update all prebid-server import statements to reference the v2 name change. Thank you.

…rebid#3096)

VeronikaSolovei9 added 2 commits September 11, 2023 20:44

Replaced BidRequest to RequestWrapper in BidderRequestPayload

79ac96c

Removed test config

cdc8f22

SyntaxNode reviewed Sep 12, 2023

View reviewed changes

exchange/bidder.go Outdated Show resolved Hide resolved

SyntaxNode added the do not port label Sep 12, 2023

hhhjort self-assigned this Sep 12, 2023

Added RebuildRequest

39f39d4

SyntaxNode self-assigned this Sep 12, 2023

hhhjort previously approved these changes Sep 19, 2023

View reviewed changes

SyntaxNode reviewed Sep 19, 2023

View reviewed changes

Code clean up

80daab8

VeronikaSolovei9 dismissed hhhjort’s stale review via 80daab8 September 24, 2023 23:09

Merge branch 'master' into bidderRequestPayload_RequestWrapper

64ca130

bsardo assigned bsardo and unassigned SyntaxNode Sep 27, 2023

bsardo changed the title ~~BidderRequestPayload - RequestWrapper~~ Modularity: Make request wrapper accessible in bidder request hook Sep 28, 2023

bsardo reviewed Sep 28, 2023

View reviewed changes

Code clean up

358c3b4

hhhjort previously approved these changes Oct 3, 2023

View reviewed changes

pm-nikhil-vaidya reviewed Oct 4, 2023

View reviewed changes

bsardo assigned AlexBVolcy and unassigned bsardo Oct 4, 2023

AlexBVolcy reviewed Oct 4, 2023

View reviewed changes

Code clean up

d841a8a

VeronikaSolovei9 dismissed hhhjort’s stale review via d841a8a October 5, 2023 20:11

VeronikaSolovei9 added 2 commits October 23, 2023 14:05

Merge branch 'master' into bidderRequestPayload_RequestWrapper

37a5e5a

Merge fixes

c5a7249

AlexBVolcy approved these changes Oct 23, 2023

View reviewed changes

hhhjort approved these changes Oct 24, 2023

View reviewed changes

bsardo merged commit f9f5546 into master Nov 16, 2023
3 checks passed

SyntaxNode deleted the bidderRequestPayload_RequestWrapper branch November 17, 2023 22:13

svamiftah pushed a commit to sovrn/prebid-server that referenced this pull request Nov 21, 2023

Modularity: Make request wrapper accessible in bidder request hook (p…

72f722a

…rebid#3096)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

VeronikaSolovei9 commented Sep 12, 2023

hhhjort left a comment

SyntaxNode Sep 19, 2023

SyntaxNode Sep 19, 2023

VeronikaSolovei9 Sep 24, 2023

SyntaxNode Sep 19, 2023

VeronikaSolovei9 Sep 24, 2023

bsardo Sep 28, 2023

VeronikaSolovei9 Sep 28, 2023

bsardo Sep 28, 2023

bsardo Sep 28, 2023

hhhjort left a comment

pm-nikhil-vaidya Oct 4, 2023

SyntaxNode Oct 4, 2023

AlexBVolcy Oct 4, 2023

pm-nikhil-vaidya Oct 5, 2023

VeronikaSolovei9 Oct 5, 2023

AlexBVolcy Oct 4, 2023

VeronikaSolovei9 Oct 5, 2023

AlexBVolcy Oct 4, 2023

SyntaxNode commented Oct 23, 2023

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

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

Conversation

VeronikaSolovei9 commented Sep 12, 2023

hhhjort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhhjort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SyntaxNode commented Oct 23, 2023