From a1b9892c7739a1b8aa78922a075f0b18f52717b8 Mon Sep 17 00:00:00 2001 From: Mauricio Colli Date: Sat, 28 Mar 2020 20:08:42 -0300 Subject: [PATCH 1/2] Move exception utils to a separate class and add tests for it --- app/src/main/java/org/schabi/newpipe/App.java | 6 +- .../newpipe/fragments/BaseStateFragment.java | 4 +- .../fragments/list/search/SearchFragment.java | 3 +- .../org/schabi/newpipe/util/ExceptionUtils.kt | 82 +++++++++++++++++++ .../schabi/newpipe/util/ExtractorHelper.java | 82 ------------------- .../schabi/newpipe/util/ExceptionUtilsTest.kt | 69 ++++++++++++++++ 6 files changed, 158 insertions(+), 88 deletions(-) create mode 100644 app/src/main/java/org/schabi/newpipe/util/ExceptionUtils.kt create mode 100644 app/src/test/java/org/schabi/newpipe/util/ExceptionUtilsTest.kt diff --git a/app/src/main/java/org/schabi/newpipe/App.java b/app/src/main/java/org/schabi/newpipe/App.java index f9b3abfb1..7e9df0bed 100644 --- a/app/src/main/java/org/schabi/newpipe/App.java +++ b/app/src/main/java/org/schabi/newpipe/App.java @@ -27,7 +27,7 @@ import org.schabi.newpipe.report.AcraReportSenderFactory; import org.schabi.newpipe.report.ErrorActivity; import org.schabi.newpipe.report.UserAction; import org.schabi.newpipe.settings.SettingsActivity; -import org.schabi.newpipe.util.ExtractorHelper; +import org.schabi.newpipe.util.ExceptionUtils; import org.schabi.newpipe.util.Localization; import org.schabi.newpipe.util.ServiceHelper; import org.schabi.newpipe.util.StateSaver; @@ -173,7 +173,7 @@ public class App extends Application { private boolean isThrowableIgnored(@NonNull final Throwable throwable) { // Don't crash the application over a simple network problem - return ExtractorHelper.hasAssignableCauseThrowable(throwable, + return ExceptionUtils.hasAssignableCause(throwable, // network api cancellation IOException.class, SocketException.class, // blocking code disposed @@ -182,7 +182,7 @@ public class App extends Application { private boolean isThrowableCritical(@NonNull final Throwable throwable) { // Though these exceptions cannot be ignored - return ExtractorHelper.hasAssignableCauseThrowable(throwable, + return ExceptionUtils.hasAssignableCause(throwable, NullPointerException.class, IllegalArgumentException.class, // bug in app OnErrorNotImplementedException.class, MissingBackpressureException.class, IllegalStateException.class); // bug in operator diff --git a/app/src/main/java/org/schabi/newpipe/fragments/BaseStateFragment.java b/app/src/main/java/org/schabi/newpipe/fragments/BaseStateFragment.java index 861dc2c60..3167c4632 100644 --- a/app/src/main/java/org/schabi/newpipe/fragments/BaseStateFragment.java +++ b/app/src/main/java/org/schabi/newpipe/fragments/BaseStateFragment.java @@ -23,7 +23,7 @@ import org.schabi.newpipe.extractor.exceptions.ContentNotAvailableException; import org.schabi.newpipe.extractor.exceptions.ReCaptchaException; import org.schabi.newpipe.report.ErrorActivity; import org.schabi.newpipe.report.UserAction; -import org.schabi.newpipe.util.ExtractorHelper; +import org.schabi.newpipe.util.ExceptionUtils; import org.schabi.newpipe.util.InfoCache; import java.io.IOException; @@ -200,7 +200,7 @@ public abstract class BaseStateFragment extends BaseFragment implements ViewC return true; } - if (ExtractorHelper.isInterruptedCaused(exception)) { + if (ExceptionUtils.isInterruptedCaused(exception)) { if (DEBUG) { Log.w(TAG, "onError() isInterruptedCaused! = [" + exception + "]"); } diff --git a/app/src/main/java/org/schabi/newpipe/fragments/list/search/SearchFragment.java b/app/src/main/java/org/schabi/newpipe/fragments/list/search/SearchFragment.java index 718865f10..b3c7ae189 100644 --- a/app/src/main/java/org/schabi/newpipe/fragments/list/search/SearchFragment.java +++ b/app/src/main/java/org/schabi/newpipe/fragments/list/search/SearchFragment.java @@ -41,6 +41,7 @@ import org.schabi.newpipe.extractor.StreamingService; import org.schabi.newpipe.extractor.exceptions.ParsingException; import org.schabi.newpipe.extractor.search.SearchExtractor; import org.schabi.newpipe.extractor.search.SearchInfo; +import org.schabi.newpipe.util.ExceptionUtils; import org.schabi.newpipe.fragments.BackPressable; import org.schabi.newpipe.fragments.list.BaseListFragment; import org.schabi.newpipe.local.history.HistoryRecordManager; @@ -764,7 +765,7 @@ public class SearchFragment extends BaseListFragment): Boolean { + return hasCause(throwable, false, *causesToCheck) + } + + /** + * Calls [hasCause] with the `checkSubtypes` parameter set to true. + */ + @JvmStatic + fun hasAssignableCause(throwable: Throwable?, vararg causesToCheck: Class<*>): Boolean { + return hasCause(throwable, true, *causesToCheck) + } + + /** + * Check if throwable has some cause from the causes to check, or is itself in it. + * + * If `checkIfAssignable` is true, not only the exact type will be considered equals, but also its subtypes. + * + * @param throwable throwable that will be checked. + * @param checkSubtypes if subtypes are also checked. + * @param causesToCheck an array of causes to check. + * + * @see Class.isAssignableFrom + */ + @JvmStatic + tailrec fun hasCause(throwable: Throwable?, checkSubtypes: Boolean, vararg causesToCheck: Class<*>): Boolean { + if (throwable == null) { + return false + } + + // Check if throwable is a subtype of any of the causes to check + causesToCheck.forEach { causeClass -> + if (checkSubtypes) { + if (causeClass.isAssignableFrom(throwable.javaClass)) { + return true + } + } else { + if (causeClass == throwable.javaClass) { + return true + } + } + } + + val currentCause: Throwable? = throwable.cause + // Check if cause is not pointing to the same instance, to avoid infinite loops. + if (throwable !== currentCause) { + return hasCause(currentCause, checkSubtypes, *causesToCheck) + } + + return false + } + } +} \ No newline at end of file diff --git a/app/src/main/java/org/schabi/newpipe/util/ExtractorHelper.java b/app/src/main/java/org/schabi/newpipe/util/ExtractorHelper.java index 9c6ab1898..95e2b65d3 100644 --- a/app/src/main/java/org/schabi/newpipe/util/ExtractorHelper.java +++ b/app/src/main/java/org/schabi/newpipe/util/ExtractorHelper.java @@ -52,7 +52,6 @@ import org.schabi.newpipe.report.ErrorActivity; import org.schabi.newpipe.report.UserAction; import java.io.IOException; -import java.io.InterruptedIOException; import java.util.Collections; import java.util.List; @@ -306,85 +305,4 @@ public final class ExtractorHelper { } }); } - - /** - * Check if throwable have the cause that can be assignable from the causes to check. - * - * @see Class#isAssignableFrom(Class) - * @param throwable the throwable to be checked - * @param causesToCheck the causes to check - * @return whether the exception is an instance of a subclass of one of the causes - * or is caused by an instance of a subclass of one of the causes - */ - public static boolean hasAssignableCauseThrowable(final Throwable throwable, - final Class... causesToCheck) { - // Check if getCause is not the same as cause (the getCause is already the root), - // as it will cause a infinite loop if it is - Throwable cause; - Throwable getCause = throwable; - - // Check if throwable is a subclass of any of the filtered classes - final Class throwableClass = throwable.getClass(); - for (Class causesEl : causesToCheck) { - if (causesEl.isAssignableFrom(throwableClass)) { - return true; - } - } - - // Iteratively checks if the root cause of the throwable is a subclass of the filtered class - while ((cause = throwable.getCause()) != null && getCause != cause) { - getCause = cause; - final Class causeClass = cause.getClass(); - for (Class causesEl : causesToCheck) { - if (causesEl.isAssignableFrom(causeClass)) { - return true; - } - } - } - return false; - } - - /** - * Check if throwable have the exact cause from one of the causes to check. - * - * @param throwable the throwable to be checked - * @param causesToCheck the causes to check - * @return whether the exception is an instance of one of the causes - * or is caused by an instance of one of the causes - */ - public static boolean hasExactCauseThrowable(final Throwable throwable, - final Class... causesToCheck) { - // Check if getCause is not the same as cause (the getCause is already the root), - // as it will cause a infinite loop if it is - Throwable cause; - Throwable getCause = throwable; - - for (Class causesEl : causesToCheck) { - if (throwable.getClass().equals(causesEl)) { - return true; - } - } - - while ((cause = throwable.getCause()) != null && getCause != cause) { - getCause = cause; - for (Class causesEl : causesToCheck) { - if (cause.getClass().equals(causesEl)) { - return true; - } - } - } - return false; - } - - /** - * Check if throwable have Interrupted* exception as one of its causes. - * - * @param throwable the throwable to be checkes - * @return whether the throwable is caused by an interruption - */ - public static boolean isInterruptedCaused(final Throwable throwable) { - return ExtractorHelper.hasExactCauseThrowable(throwable, - InterruptedIOException.class, - InterruptedException.class); - } } diff --git a/app/src/test/java/org/schabi/newpipe/util/ExceptionUtilsTest.kt b/app/src/test/java/org/schabi/newpipe/util/ExceptionUtilsTest.kt new file mode 100644 index 000000000..fc0e9dcbd --- /dev/null +++ b/app/src/test/java/org/schabi/newpipe/util/ExceptionUtilsTest.kt @@ -0,0 +1,69 @@ +package org.schabi.newpipe.util + +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test +import org.schabi.newpipe.util.ExceptionUtils.Companion.hasAssignableCause +import org.schabi.newpipe.util.ExceptionUtils.Companion.hasExactCause +import java.io.IOException +import java.io.InterruptedIOException +import java.net.SocketException +import javax.net.ssl.SSLException + +class ExceptionUtilsTest { + @Test fun `assignable causes`() { + assertTrue(hasAssignableCause(Throwable(), Throwable::class.java)) + assertTrue(hasAssignableCause(Exception(), Exception::class.java)) + assertTrue(hasAssignableCause(IOException(), Exception::class.java)) + + assertTrue(hasAssignableCause(IOException(), IOException::class.java)) + assertTrue(hasAssignableCause(Exception(SocketException()), IOException::class.java)) + assertTrue(hasAssignableCause(Exception(IllegalStateException()), RuntimeException::class.java)) + assertTrue(hasAssignableCause(Exception(Exception(IOException())), IOException::class.java)) + assertTrue(hasAssignableCause(Exception(IllegalStateException(Exception(IOException()))), IOException::class.java)) + assertTrue(hasAssignableCause(Exception(IllegalStateException(Exception(SocketException()))), IOException::class.java)) + assertTrue(hasAssignableCause(Exception(IllegalStateException(Exception(SSLException("IO")))), IOException::class.java)) + assertTrue(hasAssignableCause(Exception(IllegalStateException(Exception(InterruptedIOException()))), IOException::class.java)) + assertTrue(hasAssignableCause(Exception(IllegalStateException(Exception(InterruptedIOException()))), RuntimeException::class.java)) + + assertTrue(hasAssignableCause(IllegalStateException(), Throwable::class.java)) + assertTrue(hasAssignableCause(IllegalStateException(), Exception::class.java)) + assertTrue(hasAssignableCause(Exception(IllegalStateException(Exception(InterruptedIOException()))), InterruptedIOException::class.java)) + } + + @Test fun `no assignable causes`() { + assertFalse(hasAssignableCause(Throwable(), Exception::class.java)) + assertFalse(hasAssignableCause(Exception(), IOException::class.java)) + assertFalse(hasAssignableCause(Exception(IllegalStateException()), IOException::class.java)) + assertFalse(hasAssignableCause(Exception(NullPointerException()), IOException::class.java)) + assertFalse(hasAssignableCause(Exception(IllegalStateException(Exception(Exception()))), IOException::class.java)) + assertFalse(hasAssignableCause(Exception(IllegalStateException(Exception(SocketException()))), InterruptedIOException::class.java)) + assertFalse(hasAssignableCause(Exception(IllegalStateException(Exception(InterruptedIOException()))), InterruptedException::class.java)) + } + + @Test fun `exact causes`() { + assertTrue(hasExactCause(Throwable(), Throwable::class.java)) + assertTrue(hasExactCause(Exception(), Exception::class.java)) + + assertTrue(hasExactCause(IOException(), IOException::class.java)) + assertTrue(hasExactCause(Exception(SocketException()), SocketException::class.java)) + assertTrue(hasExactCause(Exception(Exception(IOException())), IOException::class.java)) + assertTrue(hasExactCause(Exception(IllegalStateException(Exception(IOException()))), IOException::class.java)) + assertTrue(hasExactCause(Exception(IllegalStateException(Exception(SocketException()))), SocketException::class.java)) + assertTrue(hasExactCause(Exception(IllegalStateException(Exception(SSLException("IO")))), SSLException::class.java)) + assertTrue(hasExactCause(Exception(IllegalStateException(Exception(InterruptedIOException()))), InterruptedIOException::class.java)) + assertTrue(hasExactCause(Exception(IllegalStateException(Exception(InterruptedIOException()))), IllegalStateException::class.java)) + } + + @Test fun `no exact causes`() { + assertFalse(hasExactCause(Throwable(), Exception::class.java)) + assertFalse(hasExactCause(Exception(), Throwable::class.java)) + + assertFalse(hasExactCause(SocketException(), IOException::class.java)) + assertFalse(hasExactCause(IllegalStateException(), RuntimeException::class.java)) + assertFalse(hasExactCause(Exception(SocketException()), IOException::class.java)) + assertFalse(hasExactCause(Exception(IllegalStateException(Exception(IOException()))), RuntimeException::class.java)) + assertFalse(hasExactCause(Exception(IllegalStateException(Exception(SocketException()))), IOException::class.java)) + assertFalse(hasExactCause(Exception(IllegalStateException(Exception(InterruptedIOException()))), IOException::class.java)) + } +} \ No newline at end of file From 913796ff0fe0c4488648e679075ca662cf367dcd Mon Sep 17 00:00:00 2001 From: Mauricio Colli Date: Sat, 28 Mar 2020 20:10:48 -0300 Subject: [PATCH 2/2] Use exception utils in network error detection throughout the app --- .../schabi/newpipe/fragments/BaseStateFragment.java | 3 +-- .../newpipe/fragments/list/search/SearchFragment.java | 11 +---------- .../newpipe/local/feed/service/FeedLoadService.kt | 8 +++++--- .../services/BaseImportExportService.java | 4 ++-- .../services/SubscriptionsImportService.java | 5 ++++- .../java/org/schabi/newpipe/util/ExtractorHelper.java | 3 +-- 6 files changed, 14 insertions(+), 20 deletions(-) diff --git a/app/src/main/java/org/schabi/newpipe/fragments/BaseStateFragment.java b/app/src/main/java/org/schabi/newpipe/fragments/BaseStateFragment.java index 3167c4632..d71b933c2 100644 --- a/app/src/main/java/org/schabi/newpipe/fragments/BaseStateFragment.java +++ b/app/src/main/java/org/schabi/newpipe/fragments/BaseStateFragment.java @@ -26,7 +26,6 @@ import org.schabi.newpipe.report.UserAction; import org.schabi.newpipe.util.ExceptionUtils; import org.schabi.newpipe.util.InfoCache; -import java.io.IOException; import java.util.Collections; import java.util.List; import java.util.concurrent.TimeUnit; @@ -213,7 +212,7 @@ public abstract class BaseStateFragment extends BaseFragment implements ViewC } else if (exception instanceof ContentNotAvailableException) { showError(getString(R.string.content_not_available), false); return true; - } else if (exception instanceof IOException) { + } else if (ExceptionUtils.isNetworkRelated(exception)) { showError(getString(R.string.network_error), true); return true; } diff --git a/app/src/main/java/org/schabi/newpipe/fragments/list/search/SearchFragment.java b/app/src/main/java/org/schabi/newpipe/fragments/list/search/SearchFragment.java index b3c7ae189..e49126757 100644 --- a/app/src/main/java/org/schabi/newpipe/fragments/list/search/SearchFragment.java +++ b/app/src/main/java/org/schabi/newpipe/fragments/list/search/SearchFragment.java @@ -41,7 +41,6 @@ import org.schabi.newpipe.extractor.StreamingService; import org.schabi.newpipe.extractor.exceptions.ParsingException; import org.schabi.newpipe.extractor.search.SearchExtractor; import org.schabi.newpipe.extractor.search.SearchInfo; -import org.schabi.newpipe.util.ExceptionUtils; import org.schabi.newpipe.fragments.BackPressable; import org.schabi.newpipe.fragments.list.BaseListFragment; import org.schabi.newpipe.local.history.HistoryRecordManager; @@ -54,9 +53,6 @@ import org.schabi.newpipe.util.FireTvUtils; import org.schabi.newpipe.util.NavigationHelper; import org.schabi.newpipe.util.ServiceHelper; -import java.io.IOException; -import java.io.InterruptedIOException; -import java.net.SocketException; import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; @@ -764,12 +760,7 @@ public class SearchFragment extends BaseListFragment throw error - cause is IOException -> throw cause - error is ReCaptchaException -> throw error cause is ReCaptchaException -> throw cause + + error is IOException -> throw error + cause is IOException -> throw cause + ExceptionUtils.isNetworkRelated(error) -> throw IOException(error) } } } diff --git a/app/src/main/java/org/schabi/newpipe/local/subscription/services/BaseImportExportService.java b/app/src/main/java/org/schabi/newpipe/local/subscription/services/BaseImportExportService.java index 28bacaadd..f485844ea 100644 --- a/app/src/main/java/org/schabi/newpipe/local/subscription/services/BaseImportExportService.java +++ b/app/src/main/java/org/schabi/newpipe/local/subscription/services/BaseImportExportService.java @@ -38,9 +38,9 @@ import org.schabi.newpipe.extractor.subscription.SubscriptionExtractor; import org.schabi.newpipe.local.subscription.SubscriptionManager; import org.schabi.newpipe.report.ErrorActivity; import org.schabi.newpipe.report.UserAction; +import org.schabi.newpipe.util.ExceptionUtils; import java.io.FileNotFoundException; -import java.io.IOException; import java.util.Collections; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; @@ -227,7 +227,7 @@ public abstract class BaseImportExportService extends Service { message = getString(R.string.invalid_source); } else if (error instanceof FileNotFoundException) { message = getString(R.string.invalid_file); - } else if (error instanceof IOException) { + } else if (ExceptionUtils.isNetworkRelated(error)) { message = getString(R.string.network_error); } return message; diff --git a/app/src/main/java/org/schabi/newpipe/local/subscription/services/SubscriptionsImportService.java b/app/src/main/java/org/schabi/newpipe/local/subscription/services/SubscriptionsImportService.java index 70d061d7e..06ba55106 100644 --- a/app/src/main/java/org/schabi/newpipe/local/subscription/services/SubscriptionsImportService.java +++ b/app/src/main/java/org/schabi/newpipe/local/subscription/services/SubscriptionsImportService.java @@ -35,6 +35,7 @@ import org.schabi.newpipe.extractor.NewPipe; import org.schabi.newpipe.extractor.channel.ChannelInfo; import org.schabi.newpipe.extractor.subscription.SubscriptionItem; import org.schabi.newpipe.util.Constants; +import org.schabi.newpipe.util.ExceptionUtils; import org.schabi.newpipe.util.ExtractorHelper; import java.io.File; @@ -245,8 +246,10 @@ public class SubscriptionsImportService extends BaseImportExportService { final Throwable cause = error.getCause(); if (error instanceof IOException) { throw (IOException) error; - } else if (cause != null && cause instanceof IOException) { + } else if (cause instanceof IOException) { throw (IOException) cause; + } else if (ExceptionUtils.isNetworkRelated(error)) { + throw new IOException(error); } eventListener.onItemCompleted(""); diff --git a/app/src/main/java/org/schabi/newpipe/util/ExtractorHelper.java b/app/src/main/java/org/schabi/newpipe/util/ExtractorHelper.java index 95e2b65d3..813b1f6b9 100644 --- a/app/src/main/java/org/schabi/newpipe/util/ExtractorHelper.java +++ b/app/src/main/java/org/schabi/newpipe/util/ExtractorHelper.java @@ -51,7 +51,6 @@ import org.schabi.newpipe.extractor.suggestion.SuggestionExtractor; import org.schabi.newpipe.report.ErrorActivity; import org.schabi.newpipe.report.UserAction; -import java.io.IOException; import java.util.Collections; import java.util.List; @@ -288,7 +287,7 @@ public final class ExtractorHelper { Intent intent = new Intent(context, ReCaptchaActivity.class); intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK); context.startActivity(intent); - } else if (exception instanceof IOException) { + } else if (ExceptionUtils.isNetworkRelated(exception)) { Toast.makeText(context, R.string.network_error, Toast.LENGTH_LONG).show(); } else if (exception instanceof ContentNotAvailableException) { Toast.makeText(context, R.string.content_not_available, Toast.LENGTH_LONG).show();