Improve algorithm to generate a filename from an image download

app/src/androidTest/java/com/duckduckgo/app/browser/downloader/UriUtilsFilenameExtractorTest.kt

@@ -0,0 +1,152 @@
+/*
+ * Copyright (c) 2020 DuckDuckGo
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.duckduckgo.app.browser.downloader
+
+import org.junit.Assert.assertEquals
+import org.junit.Test
+
+class UriUtilsFilenameExtractorTest {
+
+    private val testee: FilenameExtractor = FilenameExtractor()
+
+    @Test
+    fun whenUrlEndsWithFilenameAsJpgNoMimeOrContentDispositionThenFilenameShouldBeExtracted() {
+        val url = "https://example.com/realFilename.jpg"
+        val mimeType: String? = null
+        val contentDisposition: String? = null
+        val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType))
+        assertEquals("realFilename.jpg", extracted)
+    }
+
+    @Test
+    fun whenUrlContainsFilenameButContainsAdditionalPathSegmentsThenFilenameShouldBeExtracted() {
+        val url = "https://foo.example.com/path/images/b/b1/realFilename.jpg/other/stuff"
+        val mimeType: String? = null
+        val contentDisposition: String? = null
+        val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType))
+        assertEquals("realFilename.jpg", extracted)
+    }
+
+    @Test
+    fun whenUrlContainsFilenameButContainsAdditionalPathSegmentsAndQueryParamsThenFilenameShouldBeExtracted() {
+        val url = "https://foo.example.com/path/images/b/b1/realFilename.jpg/other/stuff?cb=123"
+        val mimeType: String? = null
+        val contentDisposition: String? = null
+        val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType))
+        assertEquals("realFilename.jpg", extracted)
+    }
+
+    @Test
+    fun whenUrlContainsFilenameButContainsAdditionalPathSegmentsAndQueryParamsWhichLookLikeAFilenameThenFilenameShouldBeExtracted() {
+        val url = "https://foo.example.com/path/images/b/b1/realFilename.jpg/other/stuff?cb=123.com"
+        val mimeType: String? = null
+        val contentDisposition: String? = null
+        val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType))
+        assertEquals("realFilename.jpg", extracted)
+    }
+
+    @Test
+    fun whenUrlContainsFilenameButContentDispositionSaysOtherwiseThenExtractFromContentDisposition() {
+        val url = "https://example.com/filename.jpg"
+        val mimeType: String? = null
+        val contentDisposition: String? = "Content-Disposition: attachment; filename=fromDisposition.jpg"
+        val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType))
+        assertEquals("fromDisposition.jpg", extracted)
+    }
+
+    @Test
+    fun whenFilenameEndsInBinThenThatIsExtracted() {
+        val url = "https://example.com/realFilename.bin"
+        val mimeType: String? = null
+        val contentDisposition: String? = null
+        val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType))
+        assertEquals("realFilename.bin", extracted)
+    }
+
+    @Test
+    fun whenUrlContainsNoFileNameButLotsOfPathsSegmentsThenFirstSegmentNameIsUsed() {
+        val url = "https://example.com/foo/bar/files"
+        val mimeType: String? = null
+        val contentDisposition: String? = null
+        val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType))
+        assertEquals("foo.bin", extracted)
+    }
+
+    @Test
+    fun whenFilenameEndsInBinWithASlashThenThatIsExtracted() {
+        val url = "https://example.com/realFilename.bin/"
+        val mimeType: String? = null
+        val contentDisposition: String? = null
+        val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType))
+        assertEquals("realFilename.bin", extracted)
+    }
+
+    @Test
+    fun whenFilenameContainsBinThenThatIsExtracted() {
+        val url = "https://example.com/realFilename.bin/foo/bar"
+        val mimeType: String? = null
+        val contentDisposition: String? = null
+        val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType))
+        assertEquals("realFilename.bin", extracted)
+    }
+
+    @Test
+    fun whenUrlIsEmptyStringAndNoOtherDataProvidedThenDefaultNameAndBinFiletypeReturned() {
+        val url = ""
+        val mimeType: String? = null
+        val contentDisposition: String? = null
+        val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType))
+        assertEquals("downloadfile.bin", extracted)
+    }
+
+    @Test
+    fun whenUrlIsEmptyStringAndMimeTypeProvidedThenDefaultNameAndFiletypeFromMimeReturned() {
+        val url = ""
+        val mimeType: String? = "image/jpeg"
+        val contentDisposition: String? = null
+        val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType))
+        assertEquals("downloadfile.jpg", extracted)
+    }
+
+    @Test
+    fun whenUrlIsEmptyStringAndContentDispositionProvidedThenExtractFromContentDisposition() {
+        val url = ""
+        val mimeType: String? = null
+        val contentDisposition: String? = "Content-Disposition: attachment; filename=fromDisposition.jpg"
+        val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType))
+        assertEquals("fromDisposition.jpg", extracted)
+    }
+
+    @Test
+    fun whenNoFilenameAndNoPathSegmentsThenDomainNameReturned() {
+        val url = "http://example.com"
+        val mimeType: String? = null
+        val contentDisposition: String? = null
+        val extracted = testee.extract(buildPendingDownload(url, contentDisposition, mimeType))
+        assertEquals("example.com", extracted)
+    }
+
+    private fun buildPendingDownload(url: String, contentDisposition: String?, mimeType: String?): FileDownloader.PendingFileDownload {
+        return FileDownloader.PendingFileDownload(
+            url = url,
+            contentDisposition = contentDisposition,
+            mimeType = mimeType,
+            subfolder = "aFolder",
+            userAgent = "aUserAgent"
+        )
+    }
+}

app/src/main/java/com/duckduckgo/app/browser/DownloadConfirmationFragment.kt

@@ -22,7 +22,7 @@ import android.view.LayoutInflater
 import android.view.View
 import android.view.ViewGroup
 import com.duckduckgo.app.browser.downloader.FileDownloader.PendingFileDownload
-import com.duckduckgo.app.browser.downloader.guessFileName
+import com.duckduckgo.app.browser.downloader.FilenameExtractor
 import com.duckduckgo.app.browser.downloader.isDataUrl
 import com.duckduckgo.app.global.view.gone
 import com.duckduckgo.app.global.view.leftDrawable
@@ -32,12 +32,16 @@ import dagger.android.support.AndroidSupportInjection
 import kotlinx.android.synthetic.main.download_confirmation.view.*
 import timber.log.Timber
 import java.io.File
+import javax.inject.Inject
 
 class DownloadConfirmationFragment : BottomSheetDialogFragment() {
 
     val listener: DownloadConfirmationDialogListener
         get() = parentFragment as DownloadConfirmationDialogListener
 
+    @Inject
+    lateinit var filenameExtractor: FilenameExtractor
+
     private var file: File? = null
 
     private val pendingDownload: PendingFileDownload by lazy {
@@ -57,7 +61,7 @@ class DownloadConfirmationFragment : BottomSheetDialogFragment() {
     }
 
     private fun setupDownload() {
-        file = if (!pendingDownload.isDataUrl) File(pendingDownload.directory, pendingDownload.guessFileName()) else null
+        file = if (!pendingDownload.isDataUrl) File(pendingDownload.directory, filenameExtractor.extract(pendingDownload)) else null
     }
 
     private fun setupViews(view: View) {

app/src/main/java/com/duckduckgo/app/browser/downloader/FileDownloader.kt

@@ -22,7 +22,6 @@ import android.webkit.URLUtil
 import androidx.annotation.WorkerThread
 import com.duckduckgo.app.browser.downloader.FileDownloader.FileDownloadListener
 import com.duckduckgo.app.browser.downloader.FileDownloader.PendingFileDownload
-import timber.log.Timber
 import java.io.File
 import java.io.Serializable
 import javax.inject.Inject
@@ -67,12 +66,6 @@ class AndroidFileDownloader @Inject constructor(
     }
 }
 
-fun PendingFileDownload.guessFileName(): String {
-    val guessedFileName = URLUtil.guessFileName(url, contentDisposition, mimeType)
-    Timber.i("Guessed filename of $guessedFileName for url $url")
-    return guessedFileName
-}
-
 val PendingFileDownload.isDataUrl get() = URLUtil.isDataUrl(url)
 
 val PendingFileDownload.isNetworkUrl get() = URLUtil.isNetworkUrl(url)

app/src/main/java/com/duckduckgo/app/browser/downloader/FilenameExtractor.kt

@@ -0,0 +1,100 @@
+/*
+ * Copyright (c) 2020 DuckDuckGo
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.duckduckgo.app.browser.downloader
+
+import android.webkit.URLUtil
+import androidx.core.net.toUri
+import com.duckduckgo.app.browser.downloader.FileDownloader.PendingFileDownload
+import com.duckduckgo.app.browser.downloader.FilenameExtractor.GuessQuality.NotGoodEnough
+import com.duckduckgo.app.browser.downloader.FilenameExtractor.GuessQuality.TriedAllOptions
+import timber.log.Timber
+import javax.inject.Inject
+
+class FilenameExtractor @Inject constructor() {
+
+    fun extract(pendingDownload: PendingFileDownload): String {
+        val url = pendingDownload.url
+        val mimeType = pendingDownload.mimeType
+        val contentDisposition = pendingDownload.contentDisposition
+
+        val firstGuess = guessFilename(url, contentDisposition, mimeType)
+        val guesses = Guesses(bestGuess = null, latestGuess = firstGuess)
+        val baseUrl = url.toUri().host ?: ""
+        var pathSegments = pathSegments(url)
+
+        while (evaluateGuessQuality(guesses, pathSegments) != TriedAllOptions) {
+            pathSegments = pathSegments.dropLast(1)
+            guesses.latestGuess = guessFilename(baseUrl + "/" + pathSegments.rebuildUrl(), contentDisposition, mimeType)
+        }
+
+        return bestGuess(guesses)
+    }
+
+    private fun evaluateGuessQuality(guesses: Guesses, pathSegments: List<String>): GuessQuality {
+        val latestGuess = guesses.latestGuess
+
+        // if it contains a '.' then it's a good chance of a filetype and we can update our best guess
+        if (latestGuess.contains(".")) {
+            guesses.bestGuess = latestGuess
+        }
+
+        if (pathSegments.size < 2) return TriedAllOptions
+
+        return NotGoodEnough
+    }
+
+    private fun guessFilename(url: String, contentDisposition: String?, mimeType: String?): String {
+        val tidiedUrl = url.removeSuffix("/")
+        var guessedFilename = URLUtil.guessFileName(tidiedUrl, contentDisposition, mimeType)
+
+        // we only want to keep the default .bin filetype on the guess if the URL actually has that too
+        if (guessedFilename.endsWith(DEFAULT_FILE_TYPE) && !tidiedUrl.endsWith(DEFAULT_FILE_TYPE)) {
+            guessedFilename = guessedFilename.removeSuffix(DEFAULT_FILE_TYPE)
+        }
+
+        Timber.v("From URL [$url], guessed [$guessedFilename]")
+        return guessedFilename
+    }
+
+    private fun bestGuess(guesses: Guesses): String {
+        val guess = guesses.bestGuess ?: guesses.latestGuess
+        if (!guess.contains(".")) {
+            return guess + DEFAULT_FILE_TYPE
+        }
+        return guess
+    }
+
+    private fun pathSegments(url: String): List<String> {
+        return url.toUri().pathSegments ?: emptyList()
+    }
+
+    private fun List<String>.rebuildUrl(): String {
+        return joinToString(separator = "/")
+    }
+
+    companion object {
+        private const val DEFAULT_FILE_TYPE = ".bin"
+    }
+
+    sealed class GuessQuality {
+        object NotGoodEnough : GuessQuality()
+        object TriedAllOptions : GuessQuality()
+    }
+
+    data class Guesses(var latestGuess: String, var bestGuess: String? = null)
+
+}

app/src/main/java/com/duckduckgo/app/browser/downloader/NetworkFileDownloader.kt

@@ -25,7 +25,7 @@ import com.duckduckgo.app.browser.R
 import com.duckduckgo.app.browser.downloader.FileDownloader.PendingFileDownload
 import javax.inject.Inject
 
-class NetworkFileDownloader @Inject constructor(private val context: Context) {
+class NetworkFileDownloader @Inject constructor(private val context: Context, private val filenameExtractor: FilenameExtractor) {
 
     fun download(pendingDownload: PendingFileDownload, callback: FileDownloader.FileDownloadListener) {
 
@@ -34,7 +34,7 @@ class NetworkFileDownloader @Inject constructor(private val context: Context) {
             return
         }
 
-        val guessedFileName = pendingDownload.guessFileName()
+        val guessedFileName = filenameExtractor.extract(pendingDownload)
 
         val request = DownloadManager.Request(pendingDownload.url.toUri()).apply {
             allowScanningByMediaScanner()