From 424839c7934b989b609d69dd9597d140fb7c9b2b Mon Sep 17 00:00:00 2001 From: Peter Serwylo Date: Tue, 21 Apr 2015 22:27:58 +1000 Subject: [PATCH] Encapsulate functions which can produce invalid SQL. Now the other content provider functions which can result in broken SQL due to the number of arguments is private, and can only be accessed from a public helper method which ensures that limit is never hit. --- .../src/org/fdroid/fdroid/UpdateService.java | 34 ++++--------------- .../org/fdroid/fdroid/data/ApkProvider.java | 29 +++++++++++++--- .../org/fdroid/fdroid/ApkProviderTest.java | 25 ++++++++++---- 3 files changed, 50 insertions(+), 38 deletions(-) diff --git a/F-Droid/src/org/fdroid/fdroid/UpdateService.java b/F-Droid/src/org/fdroid/fdroid/UpdateService.java index c94a44bac..aebccf9bf 100644 --- a/F-Droid/src/org/fdroid/fdroid/UpdateService.java +++ b/F-Droid/src/org/fdroid/fdroid/UpdateService.java @@ -604,23 +604,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<>(); @@ -662,17 +645,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 257ed7fa1..de8dff377 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 { @@ -118,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(); Uri uri = getContentUri(apks); Cursor cursor = resolver.query(uri, fields, null, null, null); @@ -250,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: