diff --git a/LTS-CHANGELOG.adoc b/LTS-CHANGELOG.adoc index 6affcdedad..7699a2ac6e 100644 --- a/LTS-CHANGELOG.adoc +++ b/LTS-CHANGELOG.adoc @@ -18,10 +18,13 @@ The LTS changelog lists releases which are only accessible via a commercial subs All fixes and changes in LTS releases will be released the next minor release. Changes from LTS 1.4.x will be included in release 1.5.0. [[v1.10.37]] -== 1.10.37 (09.12.2024) +== 1.10.37 (19.12.2024) icon:check[] Core: The node deletion rules has been strictened, to avoid internal deletion API misusage. +icon:check[] Cache: The Image Cache refactoring, which was done in a previous hotfix release introduced an error which caused creation of empty folders in the old structure as well. This has been fixed. +Also the migration process has been fixed to really remove all folders of the old structure, even if they are empty or contain cache files of binaries that were deleted before. + [[v1.10.36]] == 1.10.36 (23.10.2024) diff --git a/common/src/main/java/com/gentics/mesh/core/image/spi/AbstractImageManipulator.java b/common/src/main/java/com/gentics/mesh/core/image/spi/AbstractImageManipulator.java index a03fafc189..4cfc4c9ecf 100644 --- a/common/src/main/java/com/gentics/mesh/core/image/spi/AbstractImageManipulator.java +++ b/common/src/main/java/com/gentics/mesh/core/image/spi/AbstractImageManipulator.java @@ -112,38 +112,40 @@ protected Single getCacheFilePathOld(String sha512sum, ImageManip String baseFolder = Paths.get(options.getImageCacheDirectory(), buffer.toString()).toString(); String baseName = "image-" + parameters.getCacheKey(); - return fs.rxMkdirs(baseFolder) - // Vert.x uses Files.createDirectories internally, which will not fail when the folder already exists. - // See https://github.com/eclipse-vertx/vert.x/issues/3029 - .andThen(fs.rxReadDir(baseFolder, baseName + "(\\..*)?")) - .map(foundFiles -> { - int numFiles = foundFiles.size(); - if (numFiles == 0) { - String retPath = Paths.get(baseFolder, baseName).toString(); - if (log.isDebugEnabled()) { - log.debug("No cache file found for base path {" + retPath + "}"); - } - return new CacheFileInfo(maybeNewPath.orElse(retPath), false); - } + return fs.rxExists(baseFolder).flatMap(exists -> { + if (exists) { + return fs.rxReadDir(baseFolder, baseName + "(\\..*)?").flatMap(foundFiles -> { + int numFiles = foundFiles.size(); + if (numFiles == 0) { + String retPath = Paths.get(baseFolder, baseName).toString(); + if (log.isDebugEnabled()) { + log.debug("No cache file found for base path {" + retPath + "}"); + } + return Single.just(new CacheFileInfo(maybeNewPath.orElse(retPath), false)); + } - if (numFiles > 1) { - String indent = System.lineSeparator() + " - "; - - log.warn( - "More than one cache file found:" - + System.lineSeparator() + " hash: " + sha512sum - + System.lineSeparator() + " key: " + parameters.getCacheKey() - + System.lineSeparator() + " files:" - + indent - + String.join(indent, foundFiles) - + System.lineSeparator() - + "The cache directory {" + options.getImageCacheDirectory() + "} should be cleared"); - } + if (numFiles > 1) { + String indent = System.lineSeparator() + " - "; + + log.warn( + "More than one cache file found:" + + System.lineSeparator() + " hash: " + sha512sum + + System.lineSeparator() + " key: " + parameters.getCacheKey() + + System.lineSeparator() + " files:" + + indent + + String.join(indent, foundFiles) + + System.lineSeparator() + + "The cache directory {" + options.getImageCacheDirectory() + "} should be cleared"); + } - if (log.isDebugEnabled()) { - log.debug("Using cache file {" + foundFiles.size() + "}"); + if (log.isDebugEnabled()) { + log.debug("Using cache file {" + foundFiles.size() + "}"); + } + return Single.just(new CacheFileInfo(foundFiles.get(0), true)); + }); + } else { + return Single.just(new CacheFileInfo(baseName, false)); } - return new CacheFileInfo(foundFiles.get(0), true); }); } diff --git a/mdm/common/src/main/java/com/gentics/mesh/core/jobs/ImageCacheMigrationProcessor.java b/mdm/common/src/main/java/com/gentics/mesh/core/jobs/ImageCacheMigrationProcessor.java index db20b6902e..51e40a7ebe 100644 --- a/mdm/common/src/main/java/com/gentics/mesh/core/jobs/ImageCacheMigrationProcessor.java +++ b/mdm/common/src/main/java/com/gentics/mesh/core/jobs/ImageCacheMigrationProcessor.java @@ -4,15 +4,19 @@ import static com.gentics.mesh.core.rest.job.JobStatus.FAILED; import java.io.File; -import java.io.FileNotFoundException; import java.io.IOException; import java.nio.file.DirectoryNotEmptyException; +import java.nio.file.FileVisitResult; +import java.nio.file.FileVisitor; import java.nio.file.Files; import java.nio.file.NoSuchFileException; import java.nio.file.Path; +import java.nio.file.attribute.BasicFileAttributes; import javax.inject.Inject; +import org.apache.commons.lang3.StringUtils; + import com.gentics.mesh.core.data.binary.HibBinary; import com.gentics.mesh.core.data.dao.BinaryDao; import com.gentics.mesh.core.data.job.HibJob; @@ -52,41 +56,68 @@ public Completable process(HibJob job) { return db.asyncTx(() -> { log.info("Image cache migration started"); Path imageCachePath = Path.of(options.getImageOptions().getImageCacheDirectory()); - Files.walk(imageCachePath) - .filter(path -> path.getFileName().toString().startsWith("image-") && Files.isRegularFile(path)) - .map(path -> { - String sha512Hash = imageCachePath.relativize(path.getParent()).toString().replace("/", "").replace("\\", ""); - HibBinary binary = null; - if (sha512Hash.length() == 128 && (binary = dao.findByHash(sha512Hash).runInExistingTx(Tx.get())) != null) { - String uuid = binary.getUuid(); - String segments = getSegmentedPath(uuid); - Path segmentsPath = Path.of(options.getImageOptions().getImageCacheDirectory(), segments); - try { - Files.createDirectories(segmentsPath); - Files.move(path, segmentsPath.resolve(uuid + "-" + path.getFileName().toString().replace("image-", ""))); - } catch (IOException e) { - log.error("Could not copy old cached file " + path, e); - } - } else { - log.error("Not a SHA512 hash or binary not found: " + sha512Hash); + + // walk the whole tree + Files.walkFileTree(imageCachePath, new FileVisitor() { + + @Override + public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) throws IOException { + // continue from the root directory + if (dir.equals(imageCachePath)) { + return FileVisitResult.CONTINUE; } - return path; - }).forEach(path -> { - while (path != null && !path.equals(imageCachePath)) { - try { - Files.delete(path); - } catch (DirectoryNotEmptyException e) { - // fair - return; - } catch (NoSuchFileException e) { - // fair, but continue with parent - } catch (IOException e) { - log.error("Could not delete image cache " + path, e); - return; + // continue with directories, which are of the old structure (identifiable by directory names with 8 characters) + return StringUtils.length(dir.toFile().getName()) == 8 ? FileVisitResult.CONTINUE : FileVisitResult.SKIP_SUBTREE; + } + + @Override + public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { + String fileName = file.toFile().getName(); + // files that start with "image-" and are located in the old structure need to be migrated + if (fileName.startsWith("image-")) { + String sha512Hash = imageCachePath.relativize(file.getParent()).toString().replace("/", "").replace("\\", ""); + HibBinary binary = null; + if (sha512Hash.length() == 128 && (binary = dao.findByHash(sha512Hash).runInExistingTx(Tx.get())) != null) { + String uuid = binary.getUuid(); + String segments = getSegmentedPath(uuid); + Path segmentsPath = Path.of(options.getImageOptions().getImageCacheDirectory(), segments); + try { + Files.createDirectories(segmentsPath); + Files.move(file, segmentsPath.resolve(uuid + "-" + fileName.replace("image-", ""))); + } catch (IOException e) { + log.error("Could not copy old cached file " + file, e); + } + } else if (sha512Hash.length() == 128) { + log.info("Binary not found: " + sha512Hash + " deleting file " + file); + Files.delete(file); + } else { + log.info("Not a SHA512: " + sha512Hash); } - path = path.getParent(); } - }); + + return FileVisitResult.CONTINUE; + } + + @Override + public FileVisitResult visitFileFailed(Path file, IOException exc) throws IOException { + return FileVisitResult.CONTINUE; + } + + @Override + public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException { + try { + Files.delete(dir); + } catch (DirectoryNotEmptyException e) { + // fair + } catch (NoSuchFileException e) { + // fair + } catch (IOException e) { + log.error("Could not delete image cache " + dir, e); + } + return FileVisitResult.CONTINUE; + } + }); + log.info("Image cache migration finished successfully"); }); }).doOnComplete(() -> { diff --git a/tests/tests-core/src/main/java/com/gentics/mesh/core/field/binary/BinaryFieldEndpointTest.java b/tests/tests-core/src/main/java/com/gentics/mesh/core/field/binary/BinaryFieldEndpointTest.java index 0bddcf56f7..71675f6e67 100644 --- a/tests/tests-core/src/main/java/com/gentics/mesh/core/field/binary/BinaryFieldEndpointTest.java +++ b/tests/tests-core/src/main/java/com/gentics/mesh/core/field/binary/BinaryFieldEndpointTest.java @@ -27,10 +27,13 @@ import java.util.List; import java.util.Optional; import java.util.Random; +import java.util.function.Predicate; import org.apache.commons.codec.digest.DigestUtils; import org.apache.commons.io.FileUtils; import org.apache.commons.io.IOUtils; +import org.apache.commons.lang3.RandomStringUtils; +import org.apache.commons.lang3.StringUtils; import org.junit.Before; import org.junit.Test; @@ -72,6 +75,22 @@ public class BinaryFieldEndpointTest extends AbstractFieldEndpointTest { private static final String FIELD_NAME = "binaryField"; + /** + * Predicate to filter out directories belonging to the old binaryImageCache structure + */ + private static final Predicate IS_OLD_STRUCTURE = p -> { + File f = p.toFile(); + return f.isDirectory() && StringUtils.length(f.getName()) == 8; + }; + + /** + * Predicate to filter out directories belonging to the new binaryImageCache structure + */ + private static final Predicate IS_NEW_STRUCTURE = p -> { + File f = p.toFile(); + return f.isDirectory() && StringUtils.length(f.getName()) == 2; + }; + /** * Update the schema and add a binary field. * @@ -362,33 +381,33 @@ public void testCacheMigration() throws IOException { } tx(tx -> { tx.binaryDao().findAll().runInExistingTx(tx).forEach(binary -> { - String sha512sum = binary.getSHA512Sum(); - String[] parts = sha512sum.split("(?<=\\G.{8})"); - StringBuffer buffer = new StringBuffer(); - buffer.append(File.separator); - for (String part : parts) { - buffer.append(part + File.separator); - } - String baseFolder = Paths.get(imageCache, buffer.toString()).toString(); - String baseName = "image-" + randomImageManipulation().getCacheKey() + ".jpg"; try { - Files.createDirectories(Path.of(baseFolder)); - try (InputStream i = binary.openBlockingStream().get(); OutputStream o = new BufferedOutputStream(new FileOutputStream(new File(Paths.get(baseFolder, baseName).toString())))) { + Path baseFolder = createOldBinaryImageCacheStructure(imageCache, binary.getSHA512Sum()); + String baseName = "image-" + randomImageManipulation().getCacheKey() + ".jpg"; + try (InputStream i = binary.openBlockingStream().get(); OutputStream o = new BufferedOutputStream(new FileOutputStream(new File(baseFolder.toFile(), baseName).toString()))) { i.transferTo(o); } } catch (Exception e) { throw new RuntimeException(e); } }); + + // also create an empty folder structure (old structure) + createOldBinaryImageCacheStructure(imageCache, createRandomSha512Sum()); + + // and create a folder structure for a binary, which does not exist + Path baseFolder = createOldBinaryImageCacheStructure(imageCache, createRandomSha512Sum()); + String baseName = "image-" + randomImageManipulation().getCacheKey() + ".jpg"; + new File(baseFolder.toFile(), baseName).createNewFile(); }); assertThat(Files.walk(Path.of(imageCache)).filter(path -> { File file = new File(path.toString()); return file.exists() && file.isFile() && file.getName().startsWith("image-"); - }).count()).isEqualTo(4); + }).count()).isEqualTo(5); assertThat(Files.walk(Path.of(imageCache)).filter(path -> { File file = new File(path.toString()); return file.exists() && file.isFile() && file.getName().endsWith(".jpg"); - }).count()).isEqualTo(4); + }).count()).isEqualTo(5); grantAdmin(); HibJob job = tx(tx -> { return tx.jobDao().enqueueImageCacheMigration(user()); }); @@ -408,6 +427,64 @@ public void testCacheMigration() throws IOException { File file = new File(path.toString()); return file.exists() && file.isFile() && file.getName().endsWith(".jpg"); }).count()).isEqualTo(4); + + assertThat(Files.walk(Path.of(imageCache)).filter(IS_OLD_STRUCTURE)).as("Directories/Files in binaryImageCache of the old structure").isEmpty(); + } + + /** + * Create a random sha512sum + * @return sha512sum + */ + protected String createRandomSha512Sum() { + Buffer dummyContent = Buffer.buffer(RandomStringUtils.random(100)); + return com.gentics.mesh.util.FileUtils.hash(dummyContent).blockingGet(); + } + + /** + * Create the old folder structure for the given sha512sum + * @param imageCache base image cache directory + * @param sha512sum sha512sum + * @return path to the deepest folder + * @throws IOException + */ + protected Path createOldBinaryImageCacheStructure(String imageCache, String sha512sum) throws IOException { + String[] parts = sha512sum.split("(?<=\\G.{8})"); + StringBuffer buffer = new StringBuffer(); + buffer.append(File.separator); + for (String part : parts) { + buffer.append(part + File.separator); + } + Path dir = Paths.get(imageCache, buffer.toString()); + Files.createDirectories(dir); + return dir; + } + + /** + * Test that getting an image variant of an existing binary will put the file into the new structure of the binaryImageCache, but not the old one + * @throws IOException + */ + @Test + public void testImageCacheUsage() throws IOException { + String imageCache = options().getImageOptions().getImageCacheDirectory(); + + // assert that image cache directory is empty + assertThat(Files.list(Path.of(imageCache))).as("Directories/Files in binaryImageCache").isEmpty(); + + // first create an image + byte[] bytes = getBinary("/pictures/blume.jpg"); + NodeResponse nodeResponse = createBinaryNode(); + call(() -> client().updateNodeBinaryField(PROJECT_NAME, nodeResponse.getUuid(), "en", nodeResponse.getVersion(), "binary", + new ByteArrayInputStream(bytes), bytes.length, "blume.jpg","image/jpg")); + + // assert that image cache directory is still empty + assertThat(Files.list(Path.of(imageCache))).as("Directories/Files in binaryImageCache").isEmpty(); + + // get a resized variant of the image + call(() -> client().downloadBinaryField(PROJECT_NAME, nodeResponse.getUuid(), "en", "binary", randomImageManipulation())); + + // assert that the image cache contains exactly two (nested) directories of the new structure, but none of the old structure + assertThat(Files.walk(Path.of(imageCache)).filter(IS_OLD_STRUCTURE)).as("Directories/Files in binaryImageCache of the old structure").isEmpty(); + assertThat(Files.walk(Path.of(imageCache)).filter(IS_NEW_STRUCTURE)).as("Directories/Files in binaryImageCache of the new structure").hasSize(2); } private ImageManipulationParameters randomImageManipulation() {