From 1f18805733091041d1646fd1072ccca2eacc70c3 Mon Sep 17 00:00:00 2001 From: Ayman Harake <42536841+aymanthefirst@users.noreply.github.com> Date: Fri, 3 Dec 2021 18:12:02 +0000 Subject: [PATCH] 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 --- .../org/alfresco/rest/api/impl/NodesImpl.java | 2 +- .../repo/content/ContentServiceImpl.java | 9 ++-- .../cmr/repository/ContentService.java | 52 ++++++++++++++++++- .../content/ContentServiceImplUnitTest.java | 20 ++++--- .../repo/version/ContentServiceImplTest.java | 38 +++++++------- 5 files changed, 83 insertions(+), 38 deletions(-) diff --git a/remote-api/src/main/java/org/alfresco/rest/api/impl/NodesImpl.java b/remote-api/src/main/java/org/alfresco/rest/api/impl/NodesImpl.java index 1f88b43ca7..46793ddeaf 100644 --- a/remote-api/src/main/java/org/alfresco/rest/api/impl/NodesImpl.java +++ b/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."); diff --git a/repository/src/main/java/org/alfresco/repo/content/ContentServiceImpl.java b/repository/src/main/java/org/alfresco/repo/content/ContentServiceImpl.java index f535a97a48..4ba6c7ebec 100644 --- a/repository/src/main/java/org/alfresco/repo/content/ContentServiceImpl.java +++ b/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) { diff --git a/repository/src/main/java/org/alfresco/service/cmr/repository/ContentService.java b/repository/src/main/java/org/alfresco/service/cmr/repository/ContentService.java index 6c2c72d9d3..830770b13a 100644 --- a/repository/src/main/java/org/alfresco/service/cmr/repository/ContentService.java +++ b/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 content + * @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 content + * @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 content + * @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. diff --git a/repository/src/test/java/org/alfresco/repo/content/ContentServiceImplUnitTest.java b/repository/src/test/java/org/alfresco/repo/content/ContentServiceImplUnitTest.java index 212c27f92c..658f9a25a3 100644 --- a/repository/src/test/java/org/alfresco/repo/content/ContentServiceImplUnitTest.java +++ b/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 - { - contentService.requestContentDirectUrl(NODE_REF, true, 20L); - fail("Expected DirectAccessUrlDisabledException"); - } - catch (DirectAccessUrlDisabledException ex) - { - verify(mockContentStore, never()).isContentDirectUrlEnabled(); - } + assertThrows(DirectAccessUrlDisabledException.class, () -> { + contentService.requestContentDirectUrl(NODE_REF, PROP_CONTENT_QNAME, true, 20L); + }); + 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()); } diff --git a/repository/src/test/java/org/alfresco/repo/version/ContentServiceImplTest.java b/repository/src/test/java/org/alfresco/repo/version/ContentServiceImplTest.java index 5423fd3077..70b232a153 100644 --- a/repository/src/test/java/org/alfresco/repo/version/ContentServiceImplTest.java +++ b/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)); - fail("nodeRef has no content"); - } - catch (IllegalArgumentException e) - { - // Expected exception - } + assertNull(contentService.requestContentDirectUrl(nodeRef, QNAME, true, validFor)); + }); - try - { - assertNull(contentService.requestContentDirectUrl(null, true, null)); - fail("nodeRef is null"); - } - catch (IllegalArgumentException e) - { - // Expected exception - } + assertThrows("nodeRef is null", IllegalArgumentException.class, () -> { + assertNull(contentService.requestContentDirectUrl(null, null, true, null)); + }); + + assertThrows("propertyQName has no content", NullPointerException.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(); + + contentService.requestContentDirectUrl(nodeRef, null, true, validFor); + }); // Create a node with content NodeRef nodeRef = createNewVersionableNode(); - assertNull(contentService.requestContentDirectUrl(nodeRef, true, null)); - assertNull(contentService.requestContentDirectUrl(nodeRef, true, validFor)); + assertNull(contentService.requestContentDirectUrl(nodeRef, QNAME, true, null)); + assertNull(contentService.requestContentDirectUrl(nodeRef, QNAME, true, validFor)); } }