-
Notifications
You must be signed in to change notification settings - Fork 17
feat: send user agent header #77
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
Open
gazreese
wants to merge
7
commits into
Flagsmith:main
Choose a base branch
from
foresightmobile:feature/send-user-agent-header
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
5cea695
implementation and testing
polatolu b184964
Test improvements
polatolu e7b671f
Review updates
polatolu 97643e4
Add hardcoded release please version like the iOS client
gazreese 6416cb7
Updated the tests to add more explicit unit testing and testing aroun…
gazreese 2d6527f
Now just use the release-please version and tidy up the tests
gazreese c1c4edc
No longer need to use the Context
gazreese File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
188 changes: 188 additions & 0 deletions
188
FlagsmithClient/src/test/java/com/flagsmith/UserAgentTests.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,188 @@ | ||
| package com.flagsmith | ||
|
|
||
| import com.flagsmith.entities.Trait | ||
| import com.flagsmith.mockResponses.MockEndpoint | ||
| import com.flagsmith.mockResponses.mockResponseFor | ||
| import kotlinx.coroutines.runBlocking | ||
| import org.junit.After | ||
| import org.junit.Assert.assertEquals | ||
| import org.junit.Assert.assertTrue | ||
| import org.junit.Before | ||
| import org.junit.Test | ||
| import org.mockserver.integration.ClientAndServer | ||
| import org.mockserver.model.HttpRequest.request | ||
|
|
||
| class UserAgentTests { | ||
|
|
||
| private lateinit var mockServer: ClientAndServer | ||
| private lateinit var flagsmith: Flagsmith | ||
|
|
||
| companion object { | ||
| // Expected version set by release-please in FlagsmithRetrofitService.getSdkVersion() | ||
| // x-release-please-start-version | ||
| private const val EXPECTED_SDK_VERSION = "1.8.0" | ||
| // x-release-please-end | ||
| private const val EXPECTED_USER_AGENT = "flagsmith-kotlin-android-sdk/$EXPECTED_SDK_VERSION" | ||
| } | ||
|
|
||
| @Before | ||
| fun setup() { | ||
| mockServer = ClientAndServer.startClientAndServer() | ||
| } | ||
|
|
||
| @After | ||
| fun tearDown() { | ||
| mockServer.stop() | ||
| } | ||
|
|
||
| @Test | ||
| fun testUserAgentHeaderSentWithGetFlags() { | ||
| // Given - SDK version is set by release-please | ||
| flagsmith = Flagsmith( | ||
| environmentKey = "test-key", | ||
| baseUrl = "http://localhost:${mockServer.localPort}", | ||
| context = null, | ||
| enableAnalytics = false, | ||
| cacheConfig = FlagsmithCacheConfig(enableCache = false) | ||
| ) | ||
|
|
||
| mockServer.mockResponseFor(MockEndpoint.GET_FLAGS) | ||
|
|
||
| // When | ||
| runBlocking { | ||
| val result = flagsmith.getFeatureFlagsSync() | ||
| assertTrue(result.isSuccess) | ||
| } | ||
|
|
||
| // Then - Verify User-Agent contains the SDK version from release-please | ||
| mockServer.verify( | ||
| request() | ||
| .withPath("/flags/") | ||
| .withMethod("GET") | ||
| .withHeader("User-Agent", EXPECTED_USER_AGENT) | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun testUserAgentHeaderSentWithNullContext() { | ||
| // Given - Context being null doesn't affect SDK version retrieval | ||
| flagsmith = Flagsmith( | ||
| environmentKey = "test-key", | ||
| baseUrl = "http://localhost:${mockServer.localPort}", | ||
| context = null, | ||
| enableAnalytics = false, | ||
| cacheConfig = FlagsmithCacheConfig(enableCache = false) | ||
| ) | ||
|
|
||
| mockServer.mockResponseFor(MockEndpoint.GET_FLAGS) | ||
|
|
||
| // When | ||
| runBlocking { | ||
| val result = flagsmith.getFeatureFlagsSync() | ||
| assertTrue(result.isSuccess) | ||
| } | ||
|
|
||
| // Then - Should get the SDK version from release-please | ||
| mockServer.verify( | ||
| request() | ||
| .withPath("/flags/") | ||
| .withMethod("GET") | ||
| .withHeader("User-Agent", EXPECTED_USER_AGENT) | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun testUserAgentHeaderSentWithIdentityRequest() { | ||
| // Given - Testing that User-Agent header is sent consistently across all API endpoints | ||
| flagsmith = Flagsmith( | ||
| environmentKey = "test-key", | ||
| baseUrl = "http://localhost:${mockServer.localPort}", | ||
| context = null, | ||
| enableAnalytics = false, | ||
| cacheConfig = FlagsmithCacheConfig(enableCache = false) | ||
| ) | ||
|
|
||
| mockServer.mockResponseFor(MockEndpoint.GET_IDENTITIES) | ||
|
|
||
| // When | ||
| runBlocking { | ||
| val result = flagsmith.getIdentitySync("test-user") | ||
| assertTrue(result.isSuccess) | ||
| } | ||
|
|
||
| // Then - Verify User-Agent header is sent with GET /identities/ | ||
| mockServer.verify( | ||
| request() | ||
| .withPath("/identities/") | ||
| .withMethod("GET") | ||
| .withQueryStringParameter("identifier", "test-user") | ||
| .withHeader("User-Agent", EXPECTED_USER_AGENT) | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun testUserAgentHeaderSentWithTraitRequest() { | ||
| // Given - Testing that User-Agent header is sent with POST requests | ||
| flagsmith = Flagsmith( | ||
| environmentKey = "test-key", | ||
| baseUrl = "http://localhost:${mockServer.localPort}", | ||
| context = null, | ||
| enableAnalytics = false, | ||
| cacheConfig = FlagsmithCacheConfig(enableCache = false) | ||
| ) | ||
|
|
||
| mockServer.mockResponseFor(MockEndpoint.SET_TRAIT) | ||
|
|
||
| // When | ||
| runBlocking { | ||
| val result = flagsmith.setTraitSync(Trait(key = "test-key", traitValue = "test-value"), "test-user") | ||
| assertTrue(result.isSuccess) | ||
| } | ||
|
|
||
| // Then - Verify User-Agent header is sent with POST /identities/ | ||
| mockServer.verify( | ||
| request() | ||
| .withPath("/identities/") | ||
| .withMethod("POST") | ||
| .withHeader("User-Agent", EXPECTED_USER_AGENT) | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun testUserAgentFormat() { | ||
| // Given | ||
| flagsmith = Flagsmith( | ||
| environmentKey = "test-key", | ||
| baseUrl = "http://localhost:${mockServer.localPort}", | ||
| context = null, | ||
| enableAnalytics = false, | ||
| cacheConfig = FlagsmithCacheConfig(enableCache = false) | ||
| ) | ||
|
|
||
| mockServer.mockResponseFor(MockEndpoint.GET_FLAGS) | ||
|
|
||
| // When | ||
| runBlocking { | ||
| flagsmith.getFeatureFlagsSync() | ||
| } | ||
|
|
||
| // Then - Verify User-Agent follows the format: flagsmith-kotlin-android-sdk/{version} | ||
| val requests = mockServer.retrieveRecordedRequests( | ||
| request().withPath("/flags/") | ||
| ) | ||
|
|
||
| assertEquals(1, requests.size) | ||
| val userAgentHeader = requests[0].getFirstHeader("User-Agent") | ||
|
|
||
| // Verify format | ||
| assertTrue("User-Agent should start with 'flagsmith-kotlin-android-sdk/'", | ||
| userAgentHeader.startsWith("flagsmith-kotlin-android-sdk/")) | ||
|
|
||
| // Verify version part exists and is not empty | ||
| val version = userAgentHeader.substringAfter("flagsmith-kotlin-android-sdk/") | ||
| assertTrue("Version should not be empty", version.isNotEmpty()) | ||
|
|
||
| // Should be the version set by release-please | ||
| assertEquals(EXPECTED_SDK_VERSION, version) | ||
| } | ||
| } |
141 changes: 141 additions & 0 deletions
141
FlagsmithClient/src/test/java/com/flagsmith/internal/SdkVersionRetrievalTest.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,141 @@ | ||
| package com.flagsmith.internal | ||
|
|
||
| import com.flagsmith.FlagsmithCacheConfig | ||
| import okhttp3.OkHttpClient | ||
| import okhttp3.Request | ||
| import okhttp3.mockwebserver.MockResponse | ||
| import okhttp3.mockwebserver.MockWebServer | ||
| import org.junit.After | ||
| import org.junit.Assert.assertEquals | ||
| import org.junit.Assert.assertNotNull | ||
| import org.junit.Assert.assertTrue | ||
| import org.junit.Before | ||
| import org.junit.Test | ||
|
|
||
| /** | ||
| * Unit tests for SDK version retrieval functionality in FlagsmithRetrofitService. | ||
| * | ||
| * These tests verify that getSdkVersion() correctly returns the version set by release-please. | ||
| */ | ||
| class SdkVersionRetrievalTest { | ||
|
|
||
| private lateinit var mockServer: MockWebServer | ||
|
|
||
| companion object { | ||
| // This should match the version in getSdkVersion() and in .release-please-manifest.json | ||
| // x-release-please-start-version | ||
| private const val EXPECTED_SDK_VERSION = "1.8.0" | ||
| // x-release-please-end | ||
| private const val USER_AGENT_PREFIX = "flagsmith-kotlin-android-sdk" | ||
| } | ||
|
|
||
| @Before | ||
| fun setup() { | ||
| mockServer = MockWebServer() | ||
| mockServer.start() | ||
| } | ||
|
|
||
| @After | ||
| fun tearDown() { | ||
| mockServer.shutdown() | ||
| } | ||
|
|
||
| @Test | ||
| fun testUserAgentInterceptorReturnsValidFormat() { | ||
| // Given - Create a client with the user agent interceptor | ||
| val interceptor = FlagsmithRetrofitService.userAgentInterceptor() | ||
| 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 the request was made with the correct User-Agent header | ||
| val recordedRequest = mockServer.takeRequest() | ||
| val userAgent = recordedRequest.getHeader("User-Agent") | ||
|
|
||
| assertNotNull("User-Agent header should be present", userAgent) | ||
| assertTrue( | ||
| "User-Agent should start with correct prefix: $userAgent", | ||
| userAgent!!.startsWith("$USER_AGENT_PREFIX/") | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| fun testVersionFormatIsValid() { | ||
| // Given - Create a client with the user agent interceptor | ||
| val interceptor = FlagsmithRetrofitService.userAgentInterceptor() | ||
| 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._-]+$")) | ||
| ) | ||
| } | ||
| } | ||
|
Comment on lines
+43
to
+102
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. IMO these two tests aren't necessary given |
||
|
|
||
| @Test | ||
| fun testUserAgentHeaderIsPersistentAcrossRequests() { | ||
| // Given - Create a client with the user agent interceptor | ||
| val interceptor = FlagsmithRetrofitService.userAgentInterceptor() | ||
| val client = OkHttpClient.Builder() | ||
| .addInterceptor(interceptor) | ||
| .build() | ||
|
|
||
| mockServer.enqueue(MockResponse().setResponseCode(200).setBody("{}")) | ||
| mockServer.enqueue(MockResponse().setResponseCode(200).setBody("{}")) | ||
|
|
||
| // When - Make multiple requests | ||
| val request1 = Request.Builder().url(mockServer.url("/first")).build() | ||
| val request2 = Request.Builder().url(mockServer.url("/second")).build() | ||
|
|
||
| client.newCall(request1).execute().close() | ||
| client.newCall(request2).execute().close() | ||
|
|
||
| // Then - Both requests should have the same User-Agent | ||
| val recordedRequest1 = mockServer.takeRequest() | ||
| val recordedRequest2 = mockServer.takeRequest() | ||
|
|
||
| val userAgent1 = recordedRequest1.getHeader("User-Agent") | ||
| val userAgent2 = recordedRequest2.getHeader("User-Agent") | ||
|
|
||
| assertEquals( | ||
| "User-Agent should be consistent across requests", | ||
| userAgent1, | ||
| userAgent2 | ||
| ) | ||
|
|
||
| assertEquals( | ||
| "User-Agent should be the expected value", | ||
| "$USER_AGENT_PREFIX/$EXPECTED_SDK_VERSION", | ||
| userAgent1 | ||
| ) | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is it possible to somehow mock the version so it can be seen in at least one test? All current tests only verify
unknownis added in the header.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.
Hi @emyller I've tried both PowerMock and MockK and there doesn't seem to be a way that I can find that can sufficiently mock the Class.forName() functionality.
I could refactor getSdkVersion into an interface so that I can mock the dependency into the main Retrofit class but again it doesn't really test any extra code and adds more complexity.
I'm a lot happier with the code now that we're falling back to release-please as per the iOS SDK and the updated function looks pretty bullet proof and as tested as I can get it really.
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.
Please check out the updated implementation and tests @emyller and let me know what you think
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 think the fate of this thread will lie on this other thread.
On the good news, I believe this becomes much simpler. See example. Let me know if you have any questions.
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.
Should be sorted now @emyller please check, thanks
Uh oh!
There was an error while loading. Please reload this page.
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.
Thanks @gazreese. Sorry I didn't make it clear in my comment above. What I really meant by "much simpler" is that we may not need these tests at all. From my perspective,
testUserAgentHeaderIsPersistentAcrossRequestsalready checks the UA header is added in the API handler exactly as expected.As pointed out in the other thread, it may even suffice as the only test in this PR. The others feel more of the same, but I'd love to hear your thoughts here.