Fix #1104: Offline Exception Logging

domain/src/main/java/org/oppia/domain/oppialogger/exceptions/ExceptionsController.kt

@@ -6,8 +6,7 @@ import org.oppia.app.model.ExceptionLog.ExceptionType
 import org.oppia.app.model.OppiaExceptionLogs
 import org.oppia.data.persistence.PersistentCacheStore
 import org.oppia.domain.oppialogger.ExceptionLogStorageCacheSize
-import org.oppia.util.data.AsyncResult
-import org.oppia.util.data.DataProviders
+import org.oppia.util.data.DataProvider
 import org.oppia.util.logging.ConsoleLogger
 import org.oppia.util.logging.ExceptionLogger
 import org.oppia.util.networking.NetworkConnectionUtil
@@ -18,7 +17,6 @@ private const val EXCEPTIONS_CONTROLLER = "Exceptions Controller"
 /** Controller for handling exception logging. */
 class ExceptionsController @Inject constructor(
   private val exceptionLogger: ExceptionLogger,
-  private val dataProviders: DataProviders,
   cacheStoreFactory: PersistentCacheStore.Factory,
   private val consoleLogger: ConsoleLogger,
   private val networkConnectionUtil: NetworkConnectionUtil,
@@ -63,8 +61,7 @@ class ExceptionsController @Inject constructor(
     when (networkConnectionUtil.getCurrentConnectionStatus()) {
       NetworkConnectionUtil.ConnectionStatus.NONE ->
         cacheExceptionLog(
-          convertExceptionToExceptionLog(
-            exception,
+          exception.toExceptionLog(
             timestampInMillis,
             exceptionType
           )
@@ -74,22 +71,22 @@ class ExceptionsController @Inject constructor(
   }
 
   /** Returns an [ExceptionLog] from a [throwable]. */
-  private fun convertExceptionToExceptionLog(
-    throwable: Throwable,
+  private fun Throwable.toExceptionLog(
     timestampInMillis: Long,
     exceptionType: ExceptionType
   ): ExceptionLog {
     val exceptionLogBuilder = ExceptionLog.newBuilder()
-    throwable.message?.let {
+    this.message?.let {
       exceptionLogBuilder.message = it
     }
     exceptionLogBuilder.timestampInMillis = timestampInMillis
-    throwable.cause?.let {
-      exceptionLogBuilder.cause =
-        convertExceptionToExceptionLog(it, timestampInMillis, exceptionType)
+    this.cause?.let {
+      exceptionLogBuilder.cause = it.toExceptionLog(timestampInMillis, exceptionType)
     }
-    throwable.stackTrace?.let {
-      exceptionLogBuilder.addAllStacktraceElement(it.map(this::convertStackTraceElementToLog))
+    this.stackTrace?.let {
+      exceptionLogBuilder.addAllStacktraceElement(
+        it.map(this@ExceptionsController::convertStackTraceElementToLog)
+      )
     }
     exceptionLogBuilder.exceptionType = exceptionType
     return exceptionLogBuilder.build()
@@ -166,8 +163,8 @@ class ExceptionsController @Inject constructor(
    * Returns a [LiveData] result which can be used to get [OppiaExceptionLogs]
    * for the purpose of uploading in the presence of network connectivity.
    */
-  fun getExceptionLogs(): LiveData<AsyncResult<OppiaExceptionLogs>> {
-    return dataProviders.convertToLiveData(exceptionLogStore)
+  fun getExceptionLogStore(): DataProvider<OppiaExceptionLogs> {
+    return exceptionLogStore
   }
 }
 

domain/src/test/java/org/oppia/domain/oppialogger/exceptions/ExceptionsControllerTest.kt

@@ -10,8 +10,11 @@ import dagger.BindsInstance
 import dagger.Component
 import dagger.Module
 import dagger.Provides
+import kotlinx.coroutines.CoroutineDispatcher
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.InternalCoroutinesApi
+import kotlinx.coroutines.ObsoleteCoroutinesApi
+import kotlinx.coroutines.test.TestCoroutineDispatcher
 import org.junit.Before
 import org.junit.Rule
 import org.junit.Test
@@ -31,6 +34,7 @@ import org.oppia.testing.TestCoroutineDispatchers
 import org.oppia.testing.TestDispatcherModule
 import org.oppia.testing.TestLogReportingModule
 import org.oppia.util.data.AsyncResult
+import org.oppia.util.data.DataProviders
 import org.oppia.util.logging.EnableConsoleLog
 import org.oppia.util.logging.EnableFileLog
 import org.oppia.util.logging.GlobalLogLevel
@@ -54,6 +58,9 @@ class ExceptionsControllerTest {
   @JvmField
   val mockitoRule: MockitoRule = MockitoJUnit.rule()
 
+  @Inject
+  lateinit var dataProviders: DataProviders
+
   @Inject
   lateinit var exceptionsController: ExceptionsController
 
@@ -74,6 +81,8 @@ class ExceptionsControllerTest {
   lateinit var oppiaExceptionLogsResultCaptor: ArgumentCaptor<AsyncResult<OppiaExceptionLogs>>
 
   @Before
+  @ExperimentalCoroutinesApi
+  @ObsoleteCoroutinesApi
   fun setUp() {
     networkConnectionUtil = NetworkConnectionUtil(ApplicationProvider.getApplicationContext())
     setUpTestApplicationComponent()
@@ -105,16 +114,19 @@ class ExceptionsControllerTest {
   @InternalCoroutinesApi
   @Test
   fun testController_logException_nonFatal_withNoNetwork_logsToCacheStore() {
-    networkConnectionUtil.setCurrentConnectionStatus(NetworkConnectionUtil.ConnectionStatus.NONE)
     val exceptionThrown = Exception("TEST MESSAGE", Throwable("TEST CAUSE"))
+    networkConnectionUtil.setCurrentConnectionStatus(NetworkConnectionUtil.ConnectionStatus.NONE)
     exceptionsController.logNonFatalException(exceptionThrown, TEST_TIMESTAMP_IN_MILLIS_ONE)
 
-    val cachedExceptions = exceptionsController.getExceptionLogs()
+    val cachedExceptions =
+      dataProviders.convertToLiveData(exceptionsController.getExceptionLogStore())
     cachedExceptions.observeForever(mockOppiaExceptionLogsObserver)
     testCoroutineDispatchers.advanceUntilIdle()
 
-    verify(mockOppiaExceptionLogsObserver, atLeastOnce())
-      .onChanged(oppiaExceptionLogsResultCaptor.capture())
+    verify(
+      mockOppiaExceptionLogsObserver,
+      atLeastOnce()
+    ).onChanged(oppiaExceptionLogsResultCaptor.capture())
 
     val exceptionLog = oppiaExceptionLogsResultCaptor.value.getOrThrow().getExceptionLog(0)
     val exception = exceptionLog.toException()
@@ -133,7 +145,8 @@ class ExceptionsControllerTest {
     val exceptionThrown = Exception("TEST MESSAGE", Throwable("TEST"))
     exceptionsController.logFatalException(exceptionThrown, TEST_TIMESTAMP_IN_MILLIS_ONE)
 
-    val cachedExceptions = exceptionsController.getExceptionLogs()
+    val cachedExceptions =
+      dataProviders.convertToLiveData(exceptionsController.getExceptionLogStore())
     cachedExceptions.observeForever(mockOppiaExceptionLogsObserver)
     testCoroutineDispatchers.advanceUntilIdle()
 
@@ -172,7 +185,8 @@ class ExceptionsControllerTest {
       TEST_TIMESTAMP_IN_MILLIS_FOUR
     )
 
-    val cachedExceptions = exceptionsController.getExceptionLogs()
+    val cachedExceptions =
+      dataProviders.convertToLiveData(exceptionsController.getExceptionLogStore())
     cachedExceptions.observeForever(mockOppiaExceptionLogsObserver)
     testCoroutineDispatchers.advanceUntilIdle()
 
@@ -182,7 +196,8 @@ class ExceptionsControllerTest {
     val exceptionOne = oppiaExceptionLogsResultCaptor.value.getOrThrow().getExceptionLog(0)
     val exceptionTwo = oppiaExceptionLogsResultCaptor.value.getOrThrow().getExceptionLog(1)
 
-    // In this case, 3 fatal and 1 non-fatal exceptions were logged. So while pruning, none of the retained logs should have non-fatal exception type.
+    // In this case, 3 fatal and 1 non-fatal exceptions were logged. The order of logging was fatal->non-fatal->fatal->fatal.
+    // So while pruning, none of the retained logs should have non-fatal exception type.
     assertThat(exceptionOne.exceptionType).isNotEqualTo(ExceptionType.NON_FATAL)
     assertThat(exceptionTwo.exceptionType).isNotEqualTo(ExceptionType.NON_FATAL)
     // If we analyse the order of logging of exceptions, we can see that record pruning will begin from the logging of the third record.
@@ -214,7 +229,8 @@ class ExceptionsControllerTest {
       TEST_TIMESTAMP_IN_MILLIS_THREE
     )
 
-    val cachedExceptions = exceptionsController.getExceptionLogs()
+    val cachedExceptions =
+      dataProviders.convertToLiveData(exceptionsController.getExceptionLogStore())
     cachedExceptions.observeForever(mockOppiaExceptionLogsObserver)
     testCoroutineDispatchers.advanceUntilIdle()
 
@@ -234,7 +250,8 @@ class ExceptionsControllerTest {
     networkConnectionUtil.setCurrentConnectionStatus(NetworkConnectionUtil.ConnectionStatus.NONE)
     exceptionsController.logFatalException(exceptionThrown, TEST_TIMESTAMP_IN_MILLIS_ONE)
 
-    val cachedExceptions = exceptionsController.getExceptionLogs()
+    val cachedExceptions =
+      dataProviders.convertToLiveData(exceptionsController.getExceptionLogStore())
     cachedExceptions.observeForever(mockOppiaExceptionLogsObserver)
     testCoroutineDispatchers.advanceUntilIdle()
 
@@ -262,7 +279,8 @@ class ExceptionsControllerTest {
     exceptionsController.logNonFatalException(exceptionThrown, TEST_TIMESTAMP_IN_MILLIS_ONE)
     exceptionsController.logFatalException(exceptionThrown, TEST_TIMESTAMP_IN_MILLIS_ONE)
 
-    val cachedExceptions = exceptionsController.getExceptionLogs()
+    val cachedExceptions =
+      dataProviders.convertToLiveData(exceptionsController.getExceptionLogStore())
     cachedExceptions.observeForever(mockOppiaExceptionLogsObserver)
     testCoroutineDispatchers.advanceUntilIdle()
 
@@ -274,6 +292,52 @@ class ExceptionsControllerTest {
     assertThat(exceptionTwo.exceptionType).isEqualTo(ExceptionType.FATAL)
   }
 
+  @ExperimentalCoroutinesApi
+  @InternalCoroutinesApi
+  @Test
+  fun testExtension_logEmptyException_withNoNetwork_verifyRecreationOfLogs() {
+    networkConnectionUtil.setCurrentConnectionStatus(NetworkConnectionUtil.ConnectionStatus.NONE)
+    val exceptionThrown = Exception()
+    exceptionsController.logNonFatalException(exceptionThrown, TEST_TIMESTAMP_IN_MILLIS_ONE)
+
+    val cachedExceptions =
+      dataProviders.convertToLiveData(exceptionsController.getExceptionLogStore())
+    cachedExceptions.observeForever(mockOppiaExceptionLogsObserver)
+    testCoroutineDispatchers.advanceUntilIdle()
+
+    verify(mockOppiaExceptionLogsObserver, atLeastOnce())
+      .onChanged(oppiaExceptionLogsResultCaptor.capture())
+    val exceptionLog = oppiaExceptionLogsResultCaptor.value.getOrThrow().getExceptionLog(0)
+    val exception = exceptionLog.toException()
+
+    assertThat(exception.message).isEqualTo(null)
+    assertThat(exception.stackTrace).isEqualTo(exceptionThrown.stackTrace)
+    assertThat(exception.cause).isEqualTo(null)
+  }
+
+  @ExperimentalCoroutinesApi
+  @InternalCoroutinesApi
+  @Test
+  fun testExtension_logException_withNoCause_withNoNetwork_verifyRecreationOfLogs() {
+    networkConnectionUtil.setCurrentConnectionStatus(NetworkConnectionUtil.ConnectionStatus.NONE)
+    val exceptionThrown = Exception("TEST")
+    exceptionsController.logNonFatalException(exceptionThrown, TEST_TIMESTAMP_IN_MILLIS_ONE)
+
+    val cachedExceptions =
+      dataProviders.convertToLiveData(exceptionsController.getExceptionLogStore())
+    cachedExceptions.observeForever(mockOppiaExceptionLogsObserver)
+    testCoroutineDispatchers.advanceUntilIdle()
+
+    verify(mockOppiaExceptionLogsObserver, atLeastOnce())
+      .onChanged(oppiaExceptionLogsResultCaptor.capture())
+    val exceptionLog = oppiaExceptionLogsResultCaptor.value.getOrThrow().getExceptionLog(0)
+    val exception = exceptionLog.toException()
+
+    assertThat(exception.message).isEqualTo("TEST")
+    assertThat(exception.stackTrace).isEqualTo(exceptionThrown.stackTrace)
+    assertThat(exception.cause).isEqualTo(null)
+  }
+
   private fun setUpTestApplicationComponent() {
     DaggerExceptionsControllerTest_TestApplicationComponent.builder()
       .setApplication(ApplicationProvider.getApplicationContext())
@@ -293,6 +357,14 @@ class ExceptionsControllerTest {
       return application
     }
 
+    @ExperimentalCoroutinesApi
+    @Singleton
+    @Provides
+    @TestDispatcher
+    fun provideTestDispatcher(): CoroutineDispatcher {
+      return TestCoroutineDispatcher()
+    }
+
     // TODO(#59): Either isolate these to their own shared test module, or use the real logging
     // module in tests to avoid needing to specify these settings for tests.
     @EnableConsoleLog