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.

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

Accommodate Apple iOS LMT bug #1718

Merged
merged 15 commits into from
Feb 25, 2021
3 changes: 3 additions & 0 deletions endpoints/openrtb2/auction.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"github.com/prebid/prebid-server/openrtb_ext"
"github.com/prebid/prebid-server/prebid_cache_client"
"github.com/prebid/prebid-server/privacy/ccpa"
"github.com/prebid/prebid-server/privacy/lmt"
"github.com/prebid/prebid-server/stored_requests"
"github.com/prebid/prebid-server/stored_requests/backends/empty_fetcher"
"github.com/prebid/prebid-server/usersync"
Expand Down Expand Up @@ -264,6 +265,8 @@ func (deps *endpointDeps) parseRequest(httpRequest *http.Request) (req *openrtb.
return
}

lmt.ModifyForIOS(req)
Copy link
Contributor

@guscarreon guscarreon Feb 24, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@SyntaxNode SyntaxNode Feb 24, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree


errL := deps.validateRequest(req)
if len(errL) > 0 {
errs = append(errs, errL...)
Expand Down
29 changes: 29 additions & 0 deletions endpoints/openrtb2/auction_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -942,6 +942,7 @@ func TestImplicitDNTEndToEnd(t *testing.T) {
assert.Equal(t, test.expectedDNT, result.Device.DNT, test.description+":dnt")
}
}

func TestImplicitSecure(t *testing.T) {
httpReq := httptest.NewRequest("POST", "/openrtb2/auction", strings.NewReader(validRequest(t, "site.json")))
httpReq.Header.Set(http.CanonicalHeaderKey("X-Forwarded-Proto"), "https")
Expand Down Expand Up @@ -2076,6 +2077,34 @@ func TestValidateBidders(t *testing.T) {
}
}

func TestIOS14EndToEnd(t *testing.T) {
exchange := &nobidExchange{}

endpoint, _ := NewEndpoint(
exchange,
newParamsValidator(t),
&mockStoredReqFetcher{},
empty_fetcher.EmptyFetcher{},
&config.Configuration{MaxRequestSize: maxSize},
newTestMetrics(),
analyticsConf.NewPBSAnalytics(&config.Analytics{}),
map[string]string{},
[]byte{},
openrtb_ext.BuildBidderMap())

httpReq := httptest.NewRequest("POST", "/openrtb2/auction", strings.NewReader(validRequest(t, "app-ios140-no-ifa.json")))

endpoint(httptest.NewRecorder(), httpReq, nil)

result := exchange.gotRequest
if !assert.NotEmpty(t, result, "request received by the exchange.") {
t.FailNow()
}

var lmtOne int8 = 1
assert.Equal(t, &lmtOne, result.Device.Lmt)
}

