Merge lp:~tomdroid-dev/tomdroid/rework-auth-error-handling into lp:~tomdroid-maintainers/tomdroid/main

[Olivier Bilodeau]

Could one of you just run the branch and tell me it works for you (tm).

please please

Then we'll have wider testing as a launchpad.net release.

[Stefan Hammer (j-4-deactivatedaccount)]

I started my testengines... wait for a result shortly!

[Stefan Hammer (j-4-deactivatedaccount)]

I authenticated now several times and at the 3rd time I entered a false address and got two error messages on top of each other.
One says: The connection th the server has hailed, please check that the address you entered is correct. Exeption Exception"
The other says: The connection to the... correct. Exeption\n JSONException"
Both Dialogs don't show up immediately in the preferences dialog, but only after pressing the back-button, they are shown on top of the notes-list.

log:
04-01 10:22:31.942: E/SnowySyncService(10200): Unidentified authentication error. Expception: java.lang.Exception Message: Something went wrong during the authorization process. auth.getAccess returned false
04-01 10:22:31.942: W/System.err(10200): java.lang.Exception: Something went wrong during the authorization process. auth.getAccess returned false
04-01 10:22:31.942: W/System.err(10200): at org.tomdroid.sync.web.SnowySyncService$2.run(SnowySyncService.java:130)
04-01 10:22:31.942: W/System.err(10200): at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1088)
04-01 10:22:31.942: W/System.err(10200): at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:581)
04-01 10:22:31.942: W/System.err(10200): at java.lang.Thread.run(Thread.java:1019)

I hope this helps!

Another thing... I i am already authenticated to the server and I do it again just like this, I often get following, but it still sends the toast message, that it was sucessfull. This shows up after the first sync:
04-01 10:34:44.867: E/SnowySyncService(10367): Unidentified authentication error. Expception: java.lang.Exception Message: Something went wrong during the authorization process. auth.getAccess returned false
04-01 10:34:44.867: W/System.err(10367): java.lang.Exception: Something went wrong during the authorization process. auth.getAccess returned false
04-01 10:34:44.867: W/System.err(10367): at org.tomdroid.sync.web.SnowySyncService$2.run(SnowySyncService.java:130)
04-01 10:34:44.867: W/System.err(10367): at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1088)
04-01 10:34:44.867: W/System.err(10367): at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:581)
04-01 10:34:44.867: W/System.err(10367): at java.lang.Thread.run(Thread.java:1019)
04-01 10:34:45.127: E/WindowManager(10367): Activity org.tomdroid.ui.Tomdroid has leaked window com.android.internal.policy.impl.PhoneWindow$DecorView@40578d30 that was originally added here
04-01 10:34:45.127: E/WindowManager(10367): android.view.WindowLeaked: Activity org.tomdroid.ui.Tomdroid has leaked window com.android.internal.policy.impl.PhoneWindow$DecorView@40578d30 that was originally added here
04-01 10:34:45.127: E/WindowManager(10367): at android.view.ViewRoot.<init>(ViewRoot.java:258)
04-01 10:34:45.127: E/WindowManager(10367): at android.view.WindowManagerImpl.addView(WindowManagerImpl.java:148)
04-01 10:34:45.127: E/WindowManager(10367): at android.view.WindowManagerImpl.addView(WindowManagerImpl.java:91)
04-01 10:34:45.127: E/WindowManager(10367): at android.view.Window$LocalWindowManager.addView(Window.java:424)
04-01 10:34:45.127: ...

[Olivier Bilodeau (plaxx)]

Great testing! I'll try to address these soon. This fix is definitely going in the next release unless there's a cataclysm because we get a lot of these types of reports.

