QPID-6016: [Java Broker] Web UI error handling

Address review comments from Alex Rudyy <orudyy@apache.org>

git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk@1618889 13f79535-47bb-0310-9956-ffa450edef68

4 files changed, 37 insertions, 33 deletions

diff --git a/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/RestServlet.java b/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/RestServlet.java
index 171c0d1e1c..dc1f5bba46 100644
--- a/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/RestServlet.java
+++ b/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/RestServlet.java
@@ -17,7 +17,6 @@
 package org.apache.qpid.server.management.plugin.servlet.rest;
 
 import java.io.IOException;
-import java.io.PrintWriter;
 import java.io.Writer;
 import java.security.AccessControlException;
 import java.util.ArrayList;
@@ -43,6 +42,7 @@ import org.apache.log4j.Logger;
 import org.codehaus.jackson.map.ObjectMapper;
 import org.codehaus.jackson.map.SerializationConfig;
 
+import org.apache.qpid.server.configuration.IllegalConfigurationException;
 import org.apache.qpid.server.model.Broker;
 import org.apache.qpid.server.model.ConfiguredObject;
 import org.apache.qpid.server.util.urlstreamhandler.data.Handler;
@@ -407,7 +407,7 @@ public class RestServlet extends AbstractServlet
                 }
                 catch (RuntimeException e)
                 {
-                    setResponseStatus(response, e);
+                    setResponseStatus(request, response, e);
                 }
                 return;
             }
@@ -423,7 +423,7 @@ public class RestServlet extends AbstractServlet
         Collection<ConfiguredObject>[] objects = new Collection[_hierarchy.length];
         if (_hierarchy.length == 1)
         {
-            createOrUpdate(providedObject, _hierarchy[0], getBroker(), null, response);
+            createOrUpdate(providedObject, _hierarchy[0], getBroker(), null, request, response);
         }
         else
         {
@@ -486,13 +486,14 @@ public class RestServlet extends AbstractServlet
             ConfiguredObject theParent = parents.remove(0);
             ConfiguredObject[] otherParents = parents.toArray(new ConfiguredObject[parents.size()]);
 
-            createOrUpdate(providedObject, objClass, theParent, otherParents, response);
+            createOrUpdate(providedObject, objClass, theParent, otherParents, request, response);
         }
 
     }
 
     private void createOrUpdate(Map<String, Object> providedObject, Class<? extends ConfiguredObject> objClass,
-            ConfiguredObject theParent, ConfiguredObject[] otherParents, HttpServletResponse response) throws IOException
+            ConfiguredObject theParent, ConfiguredObject[] otherParents, HttpServletRequest request,
+            HttpServletResponse response) throws IOException
     {
         try
         {
@@ -513,7 +514,7 @@ public class RestServlet extends AbstractServlet
         }
         catch (RuntimeException e)
         {
-            setResponseStatus(response, e);
+            setResponseStatus(request, response, e);
         }
     }
 
@@ -552,24 +553,40 @@ public class RestServlet extends AbstractServlet
         return true;
     }
 
-    private void setResponseStatus(HttpServletResponse response, RuntimeException e) throws IOException
+    private void setResponseStatus(HttpServletRequest request, HttpServletResponse response, RuntimeException e) throws IOException
     {
         if (e instanceof AccessControlException)
         {
             if (LOGGER.isDebugEnabled())
             {
-                LOGGER.debug("Caught security exception, sending " + HttpServletResponse.SC_FORBIDDEN, e);
+                LOGGER.debug("AccessControlException, sending " + HttpServletResponse.SC_FORBIDDEN, e);
             }
             response.setStatus(HttpServletResponse.SC_FORBIDDEN);
         }
         else
         {
-            LOGGER.warn("Caught exception", e);
+            if (e instanceof IllegalConfigurationException || e instanceof IllegalArgumentException)
+            {
+                if (LOGGER.isDebugEnabled())
+                {
+                    LOGGER.debug(e.getClass().getSimpleName() + " processing request : " + e.getMessage());
+                }
+                else if (LOGGER.isTraceEnabled())
+                {
+                    LOGGER.trace(e.getClass().getSimpleName() + " processing request", e);
+                }
+            }
+            else
+            {
+                LOGGER.warn("Unexpected exception processing request ", e);
+            }
+
             response.setStatus(HttpServletResponse.SC_CONFLICT);
 
             response.setContentType("application/json");
             response.setCharacterEncoding("UTF-8");
-            PrintWriter out = response.getWriter();
+
+            Writer out = getOutputWriter(request, response);
             ObjectMapper mapper = new ObjectMapper();
             mapper.configure(SerializationConfig.Feature.INDENT_OUTPUT, true);
             mapper.writeValue(out, Collections.singletonMap("errorMessage", e.getMessage()));
@@ -596,7 +613,7 @@ public class RestServlet extends AbstractServlet
         }
         catch(RuntimeException e)
         {
-            setResponseStatus(response, e);
+            setResponseStatus(request, response, e);
         }
     }
 
