Merge branch 'fix-index-v1-repo-commiting' into 'master'

Fix index-v1 repo commiting

Closes #1016

See merge request !559

app/src/main/java/org/fdroid/fdroid/IndexV1Updater.java

@@ -6,7 +6,6 @@ import android.net.Uri;
 import android.support.annotation.NonNull;
 import android.text.TextUtils;
 import android.util.Log;
-
 import com.fasterxml.jackson.annotation.JsonAutoDetect;
 import com.fasterxml.jackson.annotation.PropertyAccessor;
 import com.fasterxml.jackson.core.JsonFactory;
@@ -15,7 +14,6 @@ import com.fasterxml.jackson.core.type.TypeReference;
 import com.fasterxml.jackson.databind.DeserializationFeature;
 import com.fasterxml.jackson.databind.InjectableValues;
 import com.fasterxml.jackson.databind.ObjectMapper;
-
 import org.apache.commons.io.FileUtils;
 import org.fdroid.fdroid.data.Apk;
 import org.fdroid.fdroid.data.App;
@@ -138,14 +136,21 @@ public class IndexV1Updater extends RepoUpdater {
 
     /**
      * Parses the index and feeds it to the database via {@link Repo}, {@link App},
-     * and {@link Apk} instances.
+     * and {@link Apk} instances.  This uses {@link RepoPersister}  to add the apps
+     * and packages to the database in {@link RepoPersister#saveToDb(App, List)}
+     * to write the {@link Repo}, and commit the whole thing in
+     * {@link RepoPersister#commit(ContentValues)}.  One confusing thing about this
+     * whole process is that {@link RepoPersister} needs to first create and entry
+     * in the database, then fetch the ID from the database to populate
+     * {@link Repo#id}.  That has to happen first, then the rest of the {@code Repo}
+     * data must be added later.
      *
      * @param indexInputStream {@link InputStream} to {@code index-v1.json}
-     * @param cacheTag         the {@code etag} value from HTTP headers
+     * @param etag             the {@code etag} value from HTTP headers
      * @throws IOException
      * @throws UpdateException
      */
-    public void processIndexV1(InputStream indexInputStream, JarEntry indexEntry, String cacheTag)
+    public void processIndexV1(InputStream indexInputStream, JarEntry indexEntry, String etag)
             throws IOException, UpdateException {
         Utils.Profiler profiler = new Utils.Profiler(TAG);
         profiler.log("Starting to process index-v1.json");
@@ -200,6 +205,7 @@ public class IndexV1Updater extends RepoUpdater {
         // timestamp is absolutely required
         repo.timestamp = timestamp;
         // below are optional, can be null
+        repo.lastetag = etag;
         repo.name = getStringRepoValue(repoMap, "name");
         repo.icon = getStringRepoValue(repoMap, "icon");
         repo.description = getStringRepoValue(repoMap, "description");
@@ -234,15 +240,32 @@ public class IndexV1Updater extends RepoUpdater {
                 repoPersister.saveToDb(app, apks);
             }
         }
-
         profiler.log("Saved to database, but only a temporary table. Now persisting to database...");
-
         notifyCommittingToDb();
-        ContentValues values = prepareRepoDetailsForSaving(repo.name,
-                repo.description, repo.maxage, repo.version, repo.timestamp, repo.icon,
-                repo.mirrors, cacheTag);
-        repoPersister.commit(values);
 
+        ContentValues contentValues = new ContentValues();
+        contentValues.put(Schema.RepoTable.Cols.LAST_UPDATED, Utils.formatTime(new Date(), ""));
+        contentValues.put(Schema.RepoTable.Cols.TIMESTAMP, repo.timestamp);
+        contentValues.put(Schema.RepoTable.Cols.LAST_ETAG, repo.lastetag);
+        if (repo.version != Repo.INT_UNSET_VALUE) {
+            contentValues.put(Schema.RepoTable.Cols.VERSION, repo.version);
+        }
+        if (repo.maxage != Repo.INT_UNSET_VALUE) {
+            contentValues.put(Schema.RepoTable.Cols.MAX_AGE, repo.maxage);
+        }
+        if (repo.description != null) {
+            contentValues.put(Schema.RepoTable.Cols.DESCRIPTION, repo.description);
+        }
+        if (repo.name != null) {
+            contentValues.put(Schema.RepoTable.Cols.NAME, repo.name);
+        }
+        if (repo.icon != null) {
+            contentValues.put(Schema.RepoTable.Cols.ICON, repo.icon);
+        }
+        if (repo.mirrors != null && repo.mirrors.length > 0) {
+            contentValues.put(Schema.RepoTable.Cols.MIRRORS, Utils.serializeCommaSeparatedString(repo.mirrors));
+        }
+        repoPersister.commit(contentValues);
         profiler.log("Persited to database.");
 
 
@@ -266,10 +289,12 @@ public class IndexV1Updater extends RepoUpdater {
         return null;
     }
 
+    @SuppressWarnings("unchecked")
     private String[] getStringArrayRepoValue(Map<String, Object> repoMap, String key) {
         Object value = repoMap.get(key);
-        if (value != null && value instanceof String[]) {
+        if (value != null && value instanceof ArrayList) {
-            return (String[]) value;
+            ArrayList<String> list = (ArrayList<String>) value;
+            return list.toArray(new String[list.size()]);
         }
         return null;
     }

app/src/main/java/org/fdroid/fdroid/RepoUpdater.java

@@ -30,7 +30,6 @@ import android.content.pm.PackageManager;
 import android.support.annotation.NonNull;
 import android.text.TextUtils;
 import android.util.Log;
-
 import org.fdroid.fdroid.data.Apk;
 import org.fdroid.fdroid.data.ApkProvider;
 import org.fdroid.fdroid.data.App;
@@ -48,6 +47,9 @@ import org.xml.sax.InputSource;
 import org.xml.sax.SAXException;
 import org.xml.sax.XMLReader;
 
+import javax.xml.parsers.ParserConfigurationException;
+import javax.xml.parsers.SAXParser;
+import javax.xml.parsers.SAXParserFactory;
 import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
@@ -62,10 +64,6 @@ import java.util.List;
 import java.util.jar.JarEntry;
 import java.util.jar.JarFile;
 
-import javax.xml.parsers.ParserConfigurationException;
-import javax.xml.parsers.SAXParser;
-import javax.xml.parsers.SAXParserFactory;
-
 // TODO move to org.fdroid.fdroid.updater
 // TODO reduce visibility of methods once in .updater package (.e.g tests need it public now)
 
@@ -297,9 +295,10 @@ public class RepoUpdater {
 
     /**
      * Update tracking data for the repo represented by this instance (index version, etag,
-     * description, human-readable name, etc.
+     * description, human-readable name, etc.  This is not reused in {@link IndexV1Updater}
+     * because its too tied up into the old parsing flow in this class.
      */
-    ContentValues prepareRepoDetailsForSaving(String name, String description, int maxAge,
+    private ContentValues prepareRepoDetailsForSaving(String name, String description, int maxAge,
                                                       int version, long timestamp, String icon,
                                                       String[] mirrors, String cacheTag) {
         ContentValues values = new ContentValues();

app/src/main/java/org/fdroid/fdroid/data/RepoProvider.java

@@ -151,6 +151,13 @@ public class RepoProvider extends FDroidProvider {
             return repo;
         }
 
+        /**
+         * Updates the repo metadata in the database.  All data comes from the
+         * index file except {@link Repo#id}, which is generated by the database.
+         * That makes for an two cycle workflow, where first this must be called
+         * to fetch the {@code Repo.id} from the database, then it is called a
+         * second time to actually set the repo metadata.
+         */
         public static void update(Context context, Repo repo, ContentValues values) {
             ContentResolver resolver = context.getContentResolver();
 

app/src/test/java/org/fdroid/fdroid/updater/FDroidRepoUpdateTest.java

@@ -29,18 +29,24 @@ public class FDroidRepoUpdateTest extends MultiRepoUpdaterTest {
         assertEmpty();
         updateEarlier();
         updateLater();
+        updateV1Later();
     }
 
     protected void updateEarlier() throws RepoUpdater.UpdateException {
         Utils.debugLog(TAG, "Updating earlier version of F-Droid repo");
-        updateRepo(createUpdater(REPO_FDROID, REPO_FDROID_URI, context, REPO_FDROID_PUB_KEY),
+        updateRepo(createRepoUpdater(REPO_FDROID, REPO_FDROID_URI, context, REPO_FDROID_PUB_KEY),
                 "index.fdroid.2016-10-30.jar");
     }
 
     protected void updateLater() throws RepoUpdater.UpdateException {
         Utils.debugLog(TAG, "Updating later version of F-Droid repo");
-        updateRepo(createUpdater(REPO_FDROID, REPO_FDROID_URI, context, REPO_FDROID_PUB_KEY),
+        updateRepo(createRepoUpdater(REPO_FDROID, REPO_FDROID_URI, context, REPO_FDROID_PUB_KEY),
                 "index.fdroid.2016-11-10.jar");
     }
 
+    protected void updateV1Later() throws RepoUpdater.UpdateException {
+        Utils.debugLog(TAG, "Updating later version of F-Droid index-v1");
+        updateRepo(createIndexV1Updater(REPO_FDROID, REPO_FDROID_URI, context, REPO_FDROID_PUB_KEY),
+                "index-v1.fdroid.2017-07-07.jar");
+    }
 }

app/src/test/java/org/fdroid/fdroid/updater/IndexV1UpdaterTest.java

@@ -17,9 +17,12 @@ import org.fdroid.fdroid.Preferences;
 import org.fdroid.fdroid.RepoUpdater;
 import org.fdroid.fdroid.TestUtils;
 import org.fdroid.fdroid.data.Apk;
+import org.fdroid.fdroid.data.ApkProvider;
 import org.fdroid.fdroid.data.App;
+import org.fdroid.fdroid.data.AppProvider;
 import org.fdroid.fdroid.data.FDroidProviderTest;
 import org.fdroid.fdroid.data.Repo;
+import org.fdroid.fdroid.data.RepoProvider;
 import org.fdroid.fdroid.data.RepoPushRequest;
 import org.fdroid.fdroid.mock.RepoDetails;
 import org.junit.After;
@@ -48,6 +51,7 @@ import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 @Config(constants = BuildConfig.class, sdk = 24)
@@ -71,6 +75,12 @@ public class IndexV1UpdaterTest extends FDroidProviderTest {
 
     @Test
     public void testIndexV1Processing() throws IOException, RepoUpdater.UpdateException {
+        List<Repo> repos = RepoProvider.Helper.all(context);
+        for (Repo repo : repos) {
+            RepoProvider.Helper.remove(context, repo.getId());
+        }
+        assertEquals("No repos present", 0, RepoProvider.Helper.all(context).size());
+        assertEquals("No apps present", 0, AppProvider.Helper.all(context.getContentResolver()).size());
         Repo repo = MultiRepoUpdaterTest.createRepo("Testy", TESTY_JAR, context, TESTY_CERT);
         repo.timestamp = 1481222110;
         IndexV1Updater updater = new IndexV1Updater(context, repo);
@@ -80,6 +90,37 @@ public class IndexV1UpdaterTest extends FDroidProviderTest {
         InputStream indexInputStream = jarFile.getInputStream(indexEntry);
         updater.processIndexV1(indexInputStream, indexEntry, "fakeEtag");
         IOUtils.closeQuietly(indexInputStream);
+        List<App> apps = AppProvider.Helper.all(context.getContentResolver());
+        assertEquals("53 apps present", 53, apps.size());
+
+        String[] packages = {
+                "fake.app.one",
+                "org.adaway",
+                "This_does_not_exist",
+        };
+        for (String id : packages) {
+            assertEquals("No apks for " + id, 0, ApkProvider.Helper.findByPackageName(context, id).size());
+        }
+
+        for (App app : apps) {
+            assertTrue("Some apks for " + app.packageName,
+                    ApkProvider.Helper.findByPackageName(context, app.packageName).size() > 0);
+        }
+
+        repos = RepoProvider.Helper.all(context);
+        assertEquals("One repo", 1, repos.size());
+        Repo repoFromDb = repos.get(0);
+        assertEquals("repo.timestamp should be set", 1481222111, repoFromDb.timestamp);
+        assertEquals("repo.address should be the same", repo.address, repoFromDb.address);
+        assertEquals("repo.name should be set", "non-public test repo", repoFromDb.name);
+        assertEquals("repo.maxage should be set", 0, repoFromDb.maxage);
+        assertEquals("repo.version should be set", 18, repoFromDb.version);
+        assertEquals("repo.icon should be set", "fdroid-icon.png", repoFromDb.icon);
+        String description = "This is a repository of apps to be used with F-Droid. Applications in this repository are either official binaries built by the original application developers, or are binaries built from source by the admin of f-droid.org using the tools on https://gitlab.com/u/fdroid. "; // NOCHECKSTYLE LineLength
+        assertEquals("repo.description should be set", description, repoFromDb.description);
+        assertEquals("repo.mirrors should have items", 2, repo.mirrors.length);
+        assertEquals("repo.mirrors first URL", "http://frkcchxlcvnb4m5a.onion/fdroid/repo", repo.mirrors[0]);
+        assertEquals("repo.mirrors second URL", "http://testy.at.or.at/fdroid/repo", repo.mirrors[1]);
     }
 
     @Test(expected = RepoUpdater.SigningException.class)

app/src/test/java/org/fdroid/fdroid/updater/MultiRepoUpdaterTest.java

@@ -5,7 +5,7 @@ import android.content.ContentValues;
 import android.content.Context;
 import android.support.annotation.NonNull;
 import android.text.TextUtils;
-
+import org.fdroid.fdroid.IndexV1Updater;
 import org.fdroid.fdroid.Preferences;
 import org.fdroid.fdroid.RepoUpdater;
 import org.fdroid.fdroid.RepoUpdater.UpdateException;
@@ -21,11 +21,16 @@ import org.junit.After;
 import org.junit.Before;
 
 import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
 import java.util.List;
+import java.util.jar.JarEntry;
+import java.util.jar.JarFile;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 public abstract class MultiRepoUpdaterTest extends FDroidProviderTest {
     @SuppressWarnings("unused")
@@ -177,30 +182,44 @@ public abstract class MultiRepoUpdaterTest extends FDroidProviderTest {
         return RepoProvider.Helper.findByAddress(context, repo.address);
     }
 
-    protected RepoUpdater createUpdater(String name, String uri, Context context) {
+    protected RepoUpdater createRepoUpdater(String name, String uri, Context context) {
         return new RepoUpdater(context, createRepo(name, uri, context));
     }
 
-    protected RepoUpdater createUpdater(String name, String uri, Context context, String signingCert) {
+    protected RepoUpdater createRepoUpdater(String name, String uri, Context context, String signingCert) {
         return new RepoUpdater(context, createRepo(name, uri, context, signingCert));
     }
 
+    protected IndexV1Updater createIndexV1Updater(String name, String uri, Context context, String signingCert) {
+        return new IndexV1Updater(context, createRepo(name, uri, context, signingCert));
+    }
+
     protected void updateConflicting() throws UpdateException {
-        updateRepo(createUpdater(REPO_CONFLICTING, REPO_CONFLICTING_URI, context), "multiRepo.conflicting.jar");
+        updateRepo(createRepoUpdater(REPO_CONFLICTING, REPO_CONFLICTING_URI, context), "multiRepo.conflicting.jar");
     }
 
     protected void updateMain() throws UpdateException {
-        updateRepo(createUpdater(REPO_MAIN, REPO_MAIN_URI, context), "multiRepo.normal.jar");
+        updateRepo(createRepoUpdater(REPO_MAIN, REPO_MAIN_URI, context), "multiRepo.normal.jar");
     }
 
     protected void updateArchive() throws UpdateException {
-        updateRepo(createUpdater(REPO_ARCHIVE, REPO_ARCHIVE_URI, context), "multiRepo.archive.jar");
+        updateRepo(createRepoUpdater(REPO_ARCHIVE, REPO_ARCHIVE_URI, context), "multiRepo.archive.jar");
     }
 
     protected void updateRepo(RepoUpdater updater, String indexJarPath) throws UpdateException {
         File indexJar = TestUtils.copyResourceToTempFile(indexJarPath);
         try {
+            if (updater instanceof IndexV1Updater) {
+                JarFile jarFile = new JarFile(indexJar);
+                JarEntry indexEntry = (JarEntry) jarFile.getEntry(IndexV1Updater.DATA_FILE_NAME);
+                InputStream indexInputStream = jarFile.getInputStream(indexEntry);
+                ((IndexV1Updater) updater).processIndexV1(indexInputStream, indexEntry, null);
+            } else {
                 updater.processDownloadedFile(indexJar);
+            }
+        } catch (IOException e) {
+            e.printStackTrace();
+            fail();
         } finally {
             if (indexJar != null && indexJar.exists()) {
                 indexJar.delete();