REPO-1646: V1 REST API - cannot unset optional fields (eg. when updating person / site details ...)

- part 1 (Update Site)

git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/BRANCHES/DEV/5.2.N/root@133290 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261

source/java/org/alfresco/rest/api/impl/SitesImpl.java

@@ -1179,15 +1179,15 @@ public class SitesImpl implements Sites
         }
 
         // Bind any provided values to the site info, allowing for "partial" updates.
-        if (update.getTitle() != null)
+        if (update.wasSet(Site.TITLE))
         {
             siteInfo.setTitle(update.getTitle());
         }
-        if (update.getDescription() != null)
+        if (update.wasSet(Site.DESCRIPTION))
         {
             siteInfo.setDescription(update.getDescription());
         }
-        if (update.getVisibility() != null)
+        if (update.wasSet(Site.VISIBILITY))
         {
             siteInfo.setVisibility(update.getVisibility());
         }

source/java/org/alfresco/rest/api/model/Site.java

@@ -29,6 +29,9 @@ import org.alfresco.rest.framework.resource.UniqueId;
 import org.alfresco.service.cmr.site.SiteInfo;
 import org.alfresco.service.cmr.site.SiteVisibility;
 
+import java.util.HashMap;
+import java.util.Map;
+
 /**
  * Represents a site.
  * 
@@ -37,8 +40,6 @@ import org.alfresco.service.cmr.site.SiteVisibility;
  */
 public class Site implements Comparable<Site>
 {
-    public static final String ROLE = "role";
-
     protected String id; // site id (aka short name)
     protected String guid; // site nodeId
     protected String title;
@@ -47,6 +48,16 @@ public class Site implements Comparable<Site>
     protected String preset;
     protected String role;
 
+    private Map<String, Boolean> setFields = new HashMap<>(7);
+
+    public static final String ID = "id";
+    public static final String GUID = "guid";
+    public static final String TITLE = "title";
+    public static final String DESCRIPTION = "description";
+    public static final String VISIBILITY = "visibility";
+    public static final String PRESET = "preset";
+    public static final String ROLE = "role";
+
     public Site()
     {
     }
@@ -57,13 +68,13 @@ public class Site implements Comparable<Site>
         {
             throw new IllegalArgumentException("Must provide siteInfo");
         }
-        this.id = siteInfo.getShortName();
+        setId(siteInfo.getShortName());
-        this.guid = siteInfo.getNodeRef().getId();
+        setGuid(siteInfo.getNodeRef().getId());
-        this.title = siteInfo.getTitle();
+        setTitle(siteInfo.getTitle());
-        this.description = siteInfo.getDescription();
+        setDescription(siteInfo.getDescription());
-        this.visibility = siteInfo.getVisibility();
+        setVisibility(siteInfo.getVisibility());
-        this.preset = siteInfo.getSitePreset();
+        setPreset(siteInfo.getSitePreset());
-        this.role = role;
+        setRole(role);
     }
 
     @UniqueId
@@ -75,6 +86,7 @@ public class Site implements Comparable<Site>
     public void setId(String id)
     {
         this.id = id;
+        setFields.put(ID, true);
     }
 
     public String getGuid()
@@ -85,6 +97,7 @@ public class Site implements Comparable<Site>
     public void setGuid(String guid)
     {
         this.guid = guid;
+        setFields.put(GUID, true);
     }
 
     public String getTitle()
@@ -95,6 +108,7 @@ public class Site implements Comparable<Site>
     public void setTitle(String title)
     {
         this.title = title;
+        setFields.put(TITLE, true);
     }
 
     public String getDescription()
@@ -105,6 +119,7 @@ public class Site implements Comparable<Site>
     public void setDescription(String description)
     {
         this.description = description;
+        setFields.put(DESCRIPTION, true);
     }
 
     public SiteVisibility getVisibility()
@@ -115,6 +130,7 @@ public class Site implements Comparable<Site>
     public void setVisibility(SiteVisibility visibility)
     {
         this.visibility = visibility;
+        setFields.put(VISIBILITY, true);
     }
 
     public String getPreset()
@@ -125,6 +141,7 @@ public class Site implements Comparable<Site>
     public void setPreset(String preset)
     {
         this.preset = preset;
+        setFields.put(PRESET, true);
     }
 
     public String getRole()
@@ -135,6 +152,13 @@ public class Site implements Comparable<Site>
     public void setRole(String role)
     {
         this.role = role;
+        setFields.put(ROLE, true);
+    }
+
+    public boolean wasSet(String fieldName)
+    {
+        Boolean b = setFields.get(fieldName);
+        return (b != null ? b : false);
     }
 
     @Override

source/java/org/alfresco/rest/api/model/SiteUpdate.java

@@ -28,6 +28,8 @@ package org.alfresco.rest.api.model;
 import org.alfresco.service.cmr.site.SiteVisibility;
 
 import java.io.Serializable;
