500 error in image service when using filters #9497

modules/core/core-image/src/main/java/com/enonic/xp/core/impl/image/ImageServiceImpl.java

@@ -156,7 +156,8 @@ private boolean createImage( final ByteSource blob, final NormalizedImageParams
 
                     final boolean toRotate = readImageParams.getOrientation() != ImageOrientation.TopLeft;
                     final boolean toApplyFilters = !readImageParams.getFilterParam().isEmpty();
-                    final boolean toAddBackground = !"png".equals( readImageParams.getFormat() ) && originalColorModel.hasAlpha();
+                    final boolean toAddBackground =
+                        !"png".equals( readImageParams.getFormat() ) && ( originalColorModel.hasAlpha() || toApplyFilters );
                     final boolean toScale = readImageParams.getScaleParams() != null;
                     final boolean toCrop = readImageParams.getCropping() != null;
 

modules/core/core-image/src/main/java/com/enonic/xp/core/impl/image/ImmutableFilesHelper.java

@@ -25,15 +25,17 @@ public static ByteSource computeIfAbsent( Path path, Function<ByteSink, Boolean>
 
         final Lock lock = FILE_LOCKS.get( path );
         lock.lock();
+
         try
         {
             if ( !Files.exists( path ) )
             {
                 Files.createDirectories( path.getParent() );
 
+                Boolean written = false;
                 try
                 {
-                    final Boolean written = consumer.apply( MoreFiles.asByteSink( path ) );
+                    written = consumer.apply( MoreFiles.asByteSink( path ) );
                     if ( !Boolean.TRUE.equals( written ) )
                     {
                         return null;
@@ -43,6 +45,13 @@ public static ByteSource computeIfAbsent( Path path, Function<ByteSink, Boolean>
                 {
                     throw e.getCause();
                 }
+                finally
+                {
+                    if ( !Boolean.TRUE.equals( written ) )
+                    {
+                        Files.deleteIfExists( path );
+                    }
+                }
             }
         }
         finally

modules/core/core-image/src/test/java/com/enonic/xp/core/impl/image/ImageServiceImplTest.java

@@ -1,15 +1,14 @@
 package com.enonic.xp.core.impl.image;
 
 import java.io.IOException;
+import java.io.InputStream;
 import java.nio.file.Path;
 
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.io.TempDir;
-import org.mockito.Mockito;
 
 import com.google.common.io.ByteSource;
-import com.google.common.io.ByteStreams;
 
 import com.enonic.xp.content.ContentId;
 import com.enonic.xp.content.ContentService;
@@ -19,19 +18,20 @@
 import com.enonic.xp.util.BinaryReference;
 
 import static org.junit.jupiter.api.Assertions.assertArrayEquals;
-import static org.mockito.ArgumentMatchers.any;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
 import static org.mockito.Mockito.when;
 
-public class ImageServiceImplTest
+class ImageServiceImplTest
 {
     @TempDir
     public Path temporaryFolder;
 
     private ContentService contentService;
 
-    private ImageFunction imageFilter;
-
     private ImageServiceImpl imageService;
 
     private ContentId contentId;
@@ -41,34 +41,51 @@ public class ImageServiceImplTest
     private byte[] imageDataOriginal;
 
     @BeforeEach
-    public void setUp()
-        throws IOException
+    void setUp()
     {
         System.setProperty( "xp.home", temporaryFolder.toFile().getPath() );
 
         contentId = ContentId.from( "contentId" );
         binaryReference = BinaryReference.from( "binaryRef" );
         contentService = mock( ContentService.class );
-        imageDataOriginal = ByteStreams.toByteArray( getClass().getResourceAsStream( "effect/transparent.png" ) );
-        when( contentService.getBinary( contentId, binaryReference ) ).thenReturn( ByteSource.wrap( imageDataOriginal ) );
-        when( contentService.getBinaryKey( contentId, binaryReference ) ).thenReturn( "binaryKey" );
 
-        ImageFilterBuilder imageFilterBuilder = mock( ImageFilterBuilder.class );
-        imageFilter = mock( ImageFunction.class );
-        when( imageFilter.apply( any() ) ).thenAnswer( invocation -> invocation.getArgument( 0 ) );
-        when( imageFilterBuilder.build( any() ) ).thenReturn( imageFilter );
+        final ImageConfig imageConfig = mock( ImageConfig.class, invocation -> invocation.getMethod().getDefaultValue() );
+
+        ImageFilterBuilderImpl imageFilterBuilder = new ImageFilterBuilderImpl();
+        imageFilterBuilder.activate( imageConfig );
 
         final ImageScaleFunctionBuilderImpl imageScaleFunctionBuilder = new ImageScaleFunctionBuilderImpl();
-        final ImageConfig imageConfig = mock( ImageConfig.class, invocation -> invocation.getMethod().getDefaultValue() );
+
         imageScaleFunctionBuilder.activate( imageConfig );
 
         imageService = new ImageServiceImpl( contentService, imageScaleFunctionBuilder, imageFilterBuilder, imageConfig );
     }
 
+    private byte[] readImage( final String path )
+    {
+        try (InputStream is = getClass().getResourceAsStream( path ))
+        {
+            return is.readAllBytes();
+        }
+        catch ( IOException e )
+        {
+            throw new RuntimeException( e );
+        }
+    }
+
+    private void mockOriginalImage( final String path )
+    {
+        imageDataOriginal = readImage( path );
+        when( contentService.getBinary( contentId, binaryReference ) ).thenReturn( ByteSource.wrap( imageDataOriginal ) );
+        when( contentService.getBinaryKey( contentId, binaryReference ) ).thenReturn( "binaryKey" );
+    }
+
     @Test
-    public void testReadImageMinimal()
+    void readImage_minimal()
         throws IOException
     {
+        mockOriginalImage( "effect/transparent.png" );
+
         final ReadImageParams readImageParams =
             ReadImageParams.newImageParams().contentId( contentId ).binaryReference( binaryReference ).mimeType( "image/png" ).build();
         final ByteSource imageData = imageService.readImage( readImageParams );
@@ -78,9 +95,11 @@ public void testReadImageMinimal()
 
     @Test
     @Deprecated
-    public void testReadImageWithFormat()
+    void readImage_with_format()
         throws IOException
     {
+        mockOriginalImage( "effect/transparent.png" );
+
         final ReadImageParams readImageParams =
             ReadImageParams.newImageParams().contentId( contentId ).binaryReference( binaryReference ).format( "png" ).build();
         final ByteSource imageData = imageService.readImage( readImageParams );
@@ -89,29 +108,50 @@ public void testReadImageWithFormat()
     }
 
     @Test
-    public void testReadImageWithCache()
+    public void readImage_with_cache()
         throws IOException
     {
+        mockOriginalImage( "effect/transparent.png" );
+
         Cropping cropping = Cropping.create().top( 0.25 ).bottom( 0.75 ).left( 0.25 ).right( 0.75 ).zoom( 2 ).build();
 
-        final ReadImageParams readImageParams = ReadImageParams.newImageParams().
-            contentId( contentId ).
-            binaryReference( binaryReference ).
-            cropping( cropping ).
-            scaleSize( 128 ).
-            filterParam( "blur(10)" ).
-            mimeType( "image/jpeg" ).
-            backgroundColor( 0xFF0000 ).
-            quality( 5 ).
-            orientation( ImageOrientation.BottomLeft ).
-            build();
+        final ReadImageParams readImageParams = ReadImageParams.newImageParams()
+            .contentId( contentId )
+            .binaryReference( binaryReference )
+            .cropping( cropping )
+            .scaleSize( 128 )
+            .filterParam( "blur(10)" )
+            .mimeType( "image/jpeg" )
+            .backgroundColor( 0xFF0000 )
+            .quality( 5 )
+            .orientation( ImageOrientation.BottomLeft )
+            .build();
 
         ByteSource imageData = imageService.readImage( readImageParams );
-        assertArrayEquals( ByteStreams.toByteArray( getClass().getResourceAsStream( "processed.jpg" ) ), imageData.read() );
-        Mockito.verify( imageFilter ).apply( any() );
+
+        assertArrayEquals( readImage( "processed.jpg" ), imageData.read() );
 
         imageData = imageService.readImage( readImageParams );
-        assertArrayEquals( ByteStreams.toByteArray( getClass().getResourceAsStream( "processed.jpg" ) ), imageData.read() );
-        Mockito.verify( imageFilter ).apply( any() );
+        assertArrayEquals( readImage( "processed.jpg" ), imageData.read() );
+
+        verify( contentService, times( 2 ) ).getBinaryKey( contentId, binaryReference );
+        verify( contentService ).getBinary( contentId, binaryReference );
+        verifyNoMoreInteractions( contentService );
+    }
+
+    @Test
+    public void readImage_filter_on_jpeg()
+    {
+        mockOriginalImage( "effect/source.jpg" );
+
+        final ReadImageParams readImageParams = ReadImageParams.newImageParams()
+            .contentId( contentId )
+            .binaryReference( binaryReference )
+            .filterParam( "sepia(10)" )
+            .mimeType( "image/jpeg" )
+            .orientation( ImageOrientation.BottomLeft )
+            .build();
+
+        assertDoesNotThrow( () -> imageService.readImage( readImageParams ) );
     }
 }