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 <[email protected]>
Co-authored-by: Vitaly Zeenko <[email protected]>

Author: 3 people

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<TraitWithIdentity>) -> 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<IdentityFlagsAndTraits>) -> Unit) =
         retrofit.getIdentityFlagsAndTraits(identity).enqueueWithResult(defaults = null, result = result)

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
+}

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
             )
         }
     }

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)
+    }
+}