Accommodate Apple iOS LMT bug

[SyntaxNode]

Implements: #1699

[SyntaxNode]

There's a LOT of possible test case combinations. I likely already included too many, but I cut myself off at testing all initial values of the LMT flag. In most cases, it's set to nil at the start. There are a few test cases which ensure the LMT is overridden when set to a different non-nil value.

[SyntaxNode]

I very much disliked my original solution and rewrote it.

[SyntaxNode]

This is testing that the workaround logic is being hit during an end-to-end test.

I think the combinatorial details covered by the TestWorkaroundIOS14PrivacyBug are sufficient to prove the logic works and this test proves the logic is hit.

[SyntaxNode]

Trying out a new comment format, mimicking what exists in the OpenRTB library. Whatchya think?

[guscarreon]

Cool

[SyntaxNode]

The Go standard library doesn't have simple semantic versioning helpers and popular third party libraries provide way more functionality than needed. I opted for a simple implementation instead.

[guscarreon]

Cool

[guscarreon]

Nitpick: we already ran this switch statement inside IsKnownIOSAppTrackingStatus(v int64). Maybe we could simplify if IsKnownIOSAppTrackingStatus(v int64) returns an int pointer instead of a boolean

 48 - func IsKnownIOSAppTrackingStatus(v int64) bool {
    + func IsKnownIOSAppTrackingStatus(v int64) *int8 {
    +     var rc int8 
 49       switch IOSAppTrackingStatus(v) {
 50       case IOSAppTrackingStatusNotDetermined:
 51 -         return true
    +         rc = 0
 52       case IOSAppTrackingStatusRestricted:
 53 -         return true
    +         rc = 1
 54       case IOSAppTrackingStatusDenied:
 55 -         return true
    +         rc = 1
 56       case IOSAppTrackingStatusAuthorized:
 57 -         return true
    +         rc = 0
 58       default:
 59 -         return false
    +         return nil
 60       }
    +     return &rc
 61   }
        .
        .
        .
100 - func ParseDeviceExtATTS(deviceExt json.RawMessage) (*IOSAppTrackingStatus, error) {
    + func ParseDeviceExtATTS(deviceExt json.RawMessage) (*int8, error) {
101       v, err := jsonparser.GetInt(deviceExt, "atts")
102 
103       // node not found error
104       if err == jsonparser.KeyPathNotFoundError {
105           return nil, nil
106       }
107 
108       // unexpected parse error
109       if err != nil {
110           return nil, err
111       }
112 
113 -     // invalid value error
114 -     if !IsKnownIOSAppTrackingStatus(v) {
115 -         return nil, errors.New("invalid status")
116 -     }
117 - 
118 -     status := IOSAppTrackingStatus(v)
119 -     return &status, nil
    +     atts := IsKnownIOSAppTrackingStatus(v)
    +     if atts == nil {
    +         return nil, errors.New("invalid status")
    +     }
    +     return atts
120  }
openrtb_ext/device.go

In order to get rid of the switch statement:

51   func modifyForIOS142OrGreater(req *openrtb.BidRequest) {
52       atts, err := openrtb_ext.ParseDeviceExtATTS(req.Device.Ext)
53       if err != nil || atts == nil {
54           return
55       }
56  
   +     req.Device.Lmt = atts
57 -     switch *atts {
58 -     case openrtb_ext.IOSAppTrackingStatusNotDetermined:
59 -         req.Device.Lmt = &int8Zero
60 -     case openrtb_ext.IOSAppTrackingStatusRestricted:
61 -         req.Device.Lmt = &int8One
62 -     case openrtb_ext.IOSAppTrackingStatusDenied:
63 -         req.Device.Lmt = &int8One
64 -     case openrtb_ext.IOSAppTrackingStatusAuthorized:
65 -         req.Device.Lmt = &int8Zero
66 -     }
67   }
privacy/lmt/ios.go

Or something of that sorts

[SyntaxNode]

I see. In general I agree with removing duplicate code. For this case, the two switch statements are doing very different things. I worry that combining them would lead to harder to read code since now IsKnownIOSAppTrackingStatus would both validate it's a known value and return the LMT override value, both of which are in different pacakges.

[guscarreon]

Got it. How do you feel about grouping the cases that return an &int8One and an &int8Zero

51   func modifyForIOS142OrGreater(req *openrtb.BidRequest) {
52       atts, err := openrtb_ext.ParseDeviceExtATTS(req.Device.Ext)
53       if err != nil || atts == nil {
54           return
55       }
56  
57       switch *atts {
58 -     case openrtb_ext.IOSAppTrackingStatusNotDetermined:
   +     case openrtb_ext.IOSAppTrackingStatusNotDetermined, openrtb_ext.IOSAppTrackingStatusAuthorized:
59           req.Device.Lmt = &int8Zero
60 -     case openrtb_ext.IOSAppTrackingStatusRestricted:
   +     case openrtb_ext.IOSAppTrackingStatusRestricted, openrtb_ext.IOSAppTrackingStatusDenied:
61           req.Device.Lmt = &int8One
62 -     case openrtb_ext.IOSAppTrackingStatusDenied:
63 -         req.Device.Lmt = &int8One
64 -     case openrtb_ext.IOSAppTrackingStatusAuthorized:
65 -         req.Device.Lmt = &int8Zero
66       }
67   }
privacy/lmt/ios.go

[bsardo]

Throwing my 2 cents in, I'd vote to keep it as is. I think this is much easier to read when each case is on it's own line. In general, I think it's preferable to favor limiting line length over reducing the lines of code so it's easier to read by scanning vertically.

[SyntaxNode]

I was also concerned with readability. I don't expect this would impact performance.

[guscarreon]

Nitpick: lmt.ModifyForIOS(req) comes after processInterstitials(req) in line 263. This processInterstitials(req) function unmarshalls the Device.Ext making the jsonparser.GetInt(deviceExt, "atts") call that lmt.ModifyForIOS(req) eventually makes inside ParseDeviceExtATTS(deviceExt json.RawMessage) to be unnecessary. Actually, that 261 to 268 neighborhood makes use of Device a lot

260     // Populate any "missing" OpenRTB fields with info from other sources, (e.g. HTTP request headers).
261     deps.setFieldsImplicitly(httpRequest, req)  // <--- uses `Device`
262
263     if err := processInterstitials(req); err != nil {  // <--- unmarshalls `Device.Ext` and checks for `Device!=nil` again
264         errs = []error{err}
265         return
266     }
267
268     lmt.ModifyForIOS(req)  // <--- jsonparses `Device.Ext` even if we unmarshaled it above
269
270     errL := deps.validateRequest(req)
endpoints/openrtb2/auction.go

Do you think it'd be a good idea to maybe consolidate inside deps.setFieldsImplicitly(httpRequest, req)?

260       // Populate any "missing" OpenRTB fields with info from other sources, (e.g. HTTP request headers).
    +     if err := deps.setFieldsImplicitly(httpRequest, req); err != nil {
261 -     deps.setFieldsImplicitly(httpRequest, req)
262 -  
263 -     if err := processInterstitials(req); err != nil {
264           errs = []error{err}
265           return
266       }
267 -  
268 -     lmt.ModifyForIOS(req)
269
270       errL := deps.validateRequest(req)
             .
             .
             .
1059 - func (deps *endpointDeps) setFieldsImplicitly(httpReq *http.Request, bidReq *openrtb.BidRequest) {
1059 + func (deps *endpointDeps) setFieldsImplicitly(httpReq *http.Request, bidReq *openrtb.BidRequest) error {
1060 -     sanitizeRequest(bidReq, deps.privateNetworkIPValidator) // <-- reads Device and checks Device != nil
1061 -
1062 -     setDeviceImplicitly(httpReq, bidReq, deps.privateNetworkIPValidator) // <-- if Device==nil, creates one bidReq.Device = &openrtb.Device{}
     +     if bidReq.Device == nil {
     +         bidReq.Device = &openrtb.Device{}
     +     }
     +     sanitizeRequest(bidReq, deps.privateNetworkIPValidator) // <-- modify if needed (it does)
     +     setDeviceImplicitly(httpReq, bidReq, deps.privateNetworkIPValidator) // <-- modify if needed (it does)
     +     
     +     //unmarshal device.Ext if any     
     +     if req.Device.Ext != nil {
     +         err := json.Unmarshal(req.Device.Ext, &devExt)
     +         if err == nil {
     +             // Do the iOS and interstitials thing
     +             if err := processInterstitials(req, devExt); err != nil { // <-- modify to take Device.Ext as param
     +                 return err // I guess we'll have to return error from setFieldsImplicitly now
     +             }
     +             lmt.ModifyForIOS(req, devExt) // <-- Modify to take unmarshaled  Device.Ext (or even the ATTS value itself)
     +         }              
     +     }
1063  
1064       // Per the OpenRTB spec: A bid request must not contain both a Site and an App object.
1065       if bidReq.App == nil {
1066           setSiteImplicitly(httpReq, bidReq)
1067       }
1068       setImpsImplicitly(httpReq, bidReq.Imp)
1069  
1070       setAuctionTypeImplicitly(bidReq)
     +     return nil
1071   }
endpoints/openrtb2/auction.go

[guscarreon]

On a second thought, I realize that this is more a refactoring effort than a simple nitpick and, given the need for this feature, we should probably sort it on a separate PR.

Thoughts?

[guscarreon]

Nevermind my comment above, sometimes I tend to overcomplicate things. A far better approach could be to simply return from processInterstitials(req) the unmarshaled Device.Ext and modify lmt.ModifyForIOS(req) accordingly. What do you think? Given the time constraint, do you feel it's feasible?

260     // Populate any "missing" OpenRTB fields with info from other sources, (e.g. HTTP request headers).
261     deps.setFieldsImplicitly(httpRequest, req)
262
263 -   if err := processInterstitials(req); err != nil {
    +   if devExt, err := processInterstitials(req); err != nil {  // MODIFY TO RETURN `devExt`
264         errs = []error{err}
265         return
266     }
267
268 -   lmt.ModifyForIOS(req)
    +   lmt.ModifyForIOS(req, devExt)  // <--- MODIFY so there's no need to jsonparse `Device.Ext` now
269
270     errL := deps.validateRequest(req)
endpoints/openrtb2/auction.go

[SyntaxNode]

I'm unsure if this is worth fixing now, or if it's better to wait on the work @hhhjort is spearheading to solve this problem on a larger scale. Both processInterstitials and ModifyForIOS only unmarshal the device ext in certain circumstances, so I can't rely on processInterstitials actually performing the unmarshal.

Thoughts?

[guscarreon]

Agree

[bsardo]

The logic is looking good; I have just a few comments there. I'll review the tests shortly.

[bsardo]

Just curious, why are this function and EqualOrGreater public? Is the approach to make all methods in util files public even if they are only used in that package in anticipation of usage in other packages in the future?

[SyntaxNode]

I was using it from another package in an earlier draft. I don't see the harm in leaving it as exported even though its currently now just used by the iosutil's DetectVersionClassification method. If you feel strongly it should not be exported, I'll change it.

[bsardo]

My first thought here was null object pattern but perhaps what you have is better.

[SyntaxNode]

I love that the null object pattern was in your mind. :)

I don't think it applies in this situation since the modifier func isn't a return value passed between methods. I consider the range coverage of > 14.0 to be a coincidence which may change in the future. I did not intend for the map to be comprehensive of all possible iOS versions.

[guscarreon]

Why are we not returning an error here? privacy/lmt/ios.go's line 53 would catch it anyway

[bsardo]

I believe an error isn't being returned here because it's not actually an error if the atts field has been omitted from the request. Is that true @SyntaxNode?

[SyntaxNode]

Correct. I don't believe its fair to call the request invalid if the ATTS is missing and I'm not sure what the implications of that would be. In this PR I'm using its existence as a criteria for applying the LMT override. If the ATTS field is not present, the code will do nothing.

[guscarreon]

Should we add an "Invalid does not overwrite existing" test case?

{
	description: "Invalid does not overwrite existing",
	givenDevice: openrtb.Device{Ext: json.RawMessage(`{"atts":4}`), Lmt: openrtb.Int8Ptr(1)},
	expectedLMT: openrtb.Int8Ptr(1),
},
privacy/lmt/ios_test.go

[SyntaxNode]

Sure. Added.

[guscarreon]

Are we missing any further checks inside req.App? After after the nil check on req.App, we move on to req.Device

[SyntaxNode]

We are not missing further checks. This covers the specification "app object exists" in the linked issue. It ensures this logic is not applied to site traffic.

[guscarreon]

Are we positive the req.Device.OSV will come with iOS versions written exactly like "14.0" and "14.1"? Do you think it'd be safer to try to match with a regexp instead? Or maybe use string library functions such as string.Contains() or string.HasPrefix()

[SyntaxNode]

Yes. I asked in the linked issue if that was a valid assumption and the author confirmed it was, including a link back to the iOS version docs.

[guscarreon]

Since IOSAppTrackingStatusNotDetermined, IOSAppTrackingStatusRestricted, IOSAppTrackingStatusDenied, and IOSAppTrackingStatusAuthorized are equal to ints from 0 to 3, IsKnownIOSAppTrackingStatus could probably be simplified to:

48   func IsKnownIOSAppTrackingStatus(v int64) bool {
49 -     switch IOSAppTrackingStatus(v) {
50 -     case IOSAppTrackingStatusNotDetermined:
51 -         return true
52 -     case IOSAppTrackingStatusRestricted:
53 -         return true
54 -     case IOSAppTrackingStatusDenied:
55 -         return true
56 -     case IOSAppTrackingStatusAuthorized:
57 -         return true
58 -     default:
59 -         return false
60 -     }
   +     if v < IOSAppTrackingStatusNotDetermined || v > IOSAppTrackingStatusAuthorized {
   +         return false
   +     }
   +     return true
61   }
openrtb_ext/device.go

[SyntaxNode]

If it's alright with you, I'd prefer to keep it as a switch. The fact that the enumeration values are sequential are not a guaranteed feature of the value. I don't believe this would have a performance impact.

[bsardo]

Looks good, just one nitpick.

[bsardo]

I believe an error isn't being returned here because it's not actually an error if the atts field has been omitted from the request. Is that true @SyntaxNode?

[bsardo]

Throwing my 2 cents in, I'd vote to keep it as is. I think this is much easier to read when each case is on it's own line. In general, I think it's preferable to favor limiting line length over reducing the lines of code so it's easier to read by scanning vertically.

[bsardo]

Nitpick: Can we change this test name to TestDetectVersionClassification to more closely match the function under test?

[SyntaxNode]

Ah, yes. Good eye. I renamed the method during a refactor and forgot to update the corresponding test.

[guscarreon]

LGTM

[bsardo]

LGTM