From b37f1d78cdbf949dc4418d07b2d653a58af9418f Mon Sep 17 00:00:00 2001 From: Gareth Reese <8297652+gazreese@users.noreply.github.com> Date: Mon, 19 Feb 2024 10:24:33 +0200 Subject: [PATCH] Feat(typing)/allow non string trait types (#43) * Update the tests to reproduce the initialisation error * Fix the initialisation error * Remove TODO comment * Allow non-string trait values * Allow non-string trait types * Basic tests * Make Trait's value to be any type and add getters * Update Trait model * Add more variations of mocked responses * Update TraitEntity tests * Add getters to TraitWithIdentity * Pass traitValue to TraitWithIdentity in setTrait() method * Add tests to different Trait value's type when setting Trait * Use traitValue in TraitsEndpoint * Update value deprecation message * Remove unnecessary constructors from Trait model * Use stringValue instead of value in tests * Add constructors that accept strict-typed value * Check the value type to be one of supported in Trait's init * Update to make the convenience constructors more explicit and also verify the original behaviour * Remove unneeded constructor keyword * Make sure we're using the old 'value' parameter * Updated the TraitWithIdentity in-line with the Trait class and added unit tests --------- Co-authored-by: Matthew Elwell Co-authored-by: Vitaly Zeenko --- .../src/main/java/com/flagsmith/Flagsmith.kt | 2 +- .../main/java/com/flagsmith/entities/Trait.kt | 84 ++++++- .../test/java/com/flagsmith/TraitsTests.kt | 49 +++- .../flagsmith/entities/TraitEntityTests.kt | 142 +++++++++++ .../flagsmith/mockResponses/MockResponses.kt | 232 +++++++++++++++++- .../mockResponses/endpoints/TraitsEndpoint.kt | 2 +- 6 files changed, 495 insertions(+), 16 deletions(-) create mode 100644 FlagsmithClient/src/test/java/com/flagsmith/entities/TraitEntityTests.kt diff --git a/FlagsmithClient/src/main/java/com/flagsmith/Flagsmith.kt b/FlagsmithClient/src/main/java/com/flagsmith/Flagsmith.kt index 603ae51..bc678e4 100644 --- a/FlagsmithClient/src/main/java/com/flagsmith/Flagsmith.kt +++ b/FlagsmithClient/src/main/java/com/flagsmith/Flagsmith.kt @@ -148,7 +148,7 @@ class Flagsmith constructor( }.also { lastUsedIdentity = identity } fun setTrait(trait: Trait, identity: String, result: (Result) -> Unit) = - retrofit.postTraits(TraitWithIdentity(trait.key, trait.value, Identity(identity))).enqueueWithResult(result = result) + retrofit.postTraits(TraitWithIdentity(trait.key, trait.traitValue, Identity(identity))).enqueueWithResult(result = result) fun getIdentity(identity: String, result: (Result) -> Unit) = retrofit.getIdentityFlagsAndTraits(identity).enqueueWithResult(defaults = null, result = result) diff --git a/FlagsmithClient/src/main/java/com/flagsmith/entities/Trait.kt b/FlagsmithClient/src/main/java/com/flagsmith/entities/Trait.kt index d73aafa..f03c2c8 100644 --- a/FlagsmithClient/src/main/java/com/flagsmith/entities/Trait.kt +++ b/FlagsmithClient/src/main/java/com/flagsmith/entities/Trait.kt @@ -3,14 +3,84 @@ package com.flagsmith.entities import com.google.gson.annotations.SerializedName -data class Trait( +data class Trait ( val identifier: String? = null, @SerializedName(value = "trait_key") val key: String, - @SerializedName(value = "trait_value") val value: String -) + @SerializedName(value = "trait_value") val traitValue: Any +) { -data class TraitWithIdentity( + constructor(key: String, value: String) + : this(key = key, traitValue = value) + + constructor(key: String, value: Int) + : this(key = key, traitValue = value) + + constructor(key: String, value: Double) + : this(key = key, traitValue = value) + + constructor(key: String, value: Boolean) + : this(key = key, traitValue = value) + + @Deprecated("Use traitValue instead or one of the type-safe getters", ReplaceWith("traitValue")) + val value: String + get() { return traitValue as? String ?: traitValue.toString() } + + val stringValue: String? + get() = traitValue as? String + + val intValue: Int? + get() { + return when (traitValue) { + is Int -> traitValue + is Double -> traitValue.toInt() + else -> null + } + } + + val doubleValue: Double? + get() = traitValue as? Double + + val booleanValue: Boolean? + get() = traitValue as? Boolean + +} + +data class TraitWithIdentity ( @SerializedName(value = "trait_key") val key: String, - @SerializedName(value = "trait_value") val value: String, - val identity: Identity -) + @SerializedName(value = "trait_value") val traitValue: Any, + val identity: Identity, +) { + constructor(key: String, value: String, identity: Identity) + : this(key = key, traitValue = value, identity = identity) + + constructor(key: String, value: Int, identity: Identity) + : this(key = key, traitValue = value, identity = identity) + + constructor(key: String, value: Double, identity: Identity) + : this(key = key, traitValue = value, identity = identity) + + constructor(key: String, value: Boolean, identity: Identity) + : this(key = key, traitValue = value, identity = identity) + + @Deprecated("Use traitValue instead or one of the type-safe getters", ReplaceWith("traitValue")) + val value: String + get() { return traitValue as? String ?: traitValue.toString() } + + val stringValue: String? + get() = traitValue as? String + + val intValue: Int? + get() { + return when (traitValue) { + is Int -> traitValue + is Double -> traitValue.toInt() + else -> null + } + } + + val doubleValue: Double? + get() = traitValue as? Double + + val booleanValue: Boolean? + get() = traitValue as? Boolean +} diff --git a/FlagsmithClient/src/test/java/com/flagsmith/TraitsTests.kt b/FlagsmithClient/src/test/java/com/flagsmith/TraitsTests.kt index d586c85..fdd347d 100644 --- a/FlagsmithClient/src/test/java/com/flagsmith/TraitsTests.kt +++ b/FlagsmithClient/src/test/java/com/flagsmith/TraitsTests.kt @@ -42,7 +42,7 @@ class TraitsTests { assertTrue(result.getOrThrow().isNotEmpty()) assertEquals( "electric pink", - result.getOrThrow().find { trait -> trait.key == "favourite-colour" }?.value + result.getOrThrow().find { trait -> trait.key == "favourite-colour" }?.stringValue ) } } @@ -54,7 +54,7 @@ class TraitsTests { val result = flagsmith.getTraitsSync("person") assertTrue(result.isSuccess) assertTrue(result.getOrThrow().isNotEmpty()) - assertNull(result.getOrThrow().find { trait -> trait.key == "fake-trait" }?.value) + assertNull(result.getOrThrow().find { trait -> trait.key == "fake-trait" }?.stringValue) } } @@ -64,7 +64,7 @@ class TraitsTests { runBlocking { val result = flagsmith.getTraitSync("favourite-colour", "person") assertTrue(result.isSuccess) - assertEquals("electric pink", result.getOrThrow()?.value) + assertEquals("electric pink", result.getOrThrow()?.stringValue) } } @@ -86,7 +86,46 @@ class TraitsTests { flagsmith.setTraitSync(Trait(key = "set-from-client", value = "12345"), "person") assertTrue(result.isSuccess) assertEquals("set-from-client", result.getOrThrow().key) - assertEquals("12345", result.getOrThrow().value) + assertEquals("12345", result.getOrThrow().stringValue) + assertEquals("person", result.getOrThrow().identity.identifier) + } + } + + @Test + fun testSetTraitInteger() { + mockServer.mockResponseFor(MockEndpoint.SET_TRAIT_INTEGER) + runBlocking { + val result = + flagsmith.setTraitSync(Trait(key = "set-from-client", value = 5), "person") + assertTrue(result.isSuccess) + assertEquals("set-from-client", result.getOrThrow().key) + assertEquals(5, result.getOrThrow().intValue) + assertEquals("person", result.getOrThrow().identity.identifier) + } + } + + @Test + fun testSetTraitDouble() { + mockServer.mockResponseFor(MockEndpoint.SET_TRAIT_DOUBLE) + runBlocking { + val result = + flagsmith.setTraitSync(Trait(key = "set-from-client", value = 0.5), "person") + assertTrue(result.isSuccess) + assertEquals("set-from-client", result.getOrThrow().key) + assertEquals(0.5, result.getOrThrow().doubleValue) + assertEquals("person", result.getOrThrow().identity.identifier) + } + } + + @Test + fun testSetTraitBoolean() { + mockServer.mockResponseFor(MockEndpoint.SET_TRAIT_BOOLEAN) + runBlocking { + val result = + flagsmith.setTraitSync(Trait(key = "set-from-client", value = true), "person") + assertTrue(result.isSuccess) + assertEquals("set-from-client", result.getOrThrow().key) + assertEquals(true, result.getOrThrow().booleanValue) assertEquals("person", result.getOrThrow().identity.identifier) } } @@ -101,7 +140,7 @@ class TraitsTests { assertTrue(result.getOrThrow().flags.isNotEmpty()) assertEquals( "electric pink", - result.getOrThrow().traits.find { trait -> trait.key == "favourite-colour" }?.value + result.getOrThrow().traits.find { trait -> trait.key == "favourite-colour" }?.stringValue ) } } diff --git a/FlagsmithClient/src/test/java/com/flagsmith/entities/TraitEntityTests.kt b/FlagsmithClient/src/test/java/com/flagsmith/entities/TraitEntityTests.kt new file mode 100644 index 0000000..742b17a --- /dev/null +++ b/FlagsmithClient/src/test/java/com/flagsmith/entities/TraitEntityTests.kt @@ -0,0 +1,142 @@ +package com.flagsmith.entities + +import com.flagsmith.Flagsmith +import com.flagsmith.FlagsmithCacheConfig +import com.flagsmith.getTraitSync +import com.flagsmith.mockResponses.MockEndpoint +import com.flagsmith.mockResponses.mockResponseFor +import kotlinx.coroutines.runBlocking +import org.junit.After +import org.junit.Assert +import org.junit.Before +import org.junit.Test +import org.mockserver.integration.ClientAndServer + +class TraitEntityTests { + + private lateinit var mockServer: ClientAndServer + private lateinit var flagsmith: Flagsmith + + @Before + fun setup() { + mockServer = ClientAndServer.startClientAndServer() + flagsmith = Flagsmith( + environmentKey = "", + baseUrl = "http://localhost:${mockServer.localPort}", + enableAnalytics = false, + cacheConfig = FlagsmithCacheConfig(enableCache = false) + ) + } + + @After + fun tearDown() { + mockServer.stop() + } + + @Test + fun testTraitValueStringType() { + mockServer.mockResponseFor(MockEndpoint.GET_IDENTITIES_TRAIT_STRING) + runBlocking { + val result = flagsmith.getTraitSync("client-key", "person") + Assert.assertTrue(result.isSuccess) + Assert.assertEquals("12345", result.getOrThrow()?.stringValue) + } + } + + @Test + fun testTraitValueIntType() { + mockServer.mockResponseFor(MockEndpoint.GET_IDENTITIES_TRAIT_INTEGER) + runBlocking { + val result = flagsmith.getTraitSync("client-key", "person") + Assert.assertTrue(result.isSuccess) + Assert.assertEquals(5, result.getOrThrow()?.intValue) + Assert.assertTrue("Integers in the JSON actually get decoded as Double", + (result.getOrThrow()?.traitValue) is Double) + } + } + + @Test + fun testTraitValueDoubleType() { + mockServer.mockResponseFor(MockEndpoint.GET_IDENTITIES_TRAIT_DOUBLE) + runBlocking { + val result = flagsmith.getTraitSync("client-key", "person") + Assert.assertTrue(result.isSuccess) + Assert.assertEquals(0.5, result.getOrThrow()?.doubleValue) + } + } + + @Test + fun testTraitValueBooleanType() { + mockServer.mockResponseFor(MockEndpoint.GET_IDENTITIES_TRAIT_BOOLEAN) + runBlocking { + val result = flagsmith.getTraitSync("client-key", "person") + Assert.assertTrue(result.isSuccess) + Assert.assertEquals(true, result.getOrThrow()?.booleanValue) + } + } + + @Test + fun testTraitConstructorStringType() { + val trait = Trait( "string-key", "string-value") + Assert.assertEquals("string-value", trait.traitValue) + Assert.assertEquals("string-value", trait.stringValue) + Assert.assertNull(trait.intValue) + + val traitWithIdentity = TraitWithIdentity("string-key", "string-value", Identity("person")) + Assert.assertEquals("string-value", traitWithIdentity.traitValue) + Assert.assertEquals("string-value", traitWithIdentity.stringValue) + Assert.assertNull(traitWithIdentity.intValue) + } + + @Test + fun testTraitConstructorIntType() { + val trait = Trait("string-key", 1) + Assert.assertEquals(1, trait.traitValue) + Assert.assertEquals(1, trait.intValue) + Assert.assertNull("Can't convert an int to a double", trait.doubleValue) + Assert.assertNull(trait.stringValue) + Assert.assertEquals("We should maintain the original functionality for the String .value", + "1", trait.value) + + val traitWithIdentity = TraitWithIdentity("string-key", 1, Identity("person")) + Assert.assertEquals(1, traitWithIdentity.traitValue) + Assert.assertEquals(1, traitWithIdentity.intValue) + Assert.assertNull("Can't convert an int to a double", traitWithIdentity.doubleValue) + Assert.assertNull(traitWithIdentity.stringValue) + Assert.assertEquals("We should maintain the original functionality for the String .value", + "1", traitWithIdentity.value) + } + + @Test + fun testTraitConstructorDoubleType() { + val trait = Trait("string-key", 1.0) + Assert.assertEquals(1.0, trait.traitValue) + Assert.assertEquals(1.0, trait.doubleValue) + Assert.assertEquals("JS ints are actually doubles so we should handle this", + 1, trait.intValue) + Assert.assertNull(trait.stringValue) + Assert.assertEquals("We should maintain the original functionality for the String .value", + "1.0", trait.value) + + val traitWithIdentity = TraitWithIdentity("string-key", 1.0, Identity("person")) + Assert.assertEquals(1.0, traitWithIdentity.traitValue) + Assert.assertEquals(1.0, traitWithIdentity.doubleValue) + Assert.assertEquals("JS ints are actually doubles so we should handle this", + 1, traitWithIdentity.intValue) + Assert.assertNull(traitWithIdentity.stringValue) + Assert.assertEquals("We should maintain the original functionality for the String .value", + "1.0", traitWithIdentity.value) + } + + @Test + fun testTraitConstructorBooleanType() { + val trait = Trait("string-key", true) + Assert.assertEquals(true, trait.traitValue) + Assert.assertEquals(true, trait.booleanValue) + Assert.assertNull(trait.intValue) + Assert.assertNull(trait.doubleValue) + Assert.assertNull(trait.stringValue) + Assert.assertEquals("We should maintain the original functionality for the String .value", + "true", trait.value) + } +} \ No newline at end of file diff --git a/FlagsmithClient/src/test/java/com/flagsmith/mockResponses/MockResponses.kt b/FlagsmithClient/src/test/java/com/flagsmith/mockResponses/MockResponses.kt index 8646871..3db4529 100644 --- a/FlagsmithClient/src/test/java/com/flagsmith/mockResponses/MockResponses.kt +++ b/FlagsmithClient/src/test/java/com/flagsmith/mockResponses/MockResponses.kt @@ -15,7 +15,26 @@ import java.util.concurrent.TimeUnit enum class MockEndpoint(val path: String, val body: String) { GET_IDENTITIES(IdentityFlagsAndTraitsEndpoint("").path, MockResponses.getIdentities), GET_FLAGS(FlagsEndpoint.path, MockResponses.getFlags), - SET_TRAIT(TraitsEndpoint(Trait(key = "", value = ""), "").path, MockResponses.setTrait) + SET_TRAIT(TraitsEndpoint(Trait(key = "", traitValue = ""), "").path, MockResponses.setTrait), + SET_TRAIT_INTEGER(TraitsEndpoint(Trait(key = "", traitValue = ""), "").path, MockResponses.setTraitInteger), + SET_TRAIT_DOUBLE(TraitsEndpoint(Trait(key = "", traitValue = ""), "").path, MockResponses.setTraitDouble), + SET_TRAIT_BOOLEAN(TraitsEndpoint(Trait(key = "", traitValue = ""), "").path, MockResponses.setTraitBoolean), + GET_IDENTITIES_TRAIT_STRING( + IdentityFlagsAndTraitsEndpoint("").path, + MockResponses.getTraitString + ), + GET_IDENTITIES_TRAIT_INTEGER( + IdentityFlagsAndTraitsEndpoint("").path, + MockResponses.getTraitInteger + ), + GET_IDENTITIES_TRAIT_DOUBLE( + IdentityFlagsAndTraitsEndpoint("").path, + MockResponses.getTraitDouble + ), + GET_IDENTITIES_TRAIT_BOOLEAN( + IdentityFlagsAndTraitsEndpoint("").path, + MockResponses.getTraitBoolean + ), } fun ClientAndServer.mockResponseFor(endpoint: MockEndpoint) { @@ -33,7 +52,10 @@ fun ClientAndServer.mockDelayFor(endpoint: MockEndpoint) { response() .withContentType(MediaType.APPLICATION_JSON) .withBody(endpoint.body) - .withDelay(TimeUnit.SECONDS, 8) // REQUEST_TIMEOUT_SECONDS is 4 in the client, so needs to be more + .withDelay( + TimeUnit.SECONDS, + 8 + ) // REQUEST_TIMEOUT_SECONDS is 4 in the client, so needs to be more ) } @@ -143,4 +165,210 @@ object MockResponses { } } """.trimIndent() + + val setTraitInteger = """ + { + "trait_key": "set-from-client", + "trait_value": 5, + "identity": { + "identifier": "person" + } + } + """.trimIndent() + + val setTraitDouble = """ + { + "trait_key": "set-from-client", + "trait_value": 0.5, + "identity": { + "identifier": "person" + } + } + """.trimIndent() + + val setTraitBoolean = """ + { + "trait_key": "set-from-client", + "trait_value": true, + "identity": { + "identifier": "person" + } + } + """.trimIndent() + + val getTraitString = """ + { + "flags": [ + { + "feature_state_value": null, + "feature": { + "type": "STANDARD", + "name": "no-value", + "id": 35506 + }, + "enabled": true + }, + { + "feature_state_value": 756, + "feature": { + "type": "STANDARD", + "name": "with-value", + "id": 35507 + }, + "enabled": true + }, + { + "feature_state_value": "", + "feature": { + "type": "STANDARD", + "name": "with-value-just-person-enabled", + "id": 35508 + }, + "enabled": true + } + ], + "traits": [ + { + "trait_value": "12345", + "trait_key": "client-key" + }, + { + "trait_value": "electric pink", + "trait_key": "favourite-colour" + } + ] + } + """.trimIndent() + + val getTraitInteger = """ + { + "flags": [ + { + "feature_state_value": null, + "feature": { + "type": "STANDARD", + "name": "no-value", + "id": 35506 + }, + "enabled": true + }, + { + "feature_state_value": 756, + "feature": { + "type": "STANDARD", + "name": "with-value", + "id": 35507 + }, + "enabled": true + }, + { + "feature_state_value": "", + "feature": { + "type": "STANDARD", + "name": "with-value-just-person-enabled", + "id": 35508 + }, + "enabled": true + } + ], + "traits": [ + { + "trait_value": 5, + "trait_key": "client-key" + }, + { + "trait_value": "electric pink", + "trait_key": "favourite-colour" + } + ] + } + """.trimIndent() + + val getTraitDouble = """ + { + "flags": [ + { + "feature_state_value": null, + "feature": { + "type": "STANDARD", + "name": "no-value", + "id": 35506 + }, + "enabled": true + }, + { + "feature_state_value": 756, + "feature": { + "type": "STANDARD", + "name": "with-value", + "id": 35507 + }, + "enabled": true + }, + { + "feature_state_value": "", + "feature": { + "type": "STANDARD", + "name": "with-value-just-person-enabled", + "id": 35508 + }, + "enabled": true + } + ], + "traits": [ + { + "trait_value": 0.5, + "trait_key": "client-key" + }, + { + "trait_value": "electric pink", + "trait_key": "favourite-colour" + } + ] + } + """.trimIndent() + + val getTraitBoolean = """ + { + "flags": [ + { + "feature_state_value": null, + "feature": { + "type": "STANDARD", + "name": "no-value", + "id": 35506 + }, + "enabled": true + }, + { + "feature_state_value": 756, + "feature": { + "type": "STANDARD", + "name": "with-value", + "id": 35507 + }, + "enabled": true + }, + { + "feature_state_value": "", + "feature": { + "type": "STANDARD", + "name": "with-value-just-person-enabled", + "id": 35508 + }, + "enabled": true + } + ], + "traits": [ + { + "trait_value": true, + "trait_key": "client-key" + }, + { + "trait_value": "electric pink", + "trait_key": "favourite-colour" + } + ] + } + """.trimIndent() } \ No newline at end of file diff --git a/FlagsmithClient/src/test/java/com/flagsmith/mockResponses/endpoints/TraitsEndpoint.kt b/FlagsmithClient/src/test/java/com/flagsmith/mockResponses/endpoints/TraitsEndpoint.kt index d68ea71..366d833 100644 --- a/FlagsmithClient/src/test/java/com/flagsmith/mockResponses/endpoints/TraitsEndpoint.kt +++ b/FlagsmithClient/src/test/java/com/flagsmith/mockResponses/endpoints/TraitsEndpoint.kt @@ -11,7 +11,7 @@ data class TraitsEndpoint(private val trait: Trait, private val identity: String body = Gson().toJson( TraitWithIdentity( key = trait.key, - value = trait.value, + traitValue = trait.traitValue, identity = Identity(identity) ) ),