// nobidExchange is a well-behaved exchange which always bids "no bid".
type nobidExchange struct {
gotRequest *openrtb.BidRequest
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
{
"description": "Well Formed iOS 14.0 App Request With Unspecified LMT + Empty IFA",
"mockBidRequest": {
"id": "some-request-id",
"app": {
"id": "some-app-id"
},
"device": {
"os": "iOS",
"osv": "14.0",
"ifa": ""
},
"imp": [{
"id": "my-imp-id",
"banner": {
"format": [{
"w": 300,
"h": 600
}]
},
"ext": {
"appnexus": {
"placementId": 12883451
}
}
}]
},
"expectedBidResponse": {
"id": "some-request-id",
"bidid": "test bid id",
"nbr": 0
},
"expectedReturnCode": 200
}
71 changes: 70 additions & 1 deletion openrtb_ext/device.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package openrtb_ext

import (
"encoding/json"
"errors"
"strconv"

"github.com/buger/jsonparser"
Expand All @@ -12,14 +14,58 @@ const PrebidExtKey = "prebid"

// ExtDevice defines the contract for bidrequest.device.ext
type ExtDevice struct {
// Attribute:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool

// atts
// Type:
// integer; optional - iOS Only
// Description:
// iOS app tracking authorization status.
// Extention Spec:
// https://github.com/InteractiveAdvertisingBureau/openrtb/blob/master/extensions/community_extensions/skadnetwork.md
ATTS *IOSAppTrackingStatus `json:"atts"`

// Attribute:
// prebid
// Type:
// object; optional
// Description:
// Prebid specific extensions for the Device object.
Prebid ExtDevicePrebid `json:"prebid"`
}

// Pointer to interstitial so we do not force it to exist
// IOSAppTrackingStatus describes the values for iOS app tracking authorization status.
type IOSAppTrackingStatus int

// Values of the IOSAppTrackingStatus enumeration.
const (
IOSAppTrackingStatusNotDetermined IOSAppTrackingStatus = 0
IOSAppTrackingStatusRestricted IOSAppTrackingStatus = 1
IOSAppTrackingStatusDenied IOSAppTrackingStatus = 2
IOSAppTrackingStatusAuthorized IOSAppTrackingStatus = 3
)

// IsKnownIOSAppTrackingStatus returns true if the value is a known iOS app tracking authorization status.
func IsKnownIOSAppTrackingStatus(v int64) bool {
switch IOSAppTrackingStatus(v) {
case IOSAppTrackingStatusNotDetermined:
return true
case IOSAppTrackingStatusRestricted:
return true
case IOSAppTrackingStatusDenied:
return true
case IOSAppTrackingStatusAuthorized:
return true
default:
return false
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
}

// ExtDevicePrebid defines the contract for bidrequest.device.ext.prebid
type ExtDevicePrebid struct {
Interstitial *ExtDeviceInt `json:"interstitial"`
}

// ExtDeviceInt defines the contract for bidrequest.device.ext.prebid.interstitial
type ExtDeviceInt struct {
MinWidthPerc uint64 `json:"minwidtheperc"`
MinHeightPerc uint64 `json:"minheightperc"`
Expand Down Expand Up @@ -49,3 +95,26 @@ func (edi *ExtDeviceInt) UnmarshalJSON(b []byte) error {
}
return nil
}

// ParseDeviceExtATTS parses the ATTS value from the request.device.ext OpenRTB field.
func ParseDeviceExtATTS(deviceExt json.RawMessage) (*IOSAppTrackingStatus, error) {
v, err := jsonparser.GetInt(deviceExt, "atts")

// node not found error
if err == jsonparser.KeyPathNotFoundError {
return nil, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@SyntaxNode SyntaxNode Feb 25, 2021

Choose a reason for hiding this comment

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

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.

}

// unexpected parse error
if err != nil {
return nil, err
}

// invalid value error
if !IsKnownIOSAppTrackingStatus(v) {
return nil, errors.New("invalid status")
}

status := IOSAppTrackingStatus(v)
return &status, nil
}
83 changes: 79 additions & 4 deletions openrtb_ext/device_test.go
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
package openrtb_ext_test
package openrtb_ext

import (
"encoding/json"
"testing"

"github.com/prebid/prebid-server/openrtb_ext"
"github.com/stretchr/testify/assert"
)

func TestInvalidDeviceExt(t *testing.T) {
var s openrtb_ext.ExtDevice
var s ExtDevice
assert.EqualError(t, json.Unmarshal([]byte(`{"prebid":{"interstitial":{"minheightperc":0}}}`), &s), "request.device.ext.prebid.interstitial.minwidthperc must be a number between 0 and 100")
assert.EqualError(t, json.Unmarshal([]byte(`{"prebid":{"interstitial":{"minwidthperc":105}}}`), &s), "request.device.ext.prebid.interstitial.minwidthperc must be a number between 0 and 100")
assert.EqualError(t, json.Unmarshal([]byte(`{"prebid":{"interstitial":{"minwidthperc":true,"minheightperc":0}}}`), &s), "request.device.ext.prebid.interstitial.minwidthperc must be a number between 0 and 100")
Expand All @@ -23,7 +22,7 @@ func TestInvalidDeviceExt(t *testing.T) {
}

func TestValidDeviceExt(t *testing.T) {
var s openrtb_ext.ExtDevice
var s ExtDevice
assert.NoError(t, json.Unmarshal([]byte(`{"prebid":{}}`), &s))
assert.Nil(t, s.Prebid.Interstitial)
assert.NoError(t, json.Unmarshal([]byte(`{}`), &s))
Expand All @@ -32,3 +31,79 @@ func TestValidDeviceExt(t *testing.T) {
assert.EqualValues(t, 75, s.Prebid.Interstitial.MinWidthPerc)
assert.EqualValues(t, 60, s.Prebid.Interstitial.MinHeightPerc)
}

func TestIsKnownIOSAppTrackingStatus(t *testing.T) {
valid := []int64{0, 1, 2, 3}
invalid := []int64{-1, 4}

for _, v := range valid {
assert.True(t, IsKnownIOSAppTrackingStatus(v))
}

for _, v := range invalid {
assert.False(t, IsKnownIOSAppTrackingStatus(v))
}
}

func TestParseDeviceExtATTS(t *testing.T) {
authorized := IOSAppTrackingStatusAuthorized

tests := []struct {
description string
givenExt json.RawMessage
expectedStatus *IOSAppTrackingStatus
expectedError string
}{
{
description: "Nil",
givenExt: nil,
expectedStatus: nil,
},
{
description: "Empty",
givenExt: json.RawMessage(``),
expectedStatus: nil,
},
{
description: "Empty Object",
givenExt: json.RawMessage(`{}`),
expectedStatus: nil,
},
{
description: "Valid",
givenExt: json.RawMessage(`{"atts":3}`),
expectedStatus: &authorized,
},
{
description: "Invalid Value",
givenExt: json.RawMessage(`{"atts":5}`),
expectedStatus: nil,
expectedError: "invalid status",
},
{
// This test case produces an error with the standard Go library, but jsonparser doesn't
// return an error for malformed JSON. It treats this case the same as not being found.
description: "Malformed - Standard Test Case",
givenExt: json.RawMessage(`malformed`),
expectedStatus: nil,
},
{
description: "Malformed - Wrong Type",
givenExt: json.RawMessage(`{"atts":"1"}`),
expectedStatus: nil,
expectedError: "Value is not a number: 1",
},
}

for _, test := range tests {
status, err := ParseDeviceExtATTS(test.givenExt)

if test.expectedError == "" {
assert.NoError(t, err, test.description+":err")
} else {
assert.EqualError(t, err, test.expectedError, test.description+":err")
}

assert.Equal(t, test.expectedStatus, status, test.description+":status")
}
}
67 changes: 67 additions & 0 deletions privacy/lmt/ios.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
package lmt

import (
"strings"

"github.com/mxmCherry/openrtb"
"github.com/prebid/prebid-server/openrtb_ext"
"github.com/prebid/prebid-server/util/iosutil"
)

var (
int8Zero int8 = 1
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
int8One int8 = 1
)

// ModifyForIOS modifies the request's LMT flag based on iOS version and identity.
func ModifyForIOS(req *openrtb.BidRequest) {
modifiers := map[iosutil.VersionClassification]modifier{
iosutil.Version140: modifyForIOS14X,
iosutil.Version141: modifyForIOS14X,
iosutil.Version142OrGreater: modifyForIOS142OrGreater,
}
modifyForIOS(req, modifiers)
}

func modifyForIOS(req *openrtb.BidRequest, modifiers map[iosutil.VersionClassification]modifier) {
if !isRequestForIOS(req) {
return
}

versionClassification := iosutil.DetectVersionClassification(req.Device.OSV)
if modifier, ok := modifiers[versionClassification]; ok {
modifier(req)
}
Comment on lines +32 to +34
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@SyntaxNode SyntaxNode Feb 24, 2021

Choose a reason for hiding this comment

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

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.

}

func isRequestForIOS(req *openrtb.BidRequest) bool {
return req != nil && req.App != nil && req.Device != nil && strings.EqualFold(req.Device.OS, "ios")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@SyntaxNode SyntaxNode Feb 25, 2021

Choose a reason for hiding this comment

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

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.

}

type modifier func(req *openrtb.BidRequest)

func modifyForIOS14X(req *openrtb.BidRequest) {
if req.Device.IFA == "" || req.Device.IFA == "00000000-0000-0000-0000-000000000000" {
req.Device.Lmt = &int8One
} else {
req.Device.Lmt = &int8Zero
}
}

func modifyForIOS142OrGreater(req *openrtb.BidRequest) {
atts, err := openrtb_ext.ParseDeviceExtATTS(req.Device.Ext)
if err != nil || atts == nil {
return
}

switch *atts {
case openrtb_ext.IOSAppTrackingStatusNotDetermined:
req.Device.Lmt = &int8Zero
case openrtb_ext.IOSAppTrackingStatusRestricted:
req.Device.Lmt = &int8One
case openrtb_ext.IOSAppTrackingStatusDenied:
req.Device.Lmt = &int8One
case openrtb_ext.IOSAppTrackingStatusAuthorized:
req.Device.Lmt = &int8Zero
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 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
}
Loading