Conversation
WalkthroughThis pull request implements User-Agent header support for HTTP requests by adding interceptors to the Flagsmith SDK. The User-Agent is formatted as "flagsmith-kotlin-android-sdk/{version}" and attached to all outgoing requests. Comprehensive test coverage validates header presence and format across various API endpoints. Changes
Sequence DiagramsequenceDiagram
participant Client as HTTP Client
participant UI as UserAgentInterceptor
participant Net as Network Request
participant Server as Server
Client->>UI: Request
rect rgb(200, 220, 255)
Note over UI: Intercept Request
UI->>UI: Get SDK version (1.8.0)
UI->>UI: Compose User-Agent<br/>flagsmith-kotlin-android-sdk/1.8.0
UI->>UI: Add header to request
end
UI->>Net: Forward with header
Net->>Server: HTTP Request<br/>(includes User-Agent)
Server->>Net: Response
Net->>Client: Return Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🔇 Additional comments (8)
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.
gazreese
left a comment
gazreese
left a comment
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.
| 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.
…d release-please effectiveness
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt (1)
45-48: Unused Context parameter in getUserAgent can be removed for clarity
getUserAgent(context: Context?)ignores itscontextargument and only delegates togetSdkVersion(). Since the current implementation does not need AndroidContext, keeping the parameter is slightly misleading.Consider simplifying to:
- private fun getUserAgent(context: Context?): String { - val sdkVersion = getSdkVersion() - return "$USER_AGENT_PREFIX/$sdkVersion" - } + private fun getUserAgent(): String { + val sdkVersion = getSdkVersion() + return "$USER_AGENT_PREFIX/$sdkVersion" + }and updating
userAgentInterceptoraccordingly. This keeps the API surface tighter and avoids suggesting that app-level context influences the header.FlagsmithClient/src/test/java/com/flagsmith/internal/SdkVersionRetrievalTest.kt (1)
23-32: Good coverage of fallback/version behavior; consider making the fallback test explicitThese tests nicely validate User-Agent format, fallback version, and consistency using MockWebServer, and tying
EXPECTED_FALLBACK_VERSIONto release-please avoids drift.One minor improvement:
testFallbackVersionIsUsedWhenBuildConfigNotAvailablecurrently assumes thatBuildConfigis not present in the unit-test environment. If that assumption ever changes (e.g., different Gradle setup), the test semantics could flip. Longer term, it might be more robust to explicitly simulate the “no BuildConfig / invalid VERSION_NAME” path (e.g., by refactoringgetSdkVersionto accept an injectable lookup helper) rather than relying on classpath behavior.Also applies to: 73-100
FlagsmithClient/src/test/java/com/flagsmith/UserAgentTests.kt (1)
39-189: User-Agent integration tests are solid; minor duplication could be reducedThese tests give good end-to-end coverage that the expected User-Agent is sent for flags, identities, and trait writes, and that the format matches
flagsmith-kotlin-android-sdk/{version}with the version kept in sync via release-please.
testUserAgentHeaderSentWithGetFlagsandtestUserAgentHeaderSentWithNullContextcurrently exercise the same scenario (null context with GET /flags). If you want to trim repetition, you could fold the “null context” assertion into a single test or parameterize the setup, but it’s fine to keep as-is if the explicit naming helps readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
FlagsmithClient/build.gradle.kts(1 hunks)FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt(2 hunks)FlagsmithClient/src/test/java/com/flagsmith/UserAgentTests.kt(1 hunks)FlagsmithClient/src/test/java/com/flagsmith/internal/SdkVersionRetrievalTest.kt(1 hunks)release-please-config.json(1 hunks)
🔇 Additional comments (2)
FlagsmithClient/build.gradle.kts (1)
85-87: MockWebServer test dependency is correctly scoped and version-alignedUsing
mockwebserver:4.11.0as atestImplementationalongsideokhttp-sse:4.11.0is consistent and keeps runtime deps unchanged. No issues here.release-please-config.json (1)
11-16: extra-files wiring keeps User-Agent version single-sourcedIncluding the Retrofit service and the two version-sensitive test files in
extra-filesensures release automation keeps the SDK/User-Agent version consistent across code and tests. Good maintainability improvement.
| fun userAgentInterceptor(context: Context?): Interceptor { | ||
| return Interceptor { chain -> | ||
| val userAgent = getUserAgent(context) | ||
| val request = chain.request().newBuilder() | ||
| .addHeader(USER_AGENT_HEADER, userAgent) | ||
| .build() | ||
| chain.proceed(request) | ||
| } | ||
| } |
There was a problem hiding this comment.
Override existing User-Agent instead of adding a second header
userAgentInterceptor currently uses .addHeader(USER_AGENT_HEADER, userAgent), which will append an additional User-Agent header on top of OkHttp’s default rather than replacing it. Many servers and log/analytics stacks expect a single User-Agent value, so this can lead to ambiguity about which one is honored.
Prefer overriding the header:
- fun userAgentInterceptor(context: Context?): Interceptor {
- return Interceptor { chain ->
- val userAgent = getUserAgent(context)
- val request = chain.request().newBuilder()
- .addHeader(USER_AGENT_HEADER, userAgent)
- .build()
- chain.proceed(request)
- }
- }
+ fun userAgentInterceptor(context: Context?): Interceptor {
+ val userAgent = getUserAgent(context)
+ return Interceptor { chain ->
+ val request = chain.request().newBuilder()
+ .header(USER_AGENT_HEADER, userAgent)
+ .build()
+ chain.proceed(request)
+ }
+ }This still guarantees your custom User-Agent is sent, but without duplicating the header.
Also applies to: 137-138
🤖 Prompt for AI Agents
In
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt
around lines 75-83 (and similarly at 137-138), the interceptor currently uses
addHeader(USER_AGENT_HEADER, userAgent) which appends a second User-Agent
header; replace addHeader with header(USER_AGENT_HEADER, userAgent) (or use
removeHeader then add) so the existing User-Agent is overridden rather than
duplicated, ensuring only a single User-Agent is sent.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt (2)
56-64: Performance: User-Agent computed on every request.The
getUserAgent(context)call on line 58 executes for every HTTP request, even though the User-Agent string never changes during the app's lifecycle. This is unnecessary overhead.Compute the User-Agent once when the interceptor is created:
fun userAgentInterceptor(context: Context?): Interceptor { + val userAgent = getUserAgent(context) // Computed ONCE return Interceptor { chain -> - val userAgent = getUserAgent(context) val request = chain.request().newBuilder() .addHeader(USER_AGENT_HEADER, userAgent) .build() chain.proceed(request) } }This optimization eliminates redundant calls and improves performance.
60-60: ReplaceaddHeaderwithheaderto avoid duplicate User-Agent headers.Using
.addHeader(USER_AGENT_HEADER, userAgent)will append an additional User-Agent header on top of OkHttp's default, potentially causing multiple User-Agent headers to be sent. Many servers and analytics tools expect a single User-Agent value.Use
.header()to replace the existing header:val request = chain.request().newBuilder() - .addHeader(USER_AGENT_HEADER, userAgent) + .header(USER_AGENT_HEADER, userAgent) .build()This ensures only one User-Agent header is sent with each request.
🧹 Nitpick comments (2)
FlagsmithClient/src/test/java/com/flagsmith/internal/SdkVersionRetrievalTest.kt (2)
15-30: Class docs and version constant are clear; consider centralizing test version configurationThe doc comment and companion object make the intent around
EXPECTED_SDK_VERSIONand theUSER_AGENT_PREFIXvery explicit. Given that other tests in the suite also assert against"flagsmith-kotlin-android-sdk/$EXPECTED_SDK_VERSION", you might want to centralize these into a shared test constant (e.g., a smallTestConstantsobject) to avoid divergence between files when release-please updates the version or if the prefix ever changes. This is purely optional but can reduce future maintenance.
43-69: User-Agent prefix assertion looks good; consider using a timedtakeRequestto avoid potential hangsThe structure of this test is solid and the assertion on the
User-Agentprefix is appropriate. The only robustness tweak I’d suggest is usingtakeRequest(timeout, unit)rather than the blockingtakeRequest()so that, if for some reason the request is never sent, the test fails with a clear assertion instead of hanging until the test runner times out. For example:val recordedRequest = mockServer.takeRequest(5, java.util.concurrent.TimeUnit.SECONDS) assertNotNull("Expected a request to be recorded", recordedRequest) val userAgent = recordedRequest!!.getHeader("User-Agent")This makes failures easier to diagnose in CI without changing the core behavior of the test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt(2 hunks)FlagsmithClient/src/test/java/com/flagsmith/UserAgentTests.kt(1 hunks)FlagsmithClient/src/test/java/com/flagsmith/internal/SdkVersionRetrievalTest.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- FlagsmithClient/src/test/java/com/flagsmith/UserAgentTests.kt
🔇 Additional comments (4)
FlagsmithClient/src/main/java/com/flagsmith/internal/FlagsmithRetrofitService.kt (3)
42-43: Constants are well-defined.The User-Agent header name and prefix are correctly defined and follow standard HTTP conventions.
50-54: SDK version approach is appropriate.Using a hardcoded version string with release-please markers is a standard pattern for SDK versioning that allows automated version management during releases.
118-118: Interceptor correctly wired into OkHttpClient.The User-Agent interceptor is properly added to the OkHttpClient builder chain.
FlagsmithClient/src/test/java/com/flagsmith/internal/SdkVersionRetrievalTest.kt (1)
104-140: Good coverage of persistence and exact value of User-AgentThis test does a nice job of verifying both that the
User-Agentheader is stable across multiple requests and that it matches the exact expected value"${USER_AGENT_PREFIX}/${EXPECTED_SDK_VERSION}". That gives strong coupling between the interceptor behavior,getSdkVersion(), and the release-please–managed version. Just be aware that any intentional changes to the prefix or version derivation will require updating this assertion, which is an acceptable tradeoff for the safety it provides.
| @Test | ||
| fun testVersionFormatIsValid() { | ||
| // Given - Create a client with the user agent interceptor | ||
| val interceptor = FlagsmithRetrofitService.userAgentInterceptor(null) | ||
| val client = OkHttpClient.Builder() | ||
| .addInterceptor(interceptor) | ||
| .build() | ||
|
|
||
| mockServer.enqueue(MockResponse().setResponseCode(200).setBody("{}")) | ||
|
|
||
| // When - Make a request | ||
| val request = Request.Builder() | ||
| .url(mockServer.url("/")) | ||
| .build() | ||
|
|
||
| client.newCall(request).execute().use { response -> | ||
| // Then - Verify version format is semantic versioning compatible | ||
| val recordedRequest = mockServer.takeRequest() | ||
| val userAgent = recordedRequest.getHeader("User-Agent")!! | ||
| val version = userAgent.substringAfter("$USER_AGENT_PREFIX/") | ||
|
|
||
| assertTrue("Version should not be empty", version.isNotEmpty()) | ||
| assertTrue("Version should not contain whitespace", version.trim() == version) | ||
|
|
||
| // Version should match semantic versioning pattern (X.Y.Z) or be a valid identifier | ||
| val semverPattern = Regex("^\\d+\\.\\d+\\.\\d+.*$") | ||
| assertTrue( | ||
| "Version should follow semantic versioning or be a valid identifier: $version", | ||
| semverPattern.matches(version) || version.matches(Regex("^[a-zA-Z0-9._-]+$")) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Whitespace assertion doesn’t fully match the message; tighten the check
The intent of
assertTrue("Version should not contain whitespace", version.trim() == version)is that the version “should not contain whitespace”, but this only rejects leading/trailing whitespace. Internal whitespace (e.g. "1.8.0 beta") would still pass.
You can align the assertion with the message by rejecting any whitespace character:
- assertTrue("Version should not contain whitespace", version.trim() == version)
+ assertTrue(
+ "Version should not contain whitespace",
+ !version.any { it.isWhitespace() }
+ )The rest of the format checks (semantic-version‑compatible or a simple identifier like "unknown") look reasonable.
I'll verify this review comment by inspecting the actual test file and confirming the whitespace assertion logic.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and inspect the test file
fd -t f "SdkVersionRetrievalTest.kt" --exec cat {} \;
Length of output: 5074
🏁 Script executed:
#!/bin/bash
# Also search for similar whitespace patterns in the codebase
rg -n "trim()" --type kotlin -C 3 | head -50Length of output: 3772
🏁 Script executed:
#!/bin/bash
# Check for isWhitespace usage in Kotlin files
rg -n "isWhitespace()" --type kotlinLength of output: 0
🏁 Script executed:
#!/bin/bash
# Verify Kotlin String has 'any' method with isWhitespace
rg -n "\.any\s*{" --type kotlin -C 2 | head -30Length of output: 81
I need to verify that the suggested Kotlin syntax is valid and standard. Let me check the Kotlin documentation.
---
🌐 Web query:
```web
Kotlin String any method isWhitespace character
💡 Result:
Use Char.isWhitespace() inside String.any { ... }.
Example:
val s = "hello world"
val hasWs = s.any { it.isWhitespace() } // true
Notes:
- Char.isWhitespace() checks Unicode whitespace categories. [1]
- CharSequence.any(predicate) returns true if any character matches the predicate. [2]
- If you want to test whether the whole string is empty or only whitespace, use isBlank(). [3]
Sources:
[1] Kotlin Char.isWhitespace docs.
[2] Kotlin CharSequence.any docs.
[3] Kotlin CharSequence.isBlank docs.
Whitespace assertion doesn't fully match the message; tighten the check
The assertion version.trim() == version only rejects leading/trailing whitespace, but the message states "should not contain whitespace"—implying any whitespace. Internal whitespace like "1.8.0 beta" would incorrectly pass.
Align the assertion with the message by rejecting any whitespace character:
- assertTrue("Version should not contain whitespace", version.trim() == version)
+ assertTrue(
+ "Version should not contain whitespace",
+ !version.any { it.isWhitespace() }
+ )The rest of the format checks (semantic versioning or simple identifiers) are reasonable.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| fun testVersionFormatIsValid() { | |
| // Given - Create a client with the user agent interceptor | |
| val interceptor = FlagsmithRetrofitService.userAgentInterceptor(null) | |
| val client = OkHttpClient.Builder() | |
| .addInterceptor(interceptor) | |
| .build() | |
| mockServer.enqueue(MockResponse().setResponseCode(200).setBody("{}")) | |
| // When - Make a request | |
| val request = Request.Builder() | |
| .url(mockServer.url("/")) | |
| .build() | |
| client.newCall(request).execute().use { response -> | |
| // Then - Verify version format is semantic versioning compatible | |
| val recordedRequest = mockServer.takeRequest() | |
| val userAgent = recordedRequest.getHeader("User-Agent")!! | |
| val version = userAgent.substringAfter("$USER_AGENT_PREFIX/") | |
| assertTrue("Version should not be empty", version.isNotEmpty()) | |
| assertTrue("Version should not contain whitespace", version.trim() == version) | |
| // Version should match semantic versioning pattern (X.Y.Z) or be a valid identifier | |
| val semverPattern = Regex("^\\d+\\.\\d+\\.\\d+.*$") | |
| assertTrue( | |
| "Version should follow semantic versioning or be a valid identifier: $version", | |
| semverPattern.matches(version) || version.matches(Regex("^[a-zA-Z0-9._-]+$")) | |
| ) | |
| } | |
| } | |
| @Test | |
| fun testVersionFormatIsValid() { | |
| // Given - Create a client with the user agent interceptor | |
| val interceptor = FlagsmithRetrofitService.userAgentInterceptor(null) | |
| val client = OkHttpClient.Builder() | |
| .addInterceptor(interceptor) | |
| .build() | |
| mockServer.enqueue(MockResponse().setResponseCode(200).setBody("{}")) | |
| // When - Make a request | |
| val request = Request.Builder() | |
| .url(mockServer.url("/")) | |
| .build() | |
| client.newCall(request).execute().use { response -> | |
| // Then - Verify version format is semantic versioning compatible | |
| val recordedRequest = mockServer.takeRequest() | |
| val userAgent = recordedRequest.getHeader("User-Agent")!! | |
| val version = userAgent.substringAfter("$USER_AGENT_PREFIX/") | |
| assertTrue("Version should not be empty", version.isNotEmpty()) | |
| assertTrue( | |
| "Version should not contain whitespace", | |
| !version.any { it.isWhitespace() } | |
| ) | |
| // Version should match semantic versioning pattern (X.Y.Z) or be a valid identifier | |
| val semverPattern = Regex("^\\d+\\.\\d+\\.\\d+.*$") | |
| assertTrue( | |
| "Version should follow semantic versioning or be a valid identifier: $version", | |
| semverPattern.matches(version) || version.matches(Regex("^[a-zA-Z0-9._-]+$")) | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
In
FlagsmithClient/src/test/java/com/flagsmith/internal/SdkVersionRetrievalTest.kt
around lines 71 to 102, the whitespace assertion currently only checks
leading/trailing spaces but the message says "should not contain whitespace";
update the assertion to reject any whitespace character within the version
(e.g., assertTrue("Version should not contain whitespace", !version.any {
it.isWhitespace() } ) or equivalent regex check), and ensure the assertion
message remains accurate.
2 participants
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