Scrubber refactoring #3108

VeronikaSolovei9 · 2023-09-14T20:04:09Z

No description provided.

SyntaxNode · 2023-09-19T21:17:47Z

exchange/utils.go

@@ -177,48 +172,79 @@ func (rs *requestSplitter) cleanOpenRTBRequests(ctx context.Context,
 			}
 		}

+		ipConf := privacy.IPConf{IPV6: auctionReq.Account.Privacy.IPv6Config, IPV4: auctionReq.Account.Privacy.IPv4Config}
+
+		reqWrapper := cloneBidderReq(bidderRequest.BidRequest)


This seems like a good compromise. As a follow-up, we'll want to pass the request wrapper all the way down, but that's not possible right now since we lose it in getAuctionBidderRequests and that's too much work for this PR.

Right. Better to refactor it separately.

SyntaxNode · 2023-09-19T21:18:33Z

exchange/utils.go

+			//UFPD
+			privacy.ScrubDeviceIDs(reqWrapper)
+			privacy.ScrubUserIDs(reqWrapper)
+			privacy.ScrubUserExt(reqWrapper, "data")


As a general approach, I recommend calling just one scrubber method and manage the groupings in the scrubber code. This could be ScrubUserFPD, which per the spec includes:

user.eids, user.ext.data.*, user.data.*, user.{id, buyeruid, yob, gender} and device-specific IDs

Looks like we're missing at least eids.

Good catch! Refactored

I'll add more unit tests for these cases

SyntaxNode · 2023-09-19T21:20:32Z

exchange/utils.go

+					if !auctionPermissions.PassID {
+						privacy.ScrubDeviceIDs(reqWrapper)
+						privacy.ScrubUserDemographics(reqWrapper)
+						privacy.ScrubUserExt(reqWrapper, "eids")


Please consider calling something like privacy.ScrubGdprID and move the grouping into scrubber code. Might we be good with calling ScrubUserFPD here instead? What would be the difference in information removed?

Refactored to the separate functions. It looks cleaner to me when we have separate function per activity in this case, rather than combining then into one, despite they are very similar to each other. I'm open to refactor it to one function.

SyntaxNode · 2023-09-19T21:32:54Z

exchange/utils.go

+	var newReq *openrtb2.BidRequest
+	newReq = ptrutil.Clone(req)
+
+	if req.User != nil {


Please use ortb.CloneUser instead.

SyntaxNode · 2023-09-19T21:33:21Z

exchange/utils.go

+		}
+	}
+
+	if req.Device != nil {


Please add ortb.CloneDevice and call it from here.

There are no CloneDevice and CloneSource functions. Is there a reason not to have them? Should I add them?

Clone functions added

SyntaxNode · 2023-09-19T21:33:48Z

exchange/utils.go

+		}
+	}
+
+	if req.Source != nil {


Please add ortb.CloneSource and call it from here.

AlexBVolcy · 2023-09-19T21:52:28Z

exchange/utils.go

+	var newReq *openrtb2.BidRequest
+	newReq = ptrutil.Clone(req)


Can these lines be rewritten as newReq := ptrutil.Clone(req)

It can be, yes, but IDE will miss the variable type (because it's generic after Clone) and underlines follow up code to red. We discussed it in one of the previous PRs. It's ok to leave the variable instantiation, it doesn't take memory and CPU and makes IDE happy.

AlexBVolcy · 2023-09-19T21:52:44Z

exchange/utils.go

+		var userCopy *openrtb2.User
+		userCopy = ptrutil.Clone(req.User)


Can these lines be rewritten as userCopy := ptrutil.Clone(req.User). I see a similar pattern throughout this function as well.

Rewritten to userCopy := ortb.CloneUser(req.User)
I didn't know this function exists.

AlexBVolcy · 2023-09-19T21:57:32Z

privacy/scrubber.go

+func ScrubDeviceIP(reqWrapper *openrtb_ext.RequestWrapper, ipConf IPConf) {
+	if reqWrapper.Device != nil {
+		reqWrapper.Device.IP = scrubIP(reqWrapper.Device.IP, ipConf.IPV4.AnonKeepBits, iputil.IPv4BitSize)
+		reqWrapper.Device.IPv6 = scrubIP(reqWrapper.Device.IPv6, ipConf.IPV6.AnonKeepBits, iputil.IPv6BitSize)
 	}
+}

-	userCopy := *user
-
-	if strategy == ScrubStrategyUserIDAndDemographic {
-		userCopy.BuyerUID = ""
-		userCopy.ID = ""
-		userCopy.Ext = scrubExtIDs(userCopy.Ext, "eids")
-		userCopy.Yob = 0
-		userCopy.Gender = ""
-	}
+func ScrubDeviceIDsIPsUserDemoExt(reqWrapper *openrtb_ext.RequestWrapper, ipConf IPConf, fieldName string, scrubFullGeo bool) {
+	ScrubDeviceIDs(reqWrapper)
+	ScrubDeviceIP(reqWrapper, ipConf)
+	ScrubUserDemographics(reqWrapper)
+	ScrubUserExt(reqWrapper, fieldName)

-	switch geo {
-	case ScrubStrategyGeoFull:
-		userCopy.Geo = scrubGeoFull(user.Geo)
-	case ScrubStrategyGeoReducedPrecision:
-		userCopy.Geo = scrubGeoPrecision(user.Geo)
+	if scrubFullGeo {
+		ScrubGeoFull(reqWrapper)
+	} else {
+		ScrubGEO(reqWrapper)


I noticed a lack of test coverage here. Is this out of the scope of this PR to update this?

More unit tests are added. Consolidated scrubber functions like ScrubUserFPD are covered by integration tests in utils_test.go -> TestCleanOpenRTBRequestsActivities

SyntaxNode · 2023-10-23T01:35:09Z

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.

# Conflicts: # exchange/utils.go # exchange/utils_test.go # privacy/enforcement.go # privacy/scrubber.go

AlexBVolcy

Looks good, left test coverage and spelling comments

AlexBVolcy · 2023-10-24T22:11:38Z

privacy/scrubber.go

-func (scrubber) ScrubUser(user *openrtb2.User, strategy ScrubStrategyUser, geo ScrubStrategyGeo) *openrtb2.User {
-	if user == nil {
-		return nil
+func ScrubDeviceIP(reqWrapper *openrtb_ext.RequestWrapper, ipConf IPConf) {


Could you add tests to cover this function?

AlexBVolcy · 2023-10-24T22:11:47Z

privacy/scrubber.go


-	userCopy := *user
+func ScrubDeviceIDsIPsUserDemoExt(reqWrapper *openrtb_ext.RequestWrapper, ipConf IPConf, fieldName string, scrubFullGeo bool) {


Could you add tests to cover this function?

AlexBVolcy · 2023-10-24T22:11:58Z

privacy/scrubber.go

-	case ScrubStrategyGeoReducedPrecision:
-		userCopy.Geo = scrubGeoPrecision(user.Geo)
-	}
+func ScrubUserFPD(reqWrapper *openrtb_ext.RequestWrapper) {


Could you add tests to cover this function?

AlexBVolcy · 2023-10-24T22:12:13Z

privacy/scrubber.go

+func ScrubGdprID(reqWrapper *openrtb_ext.RequestWrapper) {
+	ScrubDeviceIDs(reqWrapper)
+	ScrubUserDemographics(reqWrapper)
+	ScrubUserExt(reqWrapper, "eids")
+}
+
+func ScrubGeoAndDeviceIP(reqWrapper *openrtb_ext.RequestWrapper, ipConf IPConf) {


Could you add tests to cover these functions?

AlexBVolcy · 2023-10-24T22:16:14Z

ortb/clone.go

+	return &c
+}
+
+func CloseSupplyChainNodes(s []openrtb2.SupplyChainNode) []openrtb2.SupplyChainNode {


Is there a type in this function name?

CloseSupplyChainNodes --> CloneSupplyChainNodes

VeronikaSolovei9 · 2023-10-25T20:45:23Z

These functions are covered from utils_test.go.
The reason for this is because they mostly have calls to other "Scrub" functions.
Utils_test.go also has tests where privacy and activities are set up in a way to execute and check logic in these functions.

bsardo · 2023-10-27T17:45:59Z

These functions are covered from utils_test.go. The reason for this is because they mostly have calls to other "Scrub" functions. Utils_test.go also has tests where privacy and activities are set up in a way to execute and check logic in these functions.

I generally agree with this approach. Even though these functions are public and I prefer to have unit tests for functions of this kind, most of the complex logic lives in private functions which are thoroughly tested in their own unit tests. There are also adequate tests in utils_test.go that ensure that these public functions are called and do what they are supposed to. One might argue that a simple unit test should be added for any function that has conditional logic just to ensure edge cases are handled (e.g. ScrubDeviceIP appropriately handles a nil Device) so that we don't have to scrutinize the integration tests in utils_test.go to make sure these edge cases are addressed, but rather can just focus on whether the right function was called.

bsardo · 2023-10-26T15:59:23Z

privacy/scrubber.go

-				delete(userExtParsed, "data")
-				userExtModified = true
-			}
+func ScrubUserExt(reqWrapper *openrtb_ext.RequestWrapper, fieldName string) error {


Should this be a private function?

bsardo · 2023-10-26T15:59:53Z

privacy/scrubber.go

-	if userExtModified {
-		userExt, _ := jsonutil.Marshal(userExtParsed)
-		userCopy.Ext = userExt
+func ScrubEids(reqWrapper *openrtb_ext.RequestWrapper) error {


Nitpick: ScrubEIDs
Edit: This function isn't used. Do we need it?

Yes, this is a function to support transmitEids activity, it will be added in the next PR.
Renamed

bsardo · 2023-10-26T16:17:03Z

privacy/scrubber.go

+	if reqWrapper.Device != nil {
+		if reqWrapper.Device.Geo != nil {


Nitpick: flatten by combining these two if statements with &&

Yes, refactored

bsardo · 2023-10-26T16:36:10Z

privacy/scrubber.go

-type scrubber struct {
-	ipV6 config.IPv6
-	ipV4 config.IPv4
+func ScrubDeviceIDs(reqWrapper *openrtb_ext.RequestWrapper) {


How about making some of these functions private that are meant to only be used within this package?

Done. All functions that are used in this package only are now private

bsardo · 2023-10-26T16:42:10Z

ortb/clone.go

+	mobileCopy := &s.Mobile
+	c.Mobile = *mobileCopy


Nitpick: I think it is more idiomatic to write this as such:

mobileCopy := *s.Mobile c.Mobile = &mobileCopy

They should be functionally equivalent but typically when I see a variable named Copy I think it is the value that has been copied and not the address. Thoughts?

Yes, modified, good point

bsardo · 2023-10-27T18:22:06Z

ortb/clone_test.go

+			TID:    "Tid",
+			PChain: "PChain",
+			SChain: &openrtb2.SupplyChain{
+				Complete: 1, Nodes: []openrtb2.SupplyChainNode{{ASI: "asi", Ext: json.RawMessage(`{"anyField":1}`)}}},


Did you want the populated ext to be openrtb2.SupplyChain.Ext instead of openrtb2.SupplyChainNode.Ext?

Yes, refactored

bsardo · 2023-10-27T18:22:27Z

ortb/clone_test.go

+
+	t.Run("populated", func(t *testing.T) {
+		given := &openrtb2.SupplyChain{
+			Complete: 1, Nodes: []openrtb2.SupplyChainNode{{ASI: "asi", Ext: json.RawMessage(`{"anyField":1}`)}}}


Did you want the populated ext to be openrtb2.SupplyChain.Ext instead of openrtb2.SupplyChainNode.Ext?

It doesn't look like this change was made.

bsardo · 2023-10-27T18:23:11Z

ortb/clone_test.go

+	})
+}
+
+func TestCloseSupplyChainNodes(t *testing.T) {


Typo: TestCloneSupplyChainNodes

bsardo · 2023-10-27T18:24:29Z

ortb/clone_test.go

+		assert.NotSame(t, given[1], result[1], "item1-pointer")
+		assert.NotSame(t, given[1].Ext, result[1].Ext, "item1-pointer-ext")
+		assert.NotSame(t, given[1].HP, result[1].HP, "item1-pointer-hp")
+	})


I think we're missing a couple of asserts here:

assert.NotSame(t, given[2].Ext, result[2].Ext, "item2-pointer-ext") assert.NotSame(t, given[2].HP, result[2].HP, "item2-pointer-hp")

bsardo · 2023-10-27T18:28:52Z

ortb/clone.go

+	return &dntCopy
+}
+
+func CloneUserAgent(s *openrtb2.UserAgent) *openrtb2.UserAgent {


I don't see a unit test for this function. It appears to be an oversight as I see all other functions have their own dedicated unit test.

Added, good catch! Sorry I don't know why I didn't add this test.

bsardo · 2023-10-30T15:52:55Z

privacy/scrubber_test.go

+			expectedDevice: &openrtb2.Device{Geo: nil},
+		},
+		{
+			name:           "with_user_geo",


Typo: "with_device_geo"

Yes, renamed

bsardo · 2023-10-30T16:00:45Z

ortb/clone_test.go

+		assert.NotSame(t, given[0], result[0], "item0-pointer")
+		assert.NotSame(t, given[0].Ext, result[0].Ext, "item0-pointer-ext")
+		assert.NotSame(t, given[1], result[1], "item1-pointer")
+		assert.NotSame(t, given[1].Ext, result[1].Ext, "item1-pointer-ext")


I think you missed making this change here.

bsardo · 2023-10-30T16:03:03Z

ortb/clone_test.go

+
+	t.Run("populated", func(t *testing.T) {
+		given := &openrtb2.SupplyChain{
+			Complete: 1, Nodes: []openrtb2.SupplyChainNode{{ASI: "asi", Ext: json.RawMessage(`{"anyField":1}`)}}}


It doesn't look like this change was made.

bsardo · 2023-10-30T16:32:22Z

analytics/build/build.go

+			if !ac.Allow(privacy.ActivityTransmitUserFPD, component, privacy.ActivityRequest{}) {
+				// scrub UFPD from ao.RequestWrapper
+			}
+			if !ac.Allow(privacy.ActivityTransmitPreciseGeo, component, privacy.ActivityRequest{}) {
+				// scrub geo from ao.RequestWrapper
+			}


Did you mean to add this in this PR?

This is a part of a future PR remained in my local changes, yes, removed.

bsardo · 2023-10-30T16:40:45Z

ortb/clone_test.go

+		assert.NotSame(t, given.Architecture, result.Architecture, "architecture")
+		assert.NotSame(t, given.Bitness, result.Bitness, "bitness")
+		assert.NotSame(t, given.Model, result.Model, "model")


Nitpick: Can we delete these three assert statements since these fields are just string values? I think we only need the NotSame asserts for pointers right?

Removed. Yes non-pointers should be copied correctly as a part of shallow copy

bsardo · 2023-10-31T20:01:35Z

ortb/clone_test.go

+		assert.NotSame(t, given, result, "pointer")
+		assert.NotSame(t, given.SChain, result.SChain, "schain")
+		assert.NotSame(t, given.SChain.Ext, result.SChain.Ext, "schain.ext")
+		assert.NotSame(t, given.Ext, result.Ext, "ext")


Maybe add assert.NotSame(t, given.SChain.Nodes[0].Ext, result.SChain.Nodes[0].Ext, "schain.nodes.ext") here?

VeronikaSolovei9 added 4 commits September 13, 2023 15:16

ScrubRequest refactoring

76c472e

First working version, needs a duplicated code refactoring

571ae2a

Duplicated code refactoring

b85dc81

Added unit tests

330af18

VeronikaSolovei9 marked this pull request as ready for review September 14, 2023 23:27

hhhjort requested a review from bsardo September 18, 2023 17:30

hhhjort assigned bsardo Sep 18, 2023

bsardo assigned AlexBVolcy Sep 19, 2023

bsardo requested a review from SyntaxNode September 19, 2023 17:38

SyntaxNode reviewed Sep 19, 2023

View reviewed changes

AlexBVolcy reviewed Sep 19, 2023

View reviewed changes

bsardo added the do not port label Sep 20, 2023

VeronikaSolovei9 added 4 commits September 24, 2023 16:44

Code clean up

edf120a

Merge branch 'master' into scrubber_refactoring

b4f9ad2

Unit tests improvements

e25b154

Added clone functions

8f3dd23

VeronikaSolovei9 added 2 commits October 23, 2023 15:12

Merge branch 'master' into scrubber_refactoring

06c9e92

# Conflicts: # exchange/utils.go # exchange/utils_test.go # privacy/enforcement.go # privacy/scrubber.go

Merge fixes

f3059c2

AlexBVolcy reviewed Oct 24, 2023

View reviewed changes

VeronikaSolovei9 added 2 commits October 25, 2023 13:45

Typo fix

57e08ea

Typo fix in tests

1ee688b

AlexBVolcy previously approved these changes Oct 26, 2023

View reviewed changes

bsardo reviewed Oct 27, 2023

View reviewed changes

Code clean up

ea36a24

VeronikaSolovei9 dismissed AlexBVolcy’s stale review via ea36a24 October 29, 2023 00:16

bsardo reviewed Oct 30, 2023

View reviewed changes

Code clean up

ea4a9e3

Code clean up

a836351

bsardo reviewed Oct 31, 2023

View reviewed changes

Code clean up

8d24ed7

AlexBVolcy approved these changes Nov 1, 2023

View reviewed changes

bsardo approved these changes Nov 1, 2023

View reviewed changes

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

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

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

Privacy scrubber refactoring (prebid#3108)

bcdebfd

		var userCopy *openrtb2.User
		userCopy = ptrutil.Clone(req.User)


		userCopy := *user
		func ScrubDeviceIDsIPsUserDemoExt(reqWrapper *openrtb_ext.RequestWrapper, ipConf IPConf, fieldName string, scrubFullGeo bool) {

		if reqWrapper.Device != nil {
		if reqWrapper.Device.Geo != nil {

Scrubber refactoring #3108

Scrubber refactoring #3108

Conversation

VeronikaSolovei9 commented Sep 14, 2023

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

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

AlexBVolcy 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

VeronikaSolovei9 commented Oct 25, 2023

bsardo commented Oct 27, 2023

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

VeronikaSolovei9 Oct 29, 2023 • 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

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

VeronikaSolovei9 Oct 29, 2023 •

edited

Loading