plaidev / karte-android-sdk

KARTE SDK for Android
https://karte.io/
Apache License 2.0
5 stars 3 forks source link

[core] Request for helper function to null filter Map type for `Event` constructor #9

Closed litmon closed 2 years ago

litmon commented 3 years ago

Is your feature request related to a problem? Please describe.

Hi, I want to change type of values argument to Map<String, Any?> in Event constructor. Because its behavior is different from initialization with JSONObject. https://github.com/plaidev/karte-android-sdk/blob/18e176a019abf23a3166c47796e05e26bbf84cc5/core/src/main/java/io/karte/android/tracking/Event.kt#L74-L83

↑In this case, the null-value is filtered through jsonObject.format() method.

https://github.com/plaidev/karte-android-sdk/blob/18e176a019abf23a3166c47796e05e26bbf84cc5/core/src/main/java/io/karte/android/tracking/Event.kt#L85-L90

↑ But this case is prohibited null-value at the compile time because the type Values is same as Map<String, Any>. https://github.com/plaidev/karte-android-sdk/blob/18e176a019abf23a3166c47796e05e26bbf84cc5/core/src/main/java/io/karte/android/tracking/Event.kt#L23

My usecase sample is below code.

import io.karte.android.tracking.Event
import io.karte.android.tracking.EventName

class SampleEvent(
    id: Long,
    name: String?,
) : Event(
    eventName = object : EventName {
        override val value: String
            get() = "sample"
    },
    values = mapOf(
        "id" to id,
        "name" to name, // <- compile error
    )
)

Describe the solution you'd like Please describe the desired behavior.

I will suggest this. https://github.com/plaidev/karte-android-sdk/blob/18e176a019abf23a3166c47796e05e26bbf84cc5/core/src/main/java/io/karte/android/tracking/Event.kt#L85-L90

    /** [Values] による初期化 */
-    constructor(eventName: EventName, values: Values? = null, isRetryable: Boolean? = null) : this(
+    constructor(eventName: EventName, values: Map<String, Any?>? = null, isRetryable: Boolean? = null) : this( 
        eventName,
        values?.let { JSONObject(values.format()) },
        isRetryable
    )

Describe alternatives you've considered Please describe alternative solutions or features you have considered.

tikidunpon commented 3 years ago

Thank you for your feature request! I will consider it and comment later.

wasnot commented 3 years ago

@litmon Thank you for your valuable feedback.

First, in JSON in values ​​accepted by KARTE, the behavior of null values ​​is undefined and ignored, so sending is not recommended.

Therefore, as you pointed out, SDK filters null values ​​in JSON as unacceptable. Currently, the Values ​​type is NonNull in order to prevent the confusingness of implicit filtering by the KARTE side.

As you said, org.json.JSONObject allows Nullable, but internally treats them as non-nullable. This is because the JSONObject itself cannot be nullable. In addition, the API that accepts JSONObject is maintained and published for the purpose of facilitating migration with the API of SDKs less than v2.

Actually, we discussed this type several times when designing the API. We decided on the current format to clarify the consistency and intent of the internal processing and the public interface.

In your use case, we think you should consider how you want to handle values ​​that can be null on KARTE.

Thank you for suggesting an alternative. It cannot be implemented as it is to maintain API compatibility. Instead, we would like to consider publishing a helper function that can be easily converted by filtering nullable types.

Internal helper function example:

https://github.com/plaidev/karte-android-sdk/blob/18e176a019abf23a3166c47796e05e26bbf84cc5/core/src/main/java/io/karte/android/utilities/Extensions.kt#L51-L54

If you have use cases that you can't solve, or better solutions, please continue to comment.

litmon commented 3 years ago

We decided on the current format to clarify the consistency and intent of the internal processing and the public interface. Instead, we would like to consider publishing a helper function that can be easily converted by filtering nullable types.

I understand. I will hope to provide helper function!