The error handling is quite convoluted (or I didn't figure it out properly) but making this possible shouldn't be too much work.

[Stefan Hammer (j-4-deactivatedaccount)]

I finally caught a log of the case where authentification with the right url fails leading to the "check your addres"-Dialog!

http://pastebin.com/aPCxfZGq

weird thing, as also in the browser of the SDK pages did not load, but there was also no "no internet connection" message... it just hung.

Here the Log: http://pastebin.com/aPCxfZGq

[lp:~tomdroid-dev/tomdroid/rework-auth-error-handling]

286. By Olivier Bilodeau on 2012-04-16

added tasks

I think it's ugly and shouldn't be done this way but I have no idea on how to better do it in a timtely fashion

287. By Olivier Bilodeau on 2012-04-16

errors now sent to proper activity

by sending the exception message to the handler which called the sync methods the error shows on the right activity

288. By Olivier Bilodeau on 2012-04-16

added parenthesis around exception stuff so diminish user confusion

[Olivier Bilodeau (plaxx)]

Preferences are trashed even by an obviously invalid URL. I tried to fix it but it's tricky and I would prefer it cleanly fixed so I filed a ticket with the whole story: lp:982749.

[Olivier Bilodeau (plaxx)]

Here's another bug experienced: the activity refresh is odd on first sync (lp:982752).

I also get exceptions on second account setup but, for now, they are handled correctly: they don't show up on the screen and are logged. I'm fine with that.

Forget about the two previously mentioned filed issues unless you think they are market release blocker. Can you test again and tell me how this works for you?

Unfortunately, through my own testing, I'm finding out that we've gone from a 'no idea' exception to a 'JSON' one since the JSON portion of the OAuth code seems to wrap all the other lower-level exceptions.. Again, we'll know more about this gets more thoroughly tested.

[lp:~tomdroid-dev/tomdroid/rework-auth-error-handling]

289. By Olivier Bilodeau on 2012-04-16

don't send an AUTH_SUCCESS if we got exceptions...

unfortunately this still doesn't fix the preference saving issue but is cleaner now

[Olivier Bilodeau (plaxx)]

@j-4 review request bump.

If you are busy that's ok, I'll do a launchpad release and we'll do the broader testing there.

As soon as I have a few contiguous hours I'll merge this, review/merge sql-args branch and do a release.

Unmerged revisions

289. By Olivier Bilodeau on 2012-04-16

don't send an AUTH_SUCCESS if we got exceptions...

unfortunately this still doesn't fix the preference saving issue but is cleaner now

288. By Olivier Bilodeau on 2012-04-16

added parenthesis around exception stuff so diminish user confusion

287. By Olivier Bilodeau on 2012-04-16

errors now sent to proper activity

by sending the exception message to the handler which called the sync methods the error shows on the right activity

286. By Olivier Bilodeau on 2012-04-16

added tasks

I think it's ugly and shouldn't be done this way but I have no idea on how to better do it in a timtely fashion

285. By Olivier Bilodeau on 2012-04-01

OAuth subsystem exception rework

OAuth authentication core is now throwing all throwable exceptions
Callers are now logging and sending to handlers
Handlers are now showing exception short name on the UI (starting with that since we don't know what to expect and don't want something like 'Token plaintext-token was rejected by server' shown on the screen)

=== modified file 'res/values/strings.xml'
--- res/values/strings.xml	2012-04-03 13:12:50 +0000
+++ res/values/strings.xml	2012-04-16 03:55:20 +0000
@@ -62,6 +62,7 @@
 	
 	<string name="prefSyncCompleteAuth">Completing authentication. Please wait...</string>
 	<string name="prefSyncConnectionFailed">The connection to the server has failed, please check that the address you entered is correct.</string>
+	<string name="prefSyncConnectionFailedWithDetails">The connection to the server has failed, please check that the address you entered is correct. (Exception: %1$s)</string>
 	<string name="prefServerEmpty">The server address changed but the new value is empty</string>
 	
 	<string name="titleSyncService">Choose the sync service to use</string>

=== modified file 'src/org/tomdroid/sync/SyncService.java'
--- src/org/tomdroid/sync/SyncService.java	2012-03-31 19:30:28 +0000
+++ src/org/tomdroid/sync/SyncService.java	2012-04-16 03:55:20 +0000
@@ -64,6 +64,9 @@
 	public final static int NO_INTERNET = 4;
 	public final static int NO_SD_CARD = 5;
 	public final static int SYNC_PROGRESS = 6;
+	public final static int AUTH_SUCCESS = 7;
+	public final static int ERROR_OAUTH_AUTHENTICATION = 8;
+	
 	
 	public SyncService(Activity activity, Handler handler) {
 		
@@ -184,14 +187,17 @@
 	 * 
 	 * @param message The message id to send, the PARSING_* or NO_INTERNET attributes can be used.
 	 */
-	
+	// TODO ok i'm not refactoring this right now but I see little value in such a wrapper 
+	// and lots of confusion for people who know android
 	protected void sendMessage(int message) {
 		
 		if(!sendMessage(message, null)) {
 			handler.sendEmptyMessage(message);
 		}
 	}
-	
+
+	// This stuff is convoluted and should be either be renamed, reworked or documented
+	// I tried hard to re-use it to propagate exceptions but I wasn't able to
 	protected boolean sendMessage(int message_id, HashMap<String, Object> payload) {
 		
 		switch(message_id) {
@@ -207,6 +213,18 @@
 		return false;
 	}
 	
+	protected void sendException(int messageId, Exception e) {
+		
+		Message message = handler.obtainMessage(messageId, e);
+		handler.sendMessage(message);
+	}
+
+	protected void sendExceptionToHandler(Handler handler, int messageId, Exception e) {
+		
+		Message message = handler.obtainMessage(messageId, e);
+		handler.sendMessage(message);
+	}
+	
 	/**
 	 * Update the synchronization progress
 	 * 

=== modified file 'src/org/tomdroid/sync/web/OAuthConnection.java'
--- src/org/tomdroid/sync/web/OAuthConnection.java	2012-03-31 15:54:09 +0000
+++ src/org/tomdroid/sync/web/OAuthConnection.java	2012-04-16 03:55:20 +0000
@@ -91,7 +91,7 @@
 		return provider;
 	}
 	
-	private void sign(HttpRequest request) {
+	private void sign(HttpRequest request) throws OAuthMessageSignerException, OAuthExpectationFailedException, OAuthCommunicationException {
 		
 		if (isAuthenticated())
 			consumer.setTokenWithSecret(accessToken, accessTokenSecret);
@@ -101,17 +101,16 @@
 		// TODO: figure out if we should throw exceptions
 		try {
 			consumer.sign(request);
-		} catch (OAuthMessageSignerException e1) {
-			e1.printStackTrace();
-		} catch (OAuthExpectationFailedException e1) {
-			e1.printStackTrace();
+		} catch (OAuthMessageSignerException e) {
+			throw e;
+		} catch (OAuthExpectationFailedException e) {
+			throw e;
 		} catch (OAuthCommunicationException e) {
-			// TODO Auto-generated catch block
-			e.printStackTrace();
+			throw e;
 		}
 	}
 	
-	public Uri getAuthorizationUrl(String server) throws UnknownHostException {
+	public Uri getAuthorizationUrl(String server) throws UnknownHostException, OAuthCommunicationException, OAuthMessageSignerException, OAuthNotAuthorizedException, OAuthExpectationFailedException, JSONException {
 		
 		String url = "";
 		
@@ -134,8 +133,7 @@
 			authorizeUrl = jsonResponse.getString("oauth_authorize_url");
 			
 		} catch (JSONException e) {
-			e.printStackTrace();
-			return null;
+			throw e;
 		}
 		
 		OAuthProvider provider = getProvider();
@@ -151,18 +149,14 @@
 			accessTokenSecret = "";
 			saveConfiguration();
 			
-		} catch (OAuthMessageSignerException e1) {
-			e1.printStackTrace();
-			return null;
-		} catch (OAuthNotAuthorizedException e1) {
-			e1.printStackTrace();
-			return null;
-		} catch (OAuthExpectationFailedException e1) {
-			e1.printStackTrace();
-			return null;
-		} catch (OAuthCommunicationException e1) {
-			e1.printStackTrace();
-			return null;
+		} catch (OAuthMessageSignerException e) {
+			throw e;
+		} catch (OAuthNotAuthorizedException e) {
+			throw e;
+		} catch (OAuthExpectationFailedException e) {
+			throw e;
+		} catch (OAuthCommunicationException e) {
+			throw e;
 		}
 		
 		TLog.i(TAG, "Authorization URL : {0}", url);
@@ -170,7 +164,7 @@
 		return Uri.parse(url);
 	}
 	
-	public boolean getAccess(String verifier) throws UnknownHostException {
+	public boolean getAccess(String verifier) throws UnknownHostException, OAuthMessageSignerException, OAuthNotAuthorizedException, OAuthExpectationFailedException, OAuthCommunicationException, JSONException {
 		
 		TLog.i(TAG, "Verifier: {0}", verifier);
 		
@@ -189,18 +183,14 @@
 		
 		try {
 			provider.retrieveAccessToken(consumer, verifier);
-		} catch (OAuthMessageSignerException e1) {
-			e1.printStackTrace();
-			return false;
-		} catch (OAuthNotAuthorizedException e1) {
-			e1.printStackTrace();
-			return false;
-		} catch (OAuthExpectationFailedException e1) {
-			e1.printStackTrace();
-			return false;
-		} catch (OAuthCommunicationException e1) {
-			e1.printStackTrace();
-			return false;
+		} catch (OAuthMessageSignerException e) {
+			throw e;
+		} catch (OAuthNotAuthorizedException e) {
+			throw e;
+		} catch (OAuthExpectationFailedException e) {
+			throw e;
+		} catch (OAuthCommunicationException e) {
+			throw e;
 		}
 		
 		// access has been granted, store the access token
@@ -214,8 +204,7 @@
 			// append a slash to the url, else the signature will fail
 			userApi = response.getJSONObject("user-ref").getString("api-ref");
 		} catch (JSONException e) {
-			// TODO Auto-generated catch block
-			e.printStackTrace();
+			throw e;
 		}
 		
 		saveConfiguration();
@@ -226,7 +215,7 @@
 	}
 	
 	@Override
-	public String get(String uri) throws java.net.UnknownHostException {
+	public String get(String uri) throws java.net.UnknownHostException, OAuthMessageSignerException, OAuthExpectationFailedException, OAuthCommunicationException {
 		
 		// Prepare a request object
 		HttpGet httpGet = new HttpGet(uri);
@@ -236,7 +225,7 @@
 	}
 	
 	@Override
-	public String put(String uri, String data) throws UnknownHostException {
+	public String put(String uri, String data) throws UnknownHostException, UnsupportedEncodingException, OAuthMessageSignerException, OAuthExpectationFailedException, OAuthCommunicationException {
 		
 		// Prepare a request object
 		HttpPut httpPut = new HttpPut(uri);
@@ -244,9 +233,8 @@
 		try {
 			// The default http content charset is ISO-8859-1, JSON requires UTF-8
 			httpPut.setEntity(new StringEntity(data, "UTF-8"));
-		} catch (UnsupportedEncodingException e1) {
-			e1.printStackTrace();
-			return null;
+		} catch (UnsupportedEncodingException e) {
+			throw e;
 		}
 		
 		httpPut.setHeader("Content-Type", "application/json");

=== modified file 'src/org/tomdroid/sync/web/SnowySyncService.java'
--- src/org/tomdroid/sync/web/SnowySyncService.java	2012-03-31 22:13:13 +0000
+++ src/org/tomdroid/sync/web/SnowySyncService.java	2012-04-16 03:55:20 +0000
@@ -26,6 +26,11 @@
 import android.net.Uri;
 import android.os.Handler;
 import android.os.Message;
+import oauth.signpost.exception.OAuthCommunicationException;
+import oauth.signpost.exception.OAuthExpectationFailedException;
+import oauth.signpost.exception.OAuthMessageSignerException;
+import oauth.signpost.exception.OAuthNotAuthorizedException;
+
 import org.json.JSONArray;
 import org.json.JSONException;
 import org.json.JSONObject;
@@ -83,21 +88,23 @@
 			
 			public void run() {
 				
-				// Reset the authentication credentials
-				OAuthConnection auth = new OAuthConnection();
-				Uri authUri = null;
-				
 				try {
-					authUri = auth.getAuthorizationUrl(server);
-					
+					// Reset the authentication credentials
+					OAuthConnection auth = new OAuthConnection();
+					Uri authUri = auth.getAuthorizationUrl(server);
+					Message message = handler.obtainMessage(AUTH_SUCCESS, authUri);
+					handler.sendMessage(message);
+
 				} catch (UnknownHostException e) {
 					TLog.e(TAG, "Internet connection not available");
 					sendMessage(NO_INTERNET);
+					
+				} catch (Exception e) {
+					TLog.e(TAG, "Unidentified authentication error. Expception: {0} Message: {1}", e.getClass().getCanonicalName(), e.getMessage());
+					e.printStackTrace();
+					sendExceptionToHandler(handler, ERROR_OAUTH_AUTHENTICATION, e);
 				}
 				
-				Message message = new Message();
-				message.obj = authUri;
-				handler.sendMessage(message);
 			}
 			
 		});
@@ -119,11 +126,15 @@
 					if (result) {
 						TLog.i(TAG, "The authorization process is complete.");
 					} else {
-						TLog.e(TAG, "Something went wrong during the authorization process.");
+						throw new Exception("Something went wrong during the authorization process. auth.getAccess returned false");
 					}
 				} catch (UnknownHostException e) {
 					TLog.e(TAG, "Internet connection not available");
 					sendMessage(NO_INTERNET);
+				} catch (Exception e) {
+					TLog.e(TAG, "Unidentified authentication error. Expception: {0} Message: {1}", e.getClass().getCanonicalName(), e.getMessage());
+					e.printStackTrace();
+					sendExceptionToHandler(handler, ERROR_OAUTH_AUTHENTICATION, e);
 				}
 				
 				// We don't care what we send, just remove the dialog
@@ -224,8 +235,15 @@
 					sendMessage(NO_INTERNET);
 					setSyncProgress(100);
 					return;
+					
+				} catch (Exception e) {
+					TLog.e(TAG, "Unidentified authentication error. Expception: {0} Message: {1}", e.getClass().getCanonicalName(), e.getMessage());
+					e.printStackTrace();
+					sendException(ERROR_OAUTH_AUTHENTICATION, e);
+					setSyncProgress(100);
+					return;
 				}
-				
+			
 				sendMessage(PARSING_COMPLETE);
 			}
 		});

=== modified file 'src/org/tomdroid/sync/web/WebConnection.java'
--- src/org/tomdroid/sync/web/WebConnection.java	2011-10-27 16:33:12 +0000
+++ src/org/tomdroid/sync/web/WebConnection.java	2012-04-16 03:55:20 +0000
@@ -22,6 +22,10 @@
  */
 package org.tomdroid.sync.web;
 
+import oauth.signpost.exception.OAuthCommunicationException;
+import oauth.signpost.exception.OAuthExpectationFailedException;
+import oauth.signpost.exception.OAuthMessageSignerException;
+
 import org.apache.http.HttpEntity;
 import org.apache.http.HttpResponse;
 import org.apache.http.client.ClientProtocolException;
@@ -33,14 +37,16 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
+import java.io.UnsupportedEncodingException;
 import java.net.UnknownHostException;
 
 public abstract class WebConnection {
 	
 	private static final String TAG = "WebConnection";
-	
-	public abstract String get(String uri) throws UnknownHostException;
-	public abstract String put(String uri, String data) throws UnknownHostException;
+
+	// TODO implementation specific exceptions should be removed from here but for now we want visibility in the UI of the problems
+	public abstract String get(String uri) throws UnknownHostException, OAuthMessageSignerException, OAuthExpectationFailedException, OAuthCommunicationException;
+	public abstract String put(String uri, String data) throws UnknownHostException, UnsupportedEncodingException, OAuthMessageSignerException, OAuthExpectationFailedException, OAuthCommunicationException;
 	
 	private static String convertStreamToString(InputStream is) {
 		/*

=== modified file 'src/org/tomdroid/ui/PreferencesActivity.java'
--- src/org/tomdroid/ui/PreferencesActivity.java	2012-04-01 13:27:09 +0000
+++ src/org/tomdroid/ui/PreferencesActivity.java	2012-04-16 03:55:20 +0000
@@ -41,6 +41,7 @@
 import android.preference.PreferenceActivity;
 import android.provider.SearchRecentSuggestions;
 import android.widget.Toast;
+
 import org.tomdroid.NoteManager;
 import org.tomdroid.R;
 import org.tomdroid.sync.ServiceAuth;
@@ -106,6 +107,7 @@
 			public boolean onPreferenceChange(Preference preference,
 					Object serverUri) {
 				
+				// FIXME this shouldn't be here, it should be up to the SyncService to decide
 				if (serverUri == null) {
 					Toast.makeText(PreferencesActivity.this,
 							getString(R.string.prefServerEmpty),
@@ -195,26 +197,27 @@
 			@Override
 			public void handleMessage(Message msg) {
 
-				boolean wasSuccsessful = false;
-				Uri authorizationUri = (Uri) msg.obj;
-				if (authorizationUri != null) {
-
-					Intent i = new Intent(Intent.ACTION_VIEW, authorizationUri);
-					startActivity(i);
-					wasSuccsessful = true;
-
-				} else {
-					// Auth failed, don't update the value
-					wasSuccsessful = false;
+				switch(msg.what) {
+					case SyncService.AUTH_SUCCESS:
+						Uri authorizationUri = (Uri) msg.obj;
+						if (authorizationUri != null) {
+							Intent i = new Intent(Intent.ACTION_VIEW, authorizationUri);
+							startActivity(i);
+							resetLocalDatabase();
+						}
+						break;
+						
+					case SyncService.ERROR_OAUTH_AUTHENTICATION:
+						connectionFailed((Exception) msg.obj);
+						break;
+					
+					default:
+						connectionFailed();
+						break;
 				}
-
-				if (authProgress != null)
+				
+				if (authProgress != null) {
 					authProgress.dismiss();
-
-				if (wasSuccsessful) {
-					resetLocalDatabase();
-				} else {
-					connectionFailed();
 				}
 			}
 		};
@@ -279,6 +282,16 @@
 			.show();
 	}
 	
+	private void connectionFailed(Exception e) {
+		new AlertDialog.Builder(this)
+			.setMessage(String.format(getString(R.string.prefSyncConnectionFailedWithDetails), e.getClass().getSimpleName()))
+			.setNeutralButton(getString(R.string.btnOk), new OnClickListener() {
+				public void onClick(DialogInterface dialog, int which) {
+					dialog.dismiss();
+				}})
+			.show();
+	}
+
 	private void folderNotExisting(String path) {
 		new AlertDialog.Builder(this)
 			.setTitle(getString(R.string.error))

=== modified file 'src/org/tomdroid/ui/SyncMessageHandler.java'
--- src/org/tomdroid/ui/SyncMessageHandler.java	2012-04-03 13:12:50 +0000
+++ src/org/tomdroid/ui/SyncMessageHandler.java	2012-04-16 03:55:20 +0000
@@ -62,6 +62,7 @@
 			case SyncService.PARSING_COMPLETE:
 				final ErrorList errors = (ErrorList)msg.obj;
 				if(errors.isEmpty()) {
+					
 					message = this.activity.getString(R.string.messageSyncComplete);
 					message = String.format(message,serviceDescription);
 					Toast.makeText(activity, message, Toast.LENGTH_SHORT).show();
@@ -92,10 +93,26 @@
 				Toast.makeText(activity, message, Toast.LENGTH_SHORT).show();
 				break;
 				
+			// FIXME shouldn't be here (should be in PreferencesActivity)
 			case SyncService.NO_INTERNET:
 				Toast.makeText(activity, this.activity.getString(R.string.messageSyncNoConnection),
 						Toast.LENGTH_SHORT).show();
 				break;
+			
+			// FIXME shouldn't be here (should be in PreferencesActivity)
+			case SyncService.ERROR_OAUTH_AUTHENTICATION:
+				new AlertDialog.Builder(this.activity)
+				.setMessage(
+						String.format(
+								this.activity.getString(R.string.prefSyncConnectionFailedWithDetails), 
+								((Exception) msg.obj).getClass().getSimpleName()))
+				.setNeutralButton(this.activity.getString(R.string.btnOk), new OnClickListener() {
+					public void onClick(DialogInterface dialog, int which) {
+						dialog.dismiss();
+					}})
+				.show();
+				break;
+			
 				
 			case SyncService.NO_SD_CARD:
 				Toast.makeText(activity, activity.getString(R.string.messageNoSDCard),