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

DSA: Inject default in bid requests if not present #3540

Merged
merged 21 commits into from
Apr 9, 2024
Merged

DSA: Inject default in bid requests if not present #3540

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
23eca73
Add account config
bsardo Feb 24, 2024
9cddf2b
Add DSA writer to insert default DSA in requests
bsardo Feb 24, 2024
e0c174e
Use DSA writer to insert default into each bidder request
bsardo Feb 25, 2024
a424d16
Add DSA default JSON test with test framework update
bsardo Feb 25, 2024
3d0a754
Insert default into request before splitting into bidder requests
bsardo Feb 25, 2024
ea58098
Change DataToPub to a pointer
bsardo Feb 27, 2024
2da4088
Rename dsawriter.go to writer.go
bsardo Feb 27, 2024
1bc45e6
Update variable name
bsardo Feb 27, 2024
3828209
Address feedback
bsardo Feb 28, 2024
d2ec47e
Update host config to accept DSA default as stringified JSON and map …
bsardo Mar 1, 2024
94857a3
Update exchange JSON test to accept stringified JSON in account config
bsardo Mar 1, 2024
e202460
Convert UnpackDSADefault from receiver method to function for use in …
bsardo Mar 1, 2024
c6c0d6f
Update account fetching to validate default DSA and unpack to struct
bsardo Mar 1, 2024
2ee9652
Update DSA writer to use unpacked default
bsardo Mar 1, 2024
8c29cb1
Handle error returned from RebuildRequest
bsardo Apr 2, 2024
ff93f52
Merge branch 'master' into DSA-3424
bsardo Apr 2, 2024
5729ab7
Group related conditions in Write
bsardo Apr 3, 2024
bdfca44
Feedback: error msg, env vars, minor refactor
bsardo Apr 4, 2024
f216c82
Clone DSA default unpacked and set on request
bsardo Apr 4, 2024
bcf23f3
Add integration test where default is ignored due to DSA on request
bsardo Apr 4, 2024
59309eb
Return error from RebuildRequest call
bsardo Apr 8, 2024
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
7 changes: 7 additions & 0 deletions config/account.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,10 +335,17 @@ func (m AccountModules) ModuleConfig(id string) (json.RawMessage, error) {

type AccountPrivacy struct {
AllowActivities *AllowActivities `mapstructure:"allowactivities" json:"allowactivities"`
DSA *AccountDSA `mapstructure:"dsa" json:"dsa"`
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
IPv6Config IPv6 `mapstructure:"ipv6" json:"ipv6"`
IPv4Config IPv4 `mapstructure:"ipv4" json:"ipv4"`
}

// AccountDSA represents DSA configuration
type AccountDSA struct {
Default *openrtb_ext.ExtRegsDSA `mapstructure:"default" json:"default"`
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 an interesting approach. It enforces validation such that the object is always valid, but requires that it be configured not in json... but in whatever config format the host uses. Would it be weird to express this in yaml? Since there is no environment binding, that wouldn't be an option.

How do you feel about accepting a json.RawMessage such that it's always a json blob and parsing it during config loading? Easy for the default account, but perhaps less clean for the account specific configs.

Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Discussed offline. I overlooked the fact that viper will make it difficult to map the transparency array of objects to the config and env vars will be a problem so we agreed that hosts and accounts will specify a default DSA object as a JSON string.

The approach I took was as follows:

Hosts specify a default DSA object as a JSON string
On startup, the JSON string is validated via unmarshaling to a DSA default struct
If it is not well-formed we fail hard

Account configs also specify DSA objects as a JSON string
The account fetching logic already merges account defaults with an account config. Given that DSA objects are expressed as a JSON string, either the account config or account default is chosen (no merging)
The selected DSA default is validated via unmarshaling to a DSA default struct
If it is not well-formed we throw a malformed JSON error (already visible via a metric)

The default structs are used when writing to a bid request or validating a bid response.

GDPROnly bool `mapstructure:"gdpr_only" json:"gdpr_only"`
}

type IPv6 struct {
AnonKeepBits int `mapstructure:"anon_keep_bits" json:"anon_keep_bits"`
}
Expand Down
35 changes: 35 additions & 0 deletions dsa/writer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package dsa

import (
"github.com/prebid/prebid-server/v2/config"
"github.com/prebid/prebid-server/v2/openrtb_ext"
)

// DSAWriter is used to write the default DSA to the request (req.regs.ext.dsa)
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
type DSAWriter struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: This is the dsa package. It's fine to call this just Writer.

Config *config.AccountDSA
GDPRInScope bool
}

