Tinder / Scarlet

A Retrofit inspired WebSocket client for Kotlin, Java, and Android
Other
3.25k stars 240 forks source link

Multiple @Receive annotated methods called for the same Message #37

Open bogdanzurac opened 6 years ago

bogdanzurac commented 6 years ago

I've got multiple methods annotated with @ Receive inside Scarlet's interface. The problem is that, for example, when a Route is sent on the socket from the server, all 4 observable callbacks are being called, instead of only the observeRoute() one. What can be causing this and why? I mean, I can get why the observeRoute() and observeOnConnectionEvent() callbacks are called; but why are the other 2 called which have nothing to do with the connection or the Route model?

 @Receive
    fun observeOnConnectionEvent(): Flowable<WebSocket.Event>

    @Receive
    fun observeAlerts(): Flowable<Alert>

    @Receive
    fun observeRoute(): Flowable<Route>

    @Receive
    fun observeVehicle(): Flowable<Vehicle>
zhxnlai commented 6 years ago

@bogdanzurac You are right. This should not have happened. What are the values passed to the other callbacks?

bogdanzurac commented 6 years ago

So when the server sends a route on the web socket, the Route model is mapped correctly with the JSON returned. The Alert and Vehicle models which are also returned by Scarlet have all their attributes set to the default initial values (which are nullable and usually set to null). Here's for example the Alert model:

data class Alert(
    @Expose @SerializedName("has_new_routes") var hasNewRoutes: Boolean = false,
    @Expose var message: String? = null)

// this consumer would be called even for a Route model returned, which doesn't contain neither has_new_routes, nor message inside the JSON
webSocketService.observeAlerts().subscribe { Log.d(TAG, "Received alert $it") }

// and this is the logcat
2018-10-30 08:12:05.075 D/LocationTrackingService: Received alert Alert(hasNewRoutes=false, message=null)
CaptnBlubber commented 6 years ago

Just ran into the same problem with Moshi Adapter

here are some exmaple Models:

data class User(
        val userId: Int,
        val name: String,
        val roles: List<Int>,
        val permanent: Boolean
) 

data class WelcomeMessage(val session: String?,
                          val version: Int,
                          val user: User?)

data class TriggeredAction(val status: String,
                           val handleId: String,
                           val actionId: Int,
                           val message: String,
                           val created: Long)

And this would be the service:

interface FooService {

    @Receive
    fun observeSocketEvents(): ReceiveChannel<WebSocket.Event>

    @Receive
    fun observeAuthSuccess(): ReceiveChannel<WelcomeMessage>

    @Receive
    fun observeActionTriggers(): ReceiveChannel<TriggeredAction>
}

Whenever the server sends me something both observe methods get triggered.

I reproduced this with versions 0.1.4-6

bogdanzurac commented 6 years ago

I've used a workaround for this by wrapping all 3 data classes that can be received from the server (Alert, Route, Vehicle) inside another data class (WebSocketMessage). This way I have a single observeWebSocketMessage() callback that is called, where I check if one of the 3 models within is not null. It's not as clean and nice as the original code, but it works. Would still like to know how and why the original solution doesn't work.

CaptnBlubber commented 5 years ago

@zhxnlai any updates on this?

sbearben commented 5 years ago

@zhxnlai I have a similar issue in that I have similar but different WebSocket events coming from my backend. A single incorrect or multiple incorrect @Receive streams are emitting in a random fashion (but almost always the incorrect one). I am using the OkHttp WebSocket protocol with Scarlet version 0.2.1-alpha4.

My four @Receive models are as follows:

data class AcceptedFriendRequestWsModel (
    @SerializedName("type") @Expose val type: String = "accepted_friend_request",
    @SerializedName("chat_uuid") @Expose var chatUuid: UUID,
    @SerializedName("acceptor_uuid") @Expose var acceptorUuid: UUID,
    @SerializedName("acceptor_email") @Expose var acceptorEmail: String,
    @SerializedName("acceptor_username") @Expose var acceptorUsername: String
)
data class CanceledFriendRequestWsModel (
    @SerializedName("type") @Expose val type: String = "canceled_friend_request",
    @SerializedName("canceler_uuid") @Expose var cancelerUuid: UUID,
    @SerializedName("canceler_email") @Expose var cancelerEmail: String,
    @SerializedName("canceler_username") @Expose var cancelerUsername: String
)
data class CreatedFriendRequestWsModel (
    @SerializedName("type") @Expose val type: String = "created_friend_request",
    @SerializedName("sender_uuid") @Expose var senderUuid: UUID,
    @SerializedName("sender_email") @Expose var senderEmail: String,
    @SerializedName("sender_username") @Expose var senderUsername: String,
    @SerializedName("date") @Expose var date: Date
)
data class RejectedFriendRequestWsModel (
    @SerializedName("type") @Expose val type: String = "rejected_friend_request",
    @SerializedName("rejector_uuid") @Expose var rejectorUuid: UUID,
    @SerializedName("rejector_email") @Expose var rejectorEmail: String,
    @SerializedName("rejector_username") @Expose var rejectorUsername: String
)
sbearben commented 5 years ago

