ACS-2216: Fix incorrect ContentService Java Interface - should accept content propQName (#813)

Squashed some of the commit messages:

* ACS-2216: Resolving failing tests
* ACS-2216: Reformatting code
* ACS-2216: Adding David's suggestion
* ACS-2216: Set some methods to default as per David's comments
* ACS-2216: Added test for when propertyQName has no content
* ACS-2216: Replaced try catch with assertThrows
* ACS-2216: Added propertyQName to Auditable list

Co-authored-by: David Edwards <david.edwards@hyland.com>

remote-api/src/main/java/org/alfresco/rest/api/impl/NodesImpl.java

@@ -3420,7 +3420,7 @@ public class NodesImpl implements Nodes
     @Override
     public DirectAccessUrl requestContentDirectUrl(NodeRef nodeRef, boolean attachment, Long validFor)
     {
-        DirectAccessUrl directAccessUrl = contentService.requestContentDirectUrl(nodeRef, attachment, validFor);
+        DirectAccessUrl directAccessUrl = contentService.requestContentDirectUrl(nodeRef, ContentModel.PROP_CONTENT, attachment, validFor);
         if (directAccessUrl == null)
         {
             throw new DisabledServiceException("Direct access url isn't available.");

repository/src/main/java/org/alfresco/repo/content/ContentServiceImpl.java

@@ -591,14 +591,14 @@ public class ContentServiceImpl implements ContentService, ApplicationContextAwa
      * {@inheritDoc}
      */
     @Override
-    public boolean isContentDirectUrlEnabled(NodeRef nodeRef)
+    public boolean isContentDirectUrlEnabled(NodeRef nodeRef, QName propertyQName)
     {
         boolean contentDirectUrlEnabled = false;
 
         // TODO: update this
         if (systemWideDirectUrlConfig.isEnabled())
         {
-            ContentData contentData = getContentData(nodeRef, ContentModel.PROP_CONTENT);
+            ContentData contentData = getContentData(nodeRef, propertyQName);
 
             // check that the URL is available
             if (contentData == null || contentData.getContentUrl() == null)
@@ -615,14 +615,15 @@ public class ContentServiceImpl implements ContentService, ApplicationContextAwa
     /**
      * {@inheritDoc}
      */
-    public DirectAccessUrl requestContentDirectUrl(NodeRef nodeRef, boolean attachment, Long validFor)
+    @Override
+    public DirectAccessUrl requestContentDirectUrl(NodeRef nodeRef, QName propertyQName, boolean attachment, Long validFor)
     {
         if (!systemWideDirectUrlConfig.isEnabled())
         {
             throw new DirectAccessUrlDisabledException("Direct access url isn't available.");
         }
 
-        ContentData contentData = getContentData(nodeRef, ContentModel.PROP_CONTENT);
+        ContentData contentData = getContentData(nodeRef, propertyQName);
         // check that the content & URL is available
         if (contentData == null || contentData.getContentUrl() == null)
         {

repository/src/main/java/org/alfresco/service/cmr/repository/ContentService.java

@@ -27,6 +27,7 @@ package org.alfresco.service.cmr.repository;
 
 
 import org.alfresco.api.AlfrescoPublicApi;
+import org.alfresco.model.ContentModel;
 import org.alfresco.service.Auditable;
 import org.alfresco.service.Experimental;
 import org.alfresco.service.cmr.dictionary.InvalidTypeException;
@@ -170,7 +171,20 @@ public interface ContentService
      *
      * @return {@code true} if direct access URLs retrieving is supported for the node, {@code false} otherwise
      */
-    boolean isContentDirectUrlEnabled(NodeRef nodeRef);
+    @Deprecated
+    default boolean isContentDirectUrlEnabled(NodeRef nodeRef)
+    {
+        return isContentDirectUrlEnabled(nodeRef, ContentModel.PROP_CONTENT);
+    }
+
+    /**
+     * Checks if the system and store supports the retrieving of a direct access {@code URL} for the given node.
+     *
+     * @param nodeRef a reference to a node having a content property
+     * @param propertyQName the name of the property, which must be of type <b>content</b>
+     * @return {@code true} if direct access URLs retrieving is supported for the node, {@code false} otherwise
+     */
+    boolean isContentDirectUrlEnabled(NodeRef nodeRef, QName propertyQName);
 
     /**
      * Gets a presigned URL to directly access the content. It is up to the actual store
@@ -181,11 +195,27 @@ public interface ContentService
      * @return A direct access {@code URL} object for the content.
      * @throws UnsupportedOperationException if the store is unable to provide the information.
      */
+    @Deprecated
     default DirectAccessUrl requestContentDirectUrl(NodeRef nodeRef, boolean attachment)
     {
         return requestContentDirectUrl(nodeRef, attachment, null);
     }
 
+    /**
+     * Gets a presigned URL to directly access the content. It is up to the actual store
+     * implementation if it can fulfil this request with an expiry time or not.
+     *
+     * @param nodeRef Node ref for which to obtain the direct access {@code URL}.
+     * @param propertyQName the name of the property, which must be of type <b>content</b>
+     * @param attachment {@code true} if an attachment URL is requested, {@code false} for an embedded {@code URL}.
+     * @return A direct access {@code URL} object for the content.
+     * @throws UnsupportedOperationException if the store is unable to provide the information.
+     */
+    default DirectAccessUrl requestContentDirectUrl(NodeRef nodeRef, QName propertyQName, boolean attachment)
+    {
+        return requestContentDirectUrl(nodeRef, propertyQName, attachment, null);
+    }
+
     /**
      * Gets a presigned URL to directly access the content. It is up to the actual store
      * implementation if it can fulfil this request with an expiry time or not.
@@ -197,7 +227,25 @@ public interface ContentService
      * @throws UnsupportedOperationException if the store is unable to provide the information.
      */
     @Auditable(parameters = {"nodeRef", "validFor"})
-    DirectAccessUrl requestContentDirectUrl(NodeRef nodeRef, boolean attachment, Long validFor);
+    @Deprecated
+    default public DirectAccessUrl requestContentDirectUrl(NodeRef nodeRef, boolean attachment, Long validFor)
+    {
+        return requestContentDirectUrl(nodeRef, ContentModel.PROP_CONTENT, attachment, validFor);
+    }
+
+    /**
+     * Gets a presigned URL to directly access the content. It is up to the actual store
+     * implementation if it can fulfil this request with an expiry time or not.
+     *
+     * @param nodeRef Node ref for which to obtain the direct access {@code URL}.
+     * @param propertyQName the name of the property, which must be of type <b>content</b>
+     * @param attachment {@code true} if an attachment URL is requested, {@code false} for an embedded {@code URL}.
+     * @param validFor The time at which the direct access {@code URL} will expire.
+     * @return A direct access {@code URL} object for the content.
+     * @throws UnsupportedOperationException if the store is unable to provide the information.
+     */
+    @Auditable(parameters = {"nodeRef", "propertyQName", "validFor"})
+    DirectAccessUrl requestContentDirectUrl(NodeRef nodeRef, QName propertyQName, boolean attachment, Long validFor);
 
     /**
      * Gets a key-value (String-String) collection of storage headers/properties with their respective values for a specific node reference.

repository/src/test/java/org/alfresco/repo/content/ContentServiceImplUnitTest.java

@@ -48,6 +48,7 @@ import org.alfresco.service.cmr.repository.ContentData;
 import org.alfresco.service.cmr.repository.DirectAccessUrl;
 import org.alfresco.service.cmr.repository.NodeRef;
 import org.alfresco.service.cmr.repository.NodeService;
+import org.alfresco.service.namespace.QName;
 import org.junit.Before;
 import org.junit.Test;
 import org.mockito.InjectMocks;
@@ -78,6 +79,8 @@ public class ContentServiceImplUnitTest
     private static final String X_AMZ_HEADER_2 = "x-amz-header-2";
     private static final String VALUE_2 = "value2";
 
+    private static final QName PROP_CONTENT_QNAME = ContentModel.PROP_CONTENT;
+
     @InjectMocks
     private ContentServiceImpl contentService;
 
@@ -131,15 +134,10 @@ public class ContentServiceImplUnitTest
     public void testRequestContentDirectUrl_SystemWideIsDisabled()
     {
         setupSystemWideDirectAccessConfig(DISABLED);
-        try
+        assertThrows(DirectAccessUrlDisabledException.class, () -> {
-        {
+            contentService.requestContentDirectUrl(NODE_REF, PROP_CONTENT_QNAME, true, 20L);
-            contentService.requestContentDirectUrl(NODE_REF, true, 20L);
+        });
-            fail("Expected DirectAccessUrlDisabledException");
+        verify(mockContentStore, never()).isContentDirectUrlEnabled();
-        }
-        catch (DirectAccessUrlDisabledException ex)
-        {
-            verify(mockContentStore, never()).isContentDirectUrlEnabled();
-        }
     }
 
     @Test
@@ -148,7 +146,7 @@ public class ContentServiceImplUnitTest
         setupSystemWideDirectAccessConfig(ENABLED);
         when(mockContentStore.isContentDirectUrlEnabled()).thenReturn(DISABLED);
 
-        DirectAccessUrl directAccessUrl = contentService.requestContentDirectUrl(NODE_REF, true, 20L);
+        DirectAccessUrl directAccessUrl = contentService.requestContentDirectUrl(NODE_REF, PROP_CONTENT_QNAME,true, 20L);
         assertNull(directAccessUrl);
         verify(mockContentStore, never()).requestContentDirectUrl(anyString(), eq(true), anyString(), anyString(), anyLong());
     }
@@ -159,7 +157,7 @@ public class ContentServiceImplUnitTest
         setupSystemWideDirectAccessConfig(ENABLED);
         when(mockContentStore.isContentDirectUrlEnabled()).thenReturn(ENABLED);
 
-        DirectAccessUrl directAccessUrl = contentService.requestContentDirectUrl(NODE_REF, true, 20L);
+        DirectAccessUrl directAccessUrl = contentService.requestContentDirectUrl(NODE_REF, PROP_CONTENT_QNAME, true, 20L);
         assertNull(directAccessUrl);
         verify(mockContentStore, times(1)).requestContentDirectUrl(anyString(), eq(true), anyString(), anyString(), anyLong());
     }

repository/src/test/java/org/alfresco/repo/version/ContentServiceImplTest.java

@@ -25,6 +25,7 @@
  */
 package org.alfresco.repo.version;
 
+import static org.junit.Assert.assertThrows;
 import static org.mockito.Mockito.when;
 import static org.mockito.MockitoAnnotations.openMocks;
 
@@ -63,6 +64,7 @@ public class ContentServiceImplTest extends BaseVersionStoreTest
      * Test content data
      */
     private final static String UPDATED_CONTENT = "This content has been updated with a new value.";
+    private static final QName QNAME = ContentModel.PROP_CONTENT;
     
     /**
      * The version content store
@@ -157,34 +159,30 @@ public class ContentServiceImplTest extends BaseVersionStoreTest
         // Set the presigned URL to expire after one minute.
         Long validFor = 60L;
 
-        try
+        assertThrows("nodeRef has no content", IllegalArgumentException.class, () -> {
-        {
             // Create a node without content
             NodeRef nodeRef = this.dbNodeService
                     .createNode(rootNodeRef, ContentModel.ASSOC_CHILDREN, QName.createQName("{test}MyNoContentNode"), TEST_TYPE_QNAME, this.nodeProperties).getChildRef();
 
-            assertNull(contentService.requestContentDirectUrl(nodeRef, true, validFor));
+            assertNull(contentService.requestContentDirectUrl(nodeRef, QNAME, true, validFor));
-            fail("nodeRef has no content");
+        });
-        }
-        catch (IllegalArgumentException e)
-        {
-            // Expected exception
-        }
 
-        try
+        assertThrows("nodeRef is null", IllegalArgumentException.class, () -> {
-        {
+            assertNull(contentService.requestContentDirectUrl(null, null, true, null));
-            assertNull(contentService.requestContentDirectUrl(null, true, null));
+        });
-            fail("nodeRef is null");
+
-        }
+        assertThrows("propertyQName has no content", NullPointerException.class, () -> {
-        catch (IllegalArgumentException e)
+            // Create a node without content
-        {
+            NodeRef nodeRef = this.dbNodeService
-            // Expected exception
+                    .createNode(rootNodeRef, ContentModel.ASSOC_CHILDREN, QName.createQName("{test}MyNoContentNode"), TEST_TYPE_QNAME, this.nodeProperties).getChildRef();
-        }
+
+            contentService.requestContentDirectUrl(nodeRef, null, true, validFor);
+        });
 
         // Create a node with content
         NodeRef nodeRef = createNewVersionableNode();
 
-        assertNull(contentService.requestContentDirectUrl(nodeRef, true, null));
+        assertNull(contentService.requestContentDirectUrl(nodeRef, QNAME, true, null));
-        assertNull(contentService.requestContentDirectUrl(nodeRef, true, validFor));
+        assertNull(contentService.requestContentDirectUrl(nodeRef, QNAME, true, validFor));
     }
 }