Blobstore can throw FileAlreadyExistsException #10447

modules/blobstore/blobstore-file/src/main/java/com/enonic/xp/internal/blobstore/file/FileBlobStore.java

@@ -2,10 +2,10 @@
 
 import java.io.IOException;
 import java.nio.file.DirectoryNotEmptyException;
+import java.nio.file.FileAlreadyExistsException;
 import java.nio.file.Files;
 import java.nio.file.NoSuchFileException;
 import java.nio.file.Path;
-import java.nio.file.attribute.FileTime;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
@@ -172,11 +172,14 @@
         if ( !Files.exists( file ) )
         {
             Files.createDirectories( file.getParent() );
-            in.copyTo( MoreFiles.asByteSink( file ) );
-        }
-        else
-        {
-            Files.setLastModifiedTime( file, FileTime.fromMillis( System.currentTimeMillis() ) );
+            try (var inStream = in.openStream())
+            {
+                Files.copy( inStream, file );
+            }
+            catch ( FileAlreadyExistsException e )
+            {
+                LOG.debug( "File already exists [{}]", file, e );
+            }
         }
 
         return new FileBlobRecord( key, file );

modules/blobstore/blobstore-file/src/test/java/com/enonic/xp/internal/blobstore/file/FileBlobStoreTest.java

@@ -19,7 +19,6 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
-import static org.junit.jupiter.api.Assertions.assertTrue;
 
 public class FileBlobStoreTest
 {
@@ -120,18 +119,6 @@ public void deleteSegment()
         assertEquals( segment, blobStore.listSegments().findFirst().get() );
     }
 
-    @Test
-    public void lastModifiedUpdated()
-        throws Exception
-    {
-        final BlobRecord rec1 = this.blobStore.addRecord( this.segment, ByteSource.wrap( "hello".getBytes() ) );
-        final long beforeUpdate = rec1.lastModified();
-        Thread.sleep( 1000 ); // ensure a second difference
-        final BlobRecord rec2 = this.blobStore.addRecord( this.segment, ByteSource.wrap( "hello".getBytes() ) );
-
-        assertTrue( beforeUpdate < rec2.lastModified() );
-    }
-
     private BlobRecord createRecord( final String str )
     {
         return createRecord( segment, str );

modules/core/core-blobstore/src/main/java/com/enonic/xp/internal/blobstore/cache/CachedBlobStore.java

@@ -1,6 +1,8 @@
 package com.enonic.xp.internal.blobstore.cache;
 
 import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.util.Objects;
 import java.util.stream.Stream;
 
 import org.slf4j.Logger;
@@ -76,7 +78,6 @@
         throws BlobStoreException
     {
         this.store.removeRecord( segment, key );
-        this.cache.invalidate( key );
     }
 
     @Override
@@ -87,21 +88,32 @@
 
     private BlobRecord addToCache( final BlobRecord record )
     {
-        if ( record.getLength() <= this.sizeThreshold )
+        // Quick check to avoid caching large blobs
+        if ( record.getLength() > this.sizeThreshold )
         {
-            try
-            {
-                final CacheBlobRecord cacheBlobRecord = new CacheBlobRecord( record );
-                this.cache.put( record.getKey(), cacheBlobRecord );
-                return cacheBlobRecord;
-            }
-            catch ( IOException e )
-            {
-                LOG.error( "Could not create cache blob-record", e );
-            }
+            return record;
+        }
+
+        // Don't do heavy lifting if the blob is already in the cache
+        final BlobKey key = record.getKey();
+        final CacheBlobRecord present = this.cache.getIfPresent( key );
+        if ( present != null )
+        {
+            return present;
         }
 
-        return record;
+        try
+        {
+            // We don't want return blobs that are not in cache - as we want them to garbage collected ASAP,
+            // so we dont use get() with loader here
+            this.cache.put( key, new CacheBlobRecord( record ) );
+        }
+        catch ( IOException e )
+        {
+            LOG.warn( "Could not cache blob-record with key {}", key, e );
+        }
+        // If we could not load the blob into cache, we return the original record
+        return Objects.requireNonNullElse( this.cache.getIfPresent( key ), record );
     }
 
     @Override
@@ -171,4 +183,58 @@
             return (int) Math.min( record.getLength(), Integer.MAX_VALUE );
         }
     }
+
+    private static final class CacheBlobRecord
+        implements BlobRecord
+    {
+        final BlobKey blobKey;
+
+        final long lastModified;
+
+        final ByteSource content;
+
+        CacheBlobRecord( final BlobRecord blobRecord )
+            throws IOException
+        {
+            this.content = ByteSource.wrap( blobRecord.getBytes().read() );
+            this.blobKey = BlobKey.from( this.content );
+            if ( !blobRecord.getKey().equals( this.blobKey ) )
+            {
+                throw new IOException( String.format( "Cache BlobKey must be the same as the key of the BlobRecord: %s != %s", this.blobKey,
+                                                      blobRecord.getKey() ) );
+            }
+            this.lastModified = blobRecord.lastModified();
+        }
+
+        @Override
+        public BlobKey getKey()
+        {
+            return this.blobKey;
+        }
+
+        @Override
+        public long getLength()
+        {
+            try
+            {
+                return content.size();
+            }
+            catch ( IOException e )
+            {
+                throw new UncheckedIOException( e );
+            }
+        }
+
+        @Override
+        public ByteSource getBytes()
+        {
+            return this.content;
+        }
+
+        @Override
+        public long lastModified()
+        {
+            return this.lastModified;
+        }
+    }
 }