// Write sets the default DSA object on the request at regs.ext.dsa if it is
// defined in the account config and it is not already present on the request
func (dw DSAWriter) Write(req *openrtb_ext.RequestWrapper) (err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: We don't often name the return argument. We typically prefer to return nil to indicate no error.

if req == nil {
return
}
if getReqDSA(req) != nil {
return
}
if dw.Config == nil || dw.Config.Default == nil {
return
}
if dw.Config.GDPROnly && !dw.GDPRInScope {
return
}
regExt, err := req.GetRegExt()
if err != nil {
return err
}
regExt.SetDSA(dw.Config.Default)
return
}
186 changes: 186 additions & 0 deletions dsa/writer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
package dsa

import (
"encoding/json"
"testing"

"github.com/prebid/openrtb/v20/openrtb2"
"github.com/prebid/prebid-server/v2/config"
"github.com/prebid/prebid-server/v2/openrtb_ext"
"github.com/prebid/prebid-server/v2/util/ptrutil"
"github.com/stretchr/testify/assert"
)

func TestWrite(t *testing.T) {
requestDSAJSON := json.RawMessage(`{"dsa":{"datatopub":1,"dsarequired":2,"pubrender":1,"transparency":[{"domain":"example1.com","dsaparams":[1,2,3]}]}}`)
defaultDSAJSON := json.RawMessage(`{"dsa":{"datatopub":2,"dsarequired":3,"pubrender":2,"transparency":[{"domain":"example2.com","dsaparams":[4,5,6]}]}}`)
defaultDSA := &openrtb_ext.ExtRegsDSA{
DataToPub: ptrutil.ToPtr[int8](2),
Required: ptrutil.ToPtr[int8](3),
PubRender: ptrutil.ToPtr[int8](2),
Transparency: []openrtb_ext.ExtBidDSATransparency{
{
Domain: "example2.com",
Params: []int{4, 5, 6},
},
},
}

tests := []struct {
name string
giveConfig *config.AccountDSA
giveGDPR bool
giveRequest *openrtb_ext.RequestWrapper
expectRequest *openrtb_ext.RequestWrapper
}{
{
name: "request_nil",
giveRequest: nil,
expectRequest: nil,
},
{
name: "config_nil",
giveConfig: nil,
giveRequest: &openrtb_ext.RequestWrapper{
BidRequest: &openrtb2.BidRequest{
Regs: &openrtb2.Regs{
Ext: nil,
},
},
},
expectRequest: &openrtb_ext.RequestWrapper{
BidRequest: &openrtb2.BidRequest{
Regs: &openrtb2.Regs{
Ext: nil,
},
},
},
},
{
name: "config_default_nil",
giveConfig: &config.AccountDSA{
Default: nil,
},
giveRequest: &openrtb_ext.RequestWrapper{
BidRequest: &openrtb2.BidRequest{
Regs: &openrtb2.Regs{
Ext: nil,
},
},
},
expectRequest: &openrtb_ext.RequestWrapper{
BidRequest: &openrtb2.BidRequest{
Regs: &openrtb2.Regs{
Ext: nil,
},
},
},
},
{
name: "request_dsa_present",
giveConfig: &config.AccountDSA{
Default: defaultDSA,
},
giveRequest: &openrtb_ext.RequestWrapper{
BidRequest: &openrtb2.BidRequest{
Regs: &openrtb2.Regs{
Ext: requestDSAJSON,
},
},
},
expectRequest: &openrtb_ext.RequestWrapper{
BidRequest: &openrtb2.BidRequest{
Regs: &openrtb2.Regs{
Ext: requestDSAJSON,
},
},
},
},
{
name: "config_default_present_with_gdpr_only_set_and_gdpr_in_scope",
giveConfig: &config.AccountDSA{
Default: defaultDSA,
GDPROnly: true,
},
giveGDPR: true,
giveRequest: &openrtb_ext.RequestWrapper{
BidRequest: &openrtb2.BidRequest{},
},
expectRequest: &openrtb_ext.RequestWrapper{
BidRequest: &openrtb2.BidRequest{
Regs: &openrtb2.Regs{
Ext: defaultDSAJSON,
},
},
},
},
{
name: "config_default_present_with_gdpr_only_set_and_gdpr_not_in_scope",
giveConfig: &config.AccountDSA{
Default: defaultDSA,
GDPROnly: true,
},
giveGDPR: false,
giveRequest: &openrtb_ext.RequestWrapper{
BidRequest: &openrtb2.BidRequest{},
},
expectRequest: &openrtb_ext.RequestWrapper{
BidRequest: &openrtb2.BidRequest{},
},
},
{
name: "config_default_present_with_gdpr_only_not_set_and_gdpr_in_scope",
giveConfig: &config.AccountDSA{
Default: defaultDSA,
GDPROnly: false,
},
giveGDPR: true,
giveRequest: &openrtb_ext.RequestWrapper{
BidRequest: &openrtb2.BidRequest{},
},
expectRequest: &openrtb_ext.RequestWrapper{
BidRequest: &openrtb2.BidRequest{
Regs: &openrtb2.Regs{
Ext: defaultDSAJSON,
},
},
},
},
{
name: "config_default_present_with_gdpr_only_not_set_and_gdpr_not_in_scope",
giveConfig: &config.AccountDSA{
Default: defaultDSA,
GDPROnly: false,
},
giveGDPR: false,
giveRequest: &openrtb_ext.RequestWrapper{
BidRequest: &openrtb2.BidRequest{},
},
expectRequest: &openrtb_ext.RequestWrapper{
BidRequest: &openrtb2.BidRequest{
Regs: &openrtb2.Regs{
Ext: defaultDSAJSON,
},
},
},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
writer := DSAWriter{
Config: tt.giveConfig,
GDPRInScope: tt.giveGDPR,
}
err := writer.Write(tt.giveRequest)

if tt.giveRequest != nil {
tt.giveRequest.RebuildRequest()
assert.Equal(t, tt.expectRequest.BidRequest, tt.giveRequest.BidRequest)
} else {
assert.Nil(t, tt.giveRequest)
}
assert.Nil(t, err)
})
}
}
6 changes: 4 additions & 2 deletions exchange/exchange_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -978,6 +978,7 @@ func TestFloorsSignalling(t *testing.T) {
Account: config.Account{DebugAllow: true, PriceFloors: config.AccountPriceFloors{Enabled: test.floorsEnable, MaxRule: 100, MaxSchemaDims: 5}},
UserSyncs: &emptyUsersync{},
HookExecutor: &hookexecution.EmptyHookExecutor{},
TCF2Config: gdpr.NewTCF2Config(config.TCF2{}, config.AccountGDPR{}),
}
outBidResponse, err := e.HoldAuction(context.Background(), auctionRequest, &DebugLog{})

