Bug 1398368: Drop malformed Pocket Top Stories. r=liuche
authorMichael Comella <michael.l.comella@gmail.com>
Tue, 12 Sep 2017 16:36:21 -0700
changeset 430227 3384e2855508cac183f295dc97b3982314e2b069
parent 430226 0d2d6244f90180c77786a1c5189496eae34e8365
child 430228 26606cf1e39b8bf750807239d5b265de5e0a46d6
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersliuche
bugs1398368
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1398368: Drop malformed Pocket Top Stories. r=liuche I tested this through the unit tests I added. In theory, we could also validate URLs to make sure they're valid but users should see 404s if they're not valid so this seems like unnecessary code. MozReview-Commit-ID: 3XqsMawLabj
mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java
mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topstories/PocketStoriesLoader.java
mobile/android/tests/background/junit4/src/org/mozilla/gecko/activitystream/homepanel/topstories/TestPocketStoriesLoader.java
--- a/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java
+++ b/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java
@@ -50,17 +50,17 @@ public class StreamRecyclerAdapter exten
     private static final String LOGTAG = StringUtils.safeSubstring("Gecko" + StreamRecyclerAdapter.class.getSimpleName(), 0, 23);
 
     private Cursor topSitesCursor;
     private List<RowModel> recyclerViewModel; // List of item types backing this RecyclerView.
     private List<TopStory> topStoriesQueue;
 
     // Content sections available on the Activity Stream page. These may be hidden if the sections are disabled.
     private final RowItemType[] ACTIVITY_STREAM_SECTIONS = { RowItemType.TOP_PANEL, RowItemType.TOP_STORIES_TITLE, RowItemType.HIGHLIGHTS_TITLE };
-    private final int MAX_TOP_STORIES = 3;
+    public static final int MAX_TOP_STORIES = 3;
 
     private HomePager.OnUrlOpenListener onUrlOpenListener;
     private HomePager.OnUrlOpenInBackgroundListener onUrlOpenInBackgroundListener;
 
     private int tiles;
     private int tilesSize;
 
     public enum RowItemType {
--- a/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topstories/PocketStoriesLoader.java
+++ b/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/topstories/PocketStoriesLoader.java
@@ -12,16 +12,17 @@ import android.support.v4.content.AsyncT
 import android.text.TextUtils;
 import android.util.Log;
 
 import org.json.JSONArray;
 import org.json.JSONException;
 import org.json.JSONObject;
 import org.mozilla.gecko.AppConstants;
 import org.mozilla.gecko.Locales;
+import org.mozilla.gecko.activitystream.homepanel.StreamRecyclerAdapter;
 import org.mozilla.gecko.activitystream.homepanel.model.TopStory;
 import org.mozilla.gecko.util.FileUtils;
 import org.mozilla.gecko.util.ProxySelector;
 
 import java.io.BufferedInputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.net.HttpURLConnection;
@@ -57,17 +58,17 @@ public class PocketStoriesLoader extends
     private static final String CACHE_TIMESTAMP_MILLIS_PREFIX = "timestampMillis-";
     private static final String STORIES_CACHE_PREFIX = "storiesCache-";
 
     // Pocket API params and defaults
     private static final String GLOBAL_ENDPOINT = "https://getpocket.cdn.mozilla.net/v3/firefox/global-recs";
     private static final String PARAM_APIKEY = "consumer_key";
     private static final String APIKEY = AppConstants.MOZ_POCKET_API_KEY;
     private static final String PARAM_COUNT = "count";
-    private static final int DEFAULT_COUNT = 3;
+    private static final int DEFAULT_COUNT = StreamRecyclerAdapter.MAX_TOP_STORIES + 1; // We have extra so we can hide malformed items.
     private static final String PARAM_LOCALE = "locale_lang";
 
     private static final long REFRESH_INTERVAL_MILLIS = TimeUnit.HOURS.toMillis(1);
 
     private static final int BUFFER_SIZE = 2048;
     private static final int CONNECT_TIMEOUT = (int) TimeUnit.SECONDS.toMillis(15);
     private static final int READ_TIMEOUT = (int) TimeUnit.SECONDS.toMillis(15);
 
@@ -159,29 +160,38 @@ public class PocketStoriesLoader extends
             final JSONObject jsonObject = new JSONObject(jsonResponse);
             JSONArray arr = jsonObject.getJSONArray("list");
             for (int i = 0; i < arr.length(); i++) {
                 try {
                     final JSONObject item = arr.getJSONObject(i);
                     final String title = item.getString("title");
                     final String url = item.getString("dedupe_url");
                     final String imageUrl = item.getString("image_src");
+
+                    // Drop malformed items.
+                    if (TextUtils.isEmpty(title) ||
+                            TextUtils.isEmpty(url) ||
+                            TextUtils.isEmpty(imageUrl)) {
+                        Log.d(LOGTAG, "Dropping malformed Pocket top story.");
+                        continue;
+                    }
+
                     topStories.add(new TopStory(title, url, imageUrl));
                 } catch (JSONException e) {
                     Log.e(LOGTAG, "Couldn't parse fields in Pocket response", e);
                 }
             }
         } catch (JSONException e) {
             Log.e(LOGTAG, "Couldn't load Pocket response", e);
         }
         return topStories;
     }
 
     private static List<TopStory> makePlaceholderStories() {
         final List<TopStory> stories = new LinkedList<>();
         final String TITLE_PREFIX = "Placeholder ";
-        for (int i = 0; i < 3; i++) {
+        for (int i = 0; i < DEFAULT_COUNT; i++) {
             // Urls must be different for bookmark/pinning UI to work properly. Assume this is true for Pocket stories.
             stories.add(new TopStory(TITLE_PREFIX + i, "https://www.mozilla.org/#" + i, null));
         }
         return stories;
     }
 }
