Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adapter alias - syncer changes #3082

Merged
merged 19 commits into from
Sep 29, 2023
Merged

Adapter alias - syncer changes #3082

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
d96281a
Rename cfg to bidderInfo
onkarvhanumante Sep 6, 2023
ce30fda
Refactor BuildSyncers
onkarvhanumante Sep 6, 2023
aa619e5
Add getSyncerKey
onkarvhanumante Sep 6, 2023
eb836cd
Update chooseSyncerConfig
onkarvhanumante Sep 6, 2023
56da6aa
fix error message
onkarvhanumante Sep 8, 2023
de6d33a
add error case for BuildSyncers
onkarvhanumante Sep 8, 2023
8a54873
minor refactoring
onkarvhanumante Sep 8, 2023
049354b
error message changes
onkarvhanumante Sep 11, 2023
4d3c059
Revert "error message changes"
onkarvhanumante Sep 26, 2023
7503c6f
Revert "minor refactoring"
onkarvhanumante Sep 26, 2023
3949a3b
Revert "add error case for BuildSyncers"
onkarvhanumante Sep 26, 2023
b068aa2
Revert "fix error message"
onkarvhanumante Sep 26, 2023
09caa29
Revert "Update chooseSyncerConfig"
onkarvhanumante Sep 26, 2023
98e1911
Revert "Add getSyncerKey"
onkarvhanumante Sep 26, 2023
6ff2f16
Revert "Refactor BuildSyncers"
onkarvhanumante Sep 26, 2023
2358273
Revert "Rename cfg to bidderInfo"
onkarvhanumante Sep 26, 2023
dd12c58
only inherit parent syncer key in alias syncer cfg
onkarvhanumante Sep 26, 2023
8b64cd6
test case updates
onkarvhanumante Sep 29, 2023
3416889
update ProcessAliasBidderInfo unit test
onkarvhanumante Sep 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions config/bidderinfo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. validates above adapter aliases rule
  2. groups all bidders with same key together (parent and aliases are grouped together)
  3. 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.

syncerKey := aliasBidderInfo.AliasOf
if parentBidderInfo.Syncer.Key != "" {
syncerKey = parentBidderInfo.Syncer.Key
}
syncer := Syncer{Key: syncerKey}
aliasBidderInfo.Syncer = &syncer
}
if aliasBidderInfo.UserSyncURL == "" {
aliasBidderInfo.UserSyncURL = parentBidderInfo.UserSyncURL
Expand Down
63 changes: 49 additions & 14 deletions config/bidderinfo_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,12 +250,6 @@ func TestProcessBidderInfo(t *testing.T) {
PlatformID: "123",
Syncer: &Syncer{
Key: "foo",
IFrame: &SyncerEndpoint{
URL: "https://foo.com/sync?mode=iframe&r={{.RedirectURL}}",
RedirectURL: "https://redirect/setuid/iframe",
ExternalURL: "https://iframe.host",
UserMacro: "UID",
},
},
UserSyncURL: "user-url",
XAPI: AdapterXAPI{
Expand All @@ -281,7 +275,7 @@ func TestProcessBidderInfo(t *testing.T) {
}

func TestProcessAliasBidderInfo(t *testing.T) {
parentBidderInfo := BidderInfo{
parentWithSyncerKey := BidderInfo{
AppSecret: "app-secret",
Capabilities: &CapabilitiesInfo{
App: &PlatformInfo{
Expand Down Expand Up @@ -377,8 +371,29 @@ func TestProcessAliasBidderInfo(t *testing.T) {
Tracker: "alias-tracker",
},
}
bidderB := parentBidderInfo
bidderB := parentWithSyncerKey
bidderB.AliasOf = "bidderA"
bidderB.Syncer = &Syncer{
Key: bidderB.Syncer.Key,
}

parentWithoutSyncerKey := BidderInfo{
Syncer: &Syncer{
IFrame: &SyncerEndpoint{
URL: "https://foo.com/sync?mode=iframe&r={{.RedirectURL}}",
RedirectURL: "https://redirect/setuid/iframe",
ExternalURL: "https://iframe.host",
UserMacro: "UID",
},
},
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

8b64cd6 makes suggested changes


bidderC := parentWithoutSyncerKey
bidderC.AliasOf = "bidderA"
bidderC.Syncer = &Syncer{
Key: "bidderA",
}

testCases := []struct {
description string
aliasInfos map[string]aliasNillableFields
Expand All @@ -387,7 +402,7 @@ func TestProcessAliasBidderInfo(t *testing.T) {
expectedErr error
}{
{
description: "inherit all parent info in alias bidder",
description: "inherit all parent info in alias bidder, use parent syncer key as syncer alias key",
aliasInfos: map[string]aliasNillableFields{
"bidderB": {
Disabled: nil,
Expand All @@ -397,14 +412,34 @@ func TestProcessAliasBidderInfo(t *testing.T) {
},
},
bidderInfos: BidderInfos{
"bidderA": parentBidderInfo,
"bidderA": parentWithSyncerKey,
"bidderB": BidderInfo{
AliasOf: "bidderA",
// all other fields should be inherited from parent bidder
},
},
expectedErr: nil,
expectedBidderInfos: BidderInfos{"bidderA": parentBidderInfo, "bidderB": bidderB},
expectedBidderInfos: BidderInfos{"bidderA": parentWithSyncerKey, "bidderB": bidderB},
},
{
description: "inherit all parent info in alias bidder, use parent name as syncer alias key",
aliasInfos: map[string]aliasNillableFields{
"bidderC": {
Disabled: nil,
ModifyingVastXmlAllowed: nil,
Experiment: nil,
XAPI: nil,
},
},
bidderInfos: BidderInfos{
"bidderA": parentWithoutSyncerKey,
"bidderC": BidderInfo{
AliasOf: "bidderA",
// all other fields should be inherited from parent bidder
},
},
expectedErr: nil,
expectedBidderInfos: BidderInfos{"bidderA": parentWithoutSyncerKey, "bidderC": bidderC},
},
{
description: "all bidder info specified for alias, do not inherit from parent bidder",
Expand All @@ -417,11 +452,11 @@ func TestProcessAliasBidderInfo(t *testing.T) {
},
},
bidderInfos: BidderInfos{
"bidderA": parentBidderInfo,
"bidderA": parentWithSyncerKey,
"bidderB": aliasBidderInfo,
},
expectedErr: nil,
expectedBidderInfos: BidderInfos{"bidderA": parentBidderInfo, "bidderB": aliasBidderInfo},
expectedBidderInfos: BidderInfos{"bidderA": parentWithSyncerKey, "bidderB": aliasBidderInfo},
},
{
description: "invalid alias",
Expand Down Expand Up @@ -449,7 +484,7 @@ func TestProcessAliasBidderInfo(t *testing.T) {
if test.expectedErr != nil {
assert.Equal(t, test.expectedErr, err)
} else {
assert.Equal(t, test.expectedBidderInfos, bidderInfos)
assert.Equal(t, test.expectedBidderInfos, bidderInfos, test.description)
}
}
}
Expand Down
Loading