Adapter alias - syncer changes #3082

onkarvhanumante · 2023-09-07T06:06:10Z

As of now on server startup, if an alias has not defined syncer cfg then PBS was inheriting entire syncer cfg of parent.

Now, if a parent has syncer cfg with key, endpoints etc then alias was inheriting syncer entire parent cfg including key. This is incorrect because as per syncer rule, bidders can have same key but only one of them should define endpoints.

Therefore inheriting entire syncer cfg of parent resulted in validation error on PBS server startup. To fix this, PR implements fix to only inherit parent key in alias syncer cfg.

Added log lines to print syncers build on PBS server start up.

	+++ b/router/router.go
	@@ -170,6 +170,9 @@ func New(cfg *config.Configuration, rateConvertor *currency.RateConverter) (r *R
	                return nil, errortypes.NewAggregateError("user sync", errs)
	        }

	+       fmt.Printf("\ntest: %+v\n", syncersByBidder["test"])
	+       fmt.Printf("\nappnexus: %+v\n", syncersByBidder["appnexus"])
	+
	        syncerKeys := make([]string, 0, len(syncersByBidder))
	        syncerKeysHashSet := map[string]struct{}{}

Case1: Parent has defined syncer key.
Expected: Alias should inherit entire Parent syncer cfg with parent syncer key as syncer key for alias

appnexus.yml:

 	...
 	userSync:
 	  key: "adnxs"
 	  redirect:
 	    url: "https://ib.adnxs.com/getuid?{{.RedirectURL}}"
 	    userMacro: "$UID"

test.yml (alias):

 	aliasOf: appnexus

Based on following logs, test bidder (alias) has inherited config of alias

 prebid-server % go build . && ./prebid-server

 test: {key:adnxs defaultSyncType:redirect iframe:<nil> redirect:0x14000425f40 supportCORS:false}

 appnexus: {key:adnxs defaultSyncType:redirect iframe:<nil> redirect:0x1400012e000 supportCORS:false}

Case 2: parent has not defined syncer key
Expected: Alias should inherit entire Parent syncer cfg with parent name as syncer key for alias.

appnexus.yml:

 	...
 	userSync:
 	  redirect:
 	    url: "https://ib.adnxs.com/getuid?{{.RedirectURL}}"
 	    userMacro: "$UID"

test.yml (alias):

 	aliasOf: appnexus

Based on following logs, test bidder (alias) has inherited config of alias

 	prebid-server % go build . && ./prebid-server

 	test: {key:appnexus defaultSyncType:redirect iframe:<nil> redirect:0x140005c5ac0 supportCORS:false}

 	appnexus: {key:appnexus defaultSyncType:redirect iframe:<nil> redirect:0x140005c5a00 supportCORS:false}

Refactor BuildSyncers to use map[string]config.BidderInfo instead of map[string]config.Syncer. This is done because config.BidderInfo is needed to determine if bidder is an alias or non-alias bidder.

getSyncerKey will help to decide syncer key for an alias or non-alias bidder

Update chooseSyncerConfig to implement following adapter alias rules: - alias and parent bidder can use same syncer key - non-alias bidder cannot have same syncer key - aliases of different parents cannot have same syncer key - aliases of same parent and non-alias bidder have same syncer key

usersync/syncersbuilder.go

usersync/syncersbuilder_test.go

addresses #3082 (comment)

usersync/syncersbuilder.go

gargcreation1992

lgtm!
Requesting @SyntaxNode to please take a look as well.

AlexBVolcy

This is looking good, i just left one comment regarding the nested if statements

usersync/syncersbuilder.go

SyntaxNode · 2023-09-12T20:50:28Z

usersync/syncersbuilder.go

+		if _, ok := parentBidderNameMap[nonAliasBidderNames[0]]; !ok {
+			sort.Strings(aliasBidderNames)
+			return namedSyncerConfig{}, fmt.Errorf("either alias bidders %s defines endpoints (iframe and/or redirect) for the same syncer key used by other bidder, but only one bidder is permitted to define endpoints", strings.Join(aliasBidderNames, ", "))
+		}


This is getting a bit confusing to follow. This logic could be left alone if you resolve the alias user sync keys up front:

If the alias defines it's own syncer or syncer key, do nothing. The validation in this function will ensure the syncer rules are adhered.

if the alias does not define it's own syncer or syncer key but it's parent does, set the syncer key to match the parent. This is either the parent's syncer key or the parent's bidder name (the default implicit syncer key) - which you already have coded.

This may simplify the validation logic.

@onkarvhanumante was this comment addressed by you? Were you able to see if this validation logic can be simplified?

@onkarvhanumante was this comment addressed by you? Were you able to see if this validation logic can be simplified?