diff --git a/qpid/java/broker-plugins/management-http/src/main/java/resources/index.html b/qpid/java/broker-plugins/management-http/src/main/java/resources/index.html
index 3c0cb9a5f6..11f229237c 100644
--- a/qpid/java/broker-plugins/management-http/src/main/java/resources/index.html
+++ b/qpid/java/broker-plugins/management-http/src/main/java/resources/index.html
@@ -125,7 +125,7 @@
             </div>
             <div class="dijitDialogPaneActionBar">
                 <input type="button" id="errorDialog.button.cancel" value="Cancel" label="Cancel" dojoType="dijit.form.Button" onClick="dijit.byId('errorDialog').hide();"/>
-                <input type="button" id="errorDialog.button.relogin"  value="Login" label="Login" dojoType="dijit.form.Button" onClick="dijit.byId('errorDialog').hide(); document.location.href = '/';"/>
+                <input type="button" id="errorDialog.button.relogin"  value="Login" label="Login" dojoType="dijit.form.Button" onClick="dijit.byId('errorDialog').hide(); window.location='logout';"/>
             </div>
         </div>
     </div>
diff --git a/qpid/java/broker-plugins/management-http/src/main/java/resources/js/qpid/common/grid/GridUpdater.js b/qpid/java/broker-plugins/management-http/src/main/java/resources/js/qpid/common/grid/GridUpdater.js
index 8d58b6dec0..e5d96d44e7 100644
--- a/qpid/java/broker-plugins/management-http/src/main/java/resources/js/qpid/common/grid/GridUpdater.js
+++ b/qpid/java/broker-plugins/management-http/src/main/java/resources/js/qpid/common/grid/GridUpdater.js
@@ -160,7 +160,7 @@ define(["dojo/_base/xhr",
              if (this.serviceUrl)
              {
                var requestUrl = lang.isFunction(this.serviceUrl) ? this.serviceUrl() : this.serviceUrl;
-               xhr.get({url: requestUrl, sync: true, handleAs: "json"}).then(processData, util.errorHandler);
+               xhr.get({url: requestUrl, sync: true, handleAs: "json"}).then(processData, util.xhrErrorHandler);
              }
              else
              {
diff --git a/qpid/java/broker-plugins/management-http/src/main/java/resources/js/qpid/common/util.js b/qpid/java/broker-plugins/management-http/src/main/java/resources/js/qpid/common/util.js
index 441d6776e7..cb0cc792e8 100644
--- a/qpid/java/broker-plugins/management-http/src/main/java/resources/js/qpid/common/util.js
+++ b/qpid/java/broker-plugins/management-http/src/main/java/resources/js/qpid/common/util.js
@@ -345,15 +345,18 @@ define(["dojo/_base/xhr",
                  var hasMessage = error.hasOwnProperty("message");
                  var message;
 
-                 if (error.status == 401)
+                 if (error.status == 0)
                  {
-                   message = hasMessage ? error.message : "Authentication required";
+                   message = "Unable to contact the Broker";
+                 }
+                 else if (error.status == 401)
+                 {
+                   message = "Authentication required";
                    userMustReauth = true;
                  }
                  else if (error.status == 403)
                  {
-                   message =  hasMessage ? error.message : "Forbidden";
-                   userMustReauth = true;
+                   message =  "Access Forbidden";
                  }
                  else
                  {
@@ -398,22 +401,6 @@ define(["dojo/_base/xhr",
              }
            };
 
-           util.errorHandler = function errorHandler(error)
-           {
-               if(error.status == 401)
-               {
-                   alert("Authentication Failed");
-               }
-               else if(error.status == 403)
-               {
-                   alert("Access Denied");
-               }
-               else
-               {
-                   alert(error);
-               }
-           }
-
            util.sendRequest = function (url, method, attributes, sync)
            {
                var success = false;