Fix crash when 'null' had been stored in RecentlyUsedLists. #562 (#579)

opentasks/build.gradle

@@ -68,6 +68,7 @@ dependencies {
     implementation 'com.github.dmfs.bolts:color-bolts:' + BOLTS_VERSION
 
     testImplementation 'junit:junit:4.12'
+    testImplementation 'org.robolectric:robolectric:3.5.1'
 
     androidTestImplementation('com.android.support.test:runner:' + ANDROID_TEST_RUNNER_VERSION) {
         exclude group: 'com.android.support', module: 'support-annotations'

opentasks/src/main/java/org/dmfs/tasks/utils/RecentlyUsedLists.java

@@ -19,6 +19,11 @@
 import android.text.TextUtils;
 import android.util.Log;
 
+import org.dmfs.iterables.Split;
+import org.dmfs.iterables.decorators.Fluent;
+import org.dmfs.optional.NullSafe;
+import org.dmfs.optional.Optional;
+
 import java.util.ArrayList;
 import java.util.List;
 
@@ -28,7 +33,7 @@
  */
 public class RecentlyUsedLists
 {
-    public static final String PREFERENCE_KEY = "RecentlyUsedLists";
+    private static final String PREFERENCE_KEY = "RecentlyUsedLists";
 
 
     /**
@@ -39,19 +44,33 @@ public class RecentlyUsedLists
      *
      * @return List of TaskLists where the most recently used list is on position 0.
      */
-    public static List<Long> getList(Context context)
+    private static List<Long> getList(Context context)
     {
-        String strLists = PreferenceManager.getDefaultSharedPreferences(context).getString(PREFERENCE_KEY, "");
-        Log.v(RecentlyUsedLists.class.getSimpleName(), "getList:  " + strLists);
-        List<Long> lists = new ArrayList<>();
-        if (strLists.length() > 0)
+        Optional<String> listStrOpt = new NullSafe<>(PreferenceManager.getDefaultSharedPreferences(context).getString(PREFERENCE_KEY, null));
+        Log.v(RecentlyUsedLists.class.getSimpleName(), "getList:  " + listStrOpt.value("empty"));
+        if (!listStrOpt.isPresent())
         {
-            for (String lid : strLists.split(","))
-            {
-                lists.add(Long.parseLong(lid));
-            }
+            return new ArrayList<>(0);
         }
-        return lists;
+
+        String listStr = listStrOpt.value();
+
+        // Handling known bug https://github.com/dmfs/opentasks/issues/562
+        // See also {@link RecentlyUsedListsNullHandlingTest}
+        if (listStr.contains("null"))
+        {
+            setList(context, toList(new Fluent<>(new Split(listStr, ','))
+                    .mapped(Object::toString)
+                    .filtered(s -> !s.isEmpty())
+                    .filtered(s -> !s.equals("null"))
+                    .mapped(Long::valueOf)));
+            return getList(context);
+        }
+
+        return toList(new Fluent<>(new Split(listStr, ','))
+                .mapped(Object::toString)
+                .filtered(s -> !s.isEmpty())
+                .mapped(Long::valueOf));
     }
 
 
@@ -67,7 +86,7 @@ private static void setList(Context context, List<Long> lists)
     {
         String strLists = TextUtils.join(",", lists);
         Log.v(RecentlyUsedLists.class.getSimpleName(), "setList:  " + strLists);
-        PreferenceManager.getDefaultSharedPreferences(context).edit().putString(PREFERENCE_KEY, strLists).commit();
+        PreferenceManager.getDefaultSharedPreferences(context).edit().putString(PREFERENCE_KEY, strLists).apply();
     }
 
 
@@ -83,6 +102,16 @@ private static void setList(Context context, List<Long> lists)
      */
     public static Long getRecentFromList(Context context, List<Long> allowedLists)
     {
+        /*
+         * It should not happen that the List contains <code>null</code>, but since there was this bug:
+         * https://github.com/dmfs/opentasks/issues/562, see also {@link RecentlyUsedListsNullHandlingTest}
+         * this check is added to catch any bug which causes <code>null</code>s in place of list ids.
+         */
+        if (allowedLists.contains(null))
+        {
+            throw new IllegalArgumentException("allowedLists cannot contain 'null'");
+        }
+
         List<Long> recentlyLists = getList(context);
         for (Long listId : recentlyLists)
         {
@@ -103,11 +132,23 @@ public static Long getRecentFromList(Context context, List<Long> allowedLists)
      * @param listId
      *         Id of the TaskList, where a task was just created.
      */
-    public static void use(Context context, Long listId)
+    public static void use(Context context, long listId)
     {
         List<Long> lists = getList(context);
         lists.remove(listId); // does nothing, if "listId" is not in "lists"
         lists.add(0, listId);
         setList(context, lists);
     }
+
+
+    private static <T> List<T> toList(Iterable<T> iterable)
+    {
+        List<T> list = new ArrayList<>();
+        for (T t : iterable)
+        {
+            list.add(t);
+        }
+        return list;
+
+    }
 }

opentasks/src/test/java/org/dmfs/tasks/utils/RecentlyUsedListsNullHandlingTest.java

@@ -0,0 +1,178 @@
+/*
+ * Copyright 2017 dmfs GmbH
+ *
+ * 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 org.dmfs.tasks.utils;
+
+import android.app.Application;
+import android.content.Context;
+import android.content.SharedPreferences;
+import android.preference.PreferenceManager;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.robolectric.RobolectricTestRunner;
+import org.robolectric.RuntimeEnvironment;
+import org.robolectric.annotation.Config;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.junit.Assert.assertThat;
+
+
+/**
+ * This test is for checking that the bug <a href="https://github.com/dmfs/opentasks/issues/562">562</a>, which was caused by storing a "null" string in the
+ * recently used list prefs string, like "null,3,2", is fixed correctly as far as {@link RecentlyUsedLists} is concerned. Calling {@link
+ * RecentlyUsedLists#use(Context, long)} with <code>null</code> list id will still cause a NPE crash, but this fix will help to track down the original problem
+ * if it happens again. It also checks that {@link RecentlyUsedLists} removes the "null" so we can possibly remove the code handling that later.
+ *
+ * @author Gabor Keszthelyi
+ */
+@RunWith(RobolectricTestRunner.class)
+@Config(manifest = Config.NONE)
+public class RecentlyUsedListsNullHandlingTest
+{
+    private static final String PREFERENCE_KEY = "RecentlyUsedLists";
+
+
+    /**
+     * That that the {@link RecentlyUsedLists#use(Context, long)} method cannot be called with <code>null</code> value.
+     */
+    @Test(expected = NullPointerException.class)
+    public void test_use_cannot_be_called_with_null_listId()
+    {
+        Long listId = null;
+        RecentlyUsedLists.use(RuntimeEnvironment.application, listId);
+    }
+
+
+    /**
+     * Test that even if "null" string had been stored incorrectly previously, it doesn't affect the usage.
+     */
+    @Test
+    public void test_getRecentFromList_that_the_stored_null_string_is_ignored()
+    {
+        Application context = RuntimeEnvironment.application;
+        SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(context);
+
+        prefs.edit().putString(PREFERENCE_KEY, "null,1,2").apply();
+        List<Long> allowedList = new ArrayList<>();
+        allowedList.add(2L);
+        assertThat(RecentlyUsedLists.getRecentFromList(context, allowedList), is(2L));
+
+        prefs.edit().putString(PREFERENCE_KEY, "1,null,2").apply();
+        allowedList = new ArrayList<>();
+        allowedList.add(5L);
+        assertThat(RecentlyUsedLists.getRecentFromList(context, allowedList), is(5L));
+
+        prefs.edit().putString(PREFERENCE_KEY, "1,2,null").apply();
+        allowedList = new ArrayList<>();
+        allowedList.add(2L);
+        assertThat(RecentlyUsedLists.getRecentFromList(context, allowedList), is(2L));
+    }
+
+
+    /**
+     * Test that if "null" string had been stored incorrectly previously, it is cleared on the first time usage of {@link
+     * RecentlyUsedLists#getRecentFromList(Context, List)}.
+     * <p>
+     * This is to ensure that we can remove the "null" handling fix later.
+     */
+    @Test
+    public void test_getRecentFromList_that_the_stored_null_string_is_removed()
+    {
+        Application context = RuntimeEnvironment.application;
+        SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(context);
+
+        List<Long> allowedList = new ArrayList<>();
+        allowedList.add(2L);
+
+        prefs.edit().putString(PREFERENCE_KEY, "null,1,2").apply();
+        RecentlyUsedLists.getRecentFromList(context, allowedList);
+        assertThat(prefs.getString(PREFERENCE_KEY, "na"), is("1,2"));
+
+        prefs.edit().putString(PREFERENCE_KEY, "1,null,2").apply();
+        RecentlyUsedLists.getRecentFromList(context, allowedList);
+        assertThat(prefs.getString(PREFERENCE_KEY, "na"), is("1,2"));
+
+        prefs.edit().putString(PREFERENCE_KEY, "null").apply();
+        RecentlyUsedLists.getRecentFromList(context, allowedList);
+        assertThat(prefs.getString(PREFERENCE_KEY, "na"), is(""));
+    }
+
+
+    /**
+     * Test that if "null" string had been stored incorrectly previously, it is cleared on the first time usage of {@link
+     * RecentlyUsedLists#use(Context, long)}.
+     * <p>
+     * This is to ensure that we can remove the "null" handling fix later.
+     */
+    @Test
+    public void test_use_that_the_stored_null_string_is_removed()
+    {
+        Application context = RuntimeEnvironment.application;
+        SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(context);
+
+        prefs.edit().putString(PREFERENCE_KEY, "null,1,2").apply();
+        RecentlyUsedLists.use(context, 3L);
+        assertThat(prefs.getString(PREFERENCE_KEY, "na"), is("3,1,2"));
+
+        prefs.edit().putString(PREFERENCE_KEY, "1,null,2").apply();
+        RecentlyUsedLists.use(context, 3L);
+        assertThat(prefs.getString(PREFERENCE_KEY, "na"), is("3,1,2"));
+
+        prefs.edit().putString(PREFERENCE_KEY, "null").apply();
+        RecentlyUsedLists.use(context, 3L);
+        assertThat(prefs.getString(PREFERENCE_KEY, "na"), is("3"));
+    }
+
+
+    /**
+     * Test that <code>null</code> is not allowed in the <code>allowedList</code> {@link List} either.
+     */
+    @Test(expected = IllegalArgumentException.class)
+    public void test_getRecentFromList_null_is_not_allowed_in_allowed_strings()
+    {
+        List<Long> allowedList = new ArrayList<>();
+        allowedList.add(null);
+        allowedList.add(1L);
+        RecentlyUsedLists.getRecentFromList(RuntimeEnvironment.application, allowedList);
+    }
+
+
+    /**
+     * Test that no value / empty value / null value stored in prefs is still handled correctly after the changes.
+     */
+    @Test
+    public void test_getRecentFromList_empty_or_null_prefs_value_still_works()
+    {
+        Application context = RuntimeEnvironment.application;
+        SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(context);
+
+        List<Long> allowedList = new ArrayList<>();
+        allowedList.add(5L);
+
+        assertThat(RecentlyUsedLists.getRecentFromList(context, allowedList), is(5L));
+
+        prefs.edit().putString(PREFERENCE_KEY, null).apply();
+        assertThat(RecentlyUsedLists.getRecentFromList(context, allowedList), is(5L));
+
+        prefs.edit().putString(PREFERENCE_KEY, "").apply();
+        assertThat(RecentlyUsedLists.getRecentFromList(context, allowedList), is(5L));
+    }
+
+}