QPID-6438: Address review comments from Keith Wall

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

1 files changed, 143 insertions, 138 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 eccf9e0d63..cb950612cf 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
@@ -420,212 +420,217 @@ public class RestServlet extends AbstractServlet
     {
         response.setContentType("application/json");
 
-        List<String> names = new ArrayList<String>();
-        String[] pathInfoElements = getPathInfoElements(request);
-
-        boolean parentRequest = false;
-        if (pathInfoElements != null)
-        {
-            parentRequest = _hierarchy.length > 0 && pathInfoElements.length == _hierarchy.length - 1;
-            if (pathInfoElements.length != _hierarchy.length && !parentRequest)
-            {
-                throw new IllegalArgumentException("Path to object to create must be fully specified. "
-                                                   + "Found "
-                                                   + names
-                                                   + " of size "
-                                                   + names.size()
-                                                   + " expecting "
-                                                   + _hierarchy.length);
-            }
-            names.addAll(Arrays.asList(pathInfoElements));
-        }
-        else
-        {
-            parentRequest = _hierarchy.length == 1;
-        }
-
-        Map<String, Object> providedObject;
-
-        ArrayList<String> headers = Collections.list(request.getHeaderNames());
-        ObjectMapper mapper = new ObjectMapper();
-
-        if(headers.contains("Content-Type") && request.getHeader("Content-Type").startsWith("multipart/form-data"))
+        List<String> names = getParentNamesFromServletPath(request);
+        Map<String, Object> providedObject = getRequestProvidedObject(request);
+        boolean isFullObjectURL = names.size() == _hierarchy.length;
+        boolean updateOnlyAllowed = isFullObjectURL && "POST".equalsIgnoreCase(request.getMethod());
+        try
         {
-            providedObject = new HashMap<>();
-            Map<String,String> fileUploads = new HashMap<>();
-            Collection<Part> parts = request.getParts();
-            for(Part part : parts)
+            if (names.isEmpty())
             {
-                if("data".equals(part.getName()) && "application/json".equals(part.getContentType()))
+                if (_hierarchy.length == 0)
                 {
-                    providedObject = mapper.readValue(part.getInputStream(), LinkedHashMap.class);
+                    getBroker().setAttributes(providedObject);
+                    response.setStatus(HttpServletResponse.SC_OK);
+                    return;
                 }
-                else
+                else if (isFullObjectURL)
                 {
-                    byte[] data = new byte[(int) part.getSize()];
-                    part.getInputStream().read(data);
-                    String inlineURL = DataUrlUtils.getDataUrlForBytes(data);
-                    fileUploads.put(part.getName(),inlineURL.toString());
+                    throw new ServletException("Cannot identify request target object");
                 }
             }
-            providedObject.putAll(fileUploads);
-        }
-        else
-        {
 
-            providedObject = mapper.readValue(request.getInputStream(), LinkedHashMap.class);
-        }
+            ConfiguredObject theParent = getBroker();
+            ConfiguredObject[] otherParents = null;
+            Class<? extends ConfiguredObject> objClass = getConfiguredClass();
+            if (_hierarchy.length > 1)
+            {
+                List<ConfiguredObject> parents = findAllObjectParents(names);
+                theParent = parents.remove(0);
+                otherParents = parents.toArray(new ConfiguredObject[parents.size()]);
+            }
 
-        if (names.isEmpty())
-        {
-            if (_hierarchy.length == 0)
+            if (isFullObjectURL)
             {
-                try
+                providedObject.put("name", names.get(names.size() - 1));
+                ConfiguredObject<?> configuredObject = findObjectToUpdateInParent(objClass, providedObject, theParent, otherParents);
+
+                if (configuredObject != null)
                 {
-                    getBroker().setAttributes(providedObject);
+                    configuredObject.setAttributes(providedObject);
                     response.setStatus(HttpServletResponse.SC_OK);
+                    return;
                 }
-                catch (RuntimeException e)
+                else if (updateOnlyAllowed)
                 {
-                    setResponseStatus(request, response, e);
+                    sendErrorResponse(request, response, HttpServletResponse.SC_NOT_FOUND, "Object with "
+                            +  (providedObject.containsKey("id") ? " id '" + providedObject.get("id") : " name '" + providedObject.get("name"))
+                            + "' does not exist!");
+                    return;
                 }
-                return;
             }
-            else if (!parentRequest)
+
+            ConfiguredObject<?> configuredObject = theParent.createChild(objClass, providedObject, otherParents);
+            StringBuffer requestURL = request.getRequestURL();
+            if (!isFullObjectURL)
             {
-                throw new ServletException("Cannot identify request target object");
+                requestURL.append("/").append(configuredObject.getName());
             }
+            response.setHeader("Location", requestURL.toString());
+            response.setStatus(HttpServletResponse.SC_CREATED);
         }
-
-        if (!parentRequest)
+        catch (RuntimeException e)
         {
-            providedObject.put("name", names.get(names.size() - 1));
+            setResponseStatus(request, response, e);
         }
 
-        @SuppressWarnings("unchecked")
+    }
+
+    private List<ConfiguredObject> findAllObjectParents(List<String> names)
+    {
         Collection<ConfiguredObject>[] objects = new Collection[_hierarchy.length];
-        if (_hierarchy.length == 1)
+        for (int i = 0; i < _hierarchy.length - 1; i++)
         {
-            createOrUpdate(providedObject, _hierarchy[0], getBroker(), null, request, response, parentRequest);
-        }
-        else
-        {
-            for (int i = 0; i < _hierarchy.length - 1; i++)
+            objects[i] = new HashSet<>();
+            if (i == 0)
             {
-                objects[i] = new HashSet<ConfiguredObject>();
-                if (i == 0)
+                for (ConfiguredObject object : getBroker().getChildren(_hierarchy[0]))
                 {
-                    for (ConfiguredObject object : getBroker().getChildren(_hierarchy[0]))
+                    if (object.getName().equals(names.get(0)))
                     {
-                        if (object.getName().equals(names.get(0)))
-                        {
-                            objects[0].add(object);
-                            break;
-                        }
+                        objects[0].add(object);
+                        break;
                     }
                 }
-                else
+            }
+            else
+            {
+                for (int j = i - 1; j >= 0; j--)
                 {
-                    for (int j = i - 1; j >= 0; j--)
+                    if (getBroker().getModel().getChildTypes(_hierarchy[j]).contains(_hierarchy[i]))
                     {
-                        if (getBroker().getModel().getChildTypes(_hierarchy[j]).contains(_hierarchy[i]))
+                        for (ConfiguredObject<?> parent : objects[j])
                         {
-                            for (ConfiguredObject<?> parent : objects[j])
+                            for (ConfiguredObject<?> object : parent.getChildren(_hierarchy[i]))
                             {
-                                for (ConfiguredObject<?> object : parent.getChildren(_hierarchy[i]))
+                                if (object.getName().equals(names.get(i)))
                                 {
-                                    if (object.getName().equals(names.get(i)))
-                                    {
-                                        objects[i].add(object);
-                                    }
+                                    objects[i].add(object);
                                 }
                             }
-                            break;
                         }
+                        break;
                     }
                 }
-
             }
-            List<ConfiguredObject> parents = new ArrayList<ConfiguredObject>();
-            Class<? extends ConfiguredObject> objClass = getConfiguredClass();
-            Collection<Class<? extends ConfiguredObject>> parentClasses =
-                    getBroker().getModel().getParentTypes(objClass);
-            for (int i = _hierarchy.length - 2; i >= 0; i--)
+
+        }
+        List<ConfiguredObject> parents = new ArrayList<>();
+        Class<? extends ConfiguredObject> objClass = getConfiguredClass();
+        Collection<Class<? extends ConfiguredObject>> parentClasses =
+                getBroker().getModel().getParentTypes(objClass);
+        for (int i = _hierarchy.length - 2; i >= 0; i--)
+        {
+            if (parentClasses.contains(_hierarchy[i]))
             {
-                if (parentClasses.contains(_hierarchy[i]))
+                if (objects[i].size() == 1)
                 {
-                    if (objects[i].size() == 1)
-                    {
-                        parents.add(objects[i].iterator().next());
-                    }
-                    else
-                    {
-                        throw new IllegalArgumentException("Cannot deduce parent of class "
-                                                           + _hierarchy[i].getSimpleName());
-                    }
+                    parents.add(objects[i].iterator().next());
+                }
+                else
+                {
+                    throw new IllegalArgumentException("Cannot deduce parent of class "
+                                                       + _hierarchy[i].getSimpleName());
                 }
-
             }
-            ConfiguredObject theParent = parents.remove(0);
-            ConfiguredObject[] otherParents = parents.toArray(new ConfiguredObject[parents.size()]);
 
-            createOrUpdate(providedObject, objClass, theParent, otherParents, request, response, parentRequest);
         }
+        return parents;
     }
 
