feat: Send a standard user-agent header #1
Conversation
WalkthroughPasses optional Android Context into SSE and HTTP clients to enable dynamic User-Agent injection: FlagsmithEventService constructor now accepts Context; Flagsmith passes Context when realtime updates enabled; Retrofit gains a user-agent interceptor and tests validate header behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Flagsmith
participant EventSvc as FlagsmithEventService
participant Retrofit as FlagsmithRetrofitService
participant OkHttp as OkHttpClient
participant UA as userAgentInterceptor
participant SSE as SSE Server
participant API as Flagsmith API
App->>Flagsmith: init(..., context, enableRealtimeUpdates=true)
Flagsmith->>EventSvc: new FlagsmithEventService(..., context, updates)
Flagsmith->>Retrofit: build client (context)
EventSvc->>OkHttp: addInterceptor(userAgentInterceptor(context))
Retrofit->>OkHttp: addInterceptor(userAgentInterceptor(context))
OkHttp->>UA: Intercept(request)
UA->>UA: compute User-Agent (from Context / BuildConfig or "unknown")
UA->>OkHttp: forward request with User-Agent header
EventSvc->>SSE: open EventSource (with headers)
SSE-->>EventSvc: send events
EventSvc-->>Flagsmith: updates(Result<FlagEvent>)
OkHttp->>API: HTTP requests include User-Agent
API-->>OkHttp: responses
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
FlagsmithClient/src/main/java/com/flagsmith/Flagsmith.kt(1 hunks)FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithEventService.kt(2 hunks)FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt(2 hunks)FlagsmithClient/src/test/java/com/flagsmith/UserAgentTests.kt(1 hunks)
🔇 Additional comments (6)
FlagsmithClient/src/main/java/com/flagsmith/Flagsmith.kt (1)
54-54: LGTM! Context parameter correctly wired for realtime updates.The change properly passes the
contextparameter toFlagsmithEventServicewhen realtime updates are enabled, which is necessary for the new User-Agent header functionality.FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithEventService.kt (2)
19-19: LGTM! Context parameter properly added.The
contextparameter is correctly added to enable User-Agent header injection for SSE connections.
24-24: LGTM! User-Agent interceptor properly wired for SSE client.The
userAgentInterceptoris correctly added to the OkHttpClient builder, ensuring the User-Agent header is present in SSE connections alongside the environment key header.FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt (3)
42-53: LGTM! User-Agent construction is robust.The implementation correctly:
- Handles null context with safe navigation operators
- Falls back to "unknown" when versionName is null or an exception occurs
- Uses appropriate error handling for defensive coding
55-63: LGTM! Interceptor follows standard OkHttp patterns.The interceptor correctly adds the User-Agent header to each request using the standard chain-based approach.
117-117: LGTM! User-Agent interceptor properly wired for REST API client.The interceptor is correctly added to the OkHttpClient builder, ensuring the User-Agent header is present in all REST API calls.
gazreese
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
FlagsmithClient/src/test/java/com/flagsmith/UserAgentTests.kt (2)
47-175: Consider extracting common mock setup to reduce duplication.The first four tests repeat similar mock setup code for
Context,PackageManager, andPackageInfo. While test clarity is important, extracting this into a helper method could improve maintainability.Example refactor:
private fun setupMockContext(versionName: String?) { mockPackageInfo.versionName = versionName Mockito.`when`(mockContext.packageManager).thenReturn(mockPackageManager) Mockito.`when`(mockContext.packageName).thenReturn("com.test.app") Mockito.`when`(mockPackageManager.getPackageInfo(mockContext.packageName, 0)) .thenReturn(mockPackageInfo) } private fun setupMockContextWithException() { Mockito.`when`(mockContext.packageManager).thenReturn(mockPackageManager) Mockito.`when`(mockContext.packageName).thenReturn("com.test.app") Mockito.`when`(mockPackageManager.getPackageInfo(mockContext.packageName, 0)) .thenThrow(PackageManager.NameNotFoundException("Package not found")) }Then use:
setupMockContext("2.4.1")orsetupMockContextWithException()in each test.
47-244: Add SSE User-Agent header test or document interceptor usage.
FlagsmithEventService’s sseClient is already configured with userAgentInterceptor(context), so SSE connections include the User-Agent header—but there’s no test covering this. Either add a unit test that mocks the SSE endpoint and verifies the User-Agent on the SSE request, or add a comment in FlagsmithEventService.kt explaining that the interceptor ensures User-Agent is applied to SSE streams.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
FlagsmithClient/src/test/java/com/flagsmith/UserAgentTests.kt(1 hunks)
🔇 Additional comments (1)
FlagsmithClient/src/test/java/com/flagsmith/UserAgentTests.kt (1)
1-245: Excellent improvements! Tests now properly validate User-Agent header injection.The test suite has been completely rewritten to address the critical issues flagged in the previous review. The tests now:
- Create real
Flagsmithinstances with properly mocked Android context- Execute actual API calls (
getFeatureFlagsSync,getIdentitySync,setTraitSync)- Use
MockServerto capture and verify HTTP requests with the expectedUser-Agentheader- Cover key scenarios: valid version, null context, exception handling, and null version name
This is a significant improvement over the previous literal string equality assertions that provided no actual test coverage.
There was a problem hiding this comment.
Comprehensive Review Summary
Thank you for this well-implemented feature! The code quality is high with excellent test coverage. However, I've identified a few important issues that should be addressed.
🔴 Critical Issues
1. Semantic Confusion: APP Version vs SDK Version (HIGH PRIORITY)
The User-Agent header uses the format flagsmith-kotlin-android-sdk/<version>, but <version> is actually the host application's version, not the SDK version. This is semantically incorrect and potentially misleading for server-side analytics.
Recommended fix: Include both SDK and app versions:
private fun getUserAgent(context: Context?): String {
val sdkVersion = BuildConfig.VERSION_NAME ?: "unknown"
return try {
val packageInfo = context?.packageManager?.getPackageInfo(context.packageName, 0)
val appVersion = packageInfo?.versionName ?: "unknown"
"$USER_AGENT_PREFIX/$sdkVersion (app/$appVersion)"
} catch (e: Exception) {
"$USER_AGENT_PREFIX/$sdkVersion"
}
}
// Result: flagsmith-kotlin-android-sdk/1.8.0 (app/2.4.1)2. Documentation Error in PR Description
The PR description states the format as flagsmith-swift-ios-sdk/<version> (appears to be copy-pasted from iOS SDK). Should be flagsmith-kotlin-android-sdk/<version>.
🟡 Important Recommendations
3. Missing Test Coverage for SSE
While the User-Agent interceptor is correctly added to the SSE client, there are no tests validating User-Agent is sent with SSE connections or analytics requests.
4. Performance Optimization
getUserAgent() is called on every HTTP request. Should compute once when the interceptor is created:
fun userAgentInterceptor(context: Context?): Interceptor {
val userAgent = getUserAgent(context) // Compute once, not per-request
return Interceptor { chain ->
val request = chain.request().newBuilder()
.addHeader(USER_AGENT_HEADER, userAgent)
.build()
chain.proceed(request)
}
}5. Missing PR Template File
PR description mentions adding a PR template, but no template file was added to the repository (.github/PULL_REQUEST_TEMPLATE.md).
6. Overly Broad Exception Handling
Consider catching specific exceptions instead of generic Exception:
} catch (e: PackageManager.NameNotFoundException) {
"$USER_AGENT_PREFIX/unknown"
} catch (e: Exception) {
Log.w("FlagsmithRetrofitService", "Unexpected error getting version", e)
"$USER_AGENT_PREFIX/unknown"
}✅ Positive Aspects
- Excellent test coverage with 6 comprehensive test cases covering edge cases
- Proper null safety throughout the implementation
- Non-breaking change with consistent implementation across all HTTP clients
- Clean code structure with good separation of concerns
- Proper test isolation using MockServer and Mockito
📋 Recommendation
APPROVE with recommended fixes - The version semantics issue (#1) should be addressed before merge. The other recommendations would improve robustness and maintainability but are not blockers.
Great work overall! 🚀
| private fun getUserAgent(context: Context?): String { | ||
| return try { | ||
| val packageInfo = context?.packageManager?.getPackageInfo(context.packageName, 0) | ||
| val version = packageInfo?.versionName ?: "unknown" |
There was a problem hiding this comment.
Critical Issue: Semantic confusion between APP and SDK versions
This retrieves the host application's version (via context.packageName), not the SDK version. However, the User-Agent prefix is flagsmith-kotlin-android-sdk, which suggests it should contain the SDK version.
This is misleading for server-side analytics and support teams who might expect to see the SDK version for debugging SDK-specific issues.
Recommendation: Include both versions for clarity:
private fun getUserAgent(context: Context?): String {
val sdkVersion = BuildConfig.VERSION_NAME ?: "unknown"
return try {
val packageInfo = context?.packageManager?.getPackageInfo(context.packageName, 0)
val appVersion = packageInfo?.versionName ?: "unknown"
"$USER_AGENT_PREFIX/$sdkVersion (app/$appVersion)"
} catch (e: Exception) {
"$USER_AGENT_PREFIX/$sdkVersion"
}
}
// Result: flagsmith-kotlin-android-sdk/1.8.0 (app/2.4.1)There was a problem hiding this comment.
Hi @polatolu I think this needs to be looked at. The previous implementation got the package version in various ways depending on Cocoapods or SPM, and if we don't find those I'd default to 'unknown'
There was a problem hiding this comment.
I agree, I'll address this now.
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt
Show resolved
Hide resolved
| val version = packageInfo?.versionName ?: "unknown" | ||
| "$USER_AGENT_PREFIX/$version" | ||
| } catch (e: Exception) { | ||
| "$USER_AGENT_PREFIX/unknown" |
There was a problem hiding this comment.
Overly broad exception handling
Catching all Exception types is too broad and might hide unexpected runtime exceptions that could indicate bugs.
Recommendation: Catch specific exceptions:
} catch (e: PackageManager.NameNotFoundException) {
"$USER_AGENT_PREFIX/unknown"
} catch (e: Exception) {
// Log unexpected exceptions for debugging
Log.w("FlagsmithRetrofitService", "Unexpected error getting version", e)
"$USER_AGENT_PREFIX/unknown"
}This helps identify unexpected errors during development while still providing graceful fallback.
There was a problem hiding this comment.
Honestly I think this is fine in an SDK
| .withHeader("User-Agent", "flagsmith-kotlin-android-sdk/$expectedAppVersion") | ||
| ) | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Missing Test Coverage: SSE and Analytics
Great test coverage for REST API calls! However, the User-Agent interceptor is also added to:
- SSE client (Server-Sent Events) in
FlagsmithEventService.kt:24 - Analytics requests
Recommendation: Add tests to verify User-Agent is sent with SSE and analytics requests:
@Test
fun testUserAgentHeaderSentWithSSEConnection() {
// Test that User-Agent is included when connecting to SSE endpoint
}
@Test
fun testUserAgentHeaderSentWithAnalyticsRequest() {
// Test that User-Agent is included when posting analytics
}This ensures complete coverage of all network request types.
There was a problem hiding this comment.
This will be a total pain, I think we can leave this
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt (1)
62-70: Consider caching the User-Agent string as previously suggested.The User-Agent string is recomputed on every request, even though it never changes during the app's lifecycle. A previous reviewer suggested computing it once when the interceptor is created, which remains a valid optimization.
Apply this diff to cache the User-Agent string:
fun userAgentInterceptor(context: Context?): Interceptor { + val userAgent = getUserAgent(context) return Interceptor { chain -> - val userAgent = getUserAgent(context) val request = chain.request().newBuilder() .addHeader(USER_AGENT_HEADER, userAgent) .build() chain.proceed(request) } }
🧹 Nitpick comments (1)
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt (1)
45-48: Consider removing unused Context parameter.The
contextparameter is passed but never used in this function. Since you've correctly resolved the semantic confusion from previous reviews (this now gets SDK version only, not app version), the Context is no longer needed here.Apply this diff to remove the unused parameter:
-private fun getUserAgent(context: Context?): String { +private fun getUserAgent(): String { val sdkVersion = getSdkVersion() return "$USER_AGENT_PREFIX/$sdkVersion" } fun userAgentInterceptor(context: Context?): Interceptor { return Interceptor { chain -> - val userAgent = getUserAgent(context) + val userAgent = getUserAgent() val request = chain.request().newBuilder() .addHeader(USER_AGENT_HEADER, userAgent) .build() chain.proceed(request) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt(2 hunks)FlagsmithClient/src/test/java/com/flagsmith/UserAgentTests.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- FlagsmithClient/src/test/java/com/flagsmith/UserAgentTests.kt
🧰 Additional context used
🪛 detekt (1.23.8)
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt
[warning] 56-56: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (3)
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt (3)
42-43: LGTM!The constants are correctly defined for User-Agent header injection.
124-124: LGTM!The User-Agent interceptor is correctly wired into the OkHttpClient builder.
50-60: No issues found—the BuildConfig class name is correct.The build.gradle.kts file defines
namespace = "com.flagsmith.kotlin", which is where Gradle generates the BuildConfig class. The reflection call ingetSdkVersion()correctly targetscom.flagsmith.kotlin.BuildConfig. The kover configuration also confirms this:"${android.namespace}.BuildConfig". In tests, BuildConfig is unavailable, so the fallback to "unknown" is expected and correct.
Description
Added a User-Agent HTTP header to network calls. Uses the package's version at runtime, if it is not available sends it as "unknown"
Also added a PR template, once this merged new PRs will come with this PR template.
Regression Test Recommendations
Type of Change
🛠️ 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📝 DocumentationSummary by CodeRabbit
New Features
Tests