@AlexBVolcy, had some discussion with @SyntaxNode and have evaluated few cases to understand what syncer key should be assigned to alias and parent bidder. Will push updated changes and inform here

@AlexBVolcy @gargcreation1992 PR is updated with latest commits. All previous commits are reverted and dd12c58 is the one to review.

Refer #3082 (comment) for testing details

usersync/syncersbuilder.go

AlexBVolcy · 2023-09-18T22:39:13Z

usersync/syncersbuilder.go

+		if _, ok := parentBidderNameMap[nonAliasBidderNames[0]]; !ok {
+			sort.Strings(aliasBidderNames)
+			return namedSyncerConfig{}, fmt.Errorf("either alias bidders %s defines endpoints (iframe and/or redirect) for the same syncer key used by other bidder, but only one bidder is permitted to define endpoints", strings.Join(aliasBidderNames, ", "))
+		}


@onkarvhanumante was this comment addressed by you? Were you able to see if this validation logic can be simplified?

This reverts commit 049354b.

This reverts commit 8a54873.

This reverts commit de6d33a.

This reverts commit 56da6aa.

This reverts commit eb836cd.

This reverts commit aa619e5.

This reverts commit ce30fda.

This reverts commit d96281a.

As of now, in processBidderAliases if an alias has not defined syncer cfg then PBS was inheriting entire syncer cfg of parent. Now, if a parent has syncer cfg with key, endpoints etc then alias was inheriting syncer entire parent cfg including key. This is incorrect because as per syncer rule, bidders can have same key but only one of them should define endpoints. Therefore inheriting entire syncer cfg of parent resulted in validation error on PBS server startup. To fix this, commit updates processBidderAliases to only inherit parent key in alias syncer cfg.

gargcreation1992 · 2023-09-27T09:32:51Z

config/bidderinfo_test.go

+			Password: "pwd",
+			Tracker:  "tracker",
+		},
+	}


This is correct. Suggestion - might wanna check if you could reduce BidderInfo struct fields to only include syncer since other fields seems un-important for this test case.

Yeah you can remove the other fields, just tested it myself.

8b64cd6 makes suggested changes

AlexBVolcy

This looks good, left one minor comment

AlexBVolcy · 2023-09-28T19:20:45Z

config/bidderinfo_test.go

+			Password: "pwd",
+			Tracker:  "tracker",
+		},
+	}


Yeah you can remove the other fields, just tested it myself.

AlexBVolcy

I believe this PR was merged before receiving two approvals, but I left a question regarding one of the cases this PR is addressing.

AlexBVolcy · 2023-10-02T20:26:43Z

config/bidderinfo.go

