Niedzielski has uploaded a new change for review. ( https://gerrit.wikimedia.org/r/340680 )
Change subject: Hygiene: move MediaWiki fallback checks from clients to response interceptor ...................................................................... Hygiene: move MediaWiki fallback checks from clients to response interceptor For testing, change the RESTBaseUriFormat dev setting to http://host:6927/%2$s/v1/ on a fresh install, start a local Content Service instance, and observe that the requestSucesses dev setting remains unset after making several successful page requests. Then stop the Content Service instance, make an endpoint throw an unexpected error[0], and exercise the endpoint. requestSuccesses should drop to -1 for errors, then back to 0 once successful endpoints are interacted with This patch is intended to refactor the existing functionality to use interceptors but no other functional change is intended except: • All requests are now considered • RESTBase development endpoints are now considered by the app to be RESTBase endpoints [0] For example, add `throw new Error` to the start of parseDefinition.hasUsageExamples() and then try to define a word in the app Bug: T156917 Change-Id: Ie0526793f2577eaade2ffc365c2d0a9cf54e2ead --- M app/src/main/java/org/wikipedia/dataclient/WikiSite.java M app/src/main/java/org/wikipedia/dataclient/mwapi/page/MwPageClient.java M app/src/main/java/org/wikipedia/dataclient/okhttp/OkHttpConnectionFactory.java A app/src/main/java/org/wikipedia/dataclient/okhttp/StatusResponseInterceptor.java M app/src/main/java/org/wikipedia/dataclient/restbase/page/RbPageClient.java M app/src/main/java/org/wikipedia/dataclient/retrofit/RetrofitException.java M app/src/main/java/org/wikipedia/settings/Prefs.java M app/src/main/java/org/wikipedia/settings/RbSwitch.java 8 files changed, 89 insertions(+), 34 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/apps/android/wikipedia refs/changes/80/340680/1 diff --git a/app/src/main/java/org/wikipedia/dataclient/WikiSite.java b/app/src/main/java/org/wikipedia/dataclient/WikiSite.java index 0c72a6c..4828dc4 100644 --- a/app/src/main/java/org/wikipedia/dataclient/WikiSite.java +++ b/app/src/main/java/org/wikipedia/dataclient/WikiSite.java @@ -16,7 +16,7 @@ import java.util.Locale; /** - * The base URL and Wikipedia language code for a wiki site. Examples: + * The base URL and Wikipedia language code for a MediaWiki site. Examples: * * <ul> * <lh>Name: scheme / authority / language code</lh> diff --git a/app/src/main/java/org/wikipedia/dataclient/mwapi/page/MwPageClient.java b/app/src/main/java/org/wikipedia/dataclient/mwapi/page/MwPageClient.java index 41335a1..3ef5132 100644 --- a/app/src/main/java/org/wikipedia/dataclient/mwapi/page/MwPageClient.java +++ b/app/src/main/java/org/wikipedia/dataclient/mwapi/page/MwPageClient.java @@ -9,7 +9,6 @@ import org.wikipedia.dataclient.page.PageRemaining; import org.wikipedia.dataclient.page.PageSummary; import org.wikipedia.dataclient.retrofit.RetrofitException; -import org.wikipedia.settings.RbSwitch; import org.wikipedia.zero.WikipediaZeroHandler; import java.io.IOException; @@ -93,7 +92,6 @@ @Override public void onResponse(Call<MwMobileViewPageRemaining> call, Response<MwMobileViewPageRemaining> response) { if (response.isSuccessful()) { - RbSwitch.INSTANCE.onMwSuccess(); cb.success(response.body()); } else { cb.failure(RetrofitException.httpError(response)); diff --git a/app/src/main/java/org/wikipedia/dataclient/okhttp/OkHttpConnectionFactory.java b/app/src/main/java/org/wikipedia/dataclient/okhttp/OkHttpConnectionFactory.java index b04c71b..d562396 100644 --- a/app/src/main/java/org/wikipedia/dataclient/okhttp/OkHttpConnectionFactory.java +++ b/app/src/main/java/org/wikipedia/dataclient/okhttp/OkHttpConnectionFactory.java @@ -7,6 +7,7 @@ import org.wikipedia.WikipediaApp; import org.wikipedia.dataclient.SharedPreferenceCookieManager; import org.wikipedia.settings.Prefs; +import org.wikipedia.settings.RbSwitch; import java.io.IOException; import java.net.HttpURLConnection; @@ -51,6 +52,7 @@ .cookieJar(cookieJar) .cache(HTTP_CACHE) .addInterceptor(new HttpLoggingInterceptor().setLevel(Prefs.getRetrofitLogLevel())) + .addInterceptor(new StatusResponseInterceptor(RbSwitch.INSTANCE)) .addNetworkInterceptor(new StripMustRevalidateResponseInterceptor()) .addInterceptor(new CommonHeaderRequestInterceptor()) .addInterceptor(new DefaultMaxStaleRequestInterceptor()) diff --git a/app/src/main/java/org/wikipedia/dataclient/okhttp/StatusResponseInterceptor.java b/app/src/main/java/org/wikipedia/dataclient/okhttp/StatusResponseInterceptor.java new file mode 100644 index 0000000..55e98c1 --- /dev/null +++ b/app/src/main/java/org/wikipedia/dataclient/okhttp/StatusResponseInterceptor.java @@ -0,0 +1,54 @@ +package org.wikipedia.dataclient.okhttp; + +import android.support.annotation.NonNull; +import android.support.annotation.Nullable; + +import org.wikipedia.dataclient.okhttp.util.HttpUrlUtil; +import org.wikipedia.dataclient.retrofit.RetrofitException; +import org.wikipedia.settings.RbSwitch; + +import java.io.IOException; + +import okhttp3.HttpUrl; +import okhttp3.Interceptor; +import okhttp3.Response; + +public class StatusResponseInterceptor implements Interceptor { + @NonNull private final RbSwitch cb; + + public StatusResponseInterceptor(@NonNull RbSwitch cb) { + this.cb = cb; + } + + @Override public Response intercept(Chain chain) throws IOException { + HttpUrl url = chain.request().url(); + + Response rsp; + try { + rsp = chain.proceed(chain.request()); + } catch (Throwable t) { + failure(url, t); + throw t; + } + + if (rsp != null && rsp.isSuccessful()) { + success(url); + } else { + failure(url, rsp == null ? null : RetrofitException.httpError(rsp)); + } + + return rsp; + } + + private void success(@NonNull HttpUrl url) { + if (HttpUrlUtil.isMobileView(url)) { + cb.onMwSuccess(); + } + } + + private void failure(@NonNull HttpUrl url, @Nullable Throwable t) { + if (HttpUrlUtil.isRestBase(url)) { + cb.onRbRequestFailed(t); + } + } +} diff --git a/app/src/main/java/org/wikipedia/dataclient/restbase/page/RbPageClient.java b/app/src/main/java/org/wikipedia/dataclient/restbase/page/RbPageClient.java index cce7c0e..255d8d9 100644 --- a/app/src/main/java/org/wikipedia/dataclient/restbase/page/RbPageClient.java +++ b/app/src/main/java/org/wikipedia/dataclient/restbase/page/RbPageClient.java @@ -14,7 +14,6 @@ import org.wikipedia.dataclient.page.PageSummary; import org.wikipedia.dataclient.restbase.RbDefinition; import org.wikipedia.dataclient.retrofit.RetrofitException; -import org.wikipedia.settings.RbSwitch; import org.wikipedia.zero.WikipediaZeroHandler; import java.io.IOException; @@ -61,7 +60,6 @@ cb.success(response.body()); } else { Throwable throwable = RetrofitException.httpError(response); - RbSwitch.INSTANCE.onRbRequestFailed(throwable); cb.failure(throwable); } } @@ -72,7 +70,6 @@ */ @Override public void onFailure(Call<RbPageSummary> call, Throwable t) { - RbSwitch.INSTANCE.onRbRequestFailed(t); cb.failure(t); } }); @@ -92,14 +89,12 @@ cb.success(pageLead); } else { Throwable throwable = RetrofitException.httpError(response); - RbSwitch.INSTANCE.onRbRequestFailed(throwable); cb.failure(throwable); } } @Override public void onFailure(Call<RbPageLead> call, Throwable t) { - RbSwitch.INSTANCE.onRbRequestFailed(t); cb.failure(t); } }); @@ -115,14 +110,12 @@ cb.success(response.body()); } else { Throwable throwable = RetrofitException.httpError(response); - RbSwitch.INSTANCE.onRbRequestFailed(throwable); cb.failure(throwable); } } @Override public void onFailure(Call<RbPageRemaining> call, Throwable t) { - RbSwitch.INSTANCE.onRbRequestFailed(t); cb.failure(t); } }); @@ -159,14 +152,12 @@ cb.success(new RbDefinition(response.body())); } else { Throwable throwable = RetrofitException.httpError(response); - RbSwitch.INSTANCE.onRbRequestFailed(throwable); cb.failure(throwable); } } @Override public void onFailure(Call<Map<String, RbDefinition.Usage[]>> call, Throwable throwable) { - RbSwitch.INSTANCE.onRbRequestFailed(throwable); cb.failure(throwable); } }); diff --git a/app/src/main/java/org/wikipedia/dataclient/retrofit/RetrofitException.java b/app/src/main/java/org/wikipedia/dataclient/retrofit/RetrofitException.java index 86bc6be..191f3b6 100644 --- a/app/src/main/java/org/wikipedia/dataclient/retrofit/RetrofitException.java +++ b/app/src/main/java/org/wikipedia/dataclient/retrofit/RetrofitException.java @@ -1,5 +1,8 @@ package org.wikipedia.dataclient.retrofit; +import android.support.annotation.NonNull; +import android.support.annotation.Nullable; + import java.io.IOException; import retrofit2.Response; @@ -14,7 +17,13 @@ public static RetrofitException httpError(String url, Response<?> response) { String message = response.code() + " " + response.message(); - return new RetrofitException(message, url, response, Kind.HTTP, null); + return new RetrofitException(message, url, response.code(), Kind.HTTP, null); + } + + public static RetrofitException httpError(@NonNull okhttp3.Response response) { + String message = response.code() + " " + response.message(); + return new RetrofitException(message, response.request().url().toString(), response.code(), Kind.HTTP, + null); } public static RetrofitException networkError(IOException exception) { @@ -39,13 +48,13 @@ } private final String url; - private final Response<?> response; + @Nullable private final Integer code; private final Kind kind; - RetrofitException(String message, String url, Response<?> response, Kind kind, Throwable exception) { + RetrofitException(String message, String url, @Nullable Integer code, Kind kind, Throwable exception) { super(message, exception); this.url = url; - this.response = response; + this.code = code; this.kind = kind; } @@ -54,9 +63,9 @@ return url; } - /** Response object containing status code, headers, body, etc. */ - public Response<?> getResponse() { - return response; + /** HTTP status code. */ + @Nullable public Integer getCode() { + return code; } /** The event kind which triggered this error. */ diff --git a/app/src/main/java/org/wikipedia/settings/Prefs.java b/app/src/main/java/org/wikipedia/settings/Prefs.java index 746d8e0..00d5289 100644 --- a/app/src/main/java/org/wikipedia/settings/Prefs.java +++ b/app/src/main/java/org/wikipedia/settings/Prefs.java @@ -1,6 +1,7 @@ package org.wikipedia.settings; import android.net.Uri; +import android.support.annotation.IntRange; import android.support.annotation.NonNull; import android.support.annotation.Nullable; @@ -333,11 +334,11 @@ setInt(R.string.preference_key_restbase_ticket, rbTicket); } - public static int getRequestSuccessCounter(int defaultValue) { + @IntRange(from = RbSwitch.FAILED) public static int getRequestSuccessCounter(int defaultValue) { return getInt(R.string.preference_key_request_successes, defaultValue); } - public static void setRequestSuccessCounter(int successes) { + public static void setRequestSuccessCounter(@IntRange(from = RbSwitch.FAILED) int successes) { setInt(R.string.preference_key_request_successes, successes); } diff --git a/app/src/main/java/org/wikipedia/settings/RbSwitch.java b/app/src/main/java/org/wikipedia/settings/RbSwitch.java index 401fd9b..16579de 100644 --- a/app/src/main/java/org/wikipedia/settings/RbSwitch.java +++ b/app/src/main/java/org/wikipedia/settings/RbSwitch.java @@ -1,5 +1,8 @@ package org.wikipedia.settings; +import android.support.annotation.IntRange; +import android.support.annotation.Nullable; + import org.wikipedia.WikipediaApp; import org.wikipedia.dataclient.WikiSite; import org.wikipedia.dataclient.retrofit.RetrofitException; @@ -18,9 +21,11 @@ * So, 404s and network errors are ignored. */ public final class RbSwitch { + public static final int FAILED = -1; private static final int HUNDRED_PERCENT = 100; private static final int SUCCESS_THRESHOLD = 5; // page loads - private static final int FAILED = -1; + private static final String ENABLE_RESTBASE_PERCENT_CONFIG_KEY = ReleaseUtil.isProdRelease() + ? "restbaseProdPercent" : "restbaseBetaPercent"; public static final RbSwitch INSTANCE = new RbSwitch(); @@ -61,16 +66,12 @@ Prefs.setRbTicket(ticket); } - if (ReleaseUtil.isProdRelease()) { - return isAdmitted(ticket, "restbaseProdPercent"); - } else { - return isAdmitted(ticket, "restbaseBetaPercent"); - } + return isAdmitted(ticket, ENABLE_RESTBASE_PERCENT_CONFIG_KEY); } - private static boolean isAdmitted(int ticket, String configKey) { - int admittedPct = WikipediaApp.getInstance() - .getRemoteConfig().getConfig().optInt(configKey, 0); // [0, 100] + private static boolean isAdmitted(@IntRange(from = 1, to = 100) int ticket, String configKey) { + @IntRange(from = 0, to = 100) int admittedPct = WikipediaApp.getInstance() + .getRemoteConfig().getConfig().optInt(configKey, 0); // 0 = disable return ticket <= admittedPct; } @@ -97,7 +98,7 @@ /** * Call this method when a RESTBase call fails. */ - public void onRbRequestFailed(Throwable error) { + public void onRbRequestFailed(@Nullable Throwable error) { if (isSignificantFailure(error)) { markRbFailed(); if (!Prefs.useRestBaseSetManually()) { @@ -111,15 +112,14 @@ * We don't want to fallback just because of a user error (404) * or a network issue on the client side (RetrofitError.Kind.NETWORK). */ - private static boolean isSignificantFailure(Throwable throwable) { + private static boolean isSignificantFailure(@Nullable Throwable throwable) { if (!(throwable instanceof RetrofitException)) { return false; } RetrofitException error = (RetrofitException) throwable; if (error.getKind() == RetrofitException.Kind.HTTP) { - int status = error.getResponse().code(); - return status != HTTP_NOT_FOUND; + return error.getCode() != null && error.getCode() != HTTP_NOT_FOUND; } return error.getKind() != RetrofitException.Kind.NETWORK; } -- To view, visit https://gerrit.wikimedia.org/r/340680 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ie0526793f2577eaade2ffc365c2d0a9cf54e2ead Gerrit-PatchSet: 1 Gerrit-Project: apps/android/wikipedia Gerrit-Branch: master Gerrit-Owner: Niedzielski <sniedziel...@wikimedia.org> Gerrit-Reviewer: Sniedzielski <sniedziel...@wikimedia.org> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
