Merged HEAD-BUG-FIX (4.3/Cloud) to HEAD (4.3/Cloud)

63670: Merged DEV to HEAD-BUG-FIX (4.3)
      63618: Fixed ACE-762: BM-0012: NodeLockedException not handled by CMIS
             - Refactored interceptor to handle previous TODO: Dig into exceptions to find deep causal issues of interest
             - Added unit test
             - Translate lock-related Alfresco exception into CMIS exception
      63619: Further testing for ACE-762: BM-0012: NodeLockedException not handled by CMIS
             - Extended test to ensure that locked nodes generate the correct CMIS exception
      63630: Added test for ACE-762 and cleaned up pointless warnings


git-svn-id: https://svn.alfresco.com/repos/alfresco-enterprise/alfresco/HEAD/root@64309 c4b6b30b-aa2e-2d43-bbcb-ca4b014f7261

source/java/org/alfresco/opencmis/AlfrescoCmisExceptionInterceptor.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2005-2010 Alfresco Software Limited.
+ * Copyright (C) 2005-2014 Alfresco Software Limited.
  *
  * This file is part of Alfresco
  *
@@ -18,10 +18,16 @@
  */
 package org.alfresco.opencmis;
 
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
+import org.alfresco.error.ExceptionStackUtil;
 import org.alfresco.repo.node.integrity.IntegrityException;
 import org.alfresco.repo.security.authentication.AuthenticationException;
 import org.alfresco.repo.security.permissions.AccessDeniedException;
 import org.alfresco.service.cmr.coci.CheckOutCheckInServiceException;
+import org.alfresco.service.cmr.lock.NodeLockedException;
 import org.alfresco.service.cmr.model.FileExistsException;
 import org.aopalliance.intercept.MethodInterceptor;
 import org.aopalliance.intercept.MethodInvocation;
@@ -30,47 +36,60 @@ import org.apache.chemistry.opencmis.commons.exceptions.CmisConstraintException;
 import org.apache.chemistry.opencmis.commons.exceptions.CmisContentAlreadyExistsException;
 import org.apache.chemistry.opencmis.commons.exceptions.CmisPermissionDeniedException;
 import org.apache.chemistry.opencmis.commons.exceptions.CmisRuntimeException;
+import org.apache.chemistry.opencmis.commons.exceptions.CmisUpdateConflictException;
 import org.apache.chemistry.opencmis.commons.exceptions.CmisVersioningException;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
 
 /**
  * Interceptor to catch various exceptions and translate them into CMIS-related exceptions
  * <p/>
  * TODO: Externalize messages
- * TODO: Use ExceptionStackUtil to dig out exceptions of interest regardless of depth
  * 
  * @author Derek Hulley
  * @since 4.0
  */
 public class AlfrescoCmisExceptionInterceptor implements MethodInterceptor
 {
+    /**
+     * Exceptions that are specifically handled.
+     */
+    @SuppressWarnings({ "rawtypes" })
+    public static final Class[] EXCEPTIONS_OF_INTEREST;
+    static
+    {
+        Class<?>[] coreClasses = new Class[] {
+                AuthenticationException.class,
+                CheckOutCheckInServiceException.class,
+                FileExistsException.class,
+                IntegrityException.class,     // Similar to StaleObjectState
+                AccessDeniedException.class,
+                NodeLockedException.class
+                };
+     
+        List<Class<?>> retryExceptions = new ArrayList<Class<?>>();
+        // Add core classes to the list.
+        retryExceptions.addAll(Arrays.asList(coreClasses));
+        
+        EXCEPTIONS_OF_INTEREST = retryExceptions.toArray(new Class[] {});
+    }
+
+    private static Log logger = LogFactory.getLog(AlfrescoCmisExceptionInterceptor.class);
+    
     public Object invoke(MethodInvocation mi) throws Throwable
     {
         try
         {
             return mi.proceed();
         }
-        catch (AuthenticationException e)
-        {
-            throw new CmisPermissionDeniedException(e.getMessage(), e);
-        }
-        catch (CheckOutCheckInServiceException e)
-        {
-            throw new CmisVersioningException("Check out failed: " + e.getMessage(), e);
-        }
-        catch (FileExistsException fee)
-        {
-            throw new CmisContentAlreadyExistsException("An object with this name already exists!", fee);
-        }
-        catch (IntegrityException ie)
-        {
-            throw new CmisConstraintException("Constraint violation: " + ie.getMessage(), ie);
-        }
-        catch (AccessDeniedException ade)
-        {
-            throw new CmisPermissionDeniedException("Permission denied!", ade);
-        }
         catch (Exception e)
         {
+            // We dig into the exception to see if there is anything of interest to CMIS
+            Throwable cmisAffecting = ExceptionStackUtil.getCause(e, EXCEPTIONS_OF_INTEREST);
+            
+            if (cmisAffecting == null)
+            {
+                // The exception is not something that CMIS needs to handle in any special way
                 if (e instanceof CmisBaseException)
                 {
                     throw (CmisBaseException) e;
@@ -80,5 +99,37 @@ public class AlfrescoCmisExceptionInterceptor implements MethodInterceptor
                     throw new CmisRuntimeException(e.getMessage(), e);
                 }
             }
+            // All other exceptions are carried through with full stacks but treated as the exception of interest
+            else if (cmisAffecting instanceof AuthenticationException)
+            {
+                throw new CmisPermissionDeniedException(cmisAffecting.getMessage(), e);
+            }
+            else if (cmisAffecting instanceof CheckOutCheckInServiceException)
+            {
+                throw new CmisVersioningException("Check out failed: " + cmisAffecting.getMessage(), e);
+            }
+            else if (cmisAffecting instanceof FileExistsException)
+            {
+                throw new CmisContentAlreadyExistsException("An object with this name already exists: " + cmisAffecting.getMessage(), e);
+            }
+            else if (cmisAffecting instanceof IntegrityException)
+            {
+                throw new CmisConstraintException("Constraint violation: " + cmisAffecting.getMessage(), e);
+            }
+            else if (cmisAffecting instanceof AccessDeniedException)
+            {
+                throw new CmisPermissionDeniedException("Permission denied: " + cmisAffecting.getMessage(), e);
+            }
+            else if (cmisAffecting instanceof NodeLockedException)
+            {
+                throw new CmisUpdateConflictException("Update conflict: " + cmisAffecting.getMessage(), e);
+            }
+            else
+            {
+                // We should not get here, so log an error but rethrow to have CMIS handle the original cause
+                logger.error("Exception type not handled correctly: " + e.getClass().getName());
+                throw new CmisRuntimeException(e.getMessage(), e);
+            }
+        }
     }
 }

source/java/org/alfresco/service/cmr/lock/NodeLockedException.java

@@ -42,6 +42,14 @@ public class NodeLockedException extends AlfrescoRuntimeException
     private static final String ERROR_MESSAGE = I18NUtil.getMessage("lock_service.no_op");
     private static final String ERROR_MESSAGE_2 = I18NUtil.getMessage("lock_service.no_op2");
 
+    /**
+     * Constructor for tests
+     */
+    public NodeLockedException()
+    {
+        super("TEST CONSTRUCTOR INVOKED FOR NodeLockedException");
+    }
+    
     /**
      * @param message
      */

source/test-java/org/alfresco/AllUnitTestsSuite.java

@@ -23,6 +23,7 @@ public class AllUnitTestsSuite extends TestSuite
 
     static void unitTests(TestSuite suite)
     {
+        suite.addTest(new JUnit4TestAdapter(org.alfresco.opencmis.AlfrescoCmisExceptionInterceptorTest.class));
         suite.addTestSuite(org.alfresco.cmis.PropertyFilterTest.class);
         suite.addTestSuite(org.alfresco.encryption.EncryptorTest.class);
         suite.addTestSuite(org.alfresco.encryption.KeyStoreKeyProviderTest.class);

source/test-java/org/alfresco/opencmis/AlfrescoCmisExceptionInterceptorTest.java

@@ -0,0 +1,136 @@
+/*
+ * Copyright (C) 2005-2014 Alfresco Software Limited.
+ *
+ * This file is part of Alfresco
+ *
+ * Alfresco is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * Alfresco is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with Alfresco. If not, see <http://www.gnu.org/licenses/>.
+ */
+package org.alfresco.opencmis;
+
+import static org.junit.Assert.fail;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import org.alfresco.repo.node.integrity.IntegrityException;
+import org.alfresco.repo.security.authentication.AuthenticationException;
+import org.alfresco.repo.security.permissions.AccessDeniedException;
+import org.alfresco.service.cmr.coci.CheckOutCheckInServiceException;
+import org.alfresco.service.cmr.lock.NodeLockedException;
+import org.alfresco.service.cmr.model.FileExistsException;
+import org.aopalliance.intercept.MethodInvocation;
+import org.apache.chemistry.opencmis.commons.exceptions.CmisConstraintException;
+import org.apache.chemistry.opencmis.commons.exceptions.CmisContentAlreadyExistsException;
+import org.apache.chemistry.opencmis.commons.exceptions.CmisPermissionDeniedException;
+import org.apache.chemistry.opencmis.commons.exceptions.CmisUpdateConflictException;
+import org.apache.chemistry.opencmis.commons.exceptions.CmisVersioningException;
+import org.junit.Assert;
+import org.junit.Test;
+
+/**
+ * Ensure that CMIS handles specific types of Alfresco exceptions
+ * 
+ * @author Derek Hulley
+ * @since 4.3
+ */
+public class AlfrescoCmisExceptionInterceptorTest
+{
+    private AlfrescoCmisExceptionInterceptor interceptor = new AlfrescoCmisExceptionInterceptor();
+    
+    /**
+     * Does the mock call ensuring that the exception is thrown
+     * @throws throws the exception provided
+     */
+    private void doMockCall(Exception toThrow, Class<?> toCatch) throws Throwable
+    {
+        MethodInvocation mi = mock(MethodInvocation.class);
+        when(mi.proceed()).thenThrow(toThrow);
+        try
+        {
+            interceptor.invoke(mi);
+            fail("Expected an exception to be thrown here.");
+        }
+        catch (Exception e)
+        {
+            Assert.assertEquals("Incorrect exception thrown: ", toCatch, e.getClass());
+        }
+    }
+    
+    @Test
+    public void testNoException() throws Throwable
+    {
+        MethodInvocation mi = mock(MethodInvocation.class);
+        when(mi.proceed()).thenReturn("BOB");
+        interceptor.invoke(mi);
+    }
+    
+    @Test
+    public void testAuthenticationException() throws Throwable
+    {
+        Exception e = new AuthenticationException("x");
+        Class<?> toCatch = CmisPermissionDeniedException.class;
+        
+        doMockCall(e, toCatch);
+        doMockCall(new RuntimeException(new RuntimeException(e)), toCatch);
+    }
+    
+    @Test
+    public void testCheckOutCheckInServiceException() throws Throwable
+    {
+        Exception e = new CheckOutCheckInServiceException("x");
+        Class<?> toCatch = CmisVersioningException.class;
+        
+        doMockCall(e, toCatch);
+        doMockCall(new RuntimeException(new RuntimeException(e)), toCatch);
+    }
+    
+    @Test
+    public void testFileExistsException() throws Throwable
+    {
+        Exception e = new FileExistsException(null, null);
+        Class<?> toCatch = CmisContentAlreadyExistsException.class;
+        
+        doMockCall(e, toCatch);
+        doMockCall(new RuntimeException(new RuntimeException(e)), toCatch);
+    }
+    
+    @Test
+    public void testIntegrityException() throws Throwable
+    {
+        Exception e = new IntegrityException(null);
+        Class<?> toCatch = CmisConstraintException.class;
+        
+        doMockCall(e, toCatch);
+        doMockCall(new RuntimeException(new RuntimeException(e)), toCatch);
+    }
+    
+    @Test
+    public void testAccessDeniedException() throws Throwable
+    {
+        Exception e = new AccessDeniedException("x");
+        Class<?> toCatch = CmisPermissionDeniedException.class;
+        
+        doMockCall(e, toCatch);
+        doMockCall(new RuntimeException(new RuntimeException(e)), toCatch);
+    }
+    
+    @Test
+    public void testNodeLockedException() throws Throwable
+    {
+        Exception e = new NodeLockedException();
+        Class<?> toCatch = CmisUpdateConflictException.class;
+        
+        doMockCall(e, toCatch);
+        doMockCall(new RuntimeException(new RuntimeException(e)), toCatch);
+    }
+}

source/test-java/org/alfresco/opencmis/CMISTest.java

@@ -78,6 +78,7 @@ import org.apache.chemistry.opencmis.commons.enums.CmisVersion;
 import org.apache.chemistry.opencmis.commons.enums.IncludeRelationships;
 import org.apache.chemistry.opencmis.commons.enums.VersioningState;
 import org.apache.chemistry.opencmis.commons.exceptions.CmisConstraintException;
+import org.apache.chemistry.opencmis.commons.exceptions.CmisUpdateConflictException;
 import org.apache.chemistry.opencmis.commons.impl.dataobjects.CmisExtensionElementImpl;
 import org.apache.chemistry.opencmis.commons.impl.dataobjects.ContentStreamImpl;
 import org.apache.chemistry.opencmis.commons.impl.dataobjects.PropertiesImpl;
@@ -246,19 +247,12 @@ public class CMISTest
             return Long.MAX_VALUE;
         }
 
-        /* (non-Javadoc)
-         * @see org.apache.chemistry.opencmis.commons.server.CallContext#encryptTempFiles()
-         */
         @Override
         public boolean encryptTempFiles()
         {
-            // TODO Auto-generated method stub
             return false;
         }
 
-        /* (non-Javadoc)
-         * @see org.apache.chemistry.opencmis.commons.server.CallContext#getCmisVersion()
-         */
         @Override
         public CmisVersion getCmisVersion()
         {
@@ -559,6 +553,7 @@ public class CMISTest
     private static class TestContext
     {
         private String repositoryId = null;
+        @SuppressWarnings("unused")
         private ObjectData objectData = null;
         private Holder<String> objectId = null;
         
@@ -577,11 +572,6 @@ public class CMISTest
             this.repositoryId = repositoryId;
         }
         
-        public ObjectData getObjectData()
-        {
-            return objectData;
-        }
-        
         public void setObjectData(ObjectData objectData)
         {
             this.objectData = objectData;
@@ -824,11 +814,14 @@ public class CMISTest
     }
 
     /**
-     * Test MNT-8825: READ_ONLYLOCK prevent getAllVersions via new CMIS enpoint.
+     * Test
+     * <ul>
+     *   <li>MNT-8825: READ_ONLYLOCK prevent getAllVersions via new CMIS enpoint.</li>
+     *   <li>ACE-762: BM-0012: NodeLockedException not handled by CMIS</li>
+     * </ul>
      */
-
     @Test
-    public void testGetAllVersionsOnReadOnlyLockedNode()
+    public void testOperationsOnReadOnlyLockedNode()
     {
         final String folderName = "testfolder." + GUID.generate();
         final String docName = "testdoc.txt." + GUID.generate();
@@ -870,14 +863,36 @@ public class CMISTest
                     ObjectData objectData = cmisService.getObjectByPath(repositoryId, "/" + folderName + "/" + docName, null, true,
                             IncludeRelationships.NONE, null, false, true, null);
 
+                    // Expect no failure
+                    cmisService.getAllVersions(repositoryId, objectData.getId(), fileInfo.getNodeRef().getId(), null, true, null);
+
+                    return null;
+                }
+            });
+
+            withCmisService(new CmisServiceCallback<Void>()
+            {
+                @Override
+                public Void execute(CmisService cmisService)
+                {
+                    List<RepositoryInfo> repositories = cmisService.getRepositoryInfos(null);
+                    assertTrue(repositories.size() > 0);
+                    RepositoryInfo repo = repositories.get(0);
+                    String repositoryId = repo.getId();
+                    ObjectData objectData = cmisService.getObjectByPath(
+                            repositoryId, "/" + folderName + "/" + docName, null, true,
+                            IncludeRelationships.NONE, null, false, true, null);
+                    String objectId = objectData.getId();
+
+                    // Expect failure as the node is locked
                     try
                     {
-                        cmisService.getAllVersions(repositoryId, objectData.getId(), fileInfo.getNodeRef().getId(), null, true, null);
+                        cmisService.deleteObject(repositoryId, objectId, true, null);
+                        fail("Locked node should not be deletable.");
                     }
-                    catch (Throwable e)
+                    catch (CmisUpdateConflictException e)
                     {
-                        e.printStackTrace();
+                        // Expected
-                        fail();
                     }
 
                     return null;
@@ -891,7 +906,7 @@ public class CMISTest
         }
     }
 
-    /*
+    /**
      * ALF-18455
      */
     @Test
@@ -1358,6 +1373,7 @@ public class CMISTest
      * MNT-10223
      * Check the IsLatestMajorVersion for a doc with minor version.
      */
+    @SuppressWarnings("unused")
     @Test
     public void testIsLatestMajorVersion()
     {
@@ -1425,6 +1441,7 @@ public class CMISTest
             	TypeDefinition def = cmisService.getTypeDefinition(repositoryId, "cmis:item", null);
             	assertNotNull("the cmis:item type is not defined", def); 
                 
+            	@SuppressWarnings("unused")
                 TypeDefinition p = cmisService.getTypeDefinition(repositoryId, "I:cm:person", null);
             	assertNotNull("the I:cm:person type is not defined", def);