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.

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

Remove Default Request Hardcoded Aliases #4020

Merged
merged 2 commits into from
Oct 30, 2024

Conversation

SyntaxNode
Copy link
Contributor

As we move to the next major Prebid Server release, it's time to remove legacy behavior where hardcoded aliases were "promoted" from the default request to the server level.

Support for hardcoded request aliases has been fully implemented in Prebid Server for a while. The legacy approach was a quick way to provide a limited ability for hardcoded aliases. Now that we have the full implementation, it's time for this to go.

If you are host currently using this feature you will need to add a yaml file defining the alias instead for the alias to appear in the server bidder list. Aliases defined in the default request will continue to work as normal request level aliases without change.

@@ -151,6 +137,8 @@ type platform struct {
func mapDetailFromConfig(c config.BidderInfo) bidderDetail {
var bidderDetail bidderDetail

bidderDetail.AliasOf = c.AliasOf
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes an unreported bug where hardcoded aliases are not marked as such in the bidder info endpoints.

@SyntaxNode SyntaxNode changed the title Remove Default Request Aliases Promotion Remove Default Request Hardcoded Aliases Oct 28, 2024
Copy link
Collaborator

@bsardo bsardo left a comment

Choose a reason for hiding this comment

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

LGTM other than a couple of quick nitpicks. Local testing looks good as well.

router/router.go Outdated
if len(errs) > 0 {
return errortypes.NewAggregateError("default request alias errors", errs)
if err := jsonutil.UnmarshalValid(defaultRequestJSON, &openrtb2model.BidRequest{}); err != nil {
// we might not have aliases defined, but will at least show that the JSON file is parsable.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: is this comment still relevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reworded.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This snippet (https://github.com/prebid/prebid-server/blob/master/endpoints/openrtb2/auction.go#L1673-L1689) is still relevant right? We still need to do this merge of the incoming request with the default request. Aliases on the default request defined at req.ext.prebid.aliases are merged as part of this process but they are considered request aliases rather than hardcoded aliases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. That's merging in the default stored request, not specific to aliases. Not sure why the comments are indicating as such. Changing the code to not be alias specific.

@hhhjort you originally wrote this, any idea?

Comment on lines +67 to +69
name: "mixed",
givenBidders: config.BidderInfos{"a": disabledCore, "b": disabledAlias, "c": enabledCore, "d": enabledAlias},
expected: `["a","b","c","d"]`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can delete this test case. It is the same as the alias-many-mixed case on lines 57-59.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Differentiated.

Comment on lines 130 to 134
{
name: "mixed",
givenBidders: config.BidderInfos{"a": disabledCore, "b": disabledAlias, "c": enabledCore, "d": enabledAlias},
expected: `["a","c"]`,
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can delete this test case. It is the same as the alias-mixed case on lines 126-128.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Differentiated.

Comment on lines +200 to +202
name: "mixed",
givenBidders: config.BidderInfos{"a": disabledCore, "b": disabledAlias, "c": enabledCore, "d": enabledAlias},
expected: `["c","d"]`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can delete this test case. It is the same as the alias-mixed case on lines 190-192.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Differentiated.

@bsardo bsardo merged commit 3907f1a into prebid:master Oct 30, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants