Allow setting the HTTP Origin header - #61
Conversation
eugenedorfman-optable
left a comment
There was a problem hiding this comment.
Two things to change, plus a doc nit.
Blank origin sends an empty header. Source/Core/EdgeAPI.swift:157 sets the header from whatever is configured, so config.origin = "" produces a literal Origin: with an empty value. The Android SDK suppresses blank values (isNullOrBlank() in RequestInterceptor.kt) and has tests for both blank and null, so the two SDKs disagree here. Suggested fix:
// NOTE: A blank origin would send an empty `Origin:` header, which is worse than sending none.
// Android suppresses blank values too, so both SDKs behave the same.
if let origin = config.origin, !origin.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty {
headers[.origin] = origin
}with a test covering "" and " ", and the doc comment on OptableConfig.origin updated to say a blank value is treated as unset.
Unrelated dependency churn. demo-ios-objc/Podfile.lock:2-11 bumps Google-Mobile-Ads-SDK 12.14.0 -> 13.7.0 and PrebidMobile 3.1.0 -> 3.3.2. That looks like a side effect of running pod install against the unpinned Podfile rather than anything this change needs. Please revert those hunks; if a demo dependency refresh is wanted, it belongs in its own PR.
Doc example value. docs/usage-swift.md:57 and docs/usage-objc.md:81 use https://dcn.customer.com, which these same docs use as the DCN endpoint hostname. The header is meant to carry the site origin that traffic is attributed to, so a customer-site example reads better: the Android README uses https://www.acmeco.com along with the wording "the origin you want your mobile traffic attributed to". Adding Android's "Unrelated to originSlug" note to the origin doc comment would help too, since the two names are easy to confuse.
On the cross-SDK side, for reference: origin is mutable after construction here, but was an internal val on Android, which left Java callers reaching it only through the full 11-argument constructor. The Android PR has been updated to make it a settable var, matching this SDK.
eugenedorfman-optable
left a comment
There was a problem hiding this comment.
The three points from the earlier review are addressed: blank and whitespace-only origins are suppressed and covered by tests, the Podfile.lock dependency bumps are reverted so the diff against master no longer touches them, and the docs now use a site-origin example with the attribution wording plus the note that this is unrelated to originSlug. CI is green.
Closes #59