From e6d44e6f176207122fb39dd26670f058b96d8ab9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Sikora?= Date: Mon, 17 Aug 2020 18:28:14 +0200 Subject: [PATCH 01/14] Handle temp file creation - Remove FileFactory interface in favor of concrete class implementation that accepts files directory. - In case of IO failures or missing directory user gets feedback in Logcat or via Toast. --- .../chucker/api/ChuckerInterceptor.kt | 21 ++++------ .../support/AndroidCacheFileFactory.kt | 23 ---------- .../chucker/internal/support/FileFactory.kt | 27 ++++++++++-- .../chucker/internal/support/TeeSource.kt | 10 ++--- .../chucker/internal/ui/MainViewModel.kt | 10 ++--- .../ui/transaction/TransactionListFragment.kt | 11 +++-- library/src/main/res/values/strings.xml | 1 + .../java/com/chuckerteam/chucker/TestUtils.kt | 4 +- .../chucker/api/ChuckerInterceptorTest.kt | 42 +++++++++---------- .../chucker/internal/support/TeeSourceTest.kt | 4 +- 10 files changed, 73 insertions(+), 80 deletions(-) delete mode 100644 library/src/main/java/com/chuckerteam/chucker/internal/support/AndroidCacheFileFactory.kt diff --git a/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt b/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt index 8d2f0d0e5..756a822d9 100755 --- a/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt +++ b/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt @@ -2,7 +2,6 @@ package com.chuckerteam.chucker.api import android.content.Context import com.chuckerteam.chucker.internal.data.entity.HttpTransaction -import com.chuckerteam.chucker.internal.support.AndroidCacheFileFactory import com.chuckerteam.chucker.internal.support.FileFactory import com.chuckerteam.chucker.internal.support.IOUtils import com.chuckerteam.chucker.internal.support.TeeSource @@ -61,7 +60,7 @@ class ChuckerInterceptor internal constructor( collector: ChuckerCollector = ChuckerCollector(context), maxContentLength: Long = 250000L, headersToRedact: Set = emptySet() - ) : this(context, collector, maxContentLength, AndroidCacheFileFactory(context), headersToRedact) + ) : this(context, collector, maxContentLength, FileFactory(context::getCacheDir), headersToRedact) private val io: IOUtils = IOUtils(context) private val headersToRedact: MutableSet = headersToRedact.toMutableSet() @@ -234,28 +233,26 @@ class ChuckerInterceptor internal constructor( private val transaction: HttpTransaction ) : TeeSource.Callback { - override fun onClosed(file: File, totalBytesRead: Long) { - val buffer = try { - readResponseBuffer(file, response.isGzipped) - } catch (e: IOException) { - null - } + override fun onClosed(file: File?, totalBytesRead: Long) { + val buffer = file?.let { readResponseBuffer(it, response.isGzipped) } if (buffer != null) processResponseBody(response, buffer, transaction) transaction.responsePayloadSize = totalBytesRead collector.onResponseReceived(transaction) - file.delete() + file?.delete() } - override fun onFailure(file: File, exception: IOException) = exception.printStackTrace() + override fun onFailure(file: File?, exception: IOException) = exception.printStackTrace() - private fun readResponseBuffer(responseBody: File, isGzipped: Boolean): Buffer { + private fun readResponseBuffer(responseBody: File, isGzipped: Boolean) = try { val bufferedSource = Okio.buffer(Okio.source(responseBody)) val source = if (isGzipped) { GzipSource(bufferedSource) } else { bufferedSource } - return Buffer().apply { source.use { writeAll(it) } } + Buffer().apply { source.use { writeAll(it) } } + } catch (e: IOException) { + null } } diff --git a/library/src/main/java/com/chuckerteam/chucker/internal/support/AndroidCacheFileFactory.kt b/library/src/main/java/com/chuckerteam/chucker/internal/support/AndroidCacheFileFactory.kt deleted file mode 100644 index 1ab15dc8f..000000000 --- a/library/src/main/java/com/chuckerteam/chucker/internal/support/AndroidCacheFileFactory.kt +++ /dev/null @@ -1,23 +0,0 @@ -package com.chuckerteam.chucker.internal.support - -import android.content.Context -import java.io.File -import java.util.concurrent.atomic.AtomicLong - -internal const val EXPORT_FILENAME = "transactions.txt" - -internal class AndroidCacheFileFactory( - context: Context -) : FileFactory { - private val fileDir = context.cacheDir - private val uniqueIdGenerator = AtomicLong() - - override fun create() = create(filename = "chucker-${uniqueIdGenerator.getAndIncrement()}") - - override fun create(filename: String): File = File(fileDir, filename).apply { - if (exists()) { - delete() - } - createNewFile() - } -} diff --git a/library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt b/library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt index abf80ad62..384050a34 100644 --- a/library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt +++ b/library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt @@ -1,8 +1,29 @@ package com.chuckerteam.chucker.internal.support import java.io.File +import java.io.FileNotFoundException +import java.io.IOException +import java.util.concurrent.atomic.AtomicLong -internal interface FileFactory { - fun create(): File - fun create(filename: String): File +internal class FileFactory( + private val getFileDirectory: () -> File? +) { + private val uniqueIdGenerator = AtomicLong() + + fun create() = create(filename = "chucker-${uniqueIdGenerator.getAndIncrement()}") + + fun create(filename: String): File? = try { + val directory = getFileDirectory() + ?: throw FileNotFoundException("Failed to create directory for temporary Chucker files") + File(directory, filename).apply { + if (exists()) { + delete() + } + parentFile?.mkdirs() + createNewFile() + } + } catch (e: IOException) { + e.printStackTrace() + null + } } diff --git a/library/src/main/java/com/chuckerteam/chucker/internal/support/TeeSource.kt b/library/src/main/java/com/chuckerteam/chucker/internal/support/TeeSource.kt index cd62b6969..81d5da599 100644 --- a/library/src/main/java/com/chuckerteam/chucker/internal/support/TeeSource.kt +++ b/library/src/main/java/com/chuckerteam/chucker/internal/support/TeeSource.kt @@ -18,12 +18,12 @@ import java.io.IOException */ internal class TeeSource( private val upstream: Source, - private val sideChannel: File, + private val sideChannel: File?, private val callback: Callback, private val readBytesLimit: Long = Long.MAX_VALUE ) : Source { private val sideStream = try { - Okio.buffer(Okio.sink(sideChannel)) + sideChannel?.let { Okio.buffer(Okio.sink(it)) } } catch (e: FileNotFoundException) { callSideChannelFailure(IOException("Failed to use file $sideChannel by Chucker", e)) null @@ -102,14 +102,14 @@ internal class TeeSource( * Called when the upstream was closed. All read bytes are copied to the [file]. * This does not mean that the content of the [file] is valid. Only that the client * is done with the reading process. [totalBytesRead] is the exact amount of data - * that the client downloaded even if the [file] is corrupted. + * that the client downloaded even if the [file] is corrupted or does not exist. */ - fun onClosed(file: File, totalBytesRead: Long) + fun onClosed(file: File?, totalBytesRead: Long) /** * Called when an exception was thrown while reading bytes from the upstream * or when writing to a side channel file fails. Any read bytes are available in a [file]. */ - fun onFailure(file: File, exception: IOException) + fun onFailure(file: File?, exception: IOException) } } diff --git a/library/src/main/java/com/chuckerteam/chucker/internal/ui/MainViewModel.kt b/library/src/main/java/com/chuckerteam/chucker/internal/ui/MainViewModel.kt index 02209c7b1..665f2cc0d 100644 --- a/library/src/main/java/com/chuckerteam/chucker/internal/ui/MainViewModel.kt +++ b/library/src/main/java/com/chuckerteam/chucker/internal/ui/MainViewModel.kt @@ -10,13 +10,13 @@ import com.chuckerteam.chucker.internal.data.entity.HttpTransaction import com.chuckerteam.chucker.internal.data.entity.HttpTransactionTuple import com.chuckerteam.chucker.internal.data.entity.RecordedThrowableTuple import com.chuckerteam.chucker.internal.data.repository.RepositoryProvider -import com.chuckerteam.chucker.internal.support.EXPORT_FILENAME import com.chuckerteam.chucker.internal.support.FileFactory import com.chuckerteam.chucker.internal.support.NotificationHelper import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.launch import kotlinx.coroutines.withContext -import java.io.File + +internal const val EXPORT_FILENAME = "transactions.txt" internal class MainViewModel : ViewModel() { @@ -43,10 +43,8 @@ internal class MainViewModel : ViewModel() { suspend fun getAllTransactions(): List? = RepositoryProvider.transaction().getAllTransactions() - suspend fun createExportFile(content: String, fileFactory: FileFactory): File = withContext(Dispatchers.IO) { - val file = fileFactory.create(EXPORT_FILENAME) - file.writeText(content) - return@withContext file + suspend fun createExportFile(content: String, fileFactory: FileFactory) = withContext(Dispatchers.IO) { + fileFactory.create(EXPORT_FILENAME)?.apply { writeText(content) } } fun updateItemsFilter(searchQuery: String) { diff --git a/library/src/main/java/com/chuckerteam/chucker/internal/ui/transaction/TransactionListFragment.kt b/library/src/main/java/com/chuckerteam/chucker/internal/ui/transaction/TransactionListFragment.kt index 65971641a..d0a603662 100644 --- a/library/src/main/java/com/chuckerteam/chucker/internal/ui/transaction/TransactionListFragment.kt +++ b/library/src/main/java/com/chuckerteam/chucker/internal/ui/transaction/TransactionListFragment.kt @@ -23,7 +23,6 @@ import androidx.recyclerview.widget.DividerItemDecoration import com.chuckerteam.chucker.R import com.chuckerteam.chucker.databinding.ChuckerFragmentTransactionListBinding import com.chuckerteam.chucker.internal.data.model.DialogData -import com.chuckerteam.chucker.internal.support.AndroidCacheFileFactory import com.chuckerteam.chucker.internal.support.FileFactory import com.chuckerteam.chucker.internal.support.ShareUtils import com.chuckerteam.chucker.internal.support.showDialog @@ -40,7 +39,7 @@ internal class TransactionListFragment : private lateinit var transactionsBinding: ChuckerFragmentTransactionListBinding private lateinit var transactionsAdapter: TransactionAdapter private val cacheFileFactory: FileFactory by lazy { - AndroidCacheFileFactory(requireContext()) + FileFactory { requireContext().cacheDir } } override fun onCreate(savedInstanceState: Bundle?) { @@ -138,8 +137,12 @@ internal class TransactionListFragment : if (transactions.isNullOrEmpty()) { Toast.makeText(requireContext(), R.string.chucker_export_empty_text, Toast.LENGTH_SHORT).show() } else { - val filecontent = ShareUtils.getStringFromTransactions(transactions, requireContext()) - val file = viewModel.createExportFile(filecontent, cacheFileFactory) + val fileContent = ShareUtils.getStringFromTransactions(transactions, requireContext()) + val file = viewModel.createExportFile(fileContent, cacheFileFactory) + if (file == null) { + Toast.makeText(requireContext(), R.string.chucker_export_no_file, Toast.LENGTH_SHORT).show() + return@launch + } val uri = FileProvider.getUriForFile( requireContext(), "${requireContext().packageName}.com.chuckerteam.chucker.provider", diff --git a/library/src/main/res/values/strings.xml b/library/src/main/res/values/strings.xml index f7169236d..7938c1709 100644 --- a/library/src/main/res/values/strings.xml +++ b/library/src/main/res/values/strings.xml @@ -26,6 +26,7 @@ Share Export Nothing to export + Failed to create file for export Share as text Share as curl command Save body to file diff --git a/library/src/test/java/com/chuckerteam/chucker/TestUtils.kt b/library/src/test/java/com/chuckerteam/chucker/TestUtils.kt index 562842484..258be839c 100644 --- a/library/src/test/java/com/chuckerteam/chucker/TestUtils.kt +++ b/library/src/test/java/com/chuckerteam/chucker/TestUtils.kt @@ -15,9 +15,9 @@ fun getResourceFile(file: String): Buffer { } } -fun Response.readByteStringBody(): ByteString? { +fun Response.readByteStringBody(length: Long = Long.MAX_VALUE): ByteString? { return if (hasBody()) { - body()?.source()?.use { it.readByteString() } + body()?.source()?.use { it.readByteString(length) } } else { null } diff --git a/library/src/test/java/com/chuckerteam/chucker/api/ChuckerInterceptorTest.kt b/library/src/test/java/com/chuckerteam/chucker/api/ChuckerInterceptorTest.kt index 036769fce..538d139d9 100644 --- a/library/src/test/java/com/chuckerteam/chucker/api/ChuckerInterceptorTest.kt +++ b/library/src/test/java/com/chuckerteam/chucker/api/ChuckerInterceptorTest.kt @@ -48,7 +48,7 @@ class ChuckerInterceptorTest { @BeforeEach fun setUp(@TempDir tempDir: File) { - chuckerInterceptor = ChuckerInterceptorDelegate(TestFileFactory(tempDir)) + chuckerInterceptor = ChuckerInterceptorDelegate(FileFactory { tempDir }) } @ParameterizedTest @@ -75,7 +75,7 @@ class ChuckerInterceptorTest { val expectedBody = image.snapshot() val client = factory.create(chuckerInterceptor) - val responseBody = client.newCall(request).execute().body()!!.source().readByteString() + val responseBody = client.newCall(request).execute().readByteStringBody()!! assertThat(responseBody).isEqualTo(expectedBody) } @@ -109,7 +109,7 @@ class ChuckerInterceptorTest { val request = Request.Builder().url(serverUrl).build() val client = factory.create(chuckerInterceptor) - val responseBody = client.newCall(request).execute().body()!!.source().readByteString() + val responseBody = client.newCall(request).execute().readByteStringBody()!! assertThat(responseBody.utf8()).isEqualTo("Hello, world!") } @@ -204,8 +204,7 @@ class ChuckerInterceptorTest { val request = Request.Builder().url(serverUrl).build() val client = factory.create(chuckerInterceptor) - val source = client.newCall(request).execute().body()!!.source() - source.use { it.readByteString(segmentSize) } + client.newCall(request).execute().readByteStringBody(segmentSize) val transaction = chuckerInterceptor.expectTransaction() // We cannot expect exact amount of data as there are no guarantees that client @@ -227,8 +226,7 @@ class ChuckerInterceptorTest { val request = Request.Builder().url(serverUrl).build() val client = factory.create(chuckerInterceptor) - val source = client.newCall(request).execute().body()!!.source() - source.use { it.readByteString(segmentSize.toLong()) } + client.newCall(request).execute().readByteStringBody(segmentSize.toLong()) val transaction = chuckerInterceptor.expectTransaction() assertThat(transaction.responseHeaders).contains( @@ -251,7 +249,7 @@ class ChuckerInterceptorTest { val request = Request.Builder().url(serverUrl).build() val chuckerInterceptor = ChuckerInterceptorDelegate( - fileFactory = TestFileFactory(tempDir), + fileFactory = FileFactory { tempDir }, maxContentLength = 1_000 ) val client = factory.create(chuckerInterceptor) @@ -271,7 +269,7 @@ class ChuckerInterceptorTest { val request = Request.Builder().url(serverUrl).build() val chuckerInterceptor = ChuckerInterceptorDelegate( - fileFactory = TestFileFactory(tempDir), + fileFactory = FileFactory { tempDir }, maxContentLength = 1_000 ) val client = factory.create(chuckerInterceptor) @@ -284,15 +282,17 @@ class ChuckerInterceptorTest { @ParameterizedTest @EnumSource(value = ClientFactory::class) fun responseReplicationFailure_doesNotAffect_readingByConsumer(factory: ClientFactory, @TempDir tempDir: File) { - assertThat(tempDir.deleteRecursively()).isTrue() - val body = Buffer().apply { writeUtf8("Hello, world!") } server.enqueue(MockResponse().setBody(body)) val request = Request.Builder().url(serverUrl).build() - val chuckerInterceptor = ChuckerInterceptorDelegate(TestFileFactory(tempDir)) + val chuckerInterceptor = ChuckerInterceptorDelegate(FileFactory { tempDir }) val client = factory.create(chuckerInterceptor) - val responseBody = client.newCall(request).execute().body()!!.source().readByteString() + val response = client.newCall(request).execute() + + assertThat(tempDir.deleteRecursively()).isTrue() + + val responseBody = response.readByteStringBody()!! assertThat(responseBody.utf8()).isEqualTo("Hello, world!") } @@ -300,24 +300,20 @@ class ChuckerInterceptorTest { @ParameterizedTest @EnumSource(value = ClientFactory::class) fun responseReplicationFailure_doesNotInvalidate_ChuckerTransaction(factory: ClientFactory, @TempDir tempDir: File) { - assertThat(tempDir.deleteRecursively()).isTrue() - val body = Buffer().apply { writeUtf8("Hello, world!") } server.enqueue(MockResponse().setBody(body)) val request = Request.Builder().url(serverUrl).build() - val chuckerInterceptor = ChuckerInterceptorDelegate(TestFileFactory(tempDir)) + val chuckerInterceptor = ChuckerInterceptorDelegate(FileFactory { tempDir }) val client = factory.create(chuckerInterceptor) - client.newCall(request).execute().readByteStringBody() + val response = client.newCall(request).execute() + + assertThat(tempDir.deleteRecursively()).isTrue() + + response.readByteStringBody() val transaction = chuckerInterceptor.expectTransaction() assertThat(transaction.responseBody).isNull() assertThat(transaction.responsePayloadSize).isEqualTo(body.size()) } - - private class TestFileFactory(private val tempDir: File) : FileFactory { - override fun create() = create("testFile") - - override fun create(filename: String) = File(tempDir, "testFile") - } } diff --git a/library/src/test/java/com/chuckerteam/chucker/internal/support/TeeSourceTest.kt b/library/src/test/java/com/chuckerteam/chucker/internal/support/TeeSourceTest.kt index edc2c845d..28438be38 100644 --- a/library/src/test/java/com/chuckerteam/chucker/internal/support/TeeSourceTest.kt +++ b/library/src/test/java/com/chuckerteam/chucker/internal/support/TeeSourceTest.kt @@ -190,12 +190,12 @@ class TeeSourceTest { var isSuccess = false private set - override fun onClosed(file: File, totalBytesRead: Long) { + override fun onClosed(file: File?, totalBytesRead: Long) { isSuccess = true this.file = file } - override fun onFailure(file: File, exception: IOException) { + override fun onFailure(file: File?, exception: IOException) { this.exception = exception this.file = file } From b995ab3531501859e687eea238703aa0f227b659 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Sikora?= Date: Mon, 17 Aug 2020 18:33:26 +0200 Subject: [PATCH 02/14] Add a test for file factory --- .../internal/support/FileFactoryTest.kt | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 library/src/test/java/com/chuckerteam/chucker/internal/support/FileFactoryTest.kt diff --git a/library/src/test/java/com/chuckerteam/chucker/internal/support/FileFactoryTest.kt b/library/src/test/java/com/chuckerteam/chucker/internal/support/FileFactoryTest.kt new file mode 100644 index 000000000..8c13c388a --- /dev/null +++ b/library/src/test/java/com/chuckerteam/chucker/internal/support/FileFactoryTest.kt @@ -0,0 +1,18 @@ +package com.chuckerteam.chucker.internal.support + +import com.google.common.truth.Truth.assertThat +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.io.TempDir +import java.io.File + +internal class FileFactoryTest { + @TempDir lateinit var tempDir: File + + @Test + fun fileIsCreated_evenIfParentDirectory_isDeleted() { + val fileFactory = FileFactory { tempDir } + + assertThat(tempDir.deleteRecursively()).isTrue() + assertThat(fileFactory.create()!!.isFile).isTrue() + } +} From 253c705ca8586bfb990911462ef51ce19362aa16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Sikora?= Date: Wed, 19 Aug 2020 21:36:50 +0200 Subject: [PATCH 03/14] Replace '?.let' functions with 'if' expressions --- .../com/chuckerteam/chucker/api/ChuckerInterceptor.kt | 8 ++++++-- .../com/chuckerteam/chucker/internal/support/TeeSource.kt | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt b/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt index 756a822d9..02bac408e 100755 --- a/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt +++ b/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt @@ -234,8 +234,12 @@ class ChuckerInterceptor internal constructor( ) : TeeSource.Callback { override fun onClosed(file: File?, totalBytesRead: Long) { - val buffer = file?.let { readResponseBuffer(it, response.isGzipped) } - if (buffer != null) processResponseBody(response, buffer, transaction) + if (file != null) { + val buffer = readResponseBuffer(file, response.isGzipped) + if (buffer != null) { + processResponseBody(response, buffer, transaction) + } + } transaction.responsePayloadSize = totalBytesRead collector.onResponseReceived(transaction) file?.delete() diff --git a/library/src/main/java/com/chuckerteam/chucker/internal/support/TeeSource.kt b/library/src/main/java/com/chuckerteam/chucker/internal/support/TeeSource.kt index 81d5da599..357edef2b 100644 --- a/library/src/main/java/com/chuckerteam/chucker/internal/support/TeeSource.kt +++ b/library/src/main/java/com/chuckerteam/chucker/internal/support/TeeSource.kt @@ -23,7 +23,7 @@ internal class TeeSource( private val readBytesLimit: Long = Long.MAX_VALUE ) : Source { private val sideStream = try { - sideChannel?.let { Okio.buffer(Okio.sink(it)) } + if (sideChannel != null) Okio.buffer(Okio.sink(sideChannel)) else null } catch (e: FileNotFoundException) { callSideChannelFailure(IOException("Failed to use file $sideChannel by Chucker", e)) null From 6fda82bf65da11b49c11e02aaf5acc828219823f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Sikora?= Date: Thu, 20 Aug 2020 14:41:54 +0200 Subject: [PATCH 04/14] Fix attempt to reading too many bytes --- .../src/test/java/com/chuckerteam/chucker/TestUtils.kt | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/library/src/test/java/com/chuckerteam/chucker/TestUtils.kt b/library/src/test/java/com/chuckerteam/chucker/TestUtils.kt index 258be839c..b647343b2 100644 --- a/library/src/test/java/com/chuckerteam/chucker/TestUtils.kt +++ b/library/src/test/java/com/chuckerteam/chucker/TestUtils.kt @@ -15,9 +15,15 @@ fun getResourceFile(file: String): Buffer { } } -fun Response.readByteStringBody(length: Long = Long.MAX_VALUE): ByteString? { +fun Response.readByteStringBody(length: Long? = null): ByteString? { return if (hasBody()) { - body()?.source()?.use { it.readByteString(length) } + body()?.source()?.use { source -> + if (length == null) { + source.readByteString() + } else { + source.readByteString(length) + } + } } else { null } From 3da949b194e564c9ba42f216d8d28bfc5f674ceb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Sikora?= Date: Thu, 20 Aug 2020 14:08:10 +0200 Subject: [PATCH 05/14] Decouple copying bytes from reporting them --- .../chucker/api/ChuckerInterceptor.kt | 11 +- .../chucker/internal/support/ReportingSink.kt | 96 ++++++++++ .../chucker/internal/support/TeeSource.kt | 92 ++------- .../internal/support/ReportingSinkTest.kt | 113 ++++++++++++ .../chucker/internal/support/TeeSourceTest.kt | 174 +++++------------- 5 files changed, 277 insertions(+), 209 deletions(-) create mode 100644 library/src/main/java/com/chuckerteam/chucker/internal/support/ReportingSink.kt create mode 100644 library/src/test/java/com/chuckerteam/chucker/internal/support/ReportingSinkTest.kt diff --git a/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt b/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt index 02bac408e..6f0d08f79 100755 --- a/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt +++ b/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt @@ -4,6 +4,7 @@ import android.content.Context import com.chuckerteam.chucker.internal.data.entity.HttpTransaction import com.chuckerteam.chucker.internal.support.FileFactory import com.chuckerteam.chucker.internal.support.IOUtils +import com.chuckerteam.chucker.internal.support.ReportingSink import com.chuckerteam.chucker.internal.support.TeeSource import com.chuckerteam.chucker.internal.support.contentType import com.chuckerteam.chucker.internal.support.hasBody @@ -178,12 +179,12 @@ class ChuckerInterceptor internal constructor( val contentType = responseBody.contentType() val contentLength = responseBody.contentLength() - val teeSource = TeeSource( - responseBody.source(), + val reportingSink = ReportingSink( fileFactory.create(), ChuckerTransactionTeeCallback(response, transaction), maxContentLength ) + val teeSource = TeeSource(responseBody.source(), reportingSink) return response.newBuilder() .body(ResponseBody.create(contentType, contentLength, Okio.buffer(teeSource))) @@ -231,16 +232,16 @@ class ChuckerInterceptor internal constructor( private inner class ChuckerTransactionTeeCallback( private val response: Response, private val transaction: HttpTransaction - ) : TeeSource.Callback { + ) : ReportingSink.Callback { - override fun onClosed(file: File?, totalBytesRead: Long) { + override fun onClosed(file: File?, writtenBytesCount: Long) { if (file != null) { val buffer = readResponseBuffer(file, response.isGzipped) if (buffer != null) { processResponseBody(response, buffer, transaction) } } - transaction.responsePayloadSize = totalBytesRead + transaction.responsePayloadSize = writtenBytesCount collector.onResponseReceived(transaction) file?.delete() } diff --git a/library/src/main/java/com/chuckerteam/chucker/internal/support/ReportingSink.kt b/library/src/main/java/com/chuckerteam/chucker/internal/support/ReportingSink.kt new file mode 100644 index 000000000..21152508b --- /dev/null +++ b/library/src/main/java/com/chuckerteam/chucker/internal/support/ReportingSink.kt @@ -0,0 +1,96 @@ +package com.chuckerteam.chucker.internal.support + +import okio.Buffer +import okio.Okio +import okio.Sink +import okio.Timeout +import java.io.File +import java.io.IOException + +/** + * A sink that reports result of writing to it via [callback]. + * + * It takes an input [downstreamFile] and writes to it bytes from a source. Amount of bytes + * that is copied can be limited with [writeBytesLimit]. Results are reported back to a client + * when this is is closed or when an exception occurs while creating a downstream sink or while + * writing bytes. + */ +internal class ReportingSink( + private val downstreamFile: File?, + private val callback: Callback, + private val writeBytesLimit: Long = Long.MAX_VALUE +) : Sink { + private var totalByteCount = 0L + private var isFailure = false + private var isClosed = false + private var downstream = try { + if (downstreamFile != null) Okio.sink(downstreamFile) else null + } catch (e: IOException) { + callDownstreamFailure(IOException("Failed to use file $downstreamFile by Chucker", e)) + null + } + + override fun write(source: Buffer, byteCount: Long) { + val previousTotalByteCount = totalByteCount + totalByteCount += byteCount + if (previousTotalByteCount >= writeBytesLimit) return + + val bytesToWrite = if (previousTotalByteCount + byteCount <= writeBytesLimit) { + byteCount + } else { + writeBytesLimit - previousTotalByteCount + } + + if (bytesToWrite == 0L || isFailure) return + + downstream?.write(source, bytesToWrite) + } + + override fun flush() { + if (isFailure) return + try { + downstream?.flush() + } catch (e: IOException) { + callDownstreamFailure(e) + } + } + + override fun close() { + if (isClosed) return + isClosed = true + safeCloseDownstream() + callback.onClosed(downstreamFile, totalByteCount) + } + + override fun timeout(): Timeout = downstream?.timeout() ?: Timeout.NONE + + private fun callDownstreamFailure(exception: IOException) { + if (!isFailure) { + isFailure = true + safeCloseDownstream() + callback.onFailure(downstreamFile, exception) + } + } + + private fun safeCloseDownstream() = try { + downstream?.close() + } catch (e: IOException) { + callDownstreamFailure(e) + } + + interface Callback { + /** + * Called when the sink is closed. All written bytes are copied to the [file]. + * This does not mean that the content of the [file] is valid. Only that the client + * is done with the writing process. [writtenBytesCount] is the exact amount of data + * that the client wrote even if the [file] is corrupted or does not exist. + */ + fun onClosed(file: File?, writtenBytesCount: Long) + + /** + * Called when an [exception] was thrown while processing data. + * Any written bytes are available in a [file]. + */ + fun onFailure(file: File?, exception: IOException) + } +} diff --git a/library/src/main/java/com/chuckerteam/chucker/internal/support/TeeSource.kt b/library/src/main/java/com/chuckerteam/chucker/internal/support/TeeSource.kt index 357edef2b..a7762156c 100644 --- a/library/src/main/java/com/chuckerteam/chucker/internal/support/TeeSource.kt +++ b/library/src/main/java/com/chuckerteam/chucker/internal/support/TeeSource.kt @@ -1,11 +1,9 @@ package com.chuckerteam.chucker.internal.support import okio.Buffer -import okio.Okio +import okio.Sink import okio.Source import okio.Timeout -import java.io.File -import java.io.FileNotFoundException import java.io.IOException /** @@ -13,103 +11,51 @@ import java.io.IOException * * It takes the input [upstream] and reads from it serving the bytes to the end consumer * like a regular [Source]. While bytes are read from the [upstream] the are also copied - * to a [sideChannel] file. After the [upstream] is depleted or when a failure occurs - * an appropriate [callback] method is called. + * to a [sideStream]. */ internal class TeeSource( private val upstream: Source, - private val sideChannel: File?, - private val callback: Callback, - private val readBytesLimit: Long = Long.MAX_VALUE + private val sideStream: Sink ) : Source { - private val sideStream = try { - if (sideChannel != null) Okio.buffer(Okio.sink(sideChannel)) else null - } catch (e: FileNotFoundException) { - callSideChannelFailure(IOException("Failed to use file $sideChannel by Chucker", e)) - null - } - private var totalBytesRead = 0L + private val tempBuffer = Buffer() private var isFailure = false - private var isClosed = false override fun read(sink: Buffer, byteCount: Long): Long { - val bytesRead = try { - upstream.read(sink, byteCount) - } catch (e: IOException) { - callSideChannelFailure(e) - throw e - } + val bytesRead = upstream.read(sink, byteCount) if (bytesRead == -1L) { - sideStream?.close() + safeCloseSideStream() return -1L } - val previousTotalByteRead = totalBytesRead - totalBytesRead += bytesRead - if (sideStream == null) return bytesRead - - if (previousTotalByteRead >= readBytesLimit) { - sideStream.close() - return bytesRead - } - if (!isFailure) { - copyBytesToFile(sink, bytesRead) + copyBytesToSideStream(sink, bytesRead) } return bytesRead } - private fun copyBytesToFile(sink: Buffer, bytesRead: Long) { - if (sideStream == null) return - - val byteCountToCopy = if (totalBytesRead <= readBytesLimit) { - bytesRead - } else { - bytesRead - (totalBytesRead - readBytesLimit) - } + private fun copyBytesToSideStream(sink: Buffer, bytesRead: Long) { val offset = sink.size() - bytesRead - sink.copyTo(sideStream.buffer(), offset, byteCountToCopy) + sink.copyTo(tempBuffer, offset, bytesRead) try { - sideStream.emitCompleteSegments() - } catch (e: IOException) { - callSideChannelFailure(e) + sideStream.write(tempBuffer, bytesRead) + } catch (_: IOException) { + isFailure = true + safeCloseSideStream() } } override fun close() { - sideStream?.close() + safeCloseSideStream() upstream.close() - if (!isClosed) { - isClosed = true - callback.onClosed(sideChannel, totalBytesRead) - } } - override fun timeout(): Timeout = upstream.timeout() - - private fun callSideChannelFailure(exception: IOException) { - if (!isFailure) { - isFailure = true - sideStream?.close() - callback.onFailure(sideChannel, exception) - } + private fun safeCloseSideStream() = try { + sideStream.close() + } catch (_: IOException) { + isFailure = true } - interface Callback { - /** - * Called when the upstream was closed. All read bytes are copied to the [file]. - * This does not mean that the content of the [file] is valid. Only that the client - * is done with the reading process. [totalBytesRead] is the exact amount of data - * that the client downloaded even if the [file] is corrupted or does not exist. - */ - fun onClosed(file: File?, totalBytesRead: Long) - - /** - * Called when an exception was thrown while reading bytes from the upstream - * or when writing to a side channel file fails. Any read bytes are available in a [file]. - */ - fun onFailure(file: File?, exception: IOException) - } + override fun timeout(): Timeout = upstream.timeout() } diff --git a/library/src/test/java/com/chuckerteam/chucker/internal/support/ReportingSinkTest.kt b/library/src/test/java/com/chuckerteam/chucker/internal/support/ReportingSinkTest.kt new file mode 100644 index 000000000..bf1cbbccb --- /dev/null +++ b/library/src/test/java/com/chuckerteam/chucker/internal/support/ReportingSinkTest.kt @@ -0,0 +1,113 @@ +package com.chuckerteam.chucker.internal.support + +import com.google.common.truth.Truth.assertThat +import okio.Buffer +import okio.ByteString +import okio.Okio +import okio.Source +import okio.Timeout +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.io.TempDir +import java.io.File +import java.io.FileNotFoundException +import java.io.IOException +import kotlin.random.Random + +class ReportingSinkTest { + private val reportingCallback = TestReportingCallback() + + @Test + fun bytesWrittenToDownstream_areAvailableForConsumer(@TempDir tempDir: File) { + val testFile = File(tempDir, "testFile") + val testSource = TestSource() + val reportingSink = ReportingSink(testFile, reportingCallback) + + Okio.buffer(reportingSink).use { it.writeAll(testSource) } + + assertThat(reportingCallback.fileContent).isEqualTo(testSource.content) + } + + @Test + fun bytesWrittenToDownstream_canByLimited(@TempDir tempDir: File) { + val testFile = File(tempDir, "testFile") + val testSource = TestSource(10_000) + val reportingSink = ReportingSink(testFile, reportingCallback, writeBytesLimit = 9_999) + + Okio.buffer(reportingSink).use { it.writeAll(testSource) } + + val expectedContent = testSource.content.substring(0, 9_999) + assertThat(reportingCallback.fileContent).isEqualTo(expectedContent) + } + + @Test + fun partiallyConsumedUpstream_hasReadBytesAvailableForConsumer(@TempDir tempDir: File) { + val testFile = File(tempDir, "testFile") + // Okio uses 8KiB as a single size read. + val testSource = TestSource(8_192 * 2) + val reportingSink = ReportingSink(testFile, reportingCallback) + + Okio.buffer(reportingSink).use { it.write(testSource, 8_192) } + + val expectedContent = testSource.content.substring(0, 8_192) + assertThat(reportingCallback.fileContent).isEqualTo(expectedContent) + } + + @Test + fun exactlyWrittenBytesToDownstream_areAvailableForConsumer(@TempDir tempDir: File) { + val testFile = File(tempDir, "testFile") + val repetitions = Random.nextInt(1, 100) + // Okio uses 8KiB as a single size read. + val testSource = TestSource(8_192 * repetitions) + val reportingSink = ReportingSink(testFile, reportingCallback) + + Okio.buffer(reportingSink).use { sink -> + repeat(repetitions) { sink.write(testSource, 8_192) } + } + + assertThat(reportingCallback.fileContent).isEqualTo(testSource.content) + } + + @Test + fun exceptionWhileCreatingDownstream_areAvailableForConsumer(@TempDir tempDir: File) { + assertThat(tempDir.deleteRecursively()).isTrue() + + val testFile = File(tempDir, "testFile") + + ReportingSink(testFile, reportingCallback) + + assertThat(reportingCallback.exception).apply { + isInstanceOf(IOException::class.java) + hasMessageThat().isEqualTo("Failed to use file $testFile by Chucker") + hasCauseThat().isInstanceOf(FileNotFoundException::class.java) + } + } + + private class TestSource(contentLength: Int = 1_000) : Source { + val content: ByteString = ByteString.of(*Random.nextBytes(contentLength)) + private val buffer = Buffer().apply { write(content) } + + override fun read(sink: Buffer, byteCount: Long): Long = buffer.read(sink, byteCount) + + override fun close() = buffer.close() + + override fun timeout(): Timeout = buffer.timeout() + } + + private class TestReportingCallback : ReportingSink.Callback { + private var file: File? = null + val fileContent get() = file?.let { Okio.buffer(Okio.source(it)).readByteString() } + var exception: IOException? = null + var isSuccess = false + private set + + override fun onClosed(file: File?, writtenBytesCount: Long) { + isSuccess = true + this.file = file + } + + override fun onFailure(file: File?, exception: IOException) { + this.exception = exception + this.file = file + } + } +} diff --git a/library/src/test/java/com/chuckerteam/chucker/internal/support/TeeSourceTest.kt b/library/src/test/java/com/chuckerteam/chucker/internal/support/TeeSourceTest.kt index 28438be38..83806eb6a 100644 --- a/library/src/test/java/com/chuckerteam/chucker/internal/support/TeeSourceTest.kt +++ b/library/src/test/java/com/chuckerteam/chucker/internal/support/TeeSourceTest.kt @@ -2,164 +2,84 @@ package com.chuckerteam.chucker.internal.support import com.google.common.truth.Truth.assertThat import okio.Buffer +import okio.BufferedSource import okio.ByteString import okio.Okio +import okio.Sink import okio.Source import okio.Timeout import org.junit.jupiter.api.Test -import org.junit.jupiter.api.assertThrows -import org.junit.jupiter.api.io.TempDir -import java.io.File -import java.io.FileNotFoundException import java.io.IOException import kotlin.random.Random class TeeSourceTest { - private val teeCallback = TestTeeCallback() - @Test - fun bytesReadFromUpstream_areAvailableDownstream(@TempDir tempDir: File) { - val testFile = File(tempDir, "testFile") + fun bytesReadFromUpstream_areAvailableDownstream() { val testSource = TestSource() - val downstream = Buffer() - val teeSource = TeeSource(testSource, testFile, teeCallback) - Okio.buffer(teeSource).use { it.readAll(downstream) } + val teeSource = TeeSource(testSource, sideStream = Buffer()) + val downstream = Okio.buffer(teeSource).use(BufferedSource::readByteString) - assertThat(downstream.snapshot()).isEqualTo(testSource.content) + assertThat(downstream).isEqualTo(testSource.content) } @Test - fun bytesReadFromUpstream_areAvailableToSideChannel(@TempDir tempDir: File) { - val testFile = File(tempDir, "testFile") + fun bytesReadFromUpstream_areAvailableToSideChannel() { val testSource = TestSource() - val downstream = Buffer() + val sideStream = Buffer() - val teeSource = TeeSource(testSource, testFile, teeCallback) - Okio.buffer(teeSource).use { it.readAll(downstream) } + val teeSource = TeeSource(testSource, sideStream) + Okio.buffer(teeSource).use(BufferedSource::readByteString) - assertThat(teeCallback.fileContent).isEqualTo(testSource.content) + assertThat(sideStream.snapshot()).isEqualTo(testSource.content) } @Test - fun bytesPulledFromUpstream_arePulledToSideChannel_alongTheDownstream(@TempDir tempDir: File) { - val testFile = File(tempDir, "testFile") + fun bytesPulledFromUpstream_arePulledToSideChannel_alongTheDownstream() { val repetitions = Random.nextInt(1, 100) // Okio uses 8KiB as a single size read. val testSource = TestSource(8_192 * repetitions) + val sideStream = Buffer() - val teeSource = TeeSource(testSource, testFile, teeCallback) + val teeSource = TeeSource(testSource, sideStream) Okio.buffer(teeSource).use { source -> repeat(repetitions) { index -> source.readByteString(8_192) val subContent = testSource.content.substring(0, (index + 1) * 8_192) - Okio.buffer(Okio.source(testFile)).use { - assertThat(it.readByteString()).isEqualTo(subContent) - } + assertThat(sideStream.snapshot()).isEqualTo(subContent) } } } @Test - fun tooBigSources_haveReadBytesAvailable_inSideChannel(@TempDir tempDir: File) { - val testFile = File(tempDir, "testFile") - val testSource = TestSource(10_000) - val downstream = Buffer() - - val teeSource = TeeSource(testSource, testFile, teeCallback, readBytesLimit = 9_999) - Okio.buffer(teeSource).use { it.readAll(downstream) } - - val expectedContent = testSource.content.substring(0, 9_999) - assertThat(teeCallback.fileContent).isEqualTo(expectedContent) - } - - @Test - fun tooBigSources_areAvailableDownstream(@TempDir tempDir: File) { - val testFile = File(tempDir, "testFile") - val testSource = TestSource(10_000) - val downstream = Buffer() - - val teeSource = TeeSource(testSource, testFile, teeCallback, readBytesLimit = 9_999) - Okio.buffer(teeSource).use { it.readAll(downstream) } - - assertThat(downstream.snapshot()).isEqualTo(testSource.content) - } - - @Test - fun readException_informsOfFailures_inSideChannel(@TempDir tempDir: File) { - val testFile = File(tempDir, "testFile") - val testSource = ThrowingSource - - val teeSource = TeeSource(testSource, testFile, teeCallback) - - assertThrows { - Okio.buffer(teeSource).use { it.readByte() } - } - - assertThat(teeCallback.exception) - .hasMessageThat() - .isEqualTo("Hello there!") - } - - @Test - fun notConsumedUpstream_hasReadBytesAvailable_inSideChannel(@TempDir tempDir: File) { - val testFile = File(tempDir, "testFile") - // Okio uses 8KiB as a single size read. - val testSource = TestSource(8_192 * 2) - - val teeSource = TeeSource(testSource, testFile, teeCallback) - Okio.buffer(teeSource).use { source -> - source.readByteString(8_192) - } - - val expectedContent = testSource.content.substring(0, 8_192) - assertThat(teeCallback.fileContent).isEqualTo(expectedContent) - } - - @Test - fun partiallyReadBytesFromUpstream_areAvailableToSideChannel(@TempDir tempDir: File) { - val testFile = File(tempDir, "testFile") - // Okio uses 8KiB as a single size read. - val testSource = TestSource(8_192 * 2) + fun sideStreamThatFailsToWrite_doesNotFailDownstream() { + val testSource = TestSource() - val teeSource = TeeSource(testSource, testFile, teeCallback) - Okio.buffer(teeSource).use { source -> - source.readByteString(8_192) - } + val teeSource = TeeSource(testSource, sideStream = ThrowingSink(throwForWrite = true)) + val downstream = Okio.buffer(teeSource).use(BufferedSource::readByteString) - val expectedContent = testSource.content.substring(0, 8_192) - assertThat(teeCallback.fileContent).isEqualTo(expectedContent) + assertThat(downstream).isEqualTo(testSource.content) } @Test - fun exactlyReadBytesFromUpstream_areAvailableToSideChannel(@TempDir tempDir: File) { - val testFile = File(tempDir, "testFile") - val repetitions = Random.nextInt(1, 100) - // Okio uses 8KiB as a single size read. - val testSource = TestSource(8_192 * repetitions) + fun sideStreamThatFailsToFlush_doesNotFailDownstream() { + val testSource = TestSource() - val teeSource = TeeSource(testSource, testFile, teeCallback) - Okio.buffer(teeSource).use { source -> - repeat(repetitions) { source.readByteString(8_192) } - } + val teeSource = TeeSource(testSource, sideStream = ThrowingSink(throwForFlush = true)) + val downstream = Okio.buffer(teeSource).use(BufferedSource::readByteString) - assertThat(teeCallback.fileContent).isEqualTo(testSource.content) + assertThat(downstream).isEqualTo(testSource.content) } @Test - fun exceptionWhileCreatingSideChannel_informsOfFailures_inSideChannel(@TempDir tempDir: File) { - assertThat(tempDir.deleteRecursively()).isTrue() - - val testFile = File(tempDir, "testFile") + fun sideStreamThatFailsToClose_doesNotFailDownstream() { + val testSource = TestSource() - TeeSource(TestSource(), testFile, teeCallback) + val teeSource = TeeSource(testSource, sideStream = ThrowingSink(throwForClose = true)) + val downstream = Okio.buffer(teeSource).use(BufferedSource::readByteString) - assertThat(teeCallback.exception).apply { - isInstanceOf(IOException::class.java) - hasMessageThat().isEqualTo("Failed to use file $testFile by Chucker") - hasCauseThat().isInstanceOf(FileNotFoundException::class.java) - } + assertThat(downstream).isEqualTo(testSource.content) } private class TestSource(contentLength: Int = 1_000) : Source { @@ -173,31 +93,23 @@ class TeeSourceTest { override fun timeout(): Timeout = buffer.timeout() } - private object ThrowingSource : Source { - override fun read(sink: Buffer, byteCount: Long): Long { - throw IOException("Hello there!") + private class ThrowingSink( + private val throwForWrite: Boolean = false, + private val throwForFlush: Boolean = false, + private val throwForClose: Boolean = false, + ) : Sink { + override fun write(source: Buffer, byteCount: Long) { + if (throwForWrite) throw IOException("Hello there!") } - override fun close() = Unit - - override fun timeout(): Timeout = Timeout.NONE - } - - private class TestTeeCallback : TeeSource.Callback { - private var file: File? = null - val fileContent get() = file?.let { Okio.buffer(Okio.source(it)).readByteString() } - var exception: IOException? = null - var isSuccess = false - private set - - override fun onClosed(file: File?, totalBytesRead: Long) { - isSuccess = true - this.file = file + override fun flush() { + if (throwForFlush) throw IOException("Hello there!") } - override fun onFailure(file: File?, exception: IOException) { - this.exception = exception - this.file = file + override fun close() { + if (throwForClose) throw IOException("Hello there!") } + + override fun timeout(): Timeout = Timeout.NONE } } From 5b71ed37ac64271bc7326273bb0e30c5c9b8f3d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Sikora?= Date: Fri, 21 Aug 2020 19:20:41 +0200 Subject: [PATCH 06/14] CR fixes --- .../chucker/api/ChuckerInterceptor.kt | 4 +-- .../chucker/internal/support/FileFactory.kt | 6 ++-- .../chucker/internal/support/ReportingSink.kt | 30 +++++++++++-------- .../internal/support/ReportingSinkTest.kt | 25 ++++------------ 4 files changed, 30 insertions(+), 35 deletions(-) diff --git a/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt b/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt index 6f0d08f79..a68677a0e 100755 --- a/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt +++ b/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt @@ -234,14 +234,14 @@ class ChuckerInterceptor internal constructor( private val transaction: HttpTransaction ) : ReportingSink.Callback { - override fun onClosed(file: File?, writtenBytesCount: Long) { + override fun onClosed(file: File?, sourceByteCount: Long) { if (file != null) { val buffer = readResponseBuffer(file, response.isGzipped) if (buffer != null) { processResponseBody(response, buffer, transaction) } } - transaction.responsePayloadSize = writtenBytesCount + transaction.responsePayloadSize = sourceByteCount collector.onResponseReceived(transaction) file?.delete() } diff --git a/library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt b/library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt index 384050a34..6873c934b 100644 --- a/library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt +++ b/library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt @@ -14,13 +14,15 @@ internal class FileFactory( fun create(filename: String): File? = try { val directory = getFileDirectory() - ?: throw FileNotFoundException("Failed to create directory for temporary Chucker files") + ?: throw FileNotFoundException("Failed to create directory for Chucker files") File(directory, filename).apply { if (exists()) { delete() } parentFile?.mkdirs() - createNewFile() + if (!createNewFile()) { + throw IOException("Failed to create a Chucker file: $this") + } } } catch (e: IOException) { e.printStackTrace() diff --git a/library/src/main/java/com/chuckerteam/chucker/internal/support/ReportingSink.kt b/library/src/main/java/com/chuckerteam/chucker/internal/support/ReportingSink.kt index 21152508b..8fb2d849a 100644 --- a/library/src/main/java/com/chuckerteam/chucker/internal/support/ReportingSink.kt +++ b/library/src/main/java/com/chuckerteam/chucker/internal/support/ReportingSink.kt @@ -10,15 +10,15 @@ import java.io.IOException /** * A sink that reports result of writing to it via [callback]. * - * It takes an input [downstreamFile] and writes to it bytes from a source. Amount of bytes - * that is copied can be limited with [writeBytesLimit]. Results are reported back to a client - * when this is is closed or when an exception occurs while creating a downstream sink or while + * Takes an input [downstreamFile] and writes bytes from a source into this input. Amount of bytes + * to copy can be limited with [writeByteLimit]. Results are reported back to a client + * when sink is closed or when an exception occurs while creating a downstream sink or while * writing bytes. */ internal class ReportingSink( private val downstreamFile: File?, private val callback: Callback, - private val writeBytesLimit: Long = Long.MAX_VALUE + private val writeByteLimit: Long = Long.MAX_VALUE ) : Sink { private var totalByteCount = 0L private var isFailure = false @@ -33,17 +33,21 @@ internal class ReportingSink( override fun write(source: Buffer, byteCount: Long) { val previousTotalByteCount = totalByteCount totalByteCount += byteCount - if (previousTotalByteCount >= writeBytesLimit) return + if (isFailure || previousTotalByteCount >= writeByteLimit) return - val bytesToWrite = if (previousTotalByteCount + byteCount <= writeBytesLimit) { + val bytesToWrite = if (previousTotalByteCount + byteCount <= writeByteLimit) { byteCount } else { - writeBytesLimit - previousTotalByteCount + writeByteLimit - previousTotalByteCount } - if (bytesToWrite == 0L || isFailure) return + if (bytesToWrite == 0L) return - downstream?.write(source, bytesToWrite) + try { + downstream?.write(source, bytesToWrite) + } catch (e: IOException) { + callDownstreamFailure(e) + } } override fun flush() { @@ -82,10 +86,12 @@ internal class ReportingSink( /** * Called when the sink is closed. All written bytes are copied to the [file]. * This does not mean that the content of the [file] is valid. Only that the client - * is done with the writing process. [writtenBytesCount] is the exact amount of data - * that the client wrote even if the [file] is corrupted or does not exist. + * is done with the writing process. + * + * [sourceByteCount] is the exact amount of bytes that the were read from upstream even if + * the [file] is corrupted or does not exist. It is not limited by [writeByteLimit]. */ - fun onClosed(file: File?, writtenBytesCount: Long) + fun onClosed(file: File?, sourceByteCount: Long) /** * Called when an [exception] was thrown while processing data. diff --git a/library/src/test/java/com/chuckerteam/chucker/internal/support/ReportingSinkTest.kt b/library/src/test/java/com/chuckerteam/chucker/internal/support/ReportingSinkTest.kt index bf1cbbccb..fd4af5348 100644 --- a/library/src/test/java/com/chuckerteam/chucker/internal/support/ReportingSinkTest.kt +++ b/library/src/test/java/com/chuckerteam/chucker/internal/support/ReportingSinkTest.kt @@ -19,7 +19,9 @@ class ReportingSinkTest { @Test fun bytesWrittenToDownstream_areAvailableForConsumer(@TempDir tempDir: File) { val testFile = File(tempDir, "testFile") - val testSource = TestSource() + val repetitions = Random.nextInt(1, 100) + // Okio uses 8KiB as a single size read. + val testSource = TestSource(8_192 * repetitions) val reportingSink = ReportingSink(testFile, reportingCallback) Okio.buffer(reportingSink).use { it.writeAll(testSource) } @@ -28,10 +30,10 @@ class ReportingSinkTest { } @Test - fun bytesWrittenToDownstream_canByLimited(@TempDir tempDir: File) { + fun bytesWrittenToDownstream_canBeLimited(@TempDir tempDir: File) { val testFile = File(tempDir, "testFile") val testSource = TestSource(10_000) - val reportingSink = ReportingSink(testFile, reportingCallback, writeBytesLimit = 9_999) + val reportingSink = ReportingSink(testFile, reportingCallback, writeByteLimit = 9_999) Okio.buffer(reportingSink).use { it.writeAll(testSource) } @@ -52,21 +54,6 @@ class ReportingSinkTest { assertThat(reportingCallback.fileContent).isEqualTo(expectedContent) } - @Test - fun exactlyWrittenBytesToDownstream_areAvailableForConsumer(@TempDir tempDir: File) { - val testFile = File(tempDir, "testFile") - val repetitions = Random.nextInt(1, 100) - // Okio uses 8KiB as a single size read. - val testSource = TestSource(8_192 * repetitions) - val reportingSink = ReportingSink(testFile, reportingCallback) - - Okio.buffer(reportingSink).use { sink -> - repeat(repetitions) { sink.write(testSource, 8_192) } - } - - assertThat(reportingCallback.fileContent).isEqualTo(testSource.content) - } - @Test fun exceptionWhileCreatingDownstream_areAvailableForConsumer(@TempDir tempDir: File) { assertThat(tempDir.deleteRecursively()).isTrue() @@ -100,7 +87,7 @@ class ReportingSinkTest { var isSuccess = false private set - override fun onClosed(file: File?, writtenBytesCount: Long) { + override fun onClosed(file: File?, sourceByteCount: Long) { isSuccess = true this.file = file } From b74557f3f4861b152025e5bbbc42be9e7b9ce9be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Sikora?= Date: Sun, 23 Aug 2020 10:59:04 +0200 Subject: [PATCH 07/14] Update library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt Co-authored-by: Nicola Corti --- .../java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt b/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt index a68677a0e..86afd87e1 100755 --- a/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt +++ b/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt @@ -181,7 +181,7 @@ class ChuckerInterceptor internal constructor( val reportingSink = ReportingSink( fileFactory.create(), - ChuckerTransactionTeeCallback(response, transaction), + ChuckerTransactionReportingSinkCallback(response, transaction), maxContentLength ) val teeSource = TeeSource(responseBody.source(), reportingSink) @@ -229,7 +229,7 @@ class ChuckerInterceptor internal constructor( return builder.build() } - private inner class ChuckerTransactionTeeCallback( + private inner class ChuckerTransactionReportingSinkCallback( private val response: Response, private val transaction: HttpTransaction ) : ReportingSink.Callback { From 79209ef811d84c835aa46ff59c7d2c1e5a7c1f08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Sikora?= Date: Sun, 23 Aug 2020 12:12:44 +0200 Subject: [PATCH 08/14] Move file factory from constructor to method argument --- .../chucker/api/ChuckerInterceptor.kt | 29 ++++++++--- .../internal/support/CacheDirectoryFactory.kt | 7 +++ .../chucker/internal/support/FileFactory.kt | 13 ++--- .../chucker/internal/ui/MainViewModel.kt | 8 ++- .../ui/transaction/TransactionListFragment.kt | 52 ++++++++++--------- .../chucker/ChuckerInterceptorDelegate.kt | 8 +-- .../chucker/api/ChuckerInterceptorTest.kt | 11 ++-- .../internal/support/FileFactoryTest.kt | 5 +- 8 files changed, 79 insertions(+), 54 deletions(-) create mode 100644 library/src/main/java/com/chuckerteam/chucker/internal/support/CacheDirectoryFactory.kt diff --git a/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt b/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt index 86afd87e1..061d9d87f 100755 --- a/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt +++ b/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt @@ -2,6 +2,7 @@ package com.chuckerteam.chucker.api import android.content.Context import com.chuckerteam.chucker.internal.data.entity.HttpTransaction +import com.chuckerteam.chucker.internal.support.CacheDirectoryFactory import com.chuckerteam.chucker.internal.support.FileFactory import com.chuckerteam.chucker.internal.support.IOUtils import com.chuckerteam.chucker.internal.support.ReportingSink @@ -30,8 +31,8 @@ import java.nio.charset.Charset * @param maxContentLength The maximum length for request and response content * before their truncation. Warning: setting this value too high may cause unexpected * results. - * @param fileFactory Provider for [File]s where Chucker will save temporary responses before - * processing them. + * @param cacheDirectoryFactory Provider for [File] where Chucker will save temporary responses + * before processing them. * @param headersToRedact a [Set] of headers you want to redact. They will be replaced * with a `**` in the Chucker UI. */ @@ -39,8 +40,8 @@ class ChuckerInterceptor internal constructor( private val context: Context, private val collector: ChuckerCollector = ChuckerCollector(context), private val maxContentLength: Long = 250000L, - private val fileFactory: FileFactory, - headersToRedact: Set = emptySet() + private val cacheDirectoryFactory: CacheDirectoryFactory, + headersToRedact: Set = emptySet(), ) : Interceptor { /** @@ -61,7 +62,13 @@ class ChuckerInterceptor internal constructor( collector: ChuckerCollector = ChuckerCollector(context), maxContentLength: Long = 250000L, headersToRedact: Set = emptySet() - ) : this(context, collector, maxContentLength, FileFactory(context::getCacheDir), headersToRedact) + ) : this( + context = context, + collector = collector, + maxContentLength = maxContentLength, + cacheDirectoryFactory = { context.cacheDir }, + headersToRedact = headersToRedact, + ) private val io: IOUtils = IOUtils(context) private val headersToRedact: MutableSet = headersToRedact.toMutableSet() @@ -180,7 +187,7 @@ class ChuckerInterceptor internal constructor( val contentLength = responseBody.contentLength() val reportingSink = ReportingSink( - fileFactory.create(), + createTempTransactionFile(), ChuckerTransactionReportingSinkCallback(response, transaction), maxContentLength ) @@ -191,6 +198,16 @@ class ChuckerInterceptor internal constructor( .build() } + private fun createTempTransactionFile(): File? { + val cache = cacheDirectoryFactory.create() + return if (cache == null) { + println("Failed to create a cache directory for Chucker transaction file") + null + } else { + FileFactory.create(cache) + } + } + private fun processResponseBody( response: Response, responseBodyBuffer: Buffer, diff --git a/library/src/main/java/com/chuckerteam/chucker/internal/support/CacheDirectoryFactory.kt b/library/src/main/java/com/chuckerteam/chucker/internal/support/CacheDirectoryFactory.kt new file mode 100644 index 000000000..02ca74988 --- /dev/null +++ b/library/src/main/java/com/chuckerteam/chucker/internal/support/CacheDirectoryFactory.kt @@ -0,0 +1,7 @@ +package com.chuckerteam.chucker.internal.support + +import java.io.File + +internal fun interface CacheDirectoryFactory { + fun create(): File? +} diff --git a/library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt b/library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt index 6873c934b..7190a8853 100644 --- a/library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt +++ b/library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt @@ -1,21 +1,16 @@ package com.chuckerteam.chucker.internal.support import java.io.File -import java.io.FileNotFoundException import java.io.IOException import java.util.concurrent.atomic.AtomicLong -internal class FileFactory( - private val getFileDirectory: () -> File? -) { +internal object FileFactory { private val uniqueIdGenerator = AtomicLong() - fun create() = create(filename = "chucker-${uniqueIdGenerator.getAndIncrement()}") + fun create(directory: File) = create(directory, fileName = "chucker-${uniqueIdGenerator.getAndIncrement()}") - fun create(filename: String): File? = try { - val directory = getFileDirectory() - ?: throw FileNotFoundException("Failed to create directory for Chucker files") - File(directory, filename).apply { + fun create(directory: File, fileName: String): File? = try { + File(directory, fileName).apply { if (exists()) { delete() } diff --git a/library/src/main/java/com/chuckerteam/chucker/internal/ui/MainViewModel.kt b/library/src/main/java/com/chuckerteam/chucker/internal/ui/MainViewModel.kt index 665f2cc0d..ffd3748a4 100644 --- a/library/src/main/java/com/chuckerteam/chucker/internal/ui/MainViewModel.kt +++ b/library/src/main/java/com/chuckerteam/chucker/internal/ui/MainViewModel.kt @@ -15,6 +15,7 @@ import com.chuckerteam.chucker.internal.support.NotificationHelper import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.launch import kotlinx.coroutines.withContext +import java.io.File internal const val EXPORT_FILENAME = "transactions.txt" @@ -43,8 +44,11 @@ internal class MainViewModel : ViewModel() { suspend fun getAllTransactions(): List? = RepositoryProvider.transaction().getAllTransactions() - suspend fun createExportFile(content: String, fileFactory: FileFactory) = withContext(Dispatchers.IO) { - fileFactory.create(EXPORT_FILENAME)?.apply { writeText(content) } + suspend fun createExportFile( + content: String, + cacheDirectory: File, + ) = withContext(Dispatchers.IO) { + FileFactory.create(cacheDirectory, EXPORT_FILENAME)?.apply { writeText(content) } } fun updateItemsFilter(searchQuery: String) { diff --git a/library/src/main/java/com/chuckerteam/chucker/internal/ui/transaction/TransactionListFragment.kt b/library/src/main/java/com/chuckerteam/chucker/internal/ui/transaction/TransactionListFragment.kt index d0a603662..af5cfbe5e 100644 --- a/library/src/main/java/com/chuckerteam/chucker/internal/ui/transaction/TransactionListFragment.kt +++ b/library/src/main/java/com/chuckerteam/chucker/internal/ui/transaction/TransactionListFragment.kt @@ -23,7 +23,6 @@ import androidx.recyclerview.widget.DividerItemDecoration import com.chuckerteam.chucker.R import com.chuckerteam.chucker.databinding.ChuckerFragmentTransactionListBinding import com.chuckerteam.chucker.internal.data.model.DialogData -import com.chuckerteam.chucker.internal.support.FileFactory import com.chuckerteam.chucker.internal.support.ShareUtils import com.chuckerteam.chucker.internal.support.showDialog import com.chuckerteam.chucker.internal.ui.MainViewModel @@ -38,9 +37,6 @@ internal class TransactionListFragment : private lateinit var transactionsBinding: ChuckerFragmentTransactionListBinding private lateinit var transactionsAdapter: TransactionAdapter - private val cacheFileFactory: FileFactory by lazy { - FileFactory { requireContext().cacheDir } - } override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) @@ -50,7 +46,7 @@ internal class TransactionListFragment : override fun onCreateView( inflater: LayoutInflater, container: ViewGroup?, - savedInstanceState: Bundle? + savedInstanceState: Bundle?, ): View? { transactionsBinding = ChuckerFragmentTransactionListBinding.inflate(inflater, container, false) @@ -131,26 +127,34 @@ internal class TransactionListFragment : TransactionActivity.start(requireActivity(), transactionId) } - private fun exportTransactions() { - lifecycleScope.launch { - val transactions = viewModel.getAllTransactions() - if (transactions.isNullOrEmpty()) { - Toast.makeText(requireContext(), R.string.chucker_export_empty_text, Toast.LENGTH_SHORT).show() - } else { - val fileContent = ShareUtils.getStringFromTransactions(transactions, requireContext()) - val file = viewModel.createExportFile(fileContent, cacheFileFactory) - if (file == null) { - Toast.makeText(requireContext(), R.string.chucker_export_no_file, Toast.LENGTH_SHORT).show() - return@launch - } - val uri = FileProvider.getUriForFile( - requireContext(), - "${requireContext().packageName}.com.chuckerteam.chucker.provider", - file - ) - shareFile(uri) - } + private fun exportTransactions() = lifecycleScope.launch { + val transactions = viewModel.getAllTransactions() + if (transactions.isNullOrEmpty()) { + Toast.makeText(requireContext(), R.string.chucker_export_empty_text, Toast.LENGTH_SHORT).show() + return@launch + } + + val cache = requireContext().cacheDir + if (cache == null) { + println("Failed to create a cache directory for Chucker file export") + Toast.makeText(requireContext(), R.string.chucker_export_no_file, Toast.LENGTH_SHORT).show() + return@launch } + + val fileContent = ShareUtils.getStringFromTransactions(transactions, requireContext()) + val file = viewModel.createExportFile(fileContent, cache) + if (file == null) { + println("Failed to create an export file for Chucker") + Toast.makeText(requireContext(), R.string.chucker_export_no_file, Toast.LENGTH_SHORT).show() + return@launch + } + + val uri = FileProvider.getUriForFile( + requireContext(), + "${requireContext().packageName}.com.chuckerteam.chucker.provider", + file + ) + shareFile(uri) } private fun shareFile(uri: Uri) { diff --git a/library/src/test/java/com/chuckerteam/chucker/ChuckerInterceptorDelegate.kt b/library/src/test/java/com/chuckerteam/chucker/ChuckerInterceptorDelegate.kt index 30e8c8839..6149fb4c6 100644 --- a/library/src/test/java/com/chuckerteam/chucker/ChuckerInterceptorDelegate.kt +++ b/library/src/test/java/com/chuckerteam/chucker/ChuckerInterceptorDelegate.kt @@ -4,7 +4,7 @@ import android.content.Context import com.chuckerteam.chucker.api.ChuckerCollector import com.chuckerteam.chucker.api.ChuckerInterceptor import com.chuckerteam.chucker.internal.data.entity.HttpTransaction -import com.chuckerteam.chucker.internal.support.FileFactory +import com.chuckerteam.chucker.internal.support.CacheDirectoryFactory import io.mockk.every import io.mockk.mockk import okhttp3.Interceptor @@ -13,9 +13,9 @@ import java.util.concurrent.CopyOnWriteArrayList import java.util.concurrent.atomic.AtomicLong internal class ChuckerInterceptorDelegate( - fileFactory: FileFactory, maxContentLength: Long = 250000L, - headersToRedact: Set = emptySet() + headersToRedact: Set = emptySet(), + cacheDirectoryFactory: CacheDirectoryFactory, ) : Interceptor { private val idGenerator = AtomicLong() private val transactions = CopyOnWriteArrayList() @@ -37,7 +37,7 @@ internal class ChuckerInterceptorDelegate( collector = mockCollector, maxContentLength = maxContentLength, headersToRedact = headersToRedact, - fileFactory = fileFactory + cacheDirectoryFactory = cacheDirectoryFactory ) internal fun expectTransaction(): HttpTransaction { diff --git a/library/src/test/java/com/chuckerteam/chucker/api/ChuckerInterceptorTest.kt b/library/src/test/java/com/chuckerteam/chucker/api/ChuckerInterceptorTest.kt index 538d139d9..3e64089e0 100644 --- a/library/src/test/java/com/chuckerteam/chucker/api/ChuckerInterceptorTest.kt +++ b/library/src/test/java/com/chuckerteam/chucker/api/ChuckerInterceptorTest.kt @@ -2,7 +2,6 @@ package com.chuckerteam.chucker.api import com.chuckerteam.chucker.ChuckerInterceptorDelegate import com.chuckerteam.chucker.getResourceFile -import com.chuckerteam.chucker.internal.support.FileFactory import com.chuckerteam.chucker.readByteStringBody import com.google.common.collect.Range import com.google.common.truth.Truth.assertThat @@ -48,7 +47,7 @@ class ChuckerInterceptorTest { @BeforeEach fun setUp(@TempDir tempDir: File) { - chuckerInterceptor = ChuckerInterceptorDelegate(FileFactory { tempDir }) + chuckerInterceptor = ChuckerInterceptorDelegate { tempDir } } @ParameterizedTest @@ -249,7 +248,7 @@ class ChuckerInterceptorTest { val request = Request.Builder().url(serverUrl).build() val chuckerInterceptor = ChuckerInterceptorDelegate( - fileFactory = FileFactory { tempDir }, + cacheDirectoryFactory = { tempDir }, maxContentLength = 1_000 ) val client = factory.create(chuckerInterceptor) @@ -269,7 +268,7 @@ class ChuckerInterceptorTest { val request = Request.Builder().url(serverUrl).build() val chuckerInterceptor = ChuckerInterceptorDelegate( - fileFactory = FileFactory { tempDir }, + cacheDirectoryFactory = { tempDir }, maxContentLength = 1_000 ) val client = factory.create(chuckerInterceptor) @@ -286,7 +285,7 @@ class ChuckerInterceptorTest { server.enqueue(MockResponse().setBody(body)) val request = Request.Builder().url(serverUrl).build() - val chuckerInterceptor = ChuckerInterceptorDelegate(FileFactory { tempDir }) + val chuckerInterceptor = ChuckerInterceptorDelegate { tempDir } val client = factory.create(chuckerInterceptor) val response = client.newCall(request).execute() @@ -304,7 +303,7 @@ class ChuckerInterceptorTest { server.enqueue(MockResponse().setBody(body)) val request = Request.Builder().url(serverUrl).build() - val chuckerInterceptor = ChuckerInterceptorDelegate(FileFactory { tempDir }) + val chuckerInterceptor = ChuckerInterceptorDelegate { tempDir } val client = factory.create(chuckerInterceptor) val response = client.newCall(request).execute() diff --git a/library/src/test/java/com/chuckerteam/chucker/internal/support/FileFactoryTest.kt b/library/src/test/java/com/chuckerteam/chucker/internal/support/FileFactoryTest.kt index 8c13c388a..74082c100 100644 --- a/library/src/test/java/com/chuckerteam/chucker/internal/support/FileFactoryTest.kt +++ b/library/src/test/java/com/chuckerteam/chucker/internal/support/FileFactoryTest.kt @@ -10,9 +10,8 @@ internal class FileFactoryTest { @Test fun fileIsCreated_evenIfParentDirectory_isDeleted() { - val fileFactory = FileFactory { tempDir } - assertThat(tempDir.deleteRecursively()).isTrue() - assertThat(fileFactory.create()!!.isFile).isTrue() + + assertThat(FileFactory.create(tempDir)!!.isFile).isTrue() } } From f609a2bb18dd209da3323b4a7055c09c16c1c325 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Sikora?= Date: Sun, 23 Aug 2020 12:19:50 +0200 Subject: [PATCH 09/14] Add more details to file creation failures --- .../chuckerteam/chucker/internal/support/FileFactory.kt | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt b/library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt index 7190a8853..5900146fd 100644 --- a/library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt +++ b/library/src/main/java/com/chuckerteam/chucker/internal/support/FileFactory.kt @@ -11,16 +11,16 @@ internal object FileFactory { fun create(directory: File, fileName: String): File? = try { File(directory, fileName).apply { - if (exists()) { - delete() + if (exists() && !delete()) { + throw IOException("Failed to delete file $this") } parentFile?.mkdirs() if (!createNewFile()) { - throw IOException("Failed to create a Chucker file: $this") + throw IOException("File $this already exists") } } } catch (e: IOException) { - e.printStackTrace() + IOException("An error occurred while creating a Chucker file", e).printStackTrace() null } } From 6f130a996a59a12f4d2a46b421772a5a784f64a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Sikora?= Date: Sun, 23 Aug 2020 13:54:41 +0200 Subject: [PATCH 10/14] Log failures to process response body files --- .../main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt | 1 + 1 file changed, 1 insertion(+) diff --git a/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt b/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt index 061d9d87f..2d7230576 100755 --- a/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt +++ b/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt @@ -274,6 +274,7 @@ class ChuckerInterceptor internal constructor( } Buffer().apply { source.use { writeAll(it) } } } catch (e: IOException) { + IOException("Response payload couldn't be processed by Chucker", e).printStackTrace() null } } From 15de70bc6b85916f247f6a9a9d1d5c06d6d5d9fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Sikora?= Date: Mon, 24 Aug 2020 15:38:16 +0200 Subject: [PATCH 11/14] Improve missing cache log messages --- .../main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt | 2 +- .../chucker/internal/ui/transaction/TransactionListFragment.kt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt b/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt index 2d7230576..a2fcab2b8 100755 --- a/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt +++ b/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt @@ -201,7 +201,7 @@ class ChuckerInterceptor internal constructor( private fun createTempTransactionFile(): File? { val cache = cacheDirectoryFactory.create() return if (cache == null) { - println("Failed to create a cache directory for Chucker transaction file") + IOException("Failed to obtain a valid cache directory for Chucker transaction file").printStackTrace() null } else { FileFactory.create(cache) diff --git a/library/src/main/java/com/chuckerteam/chucker/internal/ui/transaction/TransactionListFragment.kt b/library/src/main/java/com/chuckerteam/chucker/internal/ui/transaction/TransactionListFragment.kt index af5cfbe5e..191100bd8 100644 --- a/library/src/main/java/com/chuckerteam/chucker/internal/ui/transaction/TransactionListFragment.kt +++ b/library/src/main/java/com/chuckerteam/chucker/internal/ui/transaction/TransactionListFragment.kt @@ -136,7 +136,7 @@ internal class TransactionListFragment : val cache = requireContext().cacheDir if (cache == null) { - println("Failed to create a cache directory for Chucker file export") + println("Failed to obtain a valid cache directory for Chucker file export") Toast.makeText(requireContext(), R.string.chucker_export_no_file, Toast.LENGTH_SHORT).show() return@launch } From 6909ae6c96475141d9758cbb3f5adf04caeb264e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Sikora?= Date: Mon, 24 Aug 2020 15:41:02 +0200 Subject: [PATCH 12/14] Rename cache factory to cache provider --- .../com/chuckerteam/chucker/api/ChuckerInterceptor.kt | 10 +++++----- ...heDirectoryFactory.kt => CacheDirectoryProvider.kt} | 4 ++-- .../chuckerteam/chucker/ChuckerInterceptorDelegate.kt | 6 +++--- .../chuckerteam/chucker/api/ChuckerInterceptorTest.kt | 4 ++-- 4 files changed, 12 insertions(+), 12 deletions(-) rename library/src/main/java/com/chuckerteam/chucker/internal/support/{CacheDirectoryFactory.kt => CacheDirectoryProvider.kt} (50%) diff --git a/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt b/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt index a2fcab2b8..ff8ca6559 100755 --- a/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt +++ b/library/src/main/java/com/chuckerteam/chucker/api/ChuckerInterceptor.kt @@ -2,7 +2,7 @@ package com.chuckerteam.chucker.api import android.content.Context import com.chuckerteam.chucker.internal.data.entity.HttpTransaction -import com.chuckerteam.chucker.internal.support.CacheDirectoryFactory +import com.chuckerteam.chucker.internal.support.CacheDirectoryProvider import com.chuckerteam.chucker.internal.support.FileFactory import com.chuckerteam.chucker.internal.support.IOUtils import com.chuckerteam.chucker.internal.support.ReportingSink @@ -31,7 +31,7 @@ import java.nio.charset.Charset * @param maxContentLength The maximum length for request and response content * before their truncation. Warning: setting this value too high may cause unexpected * results. - * @param cacheDirectoryFactory Provider for [File] where Chucker will save temporary responses + * @param cacheDirectoryProvider Provider of [File] where Chucker will save temporary responses * before processing them. * @param headersToRedact a [Set] of headers you want to redact. They will be replaced * with a `**` in the Chucker UI. @@ -40,7 +40,7 @@ class ChuckerInterceptor internal constructor( private val context: Context, private val collector: ChuckerCollector = ChuckerCollector(context), private val maxContentLength: Long = 250000L, - private val cacheDirectoryFactory: CacheDirectoryFactory, + private val cacheDirectoryProvider: CacheDirectoryProvider, headersToRedact: Set = emptySet(), ) : Interceptor { @@ -66,7 +66,7 @@ class ChuckerInterceptor internal constructor( context = context, collector = collector, maxContentLength = maxContentLength, - cacheDirectoryFactory = { context.cacheDir }, + cacheDirectoryProvider = { context.cacheDir }, headersToRedact = headersToRedact, ) @@ -199,7 +199,7 @@ class ChuckerInterceptor internal constructor( } private fun createTempTransactionFile(): File? { - val cache = cacheDirectoryFactory.create() + val cache = cacheDirectoryProvider.provide() return if (cache == null) { IOException("Failed to obtain a valid cache directory for Chucker transaction file").printStackTrace() null diff --git a/library/src/main/java/com/chuckerteam/chucker/internal/support/CacheDirectoryFactory.kt b/library/src/main/java/com/chuckerteam/chucker/internal/support/CacheDirectoryProvider.kt similarity index 50% rename from library/src/main/java/com/chuckerteam/chucker/internal/support/CacheDirectoryFactory.kt rename to library/src/main/java/com/chuckerteam/chucker/internal/support/CacheDirectoryProvider.kt index 02ca74988..978ddd6ba 100644 --- a/library/src/main/java/com/chuckerteam/chucker/internal/support/CacheDirectoryFactory.kt +++ b/library/src/main/java/com/chuckerteam/chucker/internal/support/CacheDirectoryProvider.kt @@ -2,6 +2,6 @@ package com.chuckerteam.chucker.internal.support import java.io.File -internal fun interface CacheDirectoryFactory { - fun create(): File? +internal fun interface CacheDirectoryProvider { + fun provide(): File? } diff --git a/library/src/test/java/com/chuckerteam/chucker/ChuckerInterceptorDelegate.kt b/library/src/test/java/com/chuckerteam/chucker/ChuckerInterceptorDelegate.kt index 6149fb4c6..db1872ff5 100644 --- a/library/src/test/java/com/chuckerteam/chucker/ChuckerInterceptorDelegate.kt +++ b/library/src/test/java/com/chuckerteam/chucker/ChuckerInterceptorDelegate.kt @@ -4,7 +4,7 @@ import android.content.Context import com.chuckerteam.chucker.api.ChuckerCollector import com.chuckerteam.chucker.api.ChuckerInterceptor import com.chuckerteam.chucker.internal.data.entity.HttpTransaction -import com.chuckerteam.chucker.internal.support.CacheDirectoryFactory +import com.chuckerteam.chucker.internal.support.CacheDirectoryProvider import io.mockk.every import io.mockk.mockk import okhttp3.Interceptor @@ -15,7 +15,7 @@ import java.util.concurrent.atomic.AtomicLong internal class ChuckerInterceptorDelegate( maxContentLength: Long = 250000L, headersToRedact: Set = emptySet(), - cacheDirectoryFactory: CacheDirectoryFactory, + cacheDirectoryProvider: CacheDirectoryProvider, ) : Interceptor { private val idGenerator = AtomicLong() private val transactions = CopyOnWriteArrayList() @@ -37,7 +37,7 @@ internal class ChuckerInterceptorDelegate( collector = mockCollector, maxContentLength = maxContentLength, headersToRedact = headersToRedact, - cacheDirectoryFactory = cacheDirectoryFactory + cacheDirectoryProvider = cacheDirectoryProvider ) internal fun expectTransaction(): HttpTransaction { diff --git a/library/src/test/java/com/chuckerteam/chucker/api/ChuckerInterceptorTest.kt b/library/src/test/java/com/chuckerteam/chucker/api/ChuckerInterceptorTest.kt index 3e64089e0..c742373d6 100644 --- a/library/src/test/java/com/chuckerteam/chucker/api/ChuckerInterceptorTest.kt +++ b/library/src/test/java/com/chuckerteam/chucker/api/ChuckerInterceptorTest.kt @@ -248,7 +248,7 @@ class ChuckerInterceptorTest { val request = Request.Builder().url(serverUrl).build() val chuckerInterceptor = ChuckerInterceptorDelegate( - cacheDirectoryFactory = { tempDir }, + cacheDirectoryProvider = { tempDir }, maxContentLength = 1_000 ) val client = factory.create(chuckerInterceptor) @@ -268,7 +268,7 @@ class ChuckerInterceptorTest { val request = Request.Builder().url(serverUrl).build() val chuckerInterceptor = ChuckerInterceptorDelegate( - cacheDirectoryFactory = { tempDir }, + cacheDirectoryProvider = { tempDir }, maxContentLength = 1_000 ) val client = factory.create(chuckerInterceptor) From 73d472914bf39364a8eaefb7b7a7be33ecb4e5a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Sikora?= Date: Mon, 24 Aug 2020 15:45:12 +0200 Subject: [PATCH 13/14] Add KDoc for CacheDirectoryProvider --- .../chucker/internal/support/CacheDirectoryProvider.kt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/library/src/main/java/com/chuckerteam/chucker/internal/support/CacheDirectoryProvider.kt b/library/src/main/java/com/chuckerteam/chucker/internal/support/CacheDirectoryProvider.kt index 978ddd6ba..fcc4281ee 100644 --- a/library/src/main/java/com/chuckerteam/chucker/internal/support/CacheDirectoryProvider.kt +++ b/library/src/main/java/com/chuckerteam/chucker/internal/support/CacheDirectoryProvider.kt @@ -2,6 +2,10 @@ package com.chuckerteam.chucker.internal.support import java.io.File +/** + * An interface that returns a reference to a cache directory where temporary files can be + * saved. + */ internal fun interface CacheDirectoryProvider { fun provide(): File? } From f1034e3f8b6b75419f0a79c954502fd06df2481d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Sikora?= Date: Mon, 24 Aug 2020 15:49:31 +0200 Subject: [PATCH 14/14] Add an interceptor test for missing cache --- .../chucker/api/ChuckerInterceptorTest.kt | 36 +++++++++++++++++-- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/library/src/test/java/com/chuckerteam/chucker/api/ChuckerInterceptorTest.kt b/library/src/test/java/com/chuckerteam/chucker/api/ChuckerInterceptorTest.kt index c742373d6..dc67d364c 100644 --- a/library/src/test/java/com/chuckerteam/chucker/api/ChuckerInterceptorTest.kt +++ b/library/src/test/java/com/chuckerteam/chucker/api/ChuckerInterceptorTest.kt @@ -47,7 +47,7 @@ class ChuckerInterceptorTest { @BeforeEach fun setUp(@TempDir tempDir: File) { - chuckerInterceptor = ChuckerInterceptorDelegate { tempDir } + chuckerInterceptor = ChuckerInterceptorDelegate(cacheDirectoryProvider = { tempDir }) } @ParameterizedTest @@ -285,7 +285,7 @@ class ChuckerInterceptorTest { server.enqueue(MockResponse().setBody(body)) val request = Request.Builder().url(serverUrl).build() - val chuckerInterceptor = ChuckerInterceptorDelegate { tempDir } + val chuckerInterceptor = ChuckerInterceptorDelegate(cacheDirectoryProvider = { tempDir }) val client = factory.create(chuckerInterceptor) val response = client.newCall(request).execute() @@ -303,7 +303,7 @@ class ChuckerInterceptorTest { server.enqueue(MockResponse().setBody(body)) val request = Request.Builder().url(serverUrl).build() - val chuckerInterceptor = ChuckerInterceptorDelegate { tempDir } + val chuckerInterceptor = ChuckerInterceptorDelegate(cacheDirectoryProvider = { tempDir }) val client = factory.create(chuckerInterceptor) val response = client.newCall(request).execute() @@ -315,4 +315,34 @@ class ChuckerInterceptorTest { assertThat(transaction.responseBody).isNull() assertThat(transaction.responsePayloadSize).isEqualTo(body.size()) } + + @ParameterizedTest + @EnumSource(value = ClientFactory::class) + fun missingCache_doesNotInvalidate_ChuckerTransaction(factory: ClientFactory) { + val body = Buffer().apply { writeUtf8("Hello, world!") } + server.enqueue(MockResponse().setBody(body)) + val request = Request.Builder().url(serverUrl).build() + + val chuckerInterceptor = ChuckerInterceptorDelegate(cacheDirectoryProvider = { null }) + val client = factory.create(chuckerInterceptor) + client.newCall(request).execute().readByteStringBody() + + val transaction = chuckerInterceptor.expectTransaction() + assertThat(transaction.responseBody).isNull() + assertThat(transaction.responsePayloadSize).isEqualTo(body.size()) + } + + @ParameterizedTest + @EnumSource(value = ClientFactory::class) + fun missingCache_doesNotAffect_endConsumer(factory: ClientFactory) { + val body = Buffer().apply { writeUtf8("Hello, world!") } + server.enqueue(MockResponse().setBody(body)) + val request = Request.Builder().url(serverUrl).build() + + val chuckerInterceptor = ChuckerInterceptorDelegate(cacheDirectoryProvider = { null }) + val client = factory.create(chuckerInterceptor) + val responseBody = client.newCall(request).execute().readByteStringBody() + + assertThat(responseBody!!.utf8()).isEqualTo("Hello, world!") + } }