Expand Down Expand Up @@ -2170,7 +2171,7 @@ func runSpec(t *testing.T, filename string, spec *exchangeSpec) {
impExtInfoMap[impID] = ImpExtInfo{}
}

activityControl := privacy.NewActivityControl(spec.AccountPrivacy)
activityControl := privacy.NewActivityControl(&spec.AccountPrivacy)

auctionRequest := &AuctionRequest{
BidRequestWrapper: &openrtb_ext.RequestWrapper{BidRequest: &spec.IncomingRequest.OrtbRequest},
Expand All @@ -2181,6 +2182,7 @@ func runSpec(t *testing.T, filename string, spec *exchangeSpec) {
},
DebugAllow: true,
PriceFloors: config.AccountPriceFloors{Enabled: spec.AccountFloorsEnabled},
Privacy: spec.AccountPrivacy,
Validations: spec.AccountConfigBidValidation,
},
UserSyncs: mockIdFetcher(spec.IncomingRequest.Usersyncs),
Expand Down Expand Up @@ -5484,7 +5486,7 @@ type exchangeSpec struct {
FledgeEnabled bool `json:"fledge_enabled,omitempty"`
MultiBid *multiBidSpec `json:"multiBid,omitempty"`
Server exchangeServer `json:"server,omitempty"`
AccountPrivacy *config.AccountPrivacy `json:"accountPrivacy,omitempty"`
AccountPrivacy config.AccountPrivacy `json:"accountPrivacy,omitempty"`
}

type multiBidSpec struct {
Expand Down
Loading
Loading