Skip to content
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

Feature/fix send room v6 #2010

Merged
merged 4 commits into from
Aug 27, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ Bugfix 🐛:
- Fix FontSize issue (#1483, #1787)
- Fix bad color for settings icon on Android < 24 (#1786)
- Change user or room avatar: when selecting Gallery, I'm not proposed to crop the selected image (#1590)
- Fix uploads still don't work with room v6 (#1879)

Translations 🗣:
-
Expand Down
2 changes: 0 additions & 2 deletions matrix-sdk-android/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -131,8 +131,6 @@ dependencies {
// Network
implementation "com.squareup.retrofit2:retrofit:$retrofit_version"
implementation "com.squareup.retrofit2:converter-moshi:$retrofit_version"
implementation "com.squareup.retrofit2:converter-scalars:$retrofit_version"


implementation(platform("com.squareup.okhttp3:okhttp-bom:4.8.1"))
implementation 'com.squareup.okhttp3:okhttp'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,12 @@ package org.matrix.android.sdk.internal.network

import com.squareup.moshi.Moshi
import dagger.Lazy
import org.matrix.android.sdk.internal.util.ensureTrailingSlash
import okhttp3.Call
import okhttp3.OkHttpClient
import okhttp3.Request
import org.matrix.android.sdk.internal.util.ensureTrailingSlash
import retrofit2.Retrofit
import retrofit2.converter.moshi.MoshiConverterFactory
import retrofit2.converter.scalars.ScalarsConverterFactory
import javax.inject.Inject

internal class RetrofitFactory @Inject constructor(private val moshi: Moshi) {
Expand All @@ -50,7 +49,6 @@ internal class RetrofitFactory @Inject constructor(private val moshi: Moshi) {
return okHttpClient.get().newCall(request)
}
})
.addConverterFactory(ScalarsConverterFactory.create())
.addConverterFactory(UnitConverterFactory)
.addConverterFactory(MoshiConverterFactory.create(moshi))
.build()
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
* Copyright (c) 2020 New Vector Ltd
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.matrix.android.sdk.internal.network.parsing

import androidx.annotation.Nullable
import com.squareup.moshi.JsonAdapter
import com.squareup.moshi.JsonReader
import com.squareup.moshi.JsonWriter

import com.squareup.moshi.Moshi
import java.io.IOException
import java.lang.reflect.Type
import java.math.BigDecimal

/**
* This is used to check if NUMBER in json is integer or double, so we can preserve typing when serializing/deserializing in a row.
*/
interface CheckNumberType {

companion object {
val JSON_ADAPTER_FACTORY = object : JsonAdapter.Factory {
@Nullable
override fun create(type: Type, annotations: Set<Annotation?>?, moshi: Moshi): JsonAdapter<*>? {
if (type !== Any::class.java) {
return null
}
val delegate: JsonAdapter<Any> = moshi.nextAdapter(this, Any::class.java, emptySet())
return object : JsonAdapter<Any?>() {
@Nullable
@Throws(IOException::class)
override fun fromJson(reader: JsonReader): Any? {
return if (reader.peek() !== JsonReader.Token.NUMBER) {
delegate.fromJson(reader)
} else {
val numberAsString = reader.nextString()
val decimal = BigDecimal(numberAsString)
if (decimal.scale() <= 0) {
decimal.intValueExact()
} else {
decimal.toDouble()
}
}
}

override fun toJson(writer: JsonWriter, value: Any?) {
delegate.toJson(writer, value)
}
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -130,21 +130,6 @@ internal interface RoomAPI {
@Body content: Content?
): Call<SendResponse>

/**
* Send an event to a room.
*
* @param txId the transaction Id
* @param roomId the room id
* @param eventType the event type
* @param content the event content as string
*/
@PUT(NetworkConstants.URI_API_PREFIX_PATH_R0 + "rooms/{roomId}/send/{eventType}/{txId}")
fun send(@Path("txId") txId: String,
@Path("roomId") roomId: String,
@Path("eventType") eventType: String,
@Body content: String?
): Call<SendResponse>

/**
* Get the context surrounding an event.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ internal class DefaultRelationService @AssistedInject constructor(
}

private fun createSendEventWork(event: Event, startChain: Boolean): OneTimeWorkRequest {
val sendContentWorkerParams = SendEventWorker.Params(sessionId, event)
val sendContentWorkerParams = SendEventWorker.Params(sessionId = sessionId, event = event)
val sendWorkData = WorkerParamsFactory.toData(sendContentWorkerParams)
return timeLineSendEventWorkCommon.createWork<SendEventWorker>(sendWorkData, startChain)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ internal class EncryptEventWorker(context: Context, params: WorkerParameters)
localEchoRepository.updateEncryptedEcho(localEvent.eventId, safeResult.eventContent, decryptionLocalEcho)
}

val nextWorkerParams = SendEventWorker.Params(params.sessionId, encryptedEvent)
val nextWorkerParams = SendEventWorker.Params(sessionId = params.sessionId, event = encryptedEvent)
return Result.success(WorkerParamsFactory.toData(nextWorkerParams))
} else {
val sendState = when (error) {
Expand All @@ -129,8 +129,11 @@ internal class EncryptEventWorker(context: Context, params: WorkerParameters)
}
localEchoRepository.updateSendState(localEvent.eventId, sendState)
// always return success, or the chain will be stuck for ever!
val nextWorkerParams = SendEventWorker.Params(params.sessionId, localEvent, error?.localizedMessage
?: "Error")
val nextWorkerParams = SendEventWorker.Params(
sessionId = params.sessionId,
event = localEvent,
lastFailureMessage = error?.localizedMessage ?: "Error"
)
return Result.success(WorkerParamsFactory.toData(nextWorkerParams))
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ internal class MultipleEventSendingDispatcherWorker(context: Context, params: Wo
}

private fun createSendEventWork(sessionId: String, event: Event, startChain: Boolean): OneTimeWorkRequest {
val sendContentWorkerParams = SendEventWorker.Params(sessionId, event)
val sendContentWorkerParams = SendEventWorker.Params(sessionId = sessionId, event = event)
val sendWorkData = WorkerParamsFactory.toData(sendContentWorkerParams)

return timelineSendEventWorkCommon.createWork<SendEventWorker>(sendWorkData, startChain)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ internal class RoomEventSender @Inject constructor(
}

private fun createSendEventWork(event: Event, startChain: Boolean): OneTimeWorkRequest {
val sendContentWorkerParams = SendEventWorker.Params(sessionId, event)
val sendContentWorkerParams = SendEventWorker.Params(sessionId = sessionId, event = event)
val sendWorkData = WorkerParamsFactory.toData(sendContentWorkerParams)

return timelineSendEventWorkCommon.createWork<SendEventWorker>(sendWorkData, startChain)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,16 @@ import android.content.Context
import androidx.work.CoroutineWorker
import androidx.work.WorkerParameters
import com.squareup.moshi.JsonClass
import org.greenrobot.eventbus.EventBus
import org.matrix.android.sdk.api.failure.shouldBeRetried
import org.matrix.android.sdk.api.session.events.model.Content
import org.matrix.android.sdk.api.session.events.model.Event
import org.matrix.android.sdk.api.session.room.send.SendState
import org.matrix.android.sdk.internal.database.mapper.ContentMapper
import org.matrix.android.sdk.internal.network.executeRequest
import org.matrix.android.sdk.internal.session.room.RoomAPI
import org.matrix.android.sdk.internal.worker.SessionWorkerParams
import org.matrix.android.sdk.internal.worker.WorkerParamsFactory
import org.matrix.android.sdk.internal.worker.getSessionComponent
import org.greenrobot.eventbus.EventBus
import timber.log.Timber
import javax.inject.Inject

Expand All @@ -47,24 +47,11 @@ internal class SendEventWorker(context: Context,
@JsonClass(generateAdapter = true)
internal data class Params(
override val sessionId: String,
// TODO remove after some time, it's used for compat
override val lastFailureMessage: String? = null,
val event: Event? = null,
val eventId: String? = null,
val roomId: String? = null,
val type: String? = null,
val contentStr: String? = null,
Copy link
Member

@bmarty bmarty Aug 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe keep it for the moment for compat?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won't crash as it's not mandatory, so I think it's OK

override val lastFailureMessage: String? = null
) : SessionWorkerParams {

constructor(sessionId: String, event: Event, lastFailureMessage: String? = null) : this(
sessionId = sessionId,
eventId = event.eventId,
roomId = event.roomId,
type = event.type,
contentStr = ContentMapper.map(event.content),
lastFailureMessage = lastFailureMessage
)
}
// Keep for compat at the moment, will be removed later
val eventId: String? = null
) : SessionWorkerParams

@Inject lateinit var localEchoRepository: LocalEchoRepository
@Inject lateinit var roomAPI: RoomAPI
Expand All @@ -77,38 +64,42 @@ internal class SendEventWorker(context: Context,

val sessionComponent = getSessionComponent(params.sessionId) ?: return Result.success()
sessionComponent.inject(this)
if (params.eventId == null || params.roomId == null || params.type == null) {
// compat with old params, make it fail if any
if (params.event?.eventId != null) {
localEchoRepository.updateSendState(params.event.eventId, SendState.UNDELIVERED)

val event = params.event
if (event?.eventId == null || event.roomId == null) {
// Old way of sending
if (params.eventId != null) {
localEchoRepository.updateSendState(params.eventId, SendState.UNDELIVERED)
}
return Result.success()
.also { Timber.e("Work cancelled due to bad input data") }
}

if (params.lastFailureMessage != null) {
localEchoRepository.updateSendState(params.eventId, SendState.UNDELIVERED)
localEchoRepository.updateSendState(event.eventId, SendState.UNDELIVERED)
// Transmit the error
return Result.success(inputData)
.also { Timber.e("Work cancelled due to input error from parent") }
}
return try {
sendEvent(params.eventId, params.roomId, params.type, params.contentStr)
sendEvent(event.eventId, event.roomId, event.type, event.content)
Result.success()
} catch (exception: Throwable) {
// It does start from 0, we want it to stop if it fails the third time
val currentAttemptCount = runAttemptCount + 1
if (currentAttemptCount >= MAX_NUMBER_OF_RETRY_BEFORE_FAILING || !exception.shouldBeRetried()) {
localEchoRepository.updateSendState(params.eventId, SendState.UNDELIVERED)
localEchoRepository.updateSendState(event.eventId, SendState.UNDELIVERED)
return Result.success()
} else {
Result.retry()
}
}
}

private suspend fun sendEvent(eventId: String, roomId: String, type: String, contentStr: String?) {
private suspend fun sendEvent(eventId: String, roomId: String, type: String, content: Content?) {
localEchoRepository.updateSendState(eventId, SendState.SENDING)
executeRequest<SendResponse>(eventBus) {
apiCall = roomAPI.send(eventId, roomId, type, contentStr)
apiCall = roomAPI.send(eventId, roomId, type, content)
}
localEchoRepository.updateSendState(eventId, SendState.SENT)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,23 @@ package org.matrix.android.sdk.internal.worker

import androidx.work.Data
import org.matrix.android.sdk.internal.di.MoshiProvider
import org.matrix.android.sdk.internal.network.parsing.CheckNumberType

object WorkerParamsFactory {
internal object WorkerParamsFactory {

val moshi by lazy {
// We are adding the CheckNumberType as we are serializing/deserializing multiple time in a row
// and we lost typing information doing so.
// We don't want this check to be done on all adapters, so we just add it here.
MoshiProvider.providesMoshi()
.newBuilder()
.add(CheckNumberType.JSON_ADAPTER_FACTORY)
.build()
}

const val KEY = "WORKER_PARAMS_JSON"

inline fun <reified T> toData(params: T): Data {
val moshi = MoshiProvider.providesMoshi()
val adapter = moshi.adapter(T::class.java)
val json = adapter.toJson(params)
return Data.Builder().putString(KEY, json).build()
Expand All @@ -36,7 +46,6 @@ object WorkerParamsFactory {
return if (json == null) {
null
} else {
val moshi = MoshiProvider.providesMoshi()
val adapter = moshi.adapter(T::class.java)
adapter.fromJson(json)
}
Expand Down