+import java.util.HashMap;
+import java.util.Map;
 
 /**
  * Class representing a site update API operation.
@@ -42,12 +44,21 @@ public class SiteUpdate implements Serializable
     private String description;
     private SiteVisibility visibility;
 
+    private Map<String, Boolean> setFields = new HashMap<>(3);
+
+    public static final String TITLE = "title";
+    public static final String DESCRIPTION = "description";
+    public static final String VISIBILITY = "visibility";
+
+    public SiteUpdate()
+    {
+    }
 
     public SiteUpdate(String title, String description, SiteVisibility visibility)
     {
-        this.title = title;
+        setTitle(title);
-        this.description = description;
+        setDescription(description);
-        this.visibility = visibility;
+        setVisibility(visibility);
     }
 
     public String getTitle()
@@ -58,6 +69,7 @@ public class SiteUpdate implements Serializable
     public void setTitle(String title)
     {
         this.title = title;
+        setFields.put(TITLE, true);
     }
 
     public String getDescription()
@@ -68,6 +80,7 @@ public class SiteUpdate implements Serializable
     public void setDescription(String description)
     {
         this.description = description;
+        setFields.put(DESCRIPTION, true);
     }
 
     public SiteVisibility getVisibility()
@@ -78,6 +91,13 @@ public class SiteUpdate implements Serializable
     public void setVisibility(SiteVisibility visibility)
     {
         this.visibility = visibility;
+        setFields.put(VISIBILITY, true);
+    }
+
+    public boolean wasSet(String fieldName)
+    {
+        Boolean b = setFields.get(fieldName);
+        return (b != null ? b : false);
     }
 
     @Override

source/java/org/alfresco/rest/api/sites/SiteEntityResource.java

@@ -132,35 +132,49 @@ public class SiteEntityResource implements EntityResourceAction.Read<Site>,
     @Override
     @WebApiDescription(title="Update site", description="Update the Share site")
     public Site update(String siteId, Site site, Parameters parameters)
+    {
+        return sites.updateSite(siteId, convert(site), parameters);
+    }
+
+    protected SiteUpdate convert(Site site)
     {
         // Until REPO-110 is solved, we need to explicitly test for the presence of fields
         // on the Site object that aren't valid SiteUpdate fields. Once REPO-110 is solved,
         // the update method will take a SiteUpdate as a parameter rather than a Site
         // and only the correct fields will be exposed. Any attempt to access illegal fields
         // should then result in the framework returning a 400 automatically.
-        if (site.getId() != null)
+        if (site.wasSet(Site.ID))
         {
             throw new InvalidArgumentException("Site update does not support field: id");
         }
-        if (site.getGuid() != null)
+        if (site.wasSet(Site.GUID))
         {
             throw new InvalidArgumentException("Site update does not support field: guid");
         }
-        if (site.getRole() != null)
+        if (site.wasSet(Site.ROLE))
         {
             throw new InvalidArgumentException("Site update does not support field: role");
         }
-        if (site.getPreset() != null)
+        if (site.wasSet(Site.PRESET))
         {
             throw new InvalidArgumentException("Site update does not support field: preset");
         }
 
         // Bind valid fields to a SiteUpdate instance.
-        final String title = site.getTitle();
+        SiteUpdate siteUpdate = new SiteUpdate();
-        final String description = site.getDescription();
+        if (site.wasSet(Site.TITLE))
-        final SiteVisibility visibility = site.getVisibility();
+        {
-        SiteUpdate update = new SiteUpdate(title, description, visibility);
+            siteUpdate.setTitle(site.getTitle());
+        }
+        if (site.wasSet(Site.DESCRIPTION))
+        {
+            siteUpdate.setDescription(site.getDescription());
+        }
+        if (site.wasSet(Site.VISIBILITY))
+        {
+            siteUpdate.setVisibility(site.getVisibility());
+        }
 
-        return sites.updateSite(siteId, update, parameters);
+        return siteUpdate;
     }
 }

source/test-java/org/alfresco/rest/api/tests/TestSites.java

@@ -30,7 +30,6 @@ import static org.junit.Assert.fail;
 
 import java.util.*;
 
-import org.alfresco.repo.model.filefolder.HiddenAspect;
 import org.alfresco.repo.tenant.TenantUtil;
 import org.alfresco.repo.tenant.TenantUtil.TenantRunAsWork;
 import org.alfresco.rest.api.model.SiteUpdate;
@@ -45,10 +44,10 @@ import org.alfresco.rest.api.tests.client.RequestContext;
 import org.alfresco.rest.api.tests.client.data.*;
 import org.alfresco.service.cmr.site.SiteVisibility;
 import org.alfresco.util.GUID;
-import org.apache.commons.collections.map.HashedMap;
 import org.apache.commons.httpclient.HttpStatus;
 import org.json.simple.JSONArray;
 import org.json.simple.JSONObject;
+import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -541,9 +540,9 @@ public class TestSites extends EnterpriseTestApi
         }
 
         // +ve test: remove the description.
-        // This is a workaround for being unable to eplicitly null the field (see REPO-1409)
+        // This is a workaround for being unable to explicitly null the field (see REPO-1409)
         {
-            Site site = sitesProxy.createSite(new SiteImpl("NoDescriptionTest", SiteVisibility.PUBLIC.toString()), 201);
+            Site site = sitesProxy.createSite(new SiteImpl(null, "NoDescriptionTest", "Initial description", SiteVisibility.PUBLIC.toString()), 201);
 
             Site updatedSite = sitesProxy.updateSite(
                     site.getSiteId(),
@@ -566,10 +565,16 @@ public class TestSites extends EnterpriseTestApi
             // Check the return from updateSite(...)
             expectedUpdate.expected(updatedSite);
 
+            // TODO improve expected (and/or AssertUtil) and affected test cases
+            Assert.assertNull(updatedSite.getDescription());
+
             // Double check a fresh retrieval matches.
             Site fresh = sitesProxy.getSite(site.getSiteId(), 200);
             expectedUpdate.expected(fresh);
 
+            // TODO improve expected (and/or AssertUtil) and affected test cases
+            Assert.assertNull(fresh.getDescription());
+
             removeSiteQuietly(site.getSiteId());
         }