-
Notifications
You must be signed in to change notification settings - Fork 610
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
refactor: [database/charge] dbflow to room #2312
Conversation
@Path("clientId") clientId: Int, | ||
@Query("offset") offset: Int, | ||
@Query("limit") limit: Int, | ||
): Observable<Page<Charges>> | ||
): Page<Charges> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be Flow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@niyajali You mean this right? Retrofit does not support flow
fun getListOfCharges(
@Path("clientId") clientId: Int,
@Query("offset") offset: Int,
@Query("limit") limit: Int,
): Flow<Page<Charges>>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@itsPronay No it does, create an adapter for that, like this, create the below two files and place them in the utils folder
// FlowCallAdapterFactory.kt
import kotlinx.coroutines.flow.Flow
import retrofit2.CallAdapter
import retrofit2.Response
import retrofit2.Retrofit
import java.lang.reflect.ParameterizedType
import java.lang.reflect.Type
class FlowCallAdapterFactory private constructor() : CallAdapter.Factory() {
override fun get(
returnType: Type,
annotations: Array<Annotation>,
retrofit: Retrofit
): CallAdapter<*, *>? {
if (getRawType(returnType) != Flow::class.java) {
return null
}
check(returnType is ParameterizedType) { "Flow return type must be parameterized as Flow<Foo> or Flow<out Foo>" }
val responseType = getParameterUpperBound(0, returnType)
val rawFlowType = getRawType(responseType)
return if (rawFlowType == Response::class.java) {
check(responseType is ParameterizedType) { "Response must be parameterized as Response<Foo> or Response<out Foo>" }
ResponseCallAdapter<Any>(
getParameterUpperBound(
0,
responseType
)
)
} else {
BodyCallAdapter<Any>(responseType)
}
}
companion object {
@JvmStatic
fun create() = FlowCallAdapterFactory()
}
}
// FlowCallAdapter.kt
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.flow
import kotlinx.coroutines.suspendCancellableCoroutine
import retrofit2.Call
import retrofit2.CallAdapter
import retrofit2.Callback
import retrofit2.Response
import java.lang.reflect.Type
import kotlin.coroutines.resume
import kotlin.coroutines.resumeWithException
class ResponseCallAdapter<T>(
private val responseType: Type
) : CallAdapter<T, Flow<Response<T>>> {
override fun adapt(call: Call<T>): Flow<Response<T>> {
return flow {
emit(
suspendCancellableCoroutine { continuation ->
call.enqueue(object : Callback<T> {
override fun onFailure(call: Call<T>, t: Throwable) {
continuation.resumeWithException(t)
}
override fun onResponse(call: Call<T>, response: Response<T>) {
continuation.resume(response)
}
})
continuation.invokeOnCancellation { call.cancel() }
}
)
}
}
override fun responseType() = responseType
}
class BodyCallAdapter<T>(private val responseType: Type) : CallAdapter<T, Flow<T>> {
override fun adapt(call: Call<T>): Flow<T> {
return flow {
emit(
suspendCancellableCoroutine { continuation ->
call.enqueue(object : Callback<T> {
override fun onFailure(call: Call<T>, t: Throwable) {
continuation.resumeWithException(t)
}
override fun onResponse(call: Call<T>, response: Response<T>) {
try {
continuation.resume(response.body()!!)
} catch (e: Exception) {
continuation.resumeWithException(e)
}
}
})
continuation.invokeOnCancellation { call.cancel() }
}
)
}
}
override fun responseType() = responseType
}
And use like this
val retrofit = Retrofit.Builder()
.baseUrl("https://example.com/")
.addCallAdapterFactory(FlowCallAdapterFactory.create()) // Add this adapter on your retrofit config
.build()
charges.id?.toLong()?.let { | ||
ClientDate( | ||
0, | ||
it, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always try to pass parameters names for better Readability
* @return Page of Charges | ||
*/ | ||
fun readClientCharges(clientId: Int): Flow<Page<Charges>> { | ||
return chargeDao.getClientCharges(clientId).map { chargesList -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inject ioDispatcher in this class and use flowOn(ioDispatcher) oparator, for all methods which returns as Flow, do this in other classes too
And this function implementation looks messy to me, copy this code and chat on Deepseek, Chatgpt etc and ask them to optimize/improve this codebase, nowadays we've many free AI tools consider to use those for better usability.
@@ -41,4 +42,9 @@ object DaoModule { | |||
fun providesStaffDao(database: MifosDatabase): StaffDao { | |||
return database.staffDao() | |||
} | |||
|
|||
@Provides | |||
fun providesClientDao(database: MifosDatabase): ChargeDao { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@itsPronay I think it would be a good idea to add @singleton to the providesClientDao() method. Since Room is designed to work with a single shared database connection, using @singleton ensures that all components use the same ChargeDao instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HekmatullahAmin thanks for the suggestion brother.
but we already used @InstallIn(SingletonComponent::class)
on the DaoModule, it already ensures that all the provided components in this module are scoped as singletons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @itsPronay !
Just to clarify the difference between @Installin(SingletonComponent::class) and @singleton in Hilt:
@Installin(SingletonComponent::class) means that the dependency will live as long as the application is alive, but it doesn't guarantee a single instance by itself.
@singleton tells Hilt to reuse the same instance of the dependency within the same scope.
Example:
@Module
@InstallIn(SingletonComponent::class)
object NetworkModule {
@Provides
fun provideApiService(): ApiService {
return ApiServiceImpl()
}
}
In this case, every class that requests ApiService gets a new instance, because @singleton is missing.
Singleton Example:
@Module
@InstallIn(SingletonComponent::class)
object NetworkModule {
@Singleton
@Provides
fun provideApiService(): ApiService {
return ApiServiceImpl()
}
}
By adding @singleton, only one instance of SomeService is created and shared across the entire app (within the SingletonComponent scope).
Without @singleton, multiple instances are created even if SingletonComponent is used.
If you’d like more details, you can check out the official documentation here:
Hilt - Android Developer Guide
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HekmatullahAmin Thanks for explaining.
I have a doubt, since MifosDatabase is already a singleton, wouldn't the DAO instances be shared across the app by default? meaning singleton
}) | ||
Pair(page.pageItems, page.totalFilteredRecords) | ||
} catch (exception: Throwable) { | ||
throw exception |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest wrapping the throw statement in a custom exception. It will help us to easily trace errors during debugging.
somehting like:
throw DatabaseFetchException("Failed to fetch client charges", exception)
continuation.resumeWithException(exception) | ||
} | ||
return try { | ||
val page = dataManagerCharge.getClientCharges( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See documentation, How to use coroutines in PagingSource
interface ChargeDao { | ||
|
||
@Insert(onConflict = OnConflictStrategy.REPLACE) | ||
suspend fun insertCharges(charge: List<Charges>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of charge: List<Charges>
vararg charges: Charges
Fixes - https://mifosforge.jira.com/browse/MIFOSAC-392
Please make sure these boxes are checked before submitting your pull request - thanks!
Run the static analysis check
./gradlew check
orci-prepush.sh
to make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them.