[geary/wip/410-mail-ru-redux: 1/2] Update IMAP error handling a bit for consistency

[Michael Gratton <mjog src gnome org>]

commit ec7fd5268b084d1950baa1d2c9218b81ea6ccfb0
Author: Michael Gratton <mike vee net>
Date:   Sat May 4 13:18:47 2019 +1000

    Update IMAP error handling a bit for consistency
    
    Update ImapError API docs for SERVER_ERROR and UNAVAILBALE, make sure
    IMAP classes are using those as appripriate.

 src/engine/imap/api/imap-folder-session.vala       | 98 ++++++++--------------
 src/engine/imap/command/imap-command.vala          |  4 +-
 src/engine/imap/imap-error.vala                    | 14 +++-
 src/engine/imap/transport/imap-client-session.vala |  4 +-
 4 files changed, 49 insertions(+), 71 deletions(-)
---
diff --git a/src/engine/imap/api/imap-folder-session.vala b/src/engine/imap/api/imap-folder-session.vala
index af013a9a..7885b79c 100644
--- a/src/engine/imap/api/imap-folder-session.vala
+++ b/src/engine/imap/api/imap-folder-session.vala
@@ -121,27 +121,7 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject {
         StatusResponse? response = yield session.select_async(
             mailbox, cancellable
         );
-
-        switch (response.status) {
-        case Status.OK:
-            // all good
-            break;
-
-        case Status.BAD:
-        case Status.NO:
-            throw new ImapError.NOT_SUPPORTED(
-                "Server disallowed SELECT %s: %s",
-                this.folder.path.to_string(),
-                response.to_string()
-            );
-
-        default:
-            throw new ImapError.SERVER_ERROR(
-                "Unable to SELECT %s: %s",
-                this.folder.path.to_string(),
-                response.to_string()
-            );
-        }
+        throw_on_not_ok(response, "SELECT " + this.folder.path.to_string());
 
         // if at end of SELECT command accepts_user_flags is still
         // UNKKNOWN, treat as TRUE because, according to IMAP spec, if
@@ -298,15 +278,16 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject {
         }
     }
 
-    // All commands must executed inside the cmd_mutex; returns FETCH or STORE results
+    // Executes a set of commands.
     //
