update cookie sync endpoint to be case insensitive #3103

gargcreation1992 · 2023-09-14T06:07:00Z

Tested locally with /cookie_sync POST endpoint with following body -

{
"bidders": ["AppnexUs"],
"coopsync": false,
"limit": 1
}

and got this sample response -

{
  "status" : "no_cookie",
  "bidder_status" : [
    {
      "no_cookie" : true,
      "bidder" : "AppnexUs",
      "usersync" : {
        "url" : "https://ib.adnxs.com/getuid?http%3A%2F%2Flocalhost%3A8000%2Fsetuid%3Fbidder%3Dappnexus%26gdpr%3D%26gdpr_consent%3D%26gpp%3D%26gpp_sid%3D%26f%3Di%26uid%3D%24UID",
        "type" : "redirect"
      }
    }
  ]
}

partially resolves #2400

onkarvhanumante · 2023-09-15T06:09:11Z

usersync/chooser.go

+		return nil, BidderEvaluation{Status: StatusUnknownBidder, Bidder: bidder}
+	}
+
+	syncer, exists := c.bidderSyncerLookup[coreBidder.String()]
 	if !exists {
 		return nil, BidderEvaluation{Status: StatusUnknownBidder, Bidder: bidder}


Line 145 and 150 returns same error. How about changing Line 150 to syncerKeyNotFound status

syncerKeyNotFound seems irrelevant to me here since this check is not related with syncher keys.
This is actually checking if there is a coreBidder exists for the provided bidder in the request to which we are returning UnknownBidder if there is no coreBidder found for the provided bidder and that sounds ok to me.

Line 150 will become StatusUnconfiguredBidder in #3107.

onkarvhanumante

one suggestion. otherwise looks good to me 👍🏼

AlexBVolcy · 2023-09-20T18:51:13Z

endpoints/cookie_sync_test.go

@@ -87,7 +87,7 @@ func TestNewCookieSyncEndpoint(t *testing.T) {
 	assert.IsType(t, &cookieSyncEndpoint{}, endpoint)

 	assert.Equal(t, expected.config, result.config)
-	assert.Equal(t, expected.chooser, result.chooser)
+	assert.ObjectsAreEqualValues(expected.chooser, result.chooser)


For my own understanding, why was this changed?

When we mocked normalizedBidderNamesLookup field in standardChooser - apparently, go changes the address of the struct by copying the values to a new struct. Hence, the address are different however the values are same and that is what the assert statement is doing.

gargcreation1992 · 2023-09-27T05:32:07Z

@bsardo Requesting you to please comment on the reason for the blocked label on this PR.

bsardo · 2023-09-28T19:14:59Z

This one is marked as blocked because it was determined that it conflicts with #3107 and the team came to the conclusion that we should try to merge that in first. It would be nice to get this in 2.0 but it isn't essential.

SyntaxNode · 2023-10-06T18:51:29Z

Unblocking. This is higher priority than the other PR.

SyntaxNode

You also need to add case insensitivity for type filters and account for privacy checks in the usersyncPrivacy methods. Please add tests for those areas too.

SyntaxNode · 2023-10-06T18:57:50Z

usersync/chooser.go

+		bidderSyncerLookup:          bidderSyncerLookup,
+		biddersAvailable:            bidders,
+		bidderChooser:               standardBidderChooser{shuffler: randomShuffler{}},
+		normalizedBidderNamesLookup: openrtb_ext.NormalizeBidderName,


Can shorten to normalizeBidderName, or perhaps to express this includes both normalization and validation. Something like: normalizeValidBidderName, normalizeAndValidateBidder, etc...

SyntaxNode · 2023-10-06T19:02:10Z

usersync/chooser.go

+		return nil, BidderEvaluation{Status: StatusUnknownBidder, Bidder: bidder}
+	}
+
+	syncer, exists := c.bidderSyncerLookup[coreBidder.String()]
 	if !exists {
 		return nil, BidderEvaluation{Status: StatusUnknownBidder, Bidder: bidder}


Line 150 will become StatusUnconfiguredBidder in #3107.

SyntaxNode · 2023-10-06T19:03:58Z

usersync/chooser.go

@@ -136,7 +140,12 @@ func (c standardChooser) Choose(request Request, cookie *Cookie) Result {
 }

 func (c standardChooser) evaluate(bidder string, syncersSeen map[string]struct{}, syncTypeFilter SyncTypeFilter, privacy Privacy, cookie *Cookie) (Syncer, BidderEvaluation) {
-	syncer, exists := c.bidderSyncerLookup[bidder]
+	coreBidder, exists := c.normalizedBidderNamesLookup(bidder)


This is not a coreBidder because it could also be an alias. Consider renaming to something like bidderNormalized.

There's an issue here where the original-cased bidder name is passed to GDPRAllowsBidderSync further below, which checks the original-cased bidder name against a list of all lowercased bidder names. So it means this function always returns false.

Should we just pass the coreBidder to these functions instead? To preserve the case insensitivity.

SyntaxNode · 2023-10-06T19:05:45Z

usersync/chooser_test.go

+				BiddersEvaluated: []BidderEvaluation{{Bidder: "a", Status: StatusUnknownBidder}},
+				SyncersChosen:    []SyncerChoice{},
+			},
+		},
 	}


