-
Notifications
You must be signed in to change notification settings - Fork 16
feat!: send user agent header #95
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
Changes from 5 commits
50274f9
1e8a7dc
ce3f6d3
4b1c052
97a0dae
17621a2
4b9646d
1553f84
7df2d9e
c03685e
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,29 @@ | ||
| <!-- | ||
| Provide a description of your changes below and a general summary in the title. | ||
| Please also provide some test recommendations if necessary to ensure we don't have regressions. | ||
| Please look at the following checklist to ensure that your PR can be accepted quickly: | ||
| --> | ||
|
|
||
| ## Description | ||
|
|
||
| <!--- Describe your changes in detail --> | ||
|
|
||
| ## Regression Test Recommendations | ||
|
|
||
| <!--- Functionality that could be affected by the change and any other concerns --> | ||
|
|
||
| ## Type of Change | ||
|
|
||
| <!--- Put an `x` in all the boxes that apply: --> | ||
|
|
||
| - [ ] ✨ New feature (non-breaking change which adds functionality) | ||
| - [ ] 🛠️ Bug fix (non-breaking change which fixes an issue) | ||
| - [ ] ❌ Breaking change (fix or feature that would cause existing functionality to change) | ||
| - [ ] 🧹 Code refactor | ||
| - [ ] ✅ Build configuration change | ||
| - [ ] 📝 Documentation | ||
| - [ ] 🗑️ Chore | ||
|
|
||
| ## Estimated time to fix the ticket(s) or epic(s) referenced by the PR in days | ||
|
|
||
| <!--- Add estimate to complete the work --> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,39 @@ typealias CompletionHandler<T> = @Sendable (Result<T, any Error>) -> Void | |
| public final class Flagsmith: @unchecked Sendable { | ||
| /// Shared singleton client object | ||
| public static let shared: Flagsmith = .init() | ||
|
|
||
| /// SDK version constant - should match the podspec version | ||
| /// This is used as a fallback when bundle version detection fails | ||
| private static let sdkVersionConstant = "3.8.4" | ||
|
|
||
| /// User-Agent header value for HTTP requests | ||
| /// Format: flagsmith-swift-ios-sdk/<version> | ||
| /// Falls back to "unknown" if version is not discoverable at runtime | ||
| public static let userAgent: String { | ||
| let version = getSDKVersion() | ||
| return "flagsmith-swift-ios-sdk/\(version)" | ||
| } | ||
|
|
||
| /// Get the SDK version from the bundle at runtime | ||
| /// Falls back to hardcoded constant or "unknown" if version is not discoverable | ||
| private static func getSDKVersion() -> String { | ||
| // Try CocoaPods bundle first | ||
| if let bundle = Bundle(identifier: "org.cocoapods.FlagsmithClient"), | ||
| let version = bundle.infoDictionary?["CFBundleShortVersionString"] as? String, | ||
| !version.isEmpty, | ||
| version.range(of: #"^\d+\.\d+\.\d+"#, options: .regularExpression) != nil { | ||
| return version | ||
| } | ||
|
|
||
| // Try SPM bundle | ||
| if let version = Bundle(for: Flagsmith.self).infoDictionary?["CFBundleShortVersionString"] as? String, | ||
| !version.isEmpty, | ||
| version.range(of: #"^\d+\.\d+\.\d+"#, options: .regularExpression) != nil { | ||
| return version | ||
| } | ||
|
Contributor
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 think we're OK manually checking a semver-like pattern here, though we could maybe rely on semver parsing for future-proofing, in case we occasionally adopt build or pre-releases metadata.
Contributor
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. This all becomes irrelevant if we stick to release-please markers. |
||
|
|
||
| return "unknown" | ||
| } | ||
| private let apiManager: APIManager | ||
| private let sseManager: SSEManager | ||
| private let analytics: FlagsmithAnalytics | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -21,6 +21,42 @@ final class RouterTests: FlagsmithClientTestCase { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| XCTAssertTrue(request.allHTTPHeaderFields?.contains(where: { $0.key == "X-Environment-Key" }) ?? false) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| XCTAssertNil(request.httpBody) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func testUserAgentHeader() throws { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let url = try XCTUnwrap(baseUrl) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let route = Router.getFlags | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let request = try route.request(baseUrl: url, apiKey: apiKey) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Verify User-Agent header is present | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| XCTAssertTrue(request.allHTTPHeaderFields?.contains(where: { $0.key == "User-Agent" }) ?? false) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Verify User-Agent header format | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let userAgent = request.allHTTPHeaderFields?["User-Agent"] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| XCTAssertNotNil(userAgent) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| XCTAssertTrue(userAgent?.hasPrefix("flagsmith-swift-ios-sdk/") ?? false) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Verify the format is correct (should end with a semantic version number) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let expectedPattern = "^flagsmith-swift-ios-sdk/[0-9]+\\.[0-9]+\\.[0-9]+$" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let regex = try NSRegularExpression(pattern: expectedPattern) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let range = NSRange(location: 0, length: userAgent?.count ?? 0) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let match = regex.firstMatch(in: userAgent ?? "", options: [], range: range) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let message = "User-Agent should match pattern 'flagsmith-swift-ios-sdk/<version>', got: \(userAgent ?? "nil")" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| XCTAssertTrue(match != nil, message) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func testUserAgentHeaderFormat() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Test that the User-Agent format is correct | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let userAgent = Flagsmith.userAgent | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| XCTAssertTrue(userAgent.hasPrefix("flagsmith-swift-ios-sdk/")) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Should have a semantic version number (e.g., 3.8.4) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let versionPart = String(userAgent.dropFirst("flagsmith-swift-ios-sdk/".count)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| XCTAssertTrue(versionPart.range(of: #"^\d+\.\d+\.\d+$"#, options: NSString.CompareOptions.regularExpression) != nil, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "Version part should be a semantic version number (e.g., 3.8.4), got: \(versionPart)") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Should be the expected SDK version | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| XCTAssertEqual(versionPart, "3.8.4", "Expected SDK version 3.8.4, got: \(versionPart)") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
gazreese marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+30
to
+61
Contributor
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. The two tests seem to be crossing wires. This suggestion replaces them both with a simple assertion, though I haven't tested it. Let me know if you agree with it.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| func testGetIdentityRequest() throws { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let url = try XCTUnwrap(baseUrl) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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 understand the resilience this adds given the platform, though this may prompt for some process that ensures code will be kept in sync with the actual version tagged.
@polatolu Do you think we need to implement some kind of CI script to accomplish this? Alternatively, would you consider falling back to "unknown" when bundle version detection fails?
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.
Possible we need to add this to the release please config as well to resolve this? See instructions here.
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.
FTR we were successful with the release-please approach, and most SDKs are already using it. See example.