Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SUP 17176: Fix accessing files in the binary image cache to not create folders of #1642

Open
wants to merge 1 commit into
base: hotfix-1.10.x
Choose a base branch
from
Open

SUP 17176: Fix accessing files in the binary image cache to not create folders of #1642

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion LTS-CHANGELOG.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
60 changes: 31 additions & 29 deletions common/src/main/java/com/gentics/mesh/core/image/spi/AbstractImageManipulator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,38 +112,40 @@ protected Single<CacheFileInfo> 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);
});
}

Expand Down
97 changes: 64 additions & 33 deletions mdm/common/src/main/java/com/gentics/mesh/core/jobs/ImageCacheMigrationProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<Path>() {

@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(() -> {
Expand Down
103 changes: 90 additions & 13 deletions .../tests-core/src/main/java/com/gentics/mesh/core/field/binary/BinaryFieldEndpointTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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<Path> 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<Path> IS_NEW_STRUCTURE = p -> {
File f = p.toFile();
return f.isDirectory() && StringUtils.length(f.getName()) == 2;
};

/**
* Update the schema and add a binary field.
*
Expand Down Expand Up @@ -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()); });
Expand All @@ -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() {
Expand Down