DSA: Inject default in bid requests if not present #3540
Conversation
dsa/writer.go
Outdated
| ) | ||
|
|
||
| // DSAWriter is used to write the default DSA to the request (req.regs.ext.dsa) | ||
| type DSAWriter struct { |
There was a problem hiding this comment.
Nitpick: This is the dsa package. It's fine to call this just Writer.
dsa/writer.go
Outdated
|
|
||
| // 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) { |
There was a problem hiding this comment.
Nitpick: We don't often name the return argument. We typically prefer to return nil to indicate no error.
config/account.go
Outdated
| 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"` |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
exchange/utils.go
Outdated
| if err := dsaWriter.Write(req); err != nil { | ||
| errs = append(errs, err) | ||
| } | ||
| req.RebuildRequest() |
There was a problem hiding this comment.
This can produce an error.
…config and account logic
|
|
||
| // 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 Writer) Write(req *openrtb_ext.RequestWrapper) error { |
There was a problem hiding this comment.
I don't know if this is a stronger version of the function, but here's a version where we combine all of the conditions that result in us returning nil, rather than breaking them all up:
func (dw Writer) Write(req *openrtb_ext.RequestWrapper) error {
if req == nil || getReqDSA(req) != nil || dw.Config == nil || dw.Config.DefaultUnpacked == nil ||
(dw.Config.GDPROnly && !dw.GDPRInScope) {
return nil
}
regExt, err := req.GetRegExt()
if err != nil {
return err
}
regExt.SetDSA(dw.Config.DefaultUnpacked)
return nil
}
What do you think?
There was a problem hiding this comment.
I think this is really a matter of preference. I see your perspective that there are multiple conditionals that result in the same return value, though my tendency is to break complex conditionals into logical groupings because I think it is easier to digest and instills confidence in the intended behavior. I also think it is easier to scan code vertically. In that case, at a minimum the first two conditionals could be grouped as such: if req == nil || getReqDSA(req) != nil {.
@SyntaxNode do you feel strongly either way here?
There was a problem hiding this comment.
Yeah I'm thinking your latest update looks great
|
|
||
| // 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 Writer) Write(req *openrtb_ext.RequestWrapper) error { |
There was a problem hiding this comment.
Yeah I'm thinking your latest update looks great
dsa/writer.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| regExt.SetDSA(dw.Config.DefaultUnpacked) |
There was a problem hiding this comment.
There is some danger for memory corruption in pointing the request to using the same DefaultUnpacked instance for all requests. However, we also have the convention of "clone + set" when making changes to the request which likely mitigates this issue.
There was a problem hiding this comment.
Can you please add a second integration test for where the dsa object is already provided and the default dsa object is ignored?
| GDPRInScope: gdprEnforced, | ||
| } | ||
| if err := dsaWriter.Write(req); err != nil { | ||
| errs = append(errs, err) |
There was a problem hiding this comment.
Should we return if there is an error? Same for getGDPR , RebuildRequest, and getAuctionBidderRequests.
There was a problem hiding this comment.
I think we should return an error for RebuildRequest but not for the others. getGDPR will only ever result in a wrapper unmarshal error and it should theoretically never happen here since any problem should be caught upstream. And also we plan on it not returning an error in the near future anyway. getAuctionBidderRequests returns some errors that should not result in an early return and others that are accompanied by a nil allBidderRequests which prevents the logic that follows from having any effect since it involves looping over allBidderRequests.
Implements #3424