diff --git a/F-Droid/src/org/fdroid/fdroid/UpdateService.java b/F-Droid/src/org/fdroid/fdroid/UpdateService.java index 742a5fb3c..65bf627df 100644 --- a/F-Droid/src/org/fdroid/fdroid/UpdateService.java +++ b/F-Droid/src/org/fdroid/fdroid/UpdateService.java @@ -605,23 +605,6 @@ public class UpdateService extends IntentService implements ProgressListener { return knownIds; } - /** - * If you call this with too many apks, then it will likely hit limit of - * parameters allowed for sqlite3 query. Rather, you should use - * {@link org.fdroid.fdroid.UpdateService#getKnownApks(java.util.List)} - * instead, which will only call this with the right number of apks at - * a time. - * @see org.fdroid.fdroid.UpdateService#getKnownAppIds(java.util.List) - */ - private List getKnownApksFromProvider(List apks) { - final String[] fields = { - ApkProvider.DataColumns.APK_ID, - ApkProvider.DataColumns.VERSION, - ApkProvider.DataColumns.VERSION_CODE - }; - return ApkProvider.Helper.knownApks(this, apks, fields); - } - private void updateOrInsertApps(List appsToUpdate, int totalUpdateCount, int currentCount) { List operations = new ArrayList<>(); @@ -663,17 +646,12 @@ public class UpdateService extends IntentService implements ProgressListener { * Return list of apps from the "apks" argument which are already in the database. */ private List getKnownApks(List apks) { - List knownApks = new ArrayList<>(); - if (apks.size() > ApkProvider.MAX_APKS_TO_QUERY) { - int middle = apks.size() / 2; - List apks1 = apks.subList(0, middle); - List apks2 = apks.subList(middle, apks.size()); - knownApks.addAll(getKnownApks(apks1)); - knownApks.addAll(getKnownApks(apks2)); - } else { - knownApks.addAll(getKnownApksFromProvider(apks)); - } - return knownApks; + final String[] fields = { + ApkProvider.DataColumns.APK_ID, + ApkProvider.DataColumns.VERSION, + ApkProvider.DataColumns.VERSION_CODE + }; + return ApkProvider.Helper.knownApks(this, apks, fields); } private void updateOrInsertApks(List apksToUpdate, int totalApksAppsCount, int currentCount) { diff --git a/F-Droid/src/org/fdroid/fdroid/data/ApkProvider.java b/F-Droid/src/org/fdroid/fdroid/data/ApkProvider.java index 9f4b2066c..dba300586 100644 --- a/F-Droid/src/org/fdroid/fdroid/data/ApkProvider.java +++ b/F-Droid/src/org/fdroid/fdroid/data/ApkProvider.java @@ -24,7 +24,7 @@ public class ApkProvider extends FDroidProvider { * we may want to add additional constraints, so we give our self some * room by saying only 450 apks can be queried at once. */ - public static final int MAX_APKS_TO_QUERY = 450; + protected static final int MAX_APKS_TO_QUERY = 450; public static final class Helper { @@ -64,7 +64,19 @@ public class ApkProvider extends FDroidProvider { resolver.delete(uri, null, null); } - public static void deleteApks(Context context, List apks) { + public static void deleteApks(final Context context, final List apks) { + if (apks.size() > ApkProvider.MAX_APKS_TO_QUERY) { + int middle = apks.size() / 2; + List apks1 = apks.subList(0, middle); + List apks2 = apks.subList(middle, apks.size()); + deleteApks(context, apks1); + deleteApks(context, apks2); + } else { + deleteApksSafely(context, apks); + } + } + + private static void deleteApksSafely(final Context context, final List apks) { ContentResolver resolver = context.getContentResolver(); final Uri uri = getContentUri(apks); resolver.delete(uri, null, null); @@ -106,11 +118,26 @@ public class ApkProvider extends FDroidProvider { * Returns apks in the database, which have the same id and version as * one of the apks in the "apks" argument. */ - public static List knownApks(Context context, - List apks, String[] fields) { + public static List knownApks(Context context, List apks, String[] fields) { if (apks.size() == 0) { return new ArrayList<>(); } + + List knownApks = new ArrayList<>(); + if (apks.size() > ApkProvider.MAX_APKS_TO_QUERY) { + int middle = apks.size() / 2; + List apks1 = apks.subList(0, middle); + List apks2 = apks.subList(middle, apks.size()); + knownApks.addAll(knownApks(context, apks1, fields)); + knownApks.addAll(knownApks(context,apks2, fields)); + } else { + knownApks.addAll(knownApksSafe(context,apks, fields)); + } + return knownApks; + + } + + private static List knownApksSafe(final Context context, final List apks, final String[] fields) { ContentResolver resolver = context.getContentResolver(); final Uri uri = getContentUri(apks); Cursor cursor = resolver.query(uri, fields, null, null, null); @@ -238,7 +265,13 @@ public class ApkProvider extends FDroidProvider { .build(); } - public static Uri getContentUri(List apks) { + /** + * Intentionally left protected because it will break if apks is larger than + * {@link org.fdroid.fdroid.data.ApkProvider#MAX_APKS_TO_QUERY}. Instead of using + * this directly, think about using + * {@link org.fdroid.fdroid.data.ApkProvider.Helper#knownApks(android.content.Context, java.util.List, String[])} + */ + protected static Uri getContentUri(List apks) { StringBuilder builder = new StringBuilder(); for (int i = 0; i < apks.size(); i++) { if (i != 0) { diff --git a/F-Droid/test/src/org/fdroid/fdroid/ApkProviderTest.java b/F-Droid/test/src/org/fdroid/fdroid/ApkProviderTest.java index 17893ddef..a7b5dfa73 100644 --- a/F-Droid/test/src/org/fdroid/fdroid/ApkProviderTest.java +++ b/F-Droid/test/src/org/fdroid/fdroid/ApkProviderTest.java @@ -16,6 +16,19 @@ import java.util.List; public class ApkProviderTest extends BaseApkProviderTest { + /** + * I want to test the protected {@link org.fdroid.fdroid.data.ApkProvider#getContentUri(java.util.List)} + * method, but don't want to make it public. This exposes it. + */ + private static class PublicApkProvider extends ApkProvider { + + public static final int MAX_APKS_TO_QUERY = ApkProvider.MAX_APKS_TO_QUERY; + + public static Uri getContentUri(List apks) { + return ApkProvider.getContentUri(apks); + } + } + public void testUris() { assertInvalidUri(ApkProvider.getAuthority()); assertInvalidUri(RepoProvider.getContentUri()); @@ -29,15 +42,15 @@ public class ApkProviderTest extends BaseApkProviderTest { assertValidUri(ApkProvider.getAppUri("org.fdroid.fdroid")); assertValidUri(ApkProvider.getContentUri(new MockApk("org.fdroid.fdroid", 100))); assertValidUri(ApkProvider.getContentUri()); - assertValidUri(ApkProvider.getContentUri(apks)); + assertValidUri(PublicApkProvider.getContentUri(apks)); assertValidUri(ApkProvider.getContentUri("org.fdroid.fdroid", 100)); assertValidUri(ApkProvider.getRepoUri(1000)); - List manyApks = new ArrayList(ApkProvider.MAX_APKS_TO_QUERY - 5); - for (int i = 0; i < ApkProvider.MAX_APKS_TO_QUERY - 1; i ++) { + List manyApks = new ArrayList(PublicApkProvider.MAX_APKS_TO_QUERY - 5); + for (int i = 0; i < PublicApkProvider.MAX_APKS_TO_QUERY - 1; i ++) { manyApks.add(new MockApk("com.example." + i, i)); } - assertValidUri(ApkProvider.getContentUri(manyApks)); + assertValidUri(PublicApkProvider.getContentUri(manyApks)); manyApks.add(new MockApk("org.fdroid.fdroid.1", 1)); manyApks.add(new MockApk("org.fdroid.fdroid.2", 2)); @@ -46,7 +59,7 @@ public class ApkProviderTest extends BaseApkProviderTest { // throw an UnsupportedOperationException. However it // is still not okay (we run out of bindable parameters // in the sqlite query. - assertValidUri(ApkProvider.getContentUri(manyApks)); + assertValidUri(PublicApkProvider.getContentUri(manyApks)); fail(); } catch (IllegalArgumentException e) { // This is the expected error behaviour. @@ -93,7 +106,7 @@ public class ApkProviderTest extends BaseApkProviderTest { assertCantUpdate(ApkProvider.getContentUri()); assertCantUpdate(ApkProvider.getAppUri("org.fdroid.fdroid")); assertCantUpdate(ApkProvider.getRepoUri(1)); - assertCantUpdate(ApkProvider.getContentUri(apks)); + assertCantUpdate(PublicApkProvider.getContentUri(apks)); assertCantUpdate(Uri.withAppendedPath(ApkProvider.getContentUri(), "some-random-path")); // The only valid ones are: