ORTB 2.6: Push wrapper into Eidpermissions and ApplyFPD #3998

VeronikaSolovei9 · 2024-10-19T05:43:20Z

No description provided.

VeronikaSolovei9 · 2024-10-19T05:45:51Z

exchange/utils.go

+		// eid scrubbing
+		if err := removeUnpermissionedEids(reqWrapperCopy, bidder); err != nil {
+			errs = append(errs, fmt.Errorf("unable to enforce request.ext.prebid.data.eidpermissions because %v", err))
+			continue
+		}
+


removeUnpermissionedEids function should be executed before buildRequestExtForBidder because it needs access to req.ext.prebid.data.eidpermissions.

buildRequestExtForBidder function rebuilds req.ext and doesn't add req.ext.prebid.data.eidpermissions.

I think you're right. It looks like we just need to read from req.ext.prebid.data and copy any applicable EIDs to req.user and then we can discard req.ext.prebid.data. If that's the case then we should move removeUnpermissionedEids just before buildRequestExtForBidder since buildRequestExtForBidder will delete req.ext.prebid.data.

hhhjort

LGTM

bsardo · 2024-10-21T15:42:53Z

exchange/utils.go

-
-	// exit early if there are no eids (empty array)
-	if len(eids) == 0 {
+	if reqExt.GetPrebid() == nil || reqExt.GetPrebid().Data == nil || len(reqExt.GetPrebid().Data.EidPermissions) == 0 {


You might want to consider setting GetPrebid() to a variable to avoid the unnecessary struct copy operation that will take place each time it is called:

reqExtPrebid := reqExt.GetPrebid() if reqExtPrebid == nil || reqExtPrebid.Data == nil || len(reqExtPrebid.Data.EidPermissions) == 0 { return nil }

You can also use this variable in the range loop on line 783.

bsardo · 2024-10-21T15:56:04Z

exchange/utils.go

 	if fpdToApply.User != nil {
 		//BuyerUID is a value obtained between fpd extraction and fpd application.
 		//BuyerUID needs to be set back to fpd before applying this fpd to final bidder request
-		if bidRequest.User != nil && len(bidRequest.User.BuyerUID) > 0 {
-			fpdToApply.User.BuyerUID = bidRequest.User.BuyerUID
+		if reqWrapper.User != nil && len(reqWrapper.User.BuyerUID) > 0 {
+			fpdToApply.User.BuyerUID = reqWrapper.User.BuyerUID
 		}
-		bidRequest.User = fpdToApply.User
+		reqWrapper.User = fpdToApply.User
 	}


I think we might have always had a bug here. Notice how we first update fpdToApply.User.BuyerUID to reqWrapper.User.BuyerUID before assigning fpdToApply.User to reqWrapper.User in case the buyer UID was updated after fpdToApply.User was populated upstream. Similarly, I think we should copy reqWrapper.User.EIDs to fpdToApply.User.EIDs because EIDs might have been removed by removeUnpermissionedEids.

bsardo · 2024-10-21T16:02:46Z

exchange/utils.go

 		}
-		bidRequest.User = fpdToApply.User
+		reqWrapper.User = fpdToApply.User


Just checking, this statement will change the user object pointer on reqWrapper to point to the user object on fpdToApply. I believe the fpdToApply user object may contain changes to the user ext. Will the request wrapper behave correctly when RebuildRequest is called or will the ext changes be lost? I think the changes may be lost if a change is made to the UserExt object on the request wrapper that result in it being marked as dirty so that a user ext rebuild is triggered when RebuildRequest is called.
If you all agree that this is a problem, I think it could be fixed in a future PR after we have released ORTB 2.6 support since I think this issue has always existed.

Yes, if fpdToApply.User.ext has been modified, then the wrapper will overwrite those changes when the request is rebuilt.

Good point, we will need to fix it.

bsardo · 2024-10-21T18:23:54Z

exchange/utils.go

+				//BuyerUID needs to be set back to fpd before applying this fpd to final bidder request
+				fpdToApply.User.BuyerUID = reqWrapper.User.BuyerUID
+			}
+			if len(reqWrapper.User.EIDs) > 0 {


I don't think we want the conditional. Maybe this is simply just the statement fpdToApply.User.EIDs = reqWrapper.User.EIDs so it always executes. It's possible that there were EIDs but they were all removed bringing the count to 0. Thoughts?

Discussed offline. More changes are in progress

SyntaxNode · 2024-10-21T20:19:57Z

exchange/utils.go

-	if len(userExt) == 0 {
-		setUserExtWithCopy(request, nil)
-		return nil
+		reqWrapper.User.EIDs = eidsAllowed
 	}


Do we need both cases anymore? If eidsAllowed is null when there are no eids allowed, then we can get rid of the if statement and just set reqWrapper.User.EIDs = eidsAllowed.

I think we can remove the conditional here if we change
eidsAllowed := make([]openrtb2.EID, 0, len(eids)) to var eidsAllowed []openrtb2.EID
If we do that eidsAllowed will start off as nil, however there will be the performance penalty when building up that slice via append.
@SyntaxNode thoughts?

Discussed offline. We'll keep it the way it is using make to initialize eidsAllowed and the conditional check here.

…removeUnpermissionedEids

bsardo · 2024-10-22T16:00:51Z

exchange/utils.go

@@ -157,6 +157,8 @@ func (rs *requestSplitter) cleanOpenRTBRequests(ctx context.Context,
 	bidderRequests = make([]BidderRequest, 0, len(impsByBidder))

 	for bidder, imps := range impsByBidder {
+		userEIDsOverride := fpdUserEIDExists(req, auctionReq.FirstPartyData, bidder)
+		fpdUserEIDExists(req, auctionReq.FirstPartyData, bidder)


I think you can delete the second occurrence of fpdUserEIDExists here.

Yes, good catch, I used it for the debugging and forgot to remove.

bsardo · 2024-10-22T16:06:30Z

exchange/utils.go

@@ -157,6 +157,8 @@ func (rs *requestSplitter) cleanOpenRTBRequests(ctx context.Context,
 	bidderRequests = make([]BidderRequest, 0, len(impsByBidder))

 	for bidder, imps := range impsByBidder {
+		userEIDsOverride := fpdUserEIDExists(req, auctionReq.FirstPartyData, bidder)


How about changing this variable name to fpdUserEIDsExist or fpdUserEIDsPresent? I think this will be much easier to understand downstream if you make this same change to the parameter passed to applyFPD as your conditional there will then read:

if !fpdUserEIDsExist { fpdToApply.User.EIDs = reqWrapper.User.EIDs }

The override language confuses me a bit.

bsardo · 2024-10-22T16:11:39Z

exchange/utils.go

+	if len(fpdUserEIDs) == 0 {
+		return false
+	}


Can we remove this statement? Technically it is possible that EID is specified as first party data as an empty array.
We can just lean on your length check and for loop below to make the determination.

bsardo · 2024-10-22T16:13:05Z

exchange/utils.go

+		pReqUserEID := &reqUserEIDs[i]
+		pFpdUserEID := &fpdUserEIDs[i]
+		if pReqUserEID != pFpdUserEID {
+			return false


Isn't this supposed to be return true?

bsardo · 2024-10-22T16:13:17Z

exchange/utils.go

+		}
+
+	}
+	return true


Isn't this supposed to be return false?

bsardo · 2024-10-22T16:15:33Z

exchange/utils.go

-			fpdToApply.User.EIDs = reqWrapper.User.EIDs
+
+			// if FPD config didn't have user.eids - use reqWrapper.User.EIDs after removeUnpermissionedEids
+			if userEIDsOverride {


As mentioned in an earlier comment, I suggest changing this variable name to fpdUserEIDsExist so the meaning is clearer.

Also, isn't this conditional supposed to be negated: if !fpdUserEIDsExist?

bsardo · 2024-10-22T16:21:54Z

exchange/utils.go

@@ -819,37 +844,15 @@ func removeUnpermissionedEids(request *openrtb2.BidRequest, bidder string, reque
 		return nil
 	}

-	// marshal eidsAllowed back to userExt
+	// set eidsAllowed back to userExt


Can you either change the userExt part of this comment or delete the comment?

bsardo · 2024-10-22T16:24:09Z

exchange/utils.go

-	if len(userExt) == 0 {
-		setUserExtWithCopy(request, nil)
-		return nil
+		reqWrapper.User.EIDs = eidsAllowed
 	}


I think we can remove the conditional here if we change
eidsAllowed := make([]openrtb2.EID, 0, len(eids)) to var eidsAllowed []openrtb2.EID
If we do that eidsAllowed will start off as nil, however there will be the performance penalty when building up that slice via append.
@SyntaxNode thoughts?

…r removeUnpermissionedEids

bsardo · 2024-10-23T16:39:10Z

exchange/utils.go

+		return false
+	}
+	fpdUserEIDs := fpdToApply.User.EIDs
+	reqUserEIDs := req.User.EIDs


Could User be nil?

if len(fpdUserEIDs) == 0 { return false } if req.User == nil { return true }

bsardo · 2024-10-23T16:47:26Z

exchange/utils_test.go

+			description: "req.User is defined and had bidder fpd user eids (userEIDsOverride); bidderFPD.User defined and has EIDs. Expect to see user.EIDs in result request taken from fpd",
+			inputFpd: map[openrtb_ext.BidderName]*firstpartydata.ResolvedFirstPartyData{
+				"bidderNormalized": {Site: &openrtb2.Site{ID: "SiteId"}, App: &openrtb2.App{ID: "AppId"}, User: &openrtb2.User{ID: "UserId", EIDs: []openrtb2.EID{{Source: "source1"}, {Source: "source2"}}}},
+			},
+			inputBidderName:           "bidderFromRequest",
+			inputBidderCoreName:       "bidderNormalized",
+			inputBidderIsRequestAlias: false,
+			inputRequest:              openrtb2.BidRequest{User: &openrtb2.User{ID: "UserId", EIDs: []openrtb2.EID{{Source: "source3"}, {Source: "source4"}}}},
+			expectedRequest:           openrtb2.BidRequest{Site: &openrtb2.Site{ID: "SiteId"}, App: &openrtb2.App{ID: "AppId"}, User: &openrtb2.User{ID: "UserId", EIDs: []openrtb2.EID{{Source: "source1"}, {Source: "source2"}}}},
+			fpdUserEIDsExisted:        true,
+		},
+		{
+			description: "req.User is defined and doesn't have fpr user eids (userEIDsOverride); bidderFPD.User defined and has EIDs. Expect to see user.EIDs in result request taken from original req",
+			inputFpd: map[openrtb_ext.BidderName]*firstpartydata.ResolvedFirstPartyData{
+				"bidderNormalized": {Site: &openrtb2.Site{ID: "SiteId"}, App: &openrtb2.App{ID: "AppId"}, User: &openrtb2.User{ID: "UserId", EIDs: []openrtb2.EID{{Source: "source1"}, {Source: "source2"}}}},
+			},
+			inputBidderName:           "bidderFromRequest",
+			inputBidderCoreName:       "bidderNormalized",
+			inputBidderIsRequestAlias: false,
+			inputRequest:              openrtb2.BidRequest{User: &openrtb2.User{ID: "UserId", EIDs: []openrtb2.EID{{Source: "source3"}, {Source: "source4"}}}},
+			expectedRequest:           openrtb2.BidRequest{Site: &openrtb2.Site{ID: "SiteId"}, App: &openrtb2.App{ID: "AppId"}, User: &openrtb2.User{ID: "UserId", EIDs: []openrtb2.EID{{Source: "source3"}, {Source: "source4"}}}},
+			fpdUserEIDsExisted:        false,
+		},


Nitpick: remove references to variable userEIDsOverride in the descriptions.

bsardo · 2024-10-23T16:50:53Z

exchange/utils_test.go

+				reqWrapper,
+				testCase.fpdUserEIDsExisted,
+			)
+			assert.Equal(t, &testCase.expectedRequest, reqWrapper.BidRequest, fmt.Sprintf("incorrect request after applying fpd, testcase %s", testCase.description))


Nitpick: you can delete the last parameter now that you're using t.Run.

bsardo · 2024-10-23T16:54:13Z

exchange/utils_test.go

+			assert.NoError(t, resultErr, test.description)
+			assert.Equal(t, expectedRequest, reqWrapper.BidRequest, test.description)


Nitpick: you can delete the last parameter now that you're using t.Run.

hhhjort

LGTM

Added Request wrapper to removeUnpermissionedEids

26d3c35

VeronikaSolovei9 changed the base branch from master to ortb26 October 19, 2024 05:43

VeronikaSolovei9 commented Oct 19, 2024

View reviewed changes

Added Request wrapper to applyFPD

f8d612f

bsardo changed the title ~~Eidpermissions reqwrapper~~ ORTB 2.6: Push wrapper into Eidpermissions and ApplyFPD Oct 20, 2024

bsardo assigned bsardo and hhhjort Oct 21, 2024

bsardo added the do not port label Oct 21, 2024

hhhjort approved these changes Oct 21, 2024

View reviewed changes

bsardo reviewed Oct 21, 2024

View reviewed changes

Code review fixes

3024b9c

bsardo reviewed Oct 21, 2024

View reviewed changes

Code review fixes

25dbef0

SyntaxNode reviewed Oct 21, 2024

View reviewed changes

Added handling for conflicting FPD user eids and EIDs scrubbed after …

8b1e291

…removeUnpermissionedEids

bsardo reviewed Oct 22, 2024

View reviewed changes

Fixes in handling of conflicting FPD user eids and EIDs scrubbed afte…

e6f5e65

…r removeUnpermissionedEids

hhhjort approved these changes Oct 23, 2024

View reviewed changes

bsardo reviewed Oct 23, 2024

View reviewed changes

Code review fixes

b615b24

bsardo approved these changes Oct 24, 2024

View reviewed changes

hhhjort approved these changes Oct 24, 2024

View reviewed changes

hhhjort merged commit b2d6db8 into ortb26 Oct 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ORTB 2.6: Push wrapper into Eidpermissions and ApplyFPD #3998

ORTB 2.6: Push wrapper into Eidpermissions and ApplyFPD #3998

VeronikaSolovei9 commented Oct 19, 2024

VeronikaSolovei9 Oct 19, 2024

bsardo Oct 19, 2024 •

edited

Loading

hhhjort left a comment

bsardo Oct 21, 2024

bsardo Oct 21, 2024

bsardo Oct 21, 2024

hhhjort Oct 21, 2024

VeronikaSolovei9 Oct 21, 2024

bsardo Oct 21, 2024

VeronikaSolovei9 Oct 21, 2024

VeronikaSolovei9 Oct 21, 2024

SyntaxNode Oct 21, 2024

bsardo Oct 22, 2024 •

edited

Loading

bsardo Oct 22, 2024

bsardo Oct 22, 2024

VeronikaSolovei9 Oct 22, 2024

bsardo Oct 22, 2024

bsardo Oct 22, 2024

bsardo Oct 22, 2024

bsardo Oct 22, 2024

VeronikaSolovei9 Oct 22, 2024

bsardo Oct 22, 2024

VeronikaSolovei9 Oct 22, 2024

bsardo Oct 22, 2024

bsardo Oct 22, 2024 •

edited

Loading

bsardo Oct 23, 2024

bsardo Oct 23, 2024

bsardo Oct 23, 2024

bsardo Oct 23, 2024

bsardo Oct 23, 2024

hhhjort left a comment

		assert.NoError(t, resultErr, test.description)
		assert.Equal(t, expectedRequest, reqWrapper.BidRequest, test.description)

ORTB 2.6: Push wrapper into Eidpermissions and ApplyFPD #3998

ORTB 2.6: Push wrapper into Eidpermissions and ApplyFPD #3998

Conversation

VeronikaSolovei9 commented Oct 19, 2024

Choose a reason for hiding this comment

bsardo Oct 19, 2024 • edited Loading

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

Choose a reason for hiding this comment

bsardo Oct 22, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsardo Oct 22, 2024 • edited Loading

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

bsardo Oct 19, 2024 •

edited

Loading

bsardo Oct 22, 2024 •

edited

Loading

bsardo Oct 22, 2024 •

edited

Loading