-
Notifications
You must be signed in to change notification settings - Fork 760
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
Changes from 9 commits
1b9e88d
012bf15
0ce954d
fab7f42
f0ff6e4
c45e379
5d847f5
d87dfb0
1bda2b3
3f8ab0b
fb5018b
b331957
1d90540
17d7252
32d39c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
} |
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" | ||
|
@@ -12,14 +14,58 @@ const PrebidExtKey = "prebid" | |
|
||
// ExtDevice defines the contract for bidrequest.device.ext | ||
type ExtDevice struct { | ||
// Attribute: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"` | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we not returning an error here? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
} |
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we missing any further checks inside There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick: we already ran this
In order to get rid of the
Or something of that sorts There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it. How do you feel about grouping the
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
} |
There was a problem hiding this comment.
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 afterprocessInterstitials(req)
in line 263. ThisprocessInterstitials(req)
function unmarshalls theDevice.Ext
making thejsonparser.GetInt(deviceExt, "atts")
call thatlmt.ModifyForIOS(req)
eventually makes insideParseDeviceExtATTS(deviceExt json.RawMessage)
to be unnecessary. Actually, that 261 to 268 neighborhood makes use ofDevice
a lotDo you think it'd be a good idea to maybe consolidate inside
deps.setFieldsImplicitly(httpRequest, req)
?There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 unmarshaledDevice.Ext
and modifylmt.ModifyForIOS(req)
accordingly. What do you think? Given the time constraint, do you feel it's feasible?There was a problem hiding this comment.
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
andModifyForIOS
only unmarshal the device ext in certain circumstances, so I can't rely onprocessInterstitials
actually performing the unmarshal.Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree