Resolver Specific Parameters (ID5 Mobile In-App)#58
Resolver Specific Parameters (ID5 Mobile In-App)#58OlenaPostindustria wants to merge 10 commits into
Conversation
eugenedorfman-optable
left a comment
eugenedorfman-optable
left a comment
There was a problem hiding this comment.
Review of the ID5 Mobile In-App changes. Overall a clean, well-tested feature, and the hid= wire encoding is correct: each hint is emitted as its own repeated hid= query item (never comma-joined), which is what the edge expects since it does not split hid on commas.
Inline comments below, tagged major / minor / nit. Two cross-cutting points that are not line-specific:
- README not updated. There is no
targetingcode sample in the README, so the newhidsparameter and the whole ID5 Mobile In-App feature are undocumented for integrators. A short README/demo snippet showing how to pass hints would help. - ObjC + Swift docs. See the inline note on the doc comment: it should actually explain what a hint ID is and which identifier types are valid.
Most important items to resolve before merge are the three major comments (ObjC API break, IDFA casing, and the implicit opt-in for all existing callers).
| var hids = hids ?? [] | ||
|
|
||
| enrichIfNeeded(ids: &ids) | ||
| enrichIfNeeded(ids: &hids) |
There was a problem hiding this comment.
Major - all existing callers are silently opted into hint/ID5 resolution. enrichIfNeeded(ids: &hids) runs even when the caller passed hids: nil, and in EdgeAPI.targeting the bundle/ver/ua/id5_signature params are appended unconditionally. So every current targeting() call will start sending hid=a:<idfa> plus the resolver params, with no opt-out other than skipAdvertisingIdDetection (which also disables the id= enrichment).
If that implicit opt-in is intended edge behavior, fine - otherwise gate hid emission (and arguably the resolver params) on the caller actually supplying hids.
There was a problem hiding this comment.
hid=a:<idfa> contains the same IDFA we already send as id=a:<idfa>, and it's still subject to the same ATT and skipAdvertisingIdDetection checks. id5_signature is only sent if the DCN has already returned one. ID5 usage is controlled entirely server-side. if it's enabled, the hid and the related parameters are used, otherwise, it should be ignored. this should let ID5 be enabled through the configuration without requiring an app update
| queryItems.append(URLQueryItem(name: "ua", value: userAgent)) | ||
| } | ||
|
|
||
| if let targeting = storage.getTargeting(), let id5Signature = targeting.targetingData["id5_signature"] as? String { |
There was a problem hiding this comment.
Minor - the id5_signature cache can be clobbered. It is read from the cached targeting response, but _profile (OptableSDK.swift:400) also calls setTargeting, overwriting the cache with a profile response that will not carry id5_signature. So a profile() call between two targeting() calls silently drops the signature and breaks the ID5 refresh chain. Consider persisting the signature under its own storage key.
Also worth confirming the edge returns id5_signature at the top level of the mobile in-app response (vs nested).
There was a problem hiding this comment.
we are awaiting clarification regarding the placement of id5_signature in the response.
is this comment relevant here? do we need to maintain two separate cached responses?
3 participants
Closes #57