public-mirrors/NewsBlur
1
0
Fork 0
mirror of https://github.com/samuelclay/NewsBlur.git synced 2025-09-18 21:50:56 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

Refactor API error handling to differentiate error causes.

Browse source
This commit is contained in:
dosiecki 2015-08-27 03:54:47 -07:00
parent 24020d7d3c
commit 23a28bde2f
9 changed files with 110 additions and 89 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
clients/android/NewsBlur/res/values/strings.xml
View file

@ -11,7 +11,7 @@
<string name="login_password_hint">password</string>
<string name="login_registration_email_hint">email address</string>
<string name="login_button_login">Log In</string>
<string name="login_message_error">There was problem connecting to NewsBlur. Check your internet connection.</string>
<string name="login_message_error">There was problem logging in to NewsBlur.</string>
<string name="login_logging_in">Logging in…</string>
<string name="login_logged_in">Logged in!</string>
<string name="login_retrieving_feeds">Retrieving feeds…</string>

12
clients/android/NewsBlur/src/com/newsblur/fragment/LoginProgressFragment.java
View file

@ -23,7 +23,7 @@ import com.newsblur.R;
import com.newsblur.activity.Login;
import com.newsblur.activity.Main;
import com.newsblur.network.APIManager;
import com.newsblur.network.domain.NewsBlurResponse;
import com.newsblur.network.domain.LoginResponse;
import com.newsblur.util.PrefsUtils;
import com.newsblur.util.UIUtils;
@ -69,7 +69,7 @@ public class LoginProgressFragment extends Fragment {
return v;
}
private class LoginTask extends AsyncTask<Void, Void, NewsBlurResponse> {
private class LoginTask extends AsyncTask<Void, Void, LoginResponse> {
@Override
protected void onPreExecute() {
Animation a = AnimationUtils.loadAnimation(getActivity(), R.anim.text_up);
@ -77,14 +77,14 @@ public class LoginProgressFragment extends Fragment {
}
@Override
protected NewsBlurResponse doInBackground(Void... params) {
NewsBlurResponse response = apiManager.login(username, password);
protected LoginResponse doInBackground(Void... params) {
LoginResponse response = apiManager.login(username, password);
apiManager.updateUserProfile();
return response;
}
@Override
protected void onPostExecute(NewsBlurResponse result) {
protected void onPostExecute(LoginResponse result) {
Context c = getActivity();
if (c == null) return; // we might have run past the lifecycle of the activity
if (!result.isError()) {
@ -104,7 +104,7 @@ public class LoginProgressFragment extends Fragment {
Intent startMain = new Intent(getActivity(), Main.class);
c.startActivity(startMain);
} else {
UIUtils.safeToast(c, result.getErrorMessage(), Toast.LENGTH_LONG);
UIUtils.safeToast(c, result.getErrorMessage(c.getString(R.string.login_message_error)), Toast.LENGTH_LONG);
startActivity(new Intent(c, Login.class));
}
}

1
clients/android/NewsBlur/src/com/newsblur/network/APIConstants.java
View file

@ -27,6 +27,7 @@ public class APIConstants {
public static final String URL_SOCIALFEED_STORIES = NEWSBLUR_URL + "/social/stories";
public static final String URL_SIGNUP = NEWSBLUR_URL + "/api/signup";
public static final String URL_MARK_FEED_AS_READ = NEWSBLUR_URL + "/reader/mark_feed_as_read/";
//public static final String URL_MARK_FEED_AS_READ = "http://httpstat.us/502";
public static final String URL_MARK_ALL_AS_READ = NEWSBLUR_URL + "/reader/mark_all_as_read/";
public static final String URL_MARK_STORIES_READ = NEWSBLUR_URL + "/reader/mark_story_hashes_as_read/";
public static final String URL_SHARE_STORY = NEWSBLUR_URL + "/social/share_story";

20
clients/android/NewsBlur/src/com/newsblur/network/APIManager.java
View file

@ -30,6 +30,7 @@ import com.newsblur.domain.ValueMultimap;
import com.newsblur.network.domain.ActivitiesResponse;
import com.newsblur.network.domain.FeedFolderResponse;
import com.newsblur.network.domain.InteractionsResponse;
import com.newsblur.network.domain.LoginResponse;
import com.newsblur.network.domain.NewsBlurResponse;
import com.newsblur.network.domain.ProfileResponse;
import com.newsblur.network.domain.RegisterResponse;
@ -87,7 +88,7 @@ public class APIManager {
this.httpClient = new OkHttpClient();
}
public NewsBlurResponse login(final String username, final String password) {
public LoginResponse login(final String username, final String password) {
// This call should be pretty rare, but is expensive on the server side. Log it
// at an above-debug level so it will be noticed if it ever gets called too often.
Log.i(this.getClass().getName(), "calling login API");
@ -95,7 +96,7 @@ public class APIManager {
values.put(APIConstants.PARAMETER_USERNAME, username);
values.put(APIConstants.PARAMETER_PASSWORD, password);
final APIResponse response = post(APIConstants.URL_LOGIN, values);
NewsBlurResponse loginResponse = response.getResponse(gson);
LoginResponse loginResponse = response.getLoginResponse(gson);
if (!response.isError()) {
PrefsUtils.saveLogin(context, username, response.getCookie());
}
@ -167,22 +168,14 @@ public class APIManager {
}
APIResponse response = post(APIConstants.URL_MARK_FEED_AS_READ, values);
// TODO: these calls use a different return format than others: the errors field is an array, not an object
//return response.getResponse(gson, NewsBlurResponse.class);
NewsBlurResponse nbr = new NewsBlurResponse();
if (response.isError()) nbr.message = "err";
return nbr;
return response.getResponse(gson, NewsBlurResponse.class);
}
private NewsBlurResponse markAllAsRead() {
ValueMultimap values = new ValueMultimap();
values.put(APIConstants.PARAMETER_DAYS, "0");
APIResponse response = post(APIConstants.URL_MARK_ALL_AS_READ, values);
// TODO: these calls use a different return format than others: the errors field is an array, not an object
//return response.getResponse(gson, NewsBlurResponse.class);
NewsBlurResponse nbr = new NewsBlurResponse();
if (response.isError()) nbr.message = "err";
return nbr;
return response.getResponse(gson, NewsBlurResponse.class);
}
public NewsBlurResponse markStoryAsRead(String storyHash) {
@ -375,7 +368,8 @@ public class APIManager {
APIResponse response = get(APIConstants.URL_FEEDS, params);
if (response.isError()) {
Log.e(this.getClass().getName(), "Error fetching feeds: " + response.getErrorMessage());
// we can't use the magic polymorphism of NewsBlurResponse because this result uses
// a custom parser below. let the caller know the action failed.
return null;
}

92
clients/android/NewsBlur/src/com/newsblur/network/APIResponse.java
View file

@ -10,6 +10,7 @@ import com.google.gson.Gson;
import org.apache.http.HttpStatus;
import com.newsblur.R;
import com.newsblur.network.domain.LoginResponse;
import com.newsblur.network.domain.NewsBlurResponse;
import com.newsblur.util.AppConstants;
import com.squareup.okhttp.OkHttpClient;
@ -20,12 +21,12 @@ import com.squareup.okhttp.Response;
* A JSON-encoded response from the API servers. This class encodes the possible outcomes of
* an attempted API call, including total failure, online failures, and successful responses.
* In the latter case, the GSON reader used to look for errors is left open so that the expected
* response can be read. Instances of this class should be closed after use.
* response can be read.
*/
public class APIResponse {
private boolean isError;
private String errorMessage;
private int responseCode;
private String cookie;
private String responseBody;
public long connectTime;
@ -45,61 +46,49 @@ public class APIResponse {
*/
public APIResponse(Context context, OkHttpClient httpClient, Request request, int expectedReturnCode) {
this.errorMessage = context.getResources().getString(R.string.error_unset_message);
try {
long startTime = System.currentTimeMillis();
Response response = httpClient.newCall(request).execute();
connectTime = System.currentTimeMillis() - startTime;
if (response.isSuccessful()) {
this.responseCode = response.code();
if (response.code() != expectedReturnCode) {
Log.e(this.getClass().getName(), "API returned error code " + response.code() + " calling " + request.urlString() + ". Expected " + expectedReturnCode);
this.isError = true;
this.errorMessage = context.getResources().getString(R.string.error_http_connection);
return;
}
if (responseCode != expectedReturnCode) {
Log.e(this.getClass().getName(), "API returned error code " + response.code() + " calling " + request.urlString() + " - expected " + expectedReturnCode);
this.isError = true;
return;
}
this.cookie = response.header("Set-Cookie");
this.cookie = response.header("Set-Cookie");
try {
startTime = System.currentTimeMillis();
this.responseBody = response.body().string();
readTime = System.currentTimeMillis() - startTime;
} catch (Exception e) {
Log.e(this.getClass().getName(), e.getClass().getName() + " (" + e.getMessage() + ") reading " + request.urlString(), e);
this.isError = true;
this.errorMessage = context.getResources().getString(R.string.error_read_connection);
return;
}
try {
startTime = System.currentTimeMillis();
this.responseBody = response.body().string();
readTime = System.currentTimeMillis() - startTime;
} catch (Exception e) {
Log.e(this.getClass().getName(), e.getClass().getName() + " (" + e.getMessage() + ") reading " + request.urlString(), e);
this.isError = true;
return;
}
if (AppConstants.VERBOSE_LOG_NET) {
// the default kernel truncates log lines. split by something we probably have, like a json delim
if (responseBody.length() < 2048) {
Log.d(this.getClass().getName(), "API response: \n" + this.responseBody);
} else {
Log.d(this.getClass().getName(), "API response: ");
for (String s : TextUtils.split(responseBody, "\\}")) {
Log.d(this.getClass().getName(), s + "}");
}
if (AppConstants.VERBOSE_LOG_NET) {
// the default kernel truncates log lines. split by something we probably have, like a json delim
if (responseBody.length() < 2048) {
Log.d(this.getClass().getName(), "API response: \n" + this.responseBody);
} else {
Log.d(this.getClass().getName(), "API response: ");
for (String s : TextUtils.split(responseBody, "\\}")) {
Log.d(this.getClass().getName(), s + "}");
}
}
}
if (AppConstants.VERBOSE_LOG_NET) {
Log.d(this.getClass().getName(), String.format("called %s in %dms and %dms to read %dB", request.urlString(), connectTime, readTime, responseBody.length()));
}
} else {
Log.e(this.getClass().getName(), "API call unsuccessful, error code" + response.code());
this.isError = true;
this.errorMessage = context.getResources().getString(R.string.error_http_connection);
return;
if (AppConstants.VERBOSE_LOG_NET) {
Log.d(this.getClass().getName(), String.format("called %s in %dms and %dms to read %dB", request.urlString(), connectTime, readTime, responseBody.length()));
}
} catch (IOException ioe) {
Log.e(this.getClass().getName(), "Error (" + ioe.getMessage() + ") calling " + request.urlString(), ioe);
this.isError = true;
this.errorMessage = context.getResources().getString(R.string.error_read_connection);
return;
}
}
@ -109,17 +98,12 @@ public class APIResponse {
*/
public APIResponse(Context context) {
this.isError = true;
this.errorMessage = context.getResources().getString(R.string.error_offline);
}
public boolean isError() {
return this.isError;
}
public String getErrorMessage() {
return this.errorMessage;
}
/**
* Get the response object from this call. A specific subclass of NewsBlurResponse
* may be used for calls that return data, or the parent class may be used if no
@ -132,7 +116,6 @@ public class APIResponse {
// it's message field
try {
T response = classOfT.newInstance();
response.message = this.errorMessage;
response.isProtocolError = true;
return ((T) response);
} catch (Exception e) {
@ -149,8 +132,19 @@ public class APIResponse {
}
}
public NewsBlurResponse getResponse(Gson gson) {
return getResponse(gson, NewsBlurResponse.class);
/**
* Special binder for LoginResponses, since they can't inherit from NewsBlurResponse due to
* the design of the API fields.
*/
public LoginResponse getLoginResponse(Gson gson) {
if (this.isError) {
LoginResponse response = new LoginResponse();
response.isProtocolError = true;
return response;
} else {
LoginResponse response = gson.fromJson(this.responseBody, LoginResponse.class);
return response;
}
}
public String getResponseBody() {

38
clients/android/NewsBlur/src/com/newsblur/network/domain/LoginResponse.java Normal file
View file

@ -0,0 +1,38 @@
package com.newsblur.network.domain;
import android.util.Log;
/**
* A response handler for /api/login calls, since their error format is different than
* the rest of the API. GSON won't allow multiple declarations of fields, and since all
* other NewsBlur API responses other than this one encode the "errors" field as an array
* of strings, we can't just override NewsBlurResponse.
*/
public class LoginResponse {
// not part of the response schema, but populated by the API manager to indicate
// that we never got *any* valid JSON back
public boolean isProtocolError = false;
// Normally, the 'errors' field in the JSON response is an array of string values. For the
// login API, however, it returns an *object* containing an array of string values.
public ResponseErrors errors;
public boolean isError() {
if (isProtocolError) return true;
if ((errors != null) && (errors.message != null) && (errors.message.length > 0) && (errors.message[0] != null)) {
Log.d(this.getClass().getName(), "Response interpreted as error due to 'ResponseErrors' field: " + errors.message[0]);
return true;
}
return false;
}
/**
* Gets the error message returned by the API, or defaultMessage if none was found.
*/
public String getErrorMessage(String defaultMessage) {
if ((errors != null) &&(errors.message != null) && (errors.message.length > 0) && (errors.message[0] != null)) return errors.message[0];
return defaultMessage;
}
}

27
clients/android/NewsBlur/src/com/newsblur/network/domain/NewsBlurResponse.java
View file

@ -17,32 +17,31 @@ public class NewsBlurResponse {
public boolean authenticated;
public int code;
public String message;
public ResponseErrors errors;
public String[] errors;
public long readTime;
public static final Pattern KnownUserErrors = Pattern.compile("cannot mark as unread");
public boolean isError() {
if (isProtocolError) return true;
if ((message != null) && (!message.equals(""))) {
Log.d(this.getClass().getName(), "Response interpreted as error due to 'message' field: " + message);
return true;
}
if ((errors != null) && (errors.message != null) && (errors.message.length > 0) && (errors.message[0] != null)) {
Log.d(this.getClass().getName(), "Response interpreted as error due to 'ResponseErrors' field: " + errors.message[0]);
if ((errors != null) && (errors.length > 0) && (errors[0] != null)) {
Log.d(this.getClass().getName(), "Response interpreted as error due to 'errors' field: " + errors[0]);
return true;
}
return false;
}
// TODO: can we add a canonical flag of some sort to 100% of API responses that differentiates
// between 400-type and 2/3/500-type errors? Until then, we have to sniff known bad ones.
// between user and server errors? Until then, we have to sniff known bad ones, since all
// user errors have a 200 response rather than a 4xx.
public boolean isUserError() {
if (message != null) {
Matcher m = KnownUserErrors.matcher(message);
if (m.find()) return true;
}
if ((errors != null) && (errors.message.length > 0) && (errors.message[0] != null)) {
Matcher m = KnownUserErrors.matcher(errors.message[0]);
String err = getErrorMessage(null);
if (err != null) {
Matcher m = KnownUserErrors.matcher(err);
if (m.find()) return true;
}
return false;
@ -53,14 +52,8 @@ public class NewsBlurResponse {
*/
public String getErrorMessage(String defaultMessage) {
if ((message != null) && (!message.equals(""))) return message;
if ((errors != null) &&(errors.message != null) && (errors.message.length > 0) && (errors.message[0] != null)) return errors.message[0];
if ((errors != null) && (errors.length > 0) && (errors[0] != null)) return errors[0];
return defaultMessage;
}
/**
* Gets the error message returned by the API, or a simple numeric error code if non was found.
*/
public String getErrorMessage() {
return getErrorMessage(Integer.toString(code));
}
}

3
clients/android/NewsBlur/src/com/newsblur/service/NBSyncService.java
View file

@ -325,10 +325,11 @@ public class NBSyncService extends Service {
NewsBlurResponse response = ra.doRemote(apiManager);
if (response == null) {
Log.e(this.getClass().getName(), "Discarding reading action with internal API error.");
Log.e(this.getClass().getName(), "Discarding reading action with client-side error.");
dbHelper.clearAction(id);
} else if (response.isProtocolError) {
// the network failed or we got a non-200, so be sure we retry
Log.i(this.getClass().getName(), "Holding reading action with server-side or network error.");
continue actionsloop;
} else if (response.isError()) {
Log.e(this.getClass().getName(), "Discarding reading action with user error.");

4
clients/android/NewsBlur/src/com/newsblur/util/AppConstants.java
View file

@ -24,7 +24,7 @@ public class AppConstants {
public static final String LAST_SYNC_TIME = "LAST_SYNC_TIME";
// how long to wait before auto-syncing the feed/folder list
public static final long AUTO_SYNC_TIME_MILLIS = 15L * 60L * 1000L;
public static final long AUTO_SYNC_TIME_MILLIS = 20L * 60L * 1000L;
// how often to rebuild the DB
public static final long VACUUM_TIME_MILLIS = 12L * 60L * 60L * 1000L;
@ -40,7 +40,7 @@ public class AppConstants {
public static final int MAX_API_TRIES = 3;
// the base amount for how long to sleep during exponential API failure backoff
public static final long API_BACKOFF_BASE_MILLIS = 500L;
public static final long API_BACKOFF_BASE_MILLIS = 750L;
// when generating a request for multiple feeds, limit the total number requested to prevent
// unworkably long URLs