Fix: Panic in bid adjustments

[linux019]

If auction request doesn't have bidadjustments object, but the adjustment has been enabled in the stored requests for the account there will be a panic
#3543

{
...
    "ext": {
        "prebid": {
             "bidadjustments": {},
         }
     }
}

[bsardo]

Thanks for adding the test to confirm the fix. I have just one minor suggestion. Let me know your thoughts.

[bsardo]

Thoughts on changing this logic? I'm thinking it might be easier to understand if it were like this:

if (extPrebid == nil || extPrebid.BidAdjustments == nil) {
	if acct == nil {
		return nil, nil	
	}
	return acct, nil
}
if acct == nil {
	return extPrebid.BidAdjustments, nil
}

[linux019]

	if extPrebid == nil || extPrebid.BidAdjustments == nil {
		return acct, nil
	}

	if acct == nil {
		return extPrebid.BidAdjustments, nil
	}

[hhhjort]

LGTM

[guscarreon]

The 3rd test case in the truth table below seems to still panic under the newly added logic. Do you agree?

   | extPrebid | extPrebid.BidAdjustments | acct    || returns
---|-----------|--------------------------|---------||-----------
  1|   nil     |       nil                | nil     || nil, nil
  2|   nil     |       nil                | not-nil || acct, nil          
  3|  not-nil  |       nil                | nil     || PANIC in line 73 when dereferencing nil extPrebid.BidAdjustments
  4|  not-nil  |       nil                | not-nil || acct, nil
  5|  not-nil  |     not-nil              | nil     || extPrebid.BidAdjustments, nil         
  6|  not-nil  |     not-nil              | not-nil || acct, nil          

I believe the modification below could cover the panic scenario but I think, as written below, we have repetitive checks:

62
63     if extPrebid == nil && acct == nil {
64         return nil, nil
65     }
66     if (extPrebid == nil || extPrebid.BidAdjustments == nil) && acct != nil {
67         return acct, nil
68     }
69 -   if extPrebid != nil && acct == nil {
   +   if extPrebid != nil && extPrebid.BidAdjustments != nil && acct == nil {
70         return extPrebid.BidAdjustments, nil
71     }
   +   if extPrebid != nil && extPrebid.BidAdjustments == nil {
   +       extPrebid.BidAdjustments := &ExtRequestPrebidBidAdjustments{}
   +   }
72
73     extPrebid.BidAdjustments.MediaType.Banner = mergeForMediaType(extPrebid.BidAdjustments.MediaType.Banner, acct.MediaType.Banner)
// PANIC deferencing null    ^
// pointer in scenario 3     |
// if we don't modify    ----+
bidadjustment/build_rules.go

Honestly, the modifications proposed by you and Brian Sardo above, seem a lot better than I just proposed here. Can we please put logic between lines 63 to 71 in its own function, refactor as necessary, and unit test all the scenarios?

[linux019]

@guscarreon I added 6 test cases for your table, there is no panic

[guscarreon]

Nitpick: Just to make the test case less verbose, can we remove some of the elements so we are only left with bidder "bidderA" and "bidderB"?

[linux019]

it's necessary to test all media types banner|video-instream|video-outstream|audio|native|*

[guscarreon]

Nitpick: can we remove the extra space?

[linux019]

Removed extra line breaks, the next removal will lead to poor readability

[guscarreon]

I'm sorry for the confusion. By extra space I meant line 435 only. I was ok with the rest of the line breaks. I agree that removing them leads to poor readability. I'll approve anyways.

[guscarreon]

LGTM. Thank you @linux019 for addressing my feedback