-    private void createOrUpdate(Map<String, Object> providedObject, Class<? extends ConfiguredObject> objClass,
-            ConfiguredObject theParent, ConfiguredObject[] otherParents, HttpServletRequest request,
-            HttpServletResponse response, boolean parentRequest) throws IOException
+    private List<String> getParentNamesFromServletPath(HttpServletRequest request)
     {
-        try
+        List<String> names = new ArrayList<>();
+        String[] pathInfoElements = getPathInfoElements(request);
+        if (pathInfoElements != null)
         {
-            Collection<? extends ConfiguredObject> existingChildren = theParent.getChildren(objClass);
+            if (!(pathInfoElements.length == _hierarchy.length ||
+                    (_hierarchy.length > 0 && pathInfoElements.length == _hierarchy.length - 1)))
+            {
+                throw new IllegalArgumentException("Either parent path or full object path must be specified on object creation."
+                                                   + " Full object path must be specified on object update. "
+                                                   + "Found "
+                                                   + names
+                                                   + " of size "
+                                                   + names.size()
+                                                   + " expecting "
+                                                   + _hierarchy.length);
+            }
+            names.addAll(Arrays.asList(pathInfoElements));
+        }
+        return names;
+    }
 
-            if (!parentRequest)
+    private Map<String, Object> getRequestProvidedObject(HttpServletRequest request) throws IOException, ServletException
+    {
+        Map<String, Object> providedObject;
+
+        ArrayList<String> headers = Collections.list(request.getHeaderNames());
+        ObjectMapper mapper = new ObjectMapper();
+
+        if(headers.contains("Content-Type") && request.getHeader("Content-Type").startsWith("multipart/form-data"))
+        {
+            providedObject = new HashMap<>();
+            Map<String,String> fileUploads = new HashMap<>();
+            Collection<Part> parts = request.getParts();
+            for(Part part : parts)
             {
-                for (ConfiguredObject obj : existingChildren)
+                if("data".equals(part.getName()) && "application/json".equals(part.getContentType()))
                 {
-                    if ((providedObject.containsKey("id") && String.valueOf(providedObject.get("id")).equals(obj.getId().toString()))
-                            || (obj.getName().equals(providedObject.get("name")) && equalParents(obj, otherParents, objClass)))
-                    {
-                        obj.setAttributes(providedObject);
-                        response.setStatus(HttpServletResponse.SC_OK);
-                        return;
-                    }
+                    providedObject = mapper.readValue(part.getInputStream(), LinkedHashMap.class);
                 }
-
-                if ("POST".equalsIgnoreCase(request.getMethod()))
+                else
                 {
-                    sendErrorResponse(request, response, HttpServletResponse.SC_NOT_FOUND, "Object with "
-                            +  (providedObject.containsKey("id") ? " id '" + providedObject.get("id") : " name '" + providedObject.get("name"))
-                            + "' does not exist!" );
-                    return;
+                    byte[] data = new byte[(int) part.getSize()];
+                    part.getInputStream().read(data);
+                    String inlineURL = DataUrlUtils.getDataUrlForBytes(data);
+                    fileUploads.put(part.getName(),inlineURL.toString());
                 }
             }
+            providedObject.putAll(fileUploads);
+        }
+        else
+        {
 
-            ConfiguredObject<?> co = theParent.createChild(objClass, providedObject, otherParents);
-            StringBuffer requestURL = request.getRequestURL();
-            if (parentRequest)
-            {
-                requestURL.append("/").append(co.getName());
-            }
-            response.setHeader("Location", requestURL.toString());
-            response.setStatus(HttpServletResponse.SC_CREATED);
+            providedObject = mapper.readValue(request.getInputStream(), LinkedHashMap.class);
         }
-        catch (RuntimeException e)
+        return providedObject;
+    }
+
+    private ConfiguredObject<?> findObjectToUpdateInParent(Class<? extends ConfiguredObject> objClass, Map<String, Object> providedObject, ConfiguredObject theParent, ConfiguredObject[] otherParents)
+    {
+        Collection<? extends ConfiguredObject> existingChildren = theParent.getChildren(objClass);
+
+        for (ConfiguredObject obj : existingChildren)
         {
-            setResponseStatus(request, response, e);
+            if ((providedObject.containsKey("id") && String.valueOf(providedObject.get("id")).equals(obj.getId().toString()))
+                    || (obj.getName().equals(providedObject.get("name")) && sameOtherParents(obj, otherParents, objClass)))
+            {
+                return obj;
+            }
         }
+        return null;
     }
 
-    private boolean equalParents(ConfiguredObject obj, ConfiguredObject[] otherParents, Class<? extends ConfiguredObject> objClass)
+    private boolean sameOtherParents(ConfiguredObject obj, ConfiguredObject[] otherParents, Class<? extends ConfiguredObject> objClass)
     {
+        Collection<Class<? extends ConfiguredObject>> parentClasses = obj.getModel().getParentTypes(objClass);
+
         if(otherParents == null || otherParents.length == 0)
         {
-            return true;
+            return parentClasses.size() == 1;
         }
 
-        Collection<Class<? extends ConfiguredObject>> parentClasses = obj.getModel().getParentTypes(objClass);
 
         for (ConfiguredObject parent : otherParents)
         {