--- a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/activitystream/homepanel/topstories/TestPocketStoriesLoader.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/activitystream/homepanel/topstories/TestPocketStoriesLoader.java
@@ -1,16 +1,15 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko.activitystream.homepanel.topstories;
 
 import junit.framework.Assert;
-
 import org.json.JSONArray;
 import org.json.JSONException;
 import org.json.JSONObject;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mozilla.gecko.activitystream.homepanel.model.TopStory;
 import org.mozilla.gecko.background.testhelpers.TestRunner;
 
@@ -77,16 +76,49 @@ public class TestPocketStoriesLoader {
     public void testJSONStringToTopStoriesWithMissingTitle() {
         final JSONObject storyItem = makeBasicStoryItem();
         storyItem.remove(KEY_TITLE);
         final String malformedResponseString = makeBasicPocketResponse(new JSONObject[] { storyItem });
         final List<TopStory> stories = PocketStoriesLoader.jsonStringToTopStories(malformedResponseString);
         Assert.assertEquals(0, stories.size()); // Should skip malformed item.
     }
 
+    @Test
+    public void testJSONStringToTopStoriesDropsMalformedTitle() throws Exception {
+        final JSONObject malformedStory = makeBasicStoryItem();
+        malformedStory.put(KEY_TITLE, "");
+        assertJSONStringToTopStoriesDropsMalformedStory(malformedStory);
+    }
+
+    @Test
+    public void testJSONStringToTopStoriesDropsMalformedURL() throws Exception {
+        final JSONObject malformedStory = makeBasicStoryItem();
+        malformedStory.put(KEY_DEDUPE_URL, "");
+        assertJSONStringToTopStoriesDropsMalformedStory(malformedStory);
+    }
+
+    @Test
+    public void testJSONStringToTopStoriesDropsMalformedImageURL() throws Exception {
+        final JSONObject malformedStory = makeBasicStoryItem();
+        malformedStory.put(KEY_IMAGE_SRC, "");
+        assertJSONStringToTopStoriesDropsMalformedStory(malformedStory);
+    }
+
+    private void assertJSONStringToTopStoriesDropsMalformedStory(final JSONObject malformedStory) throws JSONException {
+        final JSONObject expectedStory = makeBasicStoryItem();
+        final String expectedStoryTitle = "expectedItem";
+        expectedStory.put(KEY_TITLE, expectedStoryTitle);
+
+        final String jsonResponse = makeBasicPocketResponse(new JSONObject[] { expectedStory, malformedStory });
+        final List<TopStory> actualTopStories = PocketStoriesLoader.jsonStringToTopStories(jsonResponse);
+
+        Assert.assertEquals("Expected one malformed item to be removed, leaving size 1", 1, actualTopStories.size());
+        Assert.assertEquals(expectedStoryTitle, actualTopStories.get(0).getTitle());
+    }
+
     // Pulled 8/28 Pocket response, with some trimming for content/brevity.
     final String VALID_POCKET_RESPONSE = "{\"status\":1,\"list\":[" +
             "{\"id\":2910,\"url\":\"https:\\/\\/pocket.co\\/pocket-shorted-url\"," +
             "\"dedupe_url\":\"http:\\/\\/www.bbc.co.uk\\/actual-url\"," +
             "\"title\":\"A Title Here\"," +
             "\"excerpt\":\"Middle-aged people are being urged...\"," +
             "\"domain\":\"bbc.co.uk\"," +
             "\"image_src\":\"https:\\/\\/img.cloudfront.net\"," +