For mozilla-mobile#11905 - Filter out Pocket stories with missing ima…

…ge path

components/service/pocket/src/main/java/mozilla/components/service/pocket/api/PocketJSONParser.kt

@@ -12,10 +12,12 @@ import mozilla.components.support.ktx.android.org.json.tryGetString
 import org.json.JSONArray
 import org.json.JSONException
 import org.json.JSONObject
+import kotlin.jvm.Throws
 
 private const val JSON_STORY_TITLE_KEY = "title"
 private const val JSON_STORY_URL_KEY = "url"
-private const val JSON_STORY_IMAGE_URL_KEY = "imageUrl"
+@VisibleForTesting
+internal const val JSON_STORY_IMAGE_URL_KEY = "imageUrl"
 private const val JSON_STORY_PUBLISHER_KEY = "publisher"
 private const val JSON_STORY_CATEGORY_KEY = "category"
 private const val JSON_STORY_TIME_TO_READ_KEY = "timeToRead"
@@ -44,7 +46,7 @@ internal class PocketJSONParser {
             // These three properties are required for any valid recommendation.
             title = json.getString(JSON_STORY_TITLE_KEY),
             url = json.getString(JSON_STORY_URL_KEY),
-            imageUrl = json.getString(JSON_STORY_IMAGE_URL_KEY),
+            imageUrl = getValidImageUrl(json),
             // The following three properties are optional.
             publisher = json.tryGetString(JSON_STORY_PUBLISHER_KEY) ?: STRING_NOT_FOUND_DEFAULT_VALUE,
             category = json.tryGetString(JSON_STORY_CATEGORY_KEY) ?: STRING_NOT_FOUND_DEFAULT_VALUE,
@@ -54,6 +56,30 @@ internal class PocketJSONParser {
         null
     }
 
+    /**
+     * Validate and return the Pocket story imageUrl only if it has an actual image path.
+     * The JSON object representing a Pocket story might contains a url representing the `imageUrl`
+     * but that might not be valid.
+     *
+     * @param [jsonStory] JSON respecting the schema of a Pocket story. Expected to contain [JSON_STORY_IMAGE_URL_KEY].
+     *
+     * @return the full imageUrl from [jsonStory] if it does not end in "/null".
+     *
+     * @throws JSONException if a value for [JSON_STORY_IMAGE_URL_KEY] is not found or ends with "/null".
+     */
+    @Throws(JSONException::class)
+    @VisibleForTesting
+    internal fun getValidImageUrl(jsonStory: JSONObject): String {
+        val imageUrl = jsonStory.getString(JSON_STORY_IMAGE_URL_KEY)
+        val imagePath = imageUrl.substringAfterLast("/")
+
+        if (imagePath == "null") {
+            throw JSONException("Null image path")
+        }
+
+        return imageUrl
+    }
+
     companion object {
         @VisibleForTesting const val KEY_ARRAY_ITEMS = "recommendations"
 

components/service/pocket/src/test/java/mozilla/components/service/pocket/api/PocketJSONParserTest.kt

@@ -8,10 +8,14 @@ import androidx.test.ext.junit.runners.AndroidJUnit4
 import mozilla.components.service.pocket.api.PocketJSONParser.Companion.KEY_ARRAY_ITEMS
 import mozilla.components.service.pocket.helpers.PocketTestResources
 import mozilla.components.service.pocket.helpers.assertClassVisibility
+import mozilla.components.support.ktx.android.org.json.toList
+import org.json.JSONArray
+import org.json.JSONException
 import org.json.JSONObject
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertNotNull
 import org.junit.Assert.assertNull
+import org.junit.Assert.assertSame
 import org.junit.Before
 import org.junit.Test
 import org.junit.runner.RunWith
@@ -82,6 +86,33 @@ class PocketJSONParserTest {
         assertEquals(expectedStoriesIfMissingImageUrl.joinToString(), result.joinToString())
     }
 
+    @Test
+    fun `WHEN parsing stories recommendations with missing image paths THEN those entries are dropped`() {
+        val pocketStoriesWithMissingImagePathJSON = JSONObject(PocketTestResources.pocketEndointFiveStoriesResponse).apply {
+            val storiesWithOneMissingImagePath = JSONArray(
+                this.getJSONArray(KEY_ARRAY_ITEMS).toList<JSONObject>()
+                    .mapIndexed { index, jsonObject ->
+                        if (index == 2) {
+                            jsonObject.put(
+                                JSON_STORY_IMAGE_URL_KEY,
+                                jsonObject.getString(JSON_STORY_IMAGE_URL_KEY) + "/null"
+                            )
+                        } else {
+                            jsonObject
+                        }
+                    }
+            )
+            put(KEY_ARRAY_ITEMS, storiesWithOneMissingImagePath)
+        }
+        val expectedStoriesIfMissingImagePath = ArrayList(PocketTestResources.apiExpectedPocketStoriesRecommendations)
+            .apply { removeAt(2) }
+
+        val result = parser.jsonToPocketApiStories(pocketStoriesWithMissingImagePathJSON.toString())
+
+        assertEquals(4, result!!.size)
+        assertEquals(expectedStoriesIfMissingImagePath.joinToString(), result.joinToString())
+    }
+
     @Test
     fun `WHEN parsing stories recommendations with missing publishers THEN those entries are kept but with default values`() {
         val pocketJSON = PocketTestResources.pocketEndointFiveStoriesResponse
@@ -148,6 +179,24 @@ class PocketJSONParserTest {
     fun `WHEN parsing stories recommendations for an invalid JSON String THEN null is returned`() {
         assertNull(parser.jsonToPocketApiStories("{!!}}"))
     }
+
+    @Test
+    fun `WHEN a valid image path is present in imageUrl THEN return the full imageUrl`() {
+        val storyWithValidImagePath = PocketTestResources.pocketEndpointOneStoryJSONResponse
+
+        val result = parser.getValidImageUrl(storyWithValidImagePath)
+
+        assertSame(storyWithValidImagePath.getString(JSON_STORY_IMAGE_URL_KEY), result)
+    }
+
+    @Test(expected = JSONException::class)
+    fun `WHEN the image path is missing from imageUrl THEN an exception is thrown`() {
+        val storyWithNullImageUrl = PocketTestResources.pocketEndpointOneStoryJSONResponse.also {
+            it.put(JSON_STORY_IMAGE_URL_KEY, it.getString(JSON_STORY_IMAGE_URL_KEY) + "/null")
+        }
+
+        parser.getValidImageUrl(storyWithNullImageUrl)
+    }
 }
 
 private fun removeJsonFieldFromArrayIndex(fieldName: String, indexInArray: Int, json: String): String {

components/service/pocket/src/test/java/mozilla/components/service/pocket/helpers/PocketTestResources.kt

@@ -7,13 +7,27 @@ package mozilla.components.service.pocket.helpers
 import mozilla.components.service.pocket.PocketRecommendedStory
 import mozilla.components.service.pocket.api.PocketApiStory
 import mozilla.components.service.pocket.stories.db.PocketStoryEntity
+import org.json.JSONObject
 
 private const val POCKET_DIR = "pocket"
 
 /**
  * Accessors to resources used in testing.
  */
 internal object PocketTestResources {
+    val pocketEndpointOneStoryJSONResponse = JSONObject(
+        """
+            {
+                "category": "general",
+                "url": "https://getpocket.com/explore/item/how-to-remember-anything-you-really-want-to-remember-backed-by-science",
+                "title": "How to Remember Anything You Really Want to Remember, Backed by Science",
+                "imageUrl": "https://img-getpocket.cdn.mozilla.net/{wh}/filters:format(jpeg):quality(60):no_upscale():strip_exif()/https%3A%2F%2Fpocket-image-cache.com%2F1200x%2Ffilters%3Aformat(jpg)%3Aextract_focal()%2Fhttps%253A%252F%252Fwww.incimages.com%252Fuploaded_files%252Fimage%252F1920x1080%252Fgetty-862457080_394628.jpg",
+                "publisher": "Pocket",
+                "timeToRead": 3
+            }
+        """.trimIndent()
+    )
+
     val pocketEndointFiveStoriesResponse = this::class.java.classLoader!!.getResource(
         "$POCKET_DIR/stories_recommendations_response.json"
     )!!.readText()

docs/changelog.md

@@ -11,6 +11,9 @@ permalink: /changelog/
 * [Gecko](https://github.com/mozilla-mobile/android-components/blob/main/buildSrc/src/main/java/Gecko.kt)
 * [Configuration](https://github.com/mozilla-mobile/android-components/blob/main/.config.yml)
 
+* **service-pocket**
+  * 🚒 Bug fixed [issue #11905](https://github.com/mozilla-mobile/android-components/issues/11905) - Filter out Pocket stories if their imageUrl is invalid.
+
 * **feature-autofill**
   * 🚒 Bug fixed [issue #11869](https://github.com/mozilla-mobile/android-components/issues/11869) - Fix regression causing autofill to not work after unlocking the app doing the autofill or after accepting that the authenticity of the autofill target could not be verified.