I might be wrong but from looking at Scarlet and the GsonMessageAdapter, to me it looks possibly like the adapter is relying on an exception being thrown by the generated TypeAdapters in order to prevent incorrect incoming WebSocket messages from being pushed through the wrong stream?

From DeserializedValueStateTransitionAdapter:

internal class DeserializedValueStateTransitionAdapter(
    private val messageAdapter: MessageAdapter<Any>
) : StateTransitionAdapter<Any> {
    override fun adapt(stateTransition: StateTransition): Any? {
        val event = stateTransition.event as? Event.OnProtocolEvent ?: return null
        val protocolEvent = event.protocolEvent as? ProtocolEvent.OnMessageReceived ?: return null
        return try {
            messageAdapter.fromMessage(protocolEvent.message)
        } catch (throwable: Throwable) {
            null
        }
    }

For Gson, however, messageAdapter.fromMessage(protocolEvent.message) will rarely throw an exception and there is no type safety there.

From GsonMessageAdapter:

class GsonMessageAdapter<T> private constructor(
    private val gson: Gson,
    private val typeAdapter: TypeAdapter<T>
) : MessageAdapter<T> {

    override fun fromMessage(message: Message): T {
        val stringValue = when (message) {
            is Message.Text -> message.value
            is Message.Bytes -> String(message.value)
        }
        val jsonReader = gson.newJsonReader(StringReader(stringValue))
        return typeAdapter.read(jsonReader)!!
    }

So the serialization of a Json string into an object is done by the two lines at the end of fromMessage(message: Message). The TypeAdapter is created inside of GsonMessageAdapter.Factory:

class Factory(
        private val gson: Gson = DEFAULT_GSON
    ) : MessageAdapter.Factory {

        override fun create(type: Type, annotations: Array<Annotation>): MessageAdapter<*> {
            val typeAdapter = gson.getAdapter(TypeToken.get(type))
            return GsonMessageAdapter(gson, typeAdapter)
        }

        companion object {
            private val DEFAULT_GSON = Gson()
        }
    }

All of this seems to be related to a limitation of Gson described in this stackoverflow thread: https://stackoverflow.com/questions/14242236/let-gson-throw-exceptions-on-wrong-types

Apologies if I'm off base here.

sbearben commented 5 years ago

After messing around with Scarlet and the GsonMessageAdapter it appears the issues described in my previous post were what's causing every stream to emit.

Currently we are passing each @Receive stream the incoming WebSocket message and expecting Gson to "guess" if the message is of the respective type, relying on return typeAdapter.read(jsonReader)!! in GsonMessageAdapter to throw an exception if it isn't. However, for reasons described in the stackoverflow thread linked in my previous post, it is very rare that an exception will be thrown, resulting in each stream emitting.

I was able to prevent this from happening by modifying @Receive to have an optional type parameter which will contain a value for the 'type' key in the incoming WebSocket Json message we are expecting. In GsonMessageAdapter we peak at the Json and look for that type value in order to determine if each respective stream should deserialize and emit the message.

@Receive:

@Documented
@Target(METHOD)
@Retention(RUNTIME)
public @interface Receive {
    public String type() default "";
}

GsonMessageAdapter:

class GsonMessageAdapter<T> private constructor(
    private val gson: Gson,
    private val typeAdapter: TypeAdapter<T>,
    private val annotations: Array<Annotation>
) : MessageAdapter<T> {

    override fun fromMessage(message: Message): T {
        val stringValue = when (message) {
            is Message.Text -> message.value
            is Message.Bytes -> String(message.value)
        }
        val annotation = annotations.first { isReceiveAnnotation(it) } as Receive

        return if (annotation.type == "") {
            deserializeJson(stringValue)
        } else {
            deserializeJsonGivenReceiveType(stringValue, annotation.type)
        }
    }

    override fun toMessage(data: T): Message {
        val buffer = Buffer()
        val writer = OutputStreamWriter(buffer.outputStream(), UTF_8)
        val jsonWriter = gson.newJsonWriter(writer)
        typeAdapter.write(jsonWriter, data)
        jsonWriter.close()
        val stringValue = buffer.readByteString().utf8()
        return Message.Text(stringValue)
    }

    private fun isReceiveAnnotation(annotation: Annotation): Boolean {
        return when (annotation) {
            is Receive -> true
            else -> false
        }
    }

    private fun deserializeJson(jsonString : String) : T {
        val jsonReader = gson.newJsonReader(StringReader(jsonString))
        return typeAdapter.read(jsonReader)!!
    }

    private fun deserializeJsonGivenReceiveType(jsonString : String, type : String) : T {
        val jsonObject = JSONObject(jsonString)
        val typeFromJson = jsonObject.getString("type")

        if (typeFromJson != type) throw Exception()

        return deserializeJson(jsonString)
    }

    class Factory(
        private val gson: Gson = DEFAULT_GSON
    ) : MessageAdapter.Factory {

        override fun create(type: Type, annotations: Array<Annotation>): MessageAdapter<*> {
            val typeAdapter = gson.getAdapter(TypeToken.get(type))
            return GsonMessageAdapter(gson, typeAdapter, annotations)
        }

        companion object {
            private val DEFAULT_GSON = Gson()
        }
    }
}

I'm not sure if this would be an acceptable solution due to the receive annotation being modified, but if this is an issue with multiple adapters (someone described they had the same problem with Moshi) it might be worth considering. Also like how it provides a more "endpoint" like appearance. Anyways thought I would share. Can make a new PR if it seems fine.

ap-johns commented 5 years ago

Any official response on this ?

I'm using: implementation 'com.github.tinder.scarlet:scarlet:0.1.7' implementation 'com.github.tinder.scarlet:scarlet-message-adapter-gson:0.1.7' implementation 'com.github.tinder.scarlet:scarlet-websocket-okhttp:0.1.7' implementation 'com.github.tinder.scarlet:scarlet-stream-adapter-rxjava2:0.1.7'

and have the same issue

eg using these two test classes -

class Artist { @SerializedName("id") private var id: Int? = null @SerializedName("name") private var name: String? = null @SerializedName("url") private var url: String? = null

constructor(id: Int?, name: String?, url: String?) {
    this.id = id
    this.name = name
    this.url = url
}

override fun toString(): String {
    return "Artist(id=$id, name=$name, url=$url)"
}

}

class Album { @SerializedName("title") private val title: String? = null @SerializedName("duration") private val duration: Int? = null

override fun toString(): String {
    return "Album(title=$title, duration=$duration)"
}

}

DazzyG commented 5 years ago

@sbearben It would be great if you could knock that up into a pull request

sbearben commented 5 years ago

I never put it up in a PR since the solution doesn't seem great as we're modifying an annotation based on a specific use case. Furthermore you can implement this type of solution yourself without changing anything in the lib. See here for how I did it:

https://github.com/sbearben/ChatApp/blob/master/app/src/main/java/uk/co/victoriajanedavis/chatapp/data/realtime/websocket/ChatAppWebSocketService.kt https://github.com/sbearben/ChatApp/blob/master/app/src/main/java/uk/co/victoriajanedavis/chatapp/data/realtime/websocket/WebSocketStreams.kt https://github.com/sbearben/ChatApp/blob/master/app/src/main/java/uk/co/victoriajanedavis/chatapp/data/realtime/RealtimeEventResolver.kt

In that case I was turning incoming messages from Scarlet and FCM into the same streams (given the same message type)

ap-johns commented 5 years ago

Thanks for above. Works well for us.

Chesteer89 commented 5 years ago

The fastest workaround for my case was to modify GsonMessageAdapter a little bit.

/*
 * © 2018 Match Group, LLC.
 */

package com.tinder.scarlet.messageadapter.gson

import com.google.gson.Gson
import com.google.gson.TypeAdapter
import com.google.gson.reflect.TypeToken
import com.tinder.scarlet.Message
import com.tinder.scarlet.MessageAdapter
import okio.Buffer
import java.io.OutputStreamWriter
import java.io.StringReader
import java.lang.reflect.Type
import java.nio.charset.StandardCharsets.UTF_8

/**
 * A [message adapter][MessageAdapter] that uses Gson.
 */
class GsonMessageAdapter<T> private constructor(
    private val gson: Gson,
    private val typeAdapter: TypeAdapter<T>
) : MessageAdapter<T> {

    override fun fromMessage(message: Message): T {
        val stringValue = when (message) {
            is Message.Text -> message.value
            is Message.Bytes -> String(message.value)
        }

        return parseJson(stringValue)
    }

    override fun toMessage(data: T): Message {
        val buffer = Buffer()
        val writer = OutputStreamWriter(buffer.outputStream(), UTF_8)
        val jsonWriter = gson.newJsonWriter(writer)
        typeAdapter.write(jsonWriter, data)
        jsonWriter.close()
        val stringValue = buffer.readByteString().utf8()
        return Message.Text(stringValue)
    }

    private fun parseJson(content: String): T{
        val jsonReader = gson.newJsonReader(StringReader(content))
        val obj = typeAdapter.read(jsonReader)!!
        obj.checkDeserialization()
        return obj
    }

    @Throws(IllegalAccessException::class, JsonParseException::class)
    private fun Any.checkDeserialization() {
        // get all fields marked as Nullable
        val nullableFields = this::class.members.toTypedArray().filter { it.returnType.isMarkedNullable }.map { it.name }

        val fields = javaClass.declaredFields.filter { it.name !in nullableFields }
        for (f in fields) {
            f.isAccessible = true
            if (f.get(this) == null) {
                throw JsonParseException(f.name)
            }
        }
    }

    class JsonParseException(fieldName: String) : RuntimeException("Field $fieldName was not initialized!")

    class Factory(
        private val gson: Gson = DEFAULT_GSON
    ) : MessageAdapter.Factory {

        override fun create(type: Type, annotations: Array<Annotation>): MessageAdapter<*> {
            val typeAdapter = gson.getAdapter(TypeToken.get(type))
            return GsonMessageAdapter(gson, typeAdapter)
        }

        companion object {
            private val DEFAULT_GSON = Gson()
        }
    }
}

The most important thing is to check if any field (not marked as nullable) has null value. BTW: unique non-primitive field is a key.

loalexzzzz commented 4 years ago

The fastest workaround for my case was to modify GsonMessageAdapter a little bit.

/*
 * © 2018 Match Group, LLC.
 */

package com.tinder.scarlet.messageadapter.gson

import com.google.gson.Gson
import com.google.gson.TypeAdapter
import com.google.gson.reflect.TypeToken
import com.tinder.scarlet.Message
import com.tinder.scarlet.MessageAdapter
import okio.Buffer
import java.io.OutputStreamWriter
import java.io.StringReader
import java.lang.reflect.Type
import java.nio.charset.StandardCharsets.UTF_8

/**
 * A [message adapter][MessageAdapter] that uses Gson.
 */
class GsonMessageAdapter<T> private constructor(
    private val gson: Gson,
    private val typeAdapter: TypeAdapter<T>
) : MessageAdapter<T> {

    override fun fromMessage(message: Message): T {
        val stringValue = when (message) {
            is Message.Text -> message.value
            is Message.Bytes -> String(message.value)
        }

        return parseJson(stringValue)
    }

    override fun toMessage(data: T): Message {
        val buffer = Buffer()
        val writer = OutputStreamWriter(buffer.outputStream(), UTF_8)
        val jsonWriter = gson.newJsonWriter(writer)
        typeAdapter.write(jsonWriter, data)
        jsonWriter.close()
        val stringValue = buffer.readByteString().utf8()
        return Message.Text(stringValue)
    }

    private fun parseJson(content: String): T{
        val jsonReader = gson.newJsonReader(StringReader(content))
        val obj = typeAdapter.read(jsonReader)!!
        obj.checkDeserialization()
        return obj
    }

    @Throws(IllegalAccessException::class, JsonParseException::class)
    private fun Any.checkDeserialization() {
        // get all fields marked as Nullable
        val nullableFields = this::class.members.toTypedArray().filter { it.returnType.isMarkedNullable }.map { it.name }

        val fields = javaClass.declaredFields.filter { it.name !in nullableFields }
        for (f in fields) {
            f.isAccessible = true
            if (f.get(this) == null) {
                throw JsonParseException(f.name)
            }
        }
    }

    class JsonParseException(fieldName: String) : RuntimeException("Field $fieldName was not initialized!")

    class Factory(
        private val gson: Gson = DEFAULT_GSON
    ) : MessageAdapter.Factory {

        override fun create(type: Type, annotations: Array<Annotation>): MessageAdapter<*> {
            val typeAdapter = gson.getAdapter(TypeToken.get(type))
            return GsonMessageAdapter(gson, typeAdapter)
        }

        companion object {
            private val DEFAULT_GSON = Gson()
        }
    }
}

The most important thing is to check if any field (not marked as nullable) has null value. BTW: unique non-primitive field is a key.

but how about this situation? they both have the same field..

data class Test1(val type: String = "")

data class Test2(val type: String = "")