@@ -320,8 +320,13 @@ func processBidderAliases(aliasNillableFieldsByBidder map[string]aliasNillableFi
 		if aliasBidderInfo.PlatformID == "" {
 			aliasBidderInfo.PlatformID = parentBidderInfo.PlatformID
 		}
-		if aliasBidderInfo.Syncer == nil {
-			aliasBidderInfo.Syncer = parentBidderInfo.Syncer
+		if aliasBidderInfo.Syncer == nil && parentBidderInfo.Syncer != nil {


I have a question regarding a case you laid out in your PR description:

Case 2: parent has not defined syncer key Expected: Alias should inherit entire Parent syncer cfg with parent name as syncer key for alias.

Would this be referring to scenario where parentBidderInfo.Syncer == nil? And if so, where in this PR is the code that allows the alias to inherit the entire parent syncer cfg?

@AlexBVolcy, we leverage on existing BuildSyncer() function to inherit parent cfg for alias.

As per adapter aliases feature, bidders (parent. + aliases ) can share user sync cfg as follow:

parent should define user sync config

alias should define parent name or parent syncer key in syncer cfg

We have BuildSyncer() function that,

validates above adapter aliases rule

groups all bidders with same key together (parent and aliases are grouped together)

assigns parent syncer config to all bidders grouped on step 2

prebid-server/usersync/syncersbuilder.go

Line 28 in 831dd71

func BuildSyncers(hostConfig *config.Configuration, bidderInfos config.BidderInfos) (map[string]Syncer, []error) {

In this way, alias inherit the entire parent syncer cfg. Case where parentBidderInfo.Syncer == nil is already handled by BuildSyncer(). So changes were needed as part of this PR.

onkarvhanumante added 4 commits September 7, 2023 03:55

Rename cfg to bidderInfo

d96281a

Refactor BuildSyncers

ce30fda

Refactor BuildSyncers to use map[string]config.BidderInfo instead of map[string]config.Syncer. This is done because config.BidderInfo is needed to determine if bidder is an alias or non-alias bidder.

Add getSyncerKey

aa619e5

getSyncerKey will help to decide syncer key for an alias or non-alias bidder

onkarvhanumante changed the title ~~Adapter alias/syncer changes~~ Adapter alias - syncer changes Sep 7, 2023

gargcreation1992 self-assigned this Sep 7, 2023

SyntaxNode added the do not port label Sep 7, 2023

AlexBVolcy reviewed Sep 7, 2023

View reviewed changes

usersync/syncersbuilder.go Outdated Show resolved Hide resolved

usersync/syncersbuilder.go Outdated Show resolved Hide resolved

usersync/syncersbuilder.go Outdated Show resolved Hide resolved

gargcreation1992 reviewed Sep 8, 2023

View reviewed changes

usersync/syncersbuilder.go Outdated Show resolved Hide resolved

gargcreation1992 reviewed Sep 8, 2023

View reviewed changes

usersync/syncersbuilder.go Outdated Show resolved Hide resolved

gargcreation1992 reviewed Sep 8, 2023

View reviewed changes

usersync/syncersbuilder_test.go Outdated Show resolved Hide resolved

onkarvhanumante added 3 commits September 8, 2023 14:28

fix error message

56da6aa

add error case for BuildSyncers

de6d33a

addresses #3082 (comment)

minor refactoring

8a54873

gargcreation1992 reviewed Sep 11, 2023

View reviewed changes

usersync/syncersbuilder.go Outdated Show resolved Hide resolved

gargcreation1992 reviewed Sep 11, 2023

View reviewed changes

usersync/syncersbuilder.go Outdated Show resolved Hide resolved

error message changes

049354b

gargcreation1992 previously approved these changes Sep 12, 2023

View reviewed changes

hhhjort assigned AlexBVolcy Sep 12, 2023

AlexBVolcy reviewed Sep 12, 2023

View reviewed changes

SyntaxNode reviewed Sep 12, 2023

View reviewed changes

AlexBVolcy reviewed Sep 18, 2023

View reviewed changes

onkarvhanumante added 8 commits September 26, 2023 13:17

Revert "error message changes"

4d3c059

This reverts commit 049354b.

Revert "minor refactoring"

7503c6f

This reverts commit 8a54873.

Revert "add error case for BuildSyncers"

3949a3b

This reverts commit de6d33a.

Revert "fix error message"

b068aa2

This reverts commit 56da6aa.

Revert "Update chooseSyncerConfig"

09caa29

This reverts commit eb836cd.

Revert "Add getSyncerKey"

98e1911

This reverts commit aa619e5.

Revert "Refactor BuildSyncers"

6ff2f16

This reverts commit ce30fda.

Revert "Rename cfg to bidderInfo"

2358273

This reverts commit d96281a.

onkarvhanumante dismissed gargcreation1992’s stale review via dd12c58 September 26, 2023 11:18

gargcreation1992 reviewed Sep 27, 2023

View reviewed changes

gargcreation1992 previously approved these changes Sep 27, 2023

View reviewed changes

AlexBVolcy reviewed Sep 28, 2023

View reviewed changes

test case updates

8b64cd6

onkarvhanumante dismissed gargcreation1992’s stale review via 8b64cd6 September 29, 2023 10:05

update ProcessAliasBidderInfo unit test

3416889

gargcreation1992 approved these changes Sep 29, 2023

View reviewed changes

onkarvhanumante merged commit 061aa84 into master Sep 29, 2023
3 checks passed

onkarvhanumante deleted the adapter-alias/syncer-changes branch September 29, 2023 11:21

AlexBVolcy reviewed Oct 2, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapter alias - syncer changes #3082

Adapter alias - syncer changes #3082

onkarvhanumante commented Sep 7, 2023 •

edited

Loading

gargcreation1992 left a comment

AlexBVolcy left a comment

SyntaxNode Sep 12, 2023

AlexBVolcy Sep 18, 2023

onkarvhanumante Sep 21, 2023

onkarvhanumante Sep 26, 2023

AlexBVolcy Sep 18, 2023

gargcreation1992 Sep 27, 2023

AlexBVolcy Sep 28, 2023

onkarvhanumante Sep 29, 2023

AlexBVolcy left a comment

AlexBVolcy Sep 28, 2023

AlexBVolcy left a comment

AlexBVolcy Oct 2, 2023

onkarvhanumante Oct 3, 2023

Adapter alias - syncer changes #3082

Adapter alias - syncer changes #3082

Conversation

onkarvhanumante commented Sep 7, 2023 • edited Loading

gargcreation1992 left a comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexBVolcy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

onkarvhanumante commented Sep 7, 2023 •

edited

Loading