-    // FETCH commands can generate a FolderError.RETRY.  State will be updated to accomodate retry,
-    // but all Commands must be regenerated to ensure new state is reflected in requests.
-    private async Gee.Map<Command, StatusResponse>? exec_commands_async(Gee.Collection<Command> cmds,
-                                                                        Gee.HashMap<SequenceNumber, 
FetchedData>? fetch_results,
-                                                                        Gee.Set<Imap.UID>? search_results,
-                                                                        Cancellable? cancellable)
-        throws Error {
+    // All commands must executed inside the cmd_mutex. Collects
+    // results in fetch_results or store results.
+    private async Gee.Map<Command,StatusResponse>?
+        exec_commands_async(Gee.Collection<Command> cmds,
+                            Gee.HashMap<SequenceNumber, FetchedData>? fetch_results,
+                            Gee.Set<Imap.UID>? search_results,
+                            GLib.Cancellable? cancellable)
+        throws GLib.Error {
         ClientSession session = claim_session();
         Gee.Map<Command, StatusResponse>? responses = null;
         int token = yield this.cmd_mutex.claim_async(cancellable);
@@ -333,7 +314,7 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject {
         }
 
         foreach (Command cmd in responses.keys) {
-            throw_on_failed_status(responses.get(cmd), cmd);
+            throw_on_not_ok(responses.get(cmd), cmd.to_string());
         }
 
         return responses;
@@ -367,41 +348,6 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject {
         return false;
     }
 
-    private void throw_on_failed_status(StatusResponse response, Command cmd) throws Error {
-        assert(response.is_completion);
-
-        switch (response.status) {
-            case Status.OK:
-                return;
-
-            case Status.NO:
-                throw new ImapError.SERVER_ERROR("Request %s failed on %s: %s", cmd.to_string(),
-                    to_string(), response.to_string());
-
-            case Status.BAD: {
-                // if a FetchBodyDataSpecifier is used to request for a header field BAD is returned,
-                // could be a specific formatting mistake some servers make of not allowing a space
-                // between the "header.fields" and list of email header names, i.e.
-                //
-                // "body[header.fields (references)]"
-                //
-                // If so, then enable a hack to work around this and retry the FETCH
-                if (retry_bad_header_fields_response(cmd, response)) {
-                    imap_header_fields_hack = true;
-
-                    throw new FolderError.RETRY("BAD response to header.fields FETCH BODY, retry with hack");
-                }
-
-                throw new ImapError.INVALID("Bad request %s on %s: %s", cmd.to_string(),
-                    to_string(), response.to_string());
-            }
-
-            default:
-                throw new ImapError.NOT_SUPPORTED("Unknown response status to %s on %s: %s",
-                    cmd.to_string(), to_string(), response.to_string());
-        }
-    }
-
     // Utility method for listing UIDs on the remote within the supplied range
     public async Gee.Set<Imap.UID>? list_uids_async(MessageSet msg_set, Cancellable? cancellable)
         throws Error {
@@ -1094,7 +1040,29 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject {
         return null;
     }
 
+    private void throw_on_not_ok(StatusResponse response, string cmd)
+        throws ImapError {
+        switch (response.status) {
+        case Status.OK:
+            // All good
+            break;
+
+        case Status.NO:
+            throw new ImapError.NOT_SUPPORTED(
+                "Request %s failed on %s: %s", cmd.to_string(),
+                to_string(), response.to_string()
+            );
+
+        default:
+            throw new ImapError.SERVER_ERROR(
+                "Unknown response status to %s on %s: %s",
+                cmd.to_string(), to_string(), response.to_string()
+            );
+        }
+    }
+
     private static bool required_but_not_set(Geary.Email.Field check, Geary.Email.Field users_fields, 
Geary.Email email) {
         return users_fields.require(check) ? !email.fields.is_all_set(check) : false;
     }
+
 }
diff --git a/src/engine/imap/command/imap-command.vala b/src/engine/imap/command/imap-command.vala
index 990b0fc9..fd980831 100644
--- a/src/engine/imap/command/imap-command.vala
+++ b/src/engine/imap/command/imap-command.vala
@@ -125,12 +125,12 @@ public class Geary.Imap.Command : BaseObject {
      */
     internal void assign_tag(Tag new_tag) throws ImapError {
         if (this.tag.is_assigned()) {
-            throw new ImapError.SERVER_ERROR(
+            throw new ImapError.NOT_SUPPORTED(
                 "%s: Command tag is already assigned", to_brief_string()
             );
         }
         if (!new_tag.is_assigned()) {
-            throw new ImapError.SERVER_ERROR(
+            throw new ImapError.NOT_SUPPORTED(
                 "%s: New tag is not assigned", to_brief_string()
             );
         }
diff --git a/src/engine/imap/imap-error.vala b/src/engine/imap/imap-error.vala
index 66fe9654..ddc7274f 100644
--- a/src/engine/imap/imap-error.vala
+++ b/src/engine/imap/imap-error.vala
@@ -30,10 +30,18 @@ public errordomain Geary.ImapError {
      * Indicates the connection is already established or authentication has been granted.
      */
     ALREADY_CONNECTED,
+
     /**
-     * Indicates a request failed according to a returned response.
+     * A request failed due to an explicit or implicit BAD response.
+     *
+     * An explicit BAD response is as per RFC 3501 §7.3.1. An implicit
+     * BAD response is when the server returns an unexpected response,
+     * for example, sends a status response for the same command twice.
+     *
+     * @see https://tools.ietf.org/html/rfc3501#section-7.1.3
      */
     SERVER_ERROR,
+
     /**
      * Indicates that an operation could not proceed without prior authentication.
      */
@@ -52,9 +60,9 @@ public errordomain Geary.ImapError {
      * This indicates a local time out, not one reported by the server.
      */
     TIMED_OUT,
+
     /**
-     * Network is unavailable.
+     * The remote IMAP server not currently available.
      */
     UNAVAILABLE
 }
-
diff --git a/src/engine/imap/transport/imap-client-session.vala 
b/src/engine/imap/transport/imap-client-session.vala
index d0d86baa..6dd7b39b 100644
--- a/src/engine/imap/transport/imap-client-session.vala
+++ b/src/engine/imap/transport/imap-client-session.vala
@@ -810,7 +810,9 @@ public class Geary.Imap.ClientSession : BaseObject {
         debug("[%s] Connect denied: %s", to_string(), status_response.to_string());
 
         fsm.do_post_transition(() => { session_denied(status_response.get_text()); });
-        connect_err = new ImapError.SERVER_ERROR("Session denied: %s", status_response.get_text());
+        connect_err = new ImapError.UNAVAILABLE(
+            "Session denied: %s", status_response.get_text()
+        );
 
         return State.LOGGED_OUT;
     }