Please add test cases for case insensitivity. We're not yet testing the new normalizedBidderNamesLookup call.

SyntaxNode · 2023-10-06T19:06:21Z

usersync/chooser_test.go

 			expectedSyncer:     nil,
-			expectedEvaluation: BidderEvaluation{Bidder: "a", SyncerKey: "keyA", Status: StatusBlockedByPrivacy},
+			expectedEvaluation: BidderEvaluation{Bidder: "a", Status: StatusUnknownBidder},
 		},


Same here. Please add a case insensitivity test.

SyntaxNode · 2023-10-06T19:12:52Z

Please also add a test for the chooser StatusDuplicate case, to make sure a duplicated bidder name triggers this code where the bidder name in the evaluated list matches what was provided in the request.

AlexBVolcy · 2023-10-09T22:40:04Z

I think there's a problem in evaluate(), I left a comment describing the issue.

Test cases for case insensitive bidder names and duplicate bidder names

gargcreation1992 · 2023-10-10T07:03:41Z

You also need to add case insensitivity for type filters and account for privacy checks in the usersyncPrivacy methods. Please add tests for those areas too.

For sync type filter - 88209d7
For UserSyncPrivacy - 176b887

AlexBVolcy · 2023-10-10T23:38:59Z

usersync/chooser.go

@@ -147,7 +159,7 @@ func (c standardChooser) evaluate(bidder string, syncersSeen map[string]struct{}
 	}
 	syncersSeen[syncer.Key()] = struct{}{}

-	if !syncer.SupportsType(syncTypeFilter.ForBidder(bidder)) {
+	if !syncer.SupportsType(syncTypeFilter.ForBidder(strings.ToLower(bidder))) {


Can't this line also use bidderNormalized.String()? And we don't have include the strings package into this file?

We cannot use normalised bidder name here and the reason is there is biddersLookup map that is created in NewSpecificBidderFilter function with key of the map being lower cased. . This map is used when we call syncTypeFilter.ForBidder function. Hope this clarifies.

I see, but when I subbed in bidderNomarlized.String() for strings.ToLower(bidder), all of the tests still passed. This is more of a nitpick if anything, so I'm happy to approve if you want to stick with the current code.

@AlexBVolcy It is because test cases are setup that way. If you setup a test case where you use suntContent as bidder name, you will be able to fail your test. In fact I have updated one of the test to reflect this - c124abb

update cookie sync endpoint to be case insensitive

e8802d0

Sonali-More-Xandr self-assigned this Sep 14, 2023

SyntaxNode assigned AlexBVolcy Sep 14, 2023

onkarvhanumante self-assigned this Sep 15, 2023

onkarvhanumante reviewed Sep 15, 2023

View reviewed changes

AlexBVolcy reviewed Sep 20, 2023

View reviewed changes

gargcreation1992 added the PBS 2.0 label Sep 25, 2023

onkarvhanumante previously approved these changes Sep 26, 2023

View reviewed changes

bsardo added blocked and removed PBS 2.0 labels Sep 26, 2023

bsardo unassigned Sonali-More-Xandr Sep 26, 2023

SyntaxNode removed the blocked label Oct 6, 2023

SyntaxNode reviewed Oct 6, 2023

View reviewed changes

gargcreation1992 added 4 commits October 10, 2023 12:18

rename coreBidder to bidderNormalized

c7685f9

add missing test cases

526ce55

Test cases for case insensitive bidder names and duplicate bidder names

make syncTypeFilter case insensitive

88209d7

make privacy methods case insensitive

176b887

gargcreation1992 dismissed onkarvhanumante’s stale review via 176b887 October 10, 2023 07:01

AlexBVolcy reviewed Oct 10, 2023

View reviewed changes

bsardo added do not port PBS 2.0 labels Oct 11, 2023

update test to use case sensitive bidder name

c124abb

SyntaxNode self-assigned this Oct 16, 2023

AlexBVolcy approved these changes Oct 16, 2023

View reviewed changes

onkarvhanumante approved these changes Oct 17, 2023

View reviewed changes

gargcreation1992 merged commit 1384564 into master Oct 17, 2023
3 checks passed

gargcreation1992 deleted the PBS-1254 branch October 17, 2023 06:30

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

update cookie sync endpoint to be case insensitive (prebid#3103)

66a8996

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update cookie sync endpoint to be case insensitive #3103

update cookie sync endpoint to be case insensitive #3103

gargcreation1992 commented Sep 14, 2023 •

edited by onkarvhanumante

Loading

onkarvhanumante Sep 15, 2023

gargcreation1992 Sep 20, 2023

SyntaxNode Oct 6, 2023

onkarvhanumante left a comment

AlexBVolcy Sep 20, 2023

gargcreation1992 Sep 25, 2023

gargcreation1992 commented Sep 27, 2023

bsardo commented Sep 28, 2023

SyntaxNode commented Oct 6, 2023

SyntaxNode left a comment

SyntaxNode Oct 6, 2023

gargcreation1992 Oct 10, 2023

SyntaxNode Oct 6, 2023

SyntaxNode Oct 6, 2023

AlexBVolcy Oct 9, 2023

gargcreation1992 Oct 10, 2023

SyntaxNode Oct 6, 2023

gargcreation1992 Oct 10, 2023

SyntaxNode Oct 6, 2023

gargcreation1992 Oct 10, 2023

SyntaxNode commented Oct 6, 2023

AlexBVolcy commented Oct 9, 2023

gargcreation1992 commented Oct 10, 2023 •

edited

Loading

AlexBVolcy Oct 10, 2023

gargcreation1992 Oct 11, 2023

AlexBVolcy Oct 12, 2023

gargcreation1992 Oct 16, 2023

update cookie sync endpoint to be case insensitive #3103

update cookie sync endpoint to be case insensitive #3103

Conversation

gargcreation1992 commented Sep 14, 2023 • edited by onkarvhanumante Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onkarvhanumante left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gargcreation1992 commented Sep 27, 2023

bsardo commented Sep 28, 2023

SyntaxNode commented Oct 6, 2023

SyntaxNode 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

Choose a reason for hiding this comment

SyntaxNode commented Oct 6, 2023

AlexBVolcy commented Oct 9, 2023

gargcreation1992 commented Oct 10, 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

gargcreation1992 commented Sep 14, 2023 •

edited by onkarvhanumante

Loading

gargcreation1992 commented Oct 10, 2023 •

edited

Loading