DSA: Remove bids missing DSA object when required #3487
Conversation
exchange/exchange_test.go
Outdated
| description: "Validation is enforced, and one bid out of the two is invalid based on dimensions", | ||
| name: "One_of_two_bids_is_invalid_based_on_DSA_object_presence", | ||
| givenBidRequestExt: json.RawMessage(`{"dsa": {"dsarequired": 2}}`), | ||
| givenValidations: config.Validations{}, | ||
| givenBids: []*entities.PbsOrtbBid{{Bid: &openrtb2.Bid{Ext: json.RawMessage(`{"dsa": {}}`)}}, {Bid: &openrtb2.Bid{}}}, | ||
| givenSeat: "pubmatic", | ||
| expectedNumOfBids: 1, | ||
| expectedNonBids: &nonBids{ | ||
| seatNonBidsMap: map[string][]openrtb_ext.NonBid{ | ||
| "pubmatic": { | ||
| { | ||
| StatusCode: 300, | ||
| Ext: openrtb_ext.NonBidExt{ | ||
| Prebid: openrtb_ext.ExtResponseNonBidPrebid{ | ||
| Bid: openrtb_ext.NonBidObject{}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| expectedNumDebugWarnings: 1, | ||
| }, |
There was a problem hiding this comment.
I added this test case to verify bids were removed with respect to DSA but I also updated other test cases here with properly formatted, accurate test descriptions.
| if dataType == jsonparser.Object && err == nil { | ||
| return true | ||
| } else if err != nil && err != jsonparser.KeyPathNotFoundError { | ||
| return true |
There was a problem hiding this comment.
I noticed this line is not covered, is it possible to cover this, or it hard because of JSON?
There was a problem hiding this comment.
Yes I tried to add coverage but I'm having trouble forcing it down this path. I looked at other instances where we call jsonparser.Get and we don't have coverage for similar logic in those instances either.
dsa/validate.go
Outdated
|
|
||
| // Validate determines whether a given bid is valid from a DSA perspective. | ||
| // A bid is considered valid unless the bid request indicates that a DSA object is required | ||
| // in bid responses and it the object happens to be missing from the specified bid. |
There was a problem hiding this comment.
Nitpick: I think the word it can be removed from this comment so it'll look like:
// in bid responses and the object happens to be missing from the specified bid.
exchange/non_bid_reason.go
Outdated
| @@ -6,7 +6,8 @@ package exchange | |||
| type NonBidReason int | |||
|
|
|||
| const ( | |||
| NoBidUnknownError NonBidReason = 0 // No Bid - General | |||
| NoBidUnknownError NonBidReason = 0 // No Bid - General | |||
| ResponseRejectedGeneral NonBidReason = 300 | |||
There was a problem hiding this comment.
For my own understanding: Why is the non bid reason not specific to referencing DSA?
There was a problem hiding this comment.
The non bid reason codes are defined in the seat nonbid community extension documentation, section List: Non Bid Status Codes. There isn't a DSA specific code at this time so we agreed to use the generic bid response rejected 300 code for now.
There was a problem hiding this comment.
Actually we agreed that the status code should now be 305, Response Rejected - Privacy, so I pushed an update.
| @@ -1186,6 +1189,16 @@ func (re *RegExt) unmarshal(extJson json.RawMessage) error { | |||
| return err | |||
| } | |||
|
|
|||
| dsaJson, hasDSA := re.ext[dsaKey] | |||
| if hasDSA { | |||
| re.dsa = &ExtRegsDSA{} | |||
There was a problem hiding this comment.
Is there a way to add a test to cover this line?
| if re.dsa != nil { | ||
| rawjson, err := jsonutil.Marshal(re.dsa) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| re.ext[dsaKey] = rawjson | ||
| } else { | ||
| delete(re.ext, dsaKey) | ||
| } | ||
| re.dsaDirty = false |
There was a problem hiding this comment.
Is it possible to cover some of these lines?
| func (re *RegExt) GetDSA() *ExtRegsDSA { | ||
| if re.dsa == nil { | ||
| return nil | ||
| } | ||
| dsa := *re.dsa | ||
| return &dsa | ||
| } | ||
|
|
||
| func (re *RegExt) SetDSA(dsa *ExtRegsDSA) { | ||
| re.dsa = dsa | ||
| re.dsaDirty = true | ||
| } |
There was a problem hiding this comment.
Can you add tests for these?
Implements #3367
For requests, pass through behavior was already supported but this adds an exchange test to verify.
For bid responses, validation is added and bids are discarded